[libcxx-commits] [PATCH] D103846: [libcxx][atomic] Fix failure mapping in compare_exchange_{strong, weak}.

[Jordan Rupprecht via Phabricator via libcxx-commits]

rupprecht added a comment.

In D103846#2805849 <https://reviews.llvm.org/D103846#2805849>, @ldionne wrote:

> Is it possible to write a test for that? When you say
>
>> This was caught by tsan tests after D99434 <https://reviews.llvm.org/D99434> [...]
>
> Do you mean that any Clang that includes the fix for D99434 <https://reviews.llvm.org/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)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103846/new/

https://reviews.llvm.org/D103846