llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
29.13k stars 12.02k forks source link

clang-cl, mismatch MSVC cl about `error: redefinition of 'XXX' as different kind of symbol` #95681

Open lygstate opened 5 months ago

lygstate commented 5 months ago

Code to reproduce the issue

#include <windows.h>

namespace test
{
extern "C" __declspec(selectany) void const *const CryptProtectMemory = nullptr;
}

int main()
{
    return 0;
}

With cl.exe, it's compiles fine.

C:\work\study\runtimes\win-polyfill>cl test.cpp
用于 x64 的 Microsoft (R) C/C++ 优化编译器 19.40.33811 版
版权所有(C) Microsoft Corporation。保留所有权利。

test.cpp
Microsoft (R) Incremental Linker Version 14.40.33811.0
Copyright (C) Microsoft Corporation.  All rights reserved.

/out:test.exe
test.obj

C:\work\study\runtimes\win-polyfill>

With clang-cl.exe, it's fails with

C:\work\study\runtimes\win-polyfill>clang-cl test.cpp
test.cpp(5,52): error: redefinition of 'CryptProtectMemory' as different kind of symbol
    5 | extern "C" __declspec(selectany) void const *const CryptProtectMemory = nullptr;
      |                                                    ^
C:\Program Files (x86)\Windows Kits\10\\include\10.0.26100.0\\um\dpapi.h(312,1): note: previous definition is here
  312 | CryptProtectMemory(
      | ^
1 error generated.
zufuliu commented 5 months ago

Similar to issue #58783.

lygstate commented 5 months ago

Similar to issue #58783.

yeap, somewhat different, this have namespace and for C++ only, without the namespace, MSVC also have error message

AaronBallman commented 5 months ago

I think Clang is correct to reject that code, per https://eel.is/c++draft/dcl.link#7 and https://eel.is/c++draft/basic.scope#scope-6: https://godbolt.org/z/nPc5451fv -- they don't declare the same entity and the names conflict.

CC @rnk @zmodem for opinions on whether this is behavior we should try to implement for compatibility. My inclination is to not support this construct unless there's a strong need; this feels like a bug more than an intentional behavior (unless I'm missing something).

lygstate commented 5 months ago

I think Clang is correct to reject that code, per https://eel.is/c++draft/dcl.link#7 and https://eel.is/c++draft/basic.scope#scope-6: https://godbolt.org/z/nPc5451fv -- they don't declare the same entity and the names conflict.

CC @rnk @zmodem for opinions on whether this is behavior we should try to implement for compatibility. My inclination is to not support this construct unless there's a strong need; this feels like a bug more than an intentional behavior (unless I'm missing something).

I need this to export symbols to generate symbols to override windows sdk API, This is something I needed. I think this can be treat as warning instead of error(so that the warning can be disabled)

zmodem commented 5 months ago

I think Clang is correct to reject that code, per https://eel.is/c++draft/dcl.link#7 and https://eel.is/c++draft/basic.scope#scope-6: https://godbolt.org/z/nPc5451fv -- they don't declare the same entity and the names conflict.

CC @rnk @zmodem for opinions on whether this is behavior we should try to implement for compatibility. My inclination is to not support this construct unless there's a strong need; this feels like a bug more than an intentional behavior (unless I'm missing something).

I agree with your inclination, we shouldn't do compatibility workarounds unless it's for code which a user cannot control such as Windows system headers. That doesn't seem to be the case here.

I need this to export symbols to generate symbols to override windows sdk API, This is something I needed. I think this can be treat as warning instead of error(so that the warning can be disabled)

If you just need to generate a symbol with the same name, can't you do it in a file which doesn't include windows.h?

lygstate commented 5 months ago

I think Clang is correct to reject that code, per https://eel.is/c++draft/dcl.link#7 and https://eel.is/c++draft/basic.scope#scope-6: https://godbolt.org/z/nPc5451fv -- they don't declare the same entity and the names conflict. CC @rnk @zmodem for opinions on whether this is behavior we should try to implement for compatibility. My inclination is to not support this construct unless there's a strong need; this feels like a bug more than an intentional behavior (unless I'm missing something).

I agree with your inclination, we shouldn't do compatibility workarounds unless it's for code which a user cannot control such as Windows system headers. That doesn't seem to be the case here.

I need this to export symbols to generate symbols to override windows sdk API, This is something I needed. I think this can be treat as warning instead of error(so that the warning can be disabled)

If you just need to generate a symbol with the same name, can't you do it in a file which doesn't include windows.h?

Yeap, because I am trying to override Windows.h's api, I depends on that :(

zufuliu commented 3 months ago

It seems this is same as issue #24569, #29512.