Skip to content

Conversation

ronawho
Copy link
Contributor

@ronawho ronawho commented Jan 17, 2019

In #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 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() 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.

Testing:

  • llvm built with gcc 8.2.0 (full paratest on chapcs)
  • llvm built with gcc 4.8.5 (atomic tests on chapc)
  • system llvm (atomic tests on chapel-ubu1604-02 -- old workaround failed in nightly)
  • llvm built with clang 10.0.0 (atomic tests on High Sierra)

@ronawho ronawho requested review from dmk42 and mppf January 17, 2019 22:56
@ronawho
Copy link
Contributor Author

ronawho commented Jan 17, 2019

Here's a snippet from the clang <stdatomic.h> file:

/* 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 atomic_thread_fence(). New enough versions of libatomic happen to have an implementation of atomic_thread_fence() so in those cases adding -latomic worked, but that did not work for older libatomic versions. I'm much happier with the solution in this PR -- it answers a lot of what we didn't understand in #12045.

Copy link
Contributor

@dmk42 dmk42 left a 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.
@ronawho ronawho force-pushed the fix-llvm-cstdlib-atomic-issues branch from 234a269 to b9bf393 Compare January 17, 2019 23:24
@ronawho ronawho merged commit 517c5be into chapel-lang:master Jan 18, 2019
@ronawho ronawho deleted the fix-llvm-cstdlib-atomic-issues branch January 18, 2019 00:05
@mppf
Copy link
Member

mppf commented Jan 21, 2019

Looks good, thanks!

ronawho added a commit to ronawho/chapel that referenced this pull request Feb 5, 2019
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.
ronawho added a commit that referenced this pull request Feb 6, 2019
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants