Page MenuHomePhabricator

[compiler-rt] Implement __atomic_is_lock_free
ClosedPublic

Authored by arichardson on Nov 30 2020, 1:39 AM.

Details

Summary

This function is called by the __atomic_is_lock_free builtin if the value
cannot be resolved to true at compile time. Lack of this function is
causing the non-lockfree atomics tests in libc++ to not be run (see D91911)

This function is also added in D85044, but that also add support for using
lock-free atomics in more cases, whereas this just adds __atomic_is_lock_free
for the current state of atomic.c.

Diff Detail

Event Timeline

arichardson created this revision.Nov 30 2020, 1:39 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: Restricted Project, jfb, dberris. · View Herald Transcript
arichardson requested review of this revision.Nov 30 2020, 1:39 AM
ldionne accepted this revision.Nov 30 2020, 6:58 AM
This revision is now accepted and ready to land.Nov 30 2020, 6:58 AM
ldionne requested changes to this revision.Nov 30 2020, 6:59 AM

Actually, this appears to be missing a test.

This revision now requires changes to proceed.Nov 30 2020, 6:59 AM
arichardson planned changes to this revision.Dec 1 2020, 8:28 AM

Will add tests for this in the coming days.

I just tried adding tests for this, and it turns out the existing atomic_test.c crashes due to __c11_atomic_load being implemented with cmpxchg16b. This crashes if the source is in read-only memory (as happens with this test). Do we need change __atomic_is_lock_free to return false for 16 byte atomics on x86 if clang generates incorrect code?

arichardson updated this revision to Diff 310142.EditedDec 8 2020, 4:44 AM
  • Add a test case (passes on x86_64 macOS, not tried other architectures yet)
  • Rebased on top of D92833

Do we need change __atomic_is_lock_free to return false for 16 byte atomics on x86 if clang generates incorrect code?

I believe compiler-rt should be consistent with the code that Clang generates, even if Clang generates sub-optimal code. Then, we can fix compiler-rt at the same time as we fix Clang. Otherwise, it seems like one could say that we have two bugs: one is the fact that Clang doesn't implement 16 bytes atomics without locks, and the other one is that compiler-rt lies, by pretending that we do.

oontvoo added inline comments.Dec 8 2020, 6:06 PM
compiler-rt/test/builtins/Unit/atomic_test.c
594–595

Why should size 0 never be lock free?

(I would've thought they'd always be lock free given no operation is needed )

! In D92302#2439218, @arichardson wrote:
I just tried adding tests for this, and it turns out the existing atomic_test.c crashes due to __c11_atomic_load being implemented with cmpxchg16b. This crashes if the source is in read-only memory (as happens with this test).

I think this is expected. (GCC also has this issue - there's no 16byte load-instruction so compxchg16b is the only option right now).

Do we need change __atomic_is_lock_free to return false for 16 byte atomics on x86 if clang generates incorrect code?

I believe the answer is "no" - it should return the *right* answer.

Do we need change __atomic_is_lock_free to return false for 16 byte atomics on x86 if clang generates incorrect code?

I believe compiler-rt should be consistent with the code that Clang generates, even if Clang generates sub-optimal code. Then, we can fix compiler-rt at the same time as we fix Clang. Otherwise, it seems like one could say that we have two bugs: one is the fact that Clang doesn't implement 16 bytes atomics without locks, and the other one is that compiler-rt lies, by pretending that we do.

I agree that __atomic_is_lock_free should return a value consistent with the atomic.c implementation.
16-byte atomics are currently lock-free on amd64, but using an atomic_load of read-only memory will result in a crash (if compiler-rt was compiled with cmxchg16b support). However, that is a problem that should be fixed in a separate review and D92832 and D85044 are probably a better place to discuss this issue.

compiler-rt/test/builtins/Unit/atomic_test.c
594–595

Size zero *could* be lock-free by returning immediately, but that's not what atomic.c does. __atomic_is_lock_free should return an answer that matches the implementation.

Fixing this should be part of a separate commit IMO. Also I'm not sure whether passing size=0 to any of the libatomic functions is valid.

Do we need change __atomic_is_lock_free to return false for 16 byte atomics on x86 if clang generates incorrect code?

I believe compiler-rt should be consistent with the code that Clang generates, even if Clang generates sub-optimal code. Then, we can fix compiler-rt at the same time as we fix Clang. Otherwise, it seems like one could say that we have two bugs: one is the fact that Clang doesn't implement 16 bytes atomics without locks, and the other one is that compiler-rt lies, by pretending that we do.

I agree that __atomic_is_lock_free should return a value consistent with the atomic.c implementation.
16-byte atomics are currently lock-free on amd64, but using an atomic_load of read-only memory will result in a crash (if compiler-rt was compiled with cmxchg16b support). However, that is a problem that should be fixed in a separate review and D92832 and D85044 are probably a better place to discuss this issue.

SGTM - thanks!

bcain added a subscriber: bcain.Dec 21 2020, 2:35 PM

@ldionne Test has been added, is this okay to commit now?

ldionne accepted this revision.Jan 7 2021, 8:24 AM

Thanks!

This revision is now accepted and ready to land.Jan 7 2021, 8:24 AM
This revision was landed with ongoing or failed builds.Jan 8 2021, 4:49 AM
This revision was automatically updated to reflect the committed changes.