[PATCH] D88443: [scudo][standalone] Remove unused atomic_compare_exchange_weak

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad created this revision.
cryptoad added reviewers: pcc, mcgrathr, hctim, cferris.
Herald added subscribers: Sanitizers, jfb.
Herald added a project: Sanitizers.
cryptoad requested review of this revision.

`atomic_compare_exchange_weak` is unused in Scudo, and its associated
test is actually wrong since the weak variant is allowed to fail
spuriously (thanks Roland).

This lead to flakes such as:

  [ RUN      ] ScudoAtomicTest.AtomicCompareExchangeTest
  ../../zircon/third_party/scudo/src/tests/atomic_test.cpp:98: Failure: Expected atomic_compare_exchange_weak(reinterpret_cast<T *>(&V), &OldVal, NewVal, memory_order_relaxed) is true.
      Expected: true
      Which is: 01
      Actual  : atomic_compare_exchange_weak(reinterpret_cast<T *>(&V), &OldVal, NewVal, memory_order_relaxed)
      Which is: 00
  ../../zircon/third_party/scudo/src/tests/atomic_test.cpp:100: Failure: Expected atomic_compare_exchange_weak( reinterpret_cast<T *>(&V), &OldVal, NewVal, memory_order_relaxed) is false.
      Expected: false
      Which is: 00
      Actual  : atomic_compare_exchange_weak( reinterpret_cast<T *>(&V), &OldVal, NewVal, memory_order_relaxed)
      Which is: 01
  ../../zircon/third_party/scudo/src/tests/atomic_test.cpp:101: Failure: Expected OldVal == NewVal.
      Expected: NewVal
      Which is: 24
      Actual  : OldVal
      Which is: 42
  [  FAILED  ] ScudoAtomicTest.AtomicCompareExchangeTest (0 ms)
  [----------] 2 tests from ScudoAtomicTest (1 ms total)

So I am removing this, if someone ever needs the weak variant, feel
free to add it back with a test that is not as terrible. This test was
initially ported from sanitizer_common, but their weak version calls
the strong version, so it works for them.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88443

Files:
  compiler-rt/lib/scudo/standalone/atomic_helpers.h
  compiler-rt/lib/scudo/standalone/tests/atomic_test.cpp


Index: compiler-rt/lib/scudo/standalone/tests/atomic_test.cpp
===================================================================
--- compiler-rt/lib/scudo/standalone/tests/atomic_test.cpp
+++ compiler-rt/lib/scudo/standalone/tests/atomic_test.cpp
@@ -80,26 +80,14 @@
 
 template <typename T> void checkAtomicCompareExchange() {
   typedef typename T::Type Type;
-  {
-    Type OldVal = 42;
-    Type NewVal = 24;
-    Type V = OldVal;
-    EXPECT_TRUE(atomic_compare_exchange_strong(
-        reinterpret_cast<T *>(&V), &OldVal, NewVal, memory_order_relaxed));
-    EXPECT_FALSE(atomic_compare_exchange_strong(
-        reinterpret_cast<T *>(&V), &OldVal, NewVal, memory_order_relaxed));
-    EXPECT_EQ(NewVal, OldVal);
-  }
-  {
-    Type OldVal = 42;
-    Type NewVal = 24;
-    Type V = OldVal;
-    EXPECT_TRUE(atomic_compare_exchange_weak(reinterpret_cast<T *>(&V), &OldVal,
+  Type OldVal = 42;
+  Type NewVal = 24;
+  Type V = OldVal;
+  EXPECT_TRUE(atomic_compare_exchange_strong(reinterpret_cast<T *>(&V), &OldVal,
                                              NewVal, memory_order_relaxed));
-    EXPECT_FALSE(atomic_compare_exchange_weak(
-        reinterpret_cast<T *>(&V), &OldVal, NewVal, memory_order_relaxed));
-    EXPECT_EQ(NewVal, OldVal);
-  }
+  EXPECT_FALSE(atomic_compare_exchange_strong(
+      reinterpret_cast<T *>(&V), &OldVal, NewVal, memory_order_relaxed));
+  EXPECT_EQ(NewVal, OldVal);
 }
 
 TEST(ScudoAtomicTest, AtomicCompareExchangeTest) {
Index: compiler-rt/lib/scudo/standalone/atomic_helpers.h
===================================================================
--- compiler-rt/lib/scudo/standalone/atomic_helpers.h
+++ compiler-rt/lib/scudo/standalone/atomic_helpers.h
@@ -106,14 +106,6 @@
                                    __ATOMIC_RELAXED);
 }
 
-template <typename T>
-inline bool atomic_compare_exchange_weak(volatile T *A, typename T::Type *Cmp,
-                                         typename T::Type Xchg,
-                                         memory_order MO) {
-  return __atomic_compare_exchange(&A->ValDoNotUse, Cmp, &Xchg, true, MO,
-                                   __ATOMIC_RELAXED);
-}
-
 // Clutter-reducing helpers.
 
 template <typename T>


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D88443.294792.patch
Type: text/x-patch
Size: 2208 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200928/292798d3/attachment.bin>