This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][atomic] Fix failure mapping in compare_exchange_{strong,weak}.
ClosedPublic

Authored by rupprecht on Jun 7 2021, 2:57 PM.

Details

Summary

https://eel.is/c++draft/atomics.types.operations#23 says: ... the value of failure is order except that a value of memory_order::acq_rel shall be replaced by the value memory_order::acquire and a value of memory_order::release shall be replaced by the value memory_order::relaxed.

This failure mapping is only handled for _LIBCPP_HAS_GCC_ATOMIC_IMP. We are seeing bad code generation for compare_exchange_strong(cmp, 1, std::memory_order_acq_rel) when using libc++ in place of libstdc++: https://godbolt.org/z/v3onrrq4G.

This was caught by tsan tests after D99434, [TSAN] Honor failure memory orders in AtomicCAS, but appears to be an issue in non-tsan code.

Diff Detail

Event Timeline

rupprecht created this revision.Jun 7 2021, 2:57 PM
rupprecht requested review of this revision.Jun 7 2021, 2:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2021, 2:57 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
dvyukov accepted this revision.Jun 8 2021, 12:28 AM
ldionne requested changes to this revision.Jun 8 2021, 9:59 AM

Is it possible to write a test for that? When you say

This was caught by tsan tests after D99434 [...]

Do you mean that any Clang that includes the fix for D99434 will catch the error with the existing libc++ test suite when TSAN is enabled? If so, then I think it's fine not to add a test, since it's already tested. We could consider moving more CI configurations to Clang trunk, too.

Requesting changes until I have answers to my questions, but otherwise this LGTM.

This revision now requires changes to proceed.Jun 8 2021, 9:59 AM

Is it possible to write a test for that? When you say

This was caught by tsan tests after D99434 [...]

Do you mean that any Clang that includes the fix for D99434 will catch the error with the existing libc++ test suite when TSAN is enabled? If so, then I think it's fine not to add a test, since it's already tested. We could consider moving more CI configurations to Clang trunk, too.

Requesting changes until I have answers to my questions, but otherwise this LGTM.

No, these are some tests we run internally with tsan enabled, and found bad codegen for this dependency: https://github.com/zeromq/libzmq/blob/b40a793142326edeec5c4dc63e6d8929e979a097/src/atomic_ptr.hpp#L211

There is libcxx/test/std/atomics/atomics.general/replace_failure_order.pass.cpp that already covers this issue. It only tests that the build is successful, and AFAICT doesn't verify correctness (e.g. the right codegen), so the test passes.

Here's a test that catches the issue (verified by locally reverting changes). Is is possible to put this somewhere in the libc++ test suite?

// RUN: %clangxx -O2 -c %s -stdlib=libc++ -fsanitize=thread -S -emit-llvm -o - | FileCheck %s
#include <atomic>

// CHECK: @_Z27strong_memory_order_acq_relPNSt3__16atomicIiEEi(
// CHECK:    {{.*}} = call i32 @__tsan_atomic32_compare_exchange_val({{.*}}, i32 1, i32 4, i32 2)
bool strong_memory_order_acq_rel(std::atomic<int> *a, int cmp) {
  return a->compare_exchange_strong(cmp, 1, std::memory_order_acq_rel);
}

(it generates ..., i32 1, i32 4, i32 0) w/o this patch)

  • Add a codegen test

The test I added is not the greatest test, but it catches the issue. Lemme know if this works.

ldionne accepted this revision.Jun 14 2021, 3:12 PM

Thanks a lot! Indeed, we don't have access to FileCheck in the libc++ tests. I had once looked into doing that but I ran into issues and we didn't have a clear use case so I decided not to spend more time on it. Looks like this will do for the time being.

Please merge this once/if CI is passing! Thanks!

This revision is now accepted and ready to land.Jun 14 2021, 3:12 PM
ldionne added inline comments.Jun 14 2021, 3:13 PM
libcxx/test/std/atomics/atomics.general/replace_failure_order_codegen.sh.cpp
10

You can fix the CI failure by saying // REQUIRES: clang.

rupprecht updated this revision to Diff 352013.Jun 14 2021, 4:13 PM
rupprecht marked an inline comment as done.
  • Add requires for clang to avoid unknown gcc flag errors

Thanks! I'll wait for another CI notification to post before landing.

libcxx/test/std/atomics/atomics.general/replace_failure_order_codegen.sh.cpp
10

Ah, thanks for the tip. Done.

rupprecht updated this revision to Diff 352033.Jun 14 2021, 7:14 PM
  • Require tsan feature to avoid buildbot jobs that don't support -fsanitize=thread (asan, windows, 32bit, ARMv7/8)