-
Notifications
You must be signed in to change notification settings - Fork 434
Fix support for cstdlib atomics under --llvm (round 2) #12100
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
Fix support for cstdlib atomics under --llvm (round 2) #12100
Conversation
Here's a snippet from the clang /* These should be provided by the libc implementation. */
void atomic_thread_fence(memory_order);
void atomic_signal_fence(memory_order);
#define atomic_thread_fence(order) __c11_atomic_thread_fence(order)
#define atomic_signal_fence(order) __c11_atomic_signal_fence(order) where we effectively ignored the macros and were left without an implementation for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
In chapel-lang#12045 we started adding `-latomic` to llvm compiles for cstdlib atomic configurations to try to resolve undefined references to `atomic_thread_fence`. This ended up being a work around that didn't fix the core issue, and it didn't work in all cases (e.g. when llvm can't find a libatomic from gcc >=6.) Here we revert that change and fix this core issue. What was actually going wrong was that clang's <stdatomic.h> header has both a macro and a function prototype for `atomic_thread_fence()` and our llvm backend isn't able to handle the macro so we ended up using a function with no implementation, which led to undefined references. New enough versions of libatomic happen to have an implementation of `atomic_thread_fence()`, so throwing `-latomic` worked in that case, but not for older versions of libatomic. To avoid the problem this adds a `chpl_atomic_thread_fence()` wrapper. Note that we have done similar things in the past to avoid issues with the llvm backend and macros that it can't process.
234a269
to
b9bf393
Compare
Looks good, thanks! |
Our runtime atomic fence used to just be `atomic_thread_fence` (same name as C11 atomics), but in chapel-lang#12100 we changed the name to be `chpl_atomic_thread_fence`. It's easy to forget about the name change and it'll happen to work in configurations where cstdlib atomics are the default. Update our lookForBadRTCalls script to find them.
Update lookForBadRTCalls to find calls to atomic_thread_fence [reviewed by @gbtitus] Our runtime atomic fence used to just be `atomic_thread_fence` (same name as C11 atomics), but in #12100 we changed the name to be `chpl_atomic_thread_fence`. It's easy to forget about the name change and it'll happen to work in configurations where cstdlib atomics are the default. Update our lookForBadRTCalls script to find them. This would have caught #12255
In #12045 we started adding
-latomic
to llvm compiles for cstdlibatomic configurations to try to resolve undefined references to
atomic_thread_fence
. This ended up being a work around that didn't fixthe core issue, and it didn't work in all cases (e.g. when llvm can't
find a libatomic from gcc >=6.) Here we revert that change and fix the
core issue.
What was actually going wrong was that clang's <stdatomic.h> header has
both a macro and a function prototype for
atomic_thread_fence()
andour llvm backend isn't able to handle the macro so we ended up using a
function with no implementation, which led to undefined references. New
enough versions of libatomic happen to have an implementation of
atomic_thread_fence()
, so throwing-latomic
worked in that case, butnot for older versions of libatomic. To avoid the problem this adds a
chpl_atomic_thread_fence()
wrapper. Note that we have done similarthings in the past to avoid issues with the llvm backend and macros that
it can't process.
Testing: