Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clang 14's <stdatomic.h> is incompatible with MSVC's <stdatomic.h> ☢️ #2862

Closed
StephanTLavavej opened this issue Jul 14, 2022 · 7 comments · Fixed by #3186
Closed

Clang 14's <stdatomic.h> is incompatible with MSVC's <stdatomic.h> ☢️ #2862

StephanTLavavej opened this issue Jul 14, 2022 · 7 comments · Fixed by #3186
Labels
compiler Compiler work involved fixed Something works now, yay!

Comments

@StephanTLavavej
Copy link
Member

This repros with Clang 14, when the STL's tests are compiled with the official toolset (instead of the microsoft/STL repo - the difference is in the header locations, because Clang overrides toolset headers).

tests/std/tests/P0943R6_stdatomic_h/test.compile.pass.cpp and the many tests including stl/inc/__msvc_all_public_headers.hpp fail due to Clang's <stdatomic.h>, like this:

RUN.PM line 750:  Executing: "clang-cl -m32  /nologo /Od /W4 /w14061 /w14242 /w14265 /w14582 /w14583 /w14587 /w14588 /w14749 /w14841 /w14842 /w15038 /w15214 /w15215 /w15216 /w15217 /sdl /WX /Zc:strictStrings /D_ENABLE_STL_INTERNAL_CHECK /bigobj /FIforce_include.hpp /w14365 /D_ENFORCE_FACET_SPECIALIZATIONS=1 /D_STL_CALL_ABORT_INSTEAD_OF_INVALID_PARAMETER -fno-ms-compatibility -fno-delayed-template-parsing /EHsc /MD /std:c++14 /w14640 /Zc:threadSafeInit-   .\test.cpp  /c".
In file included from .\test.cpp:4:
In file included from D:\Contest\ca1z2udg.zif\binaries\x86chk\inc\__msvc_all_public_headers.hpp:146:
D:\Contest\ca1z2udg.zif\src\tools\LLVM\LLVM14\x86\lib\clang\14.0.5\include\stdatomic.h(70,6): error: conflicting types for 'atomic_thread_fence'
void atomic_thread_fence(memory_order);
     ^
D:\Contest\ca1z2udg.zif\binaries\x86chk\inc\atomic(166,24): note: previous definition is here
extern "C" inline void atomic_thread_fence(const memory_order _Order) noexcept {
                       ^
[...]

As a workaround, we're going to avoid including <stdatomic.h> for Clang, but solving the root cause would be better.

@fsb4000
Copy link
Contributor

fsb4000 commented Jul 23, 2022

They have many macros:

#define atomic_thread_fence(order) __c11_atomic_thread_fence(order)
#define atomic_fetch_xor_explicit __c11_atomic_fetch_xor
....

and we have real functions instead of macros in <atomic>:

extern "C" inline void atomic_thread_fence(const memory_order _Order) noexcept {
    ::_Atomic_thread_fence(static_cast<unsigned int>(_Order));
}
....

and then macro replaces to

extern "C" inline void __c11_atomic_thread_fence(const memory_order _Order) noexcept {
    ::_Atomic_thread_fence(static_cast<unsigned int>(_Order));
}

but __c11_atomic_thread_fence is builtin.
So I think Clang just should include <stdatomic.h> if it is C++ and MSVC and use their <stdatomic.h> only for MSVC and C:
https://github.com/llvm/llvm-project/blob/6aff1b7b3ca71e048375a85a11e611527da6a45c/clang/lib/Headers/stdatomic.h#L13-L22

I prepared a patch: https://reviews.llvm.org/D130419

image

@fsb4000
Copy link
Contributor

fsb4000 commented Jul 25, 2022

They landed the patch : llvm/llvm-project@ba49d39
Thanks @mordante and @AaronBallman ❤️

@h-vetinari
Copy link

Speaking of <stdatomic.h> - the conformance page says visual studio does not support it, and I cannot find a corresponding issue in the dev community for it - is there a plan to bring this to conformance eventually?

@fsb4000
Copy link
Contributor

fsb4000 commented Jul 26, 2022

Yes, DevCom-1204057

@h-vetinari
Copy link

Yes, DevCom-1204057

Thanks! Guess I was searching wrong (under "visual studio" instead of under "C++").

@mordante
Copy link
Contributor

They landed the patch : llvm/llvm-project@ba49d39 Thanks @mordante and @AaronBallman heart

You're welcome. FYI the patch landed before LLVM 15 was branched. So the fix will be in Clang-15.

@fsb4000

This comment was marked as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler Compiler work involved fixed Something works now, yay!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants