Fix asm constraints in folly::MicroSpinLock::cas

Summary:
If the compare part of cmpxchg fails, it writes the unexpected value
to %rax. At certain optimization levels this was causing me to hit an
incorrectly failed assertion in some thrift code. I also cleaned up the asm
statement to use named operands.

Test Plan: Added new test that fails before this diff.

Reviewed By: delong.j@fb.com

FB internal diff: D1083222
@override-unit-failures

diff --git a/folly/SmallLocks.h b/folly/SmallLocks.h
index 01387aee724eae06da863f6f48f346db9be4cd9d..b092a91766b1ff6e91d41b36f81feecb3315ae0d 100644 (file)
--- a/folly/SmallLocks.h
+++ b/folly/SmallLocks.h
@@ -111,12 +111,13 @@ struct MicroSpinLock {
    */
   bool cas(uint8_t compare, uint8_t newVal) {
     bool out;
-    asm volatile("lock; cmpxchgb %2, (%3);"
-                 "setz %0;"
-                 : "=r" (out)
+    bool memVal; // only set if the cmpxchg fails
+    asm volatile("lock; cmpxchgb %[newVal], (%[lockPtr]);"
+                 "setz %[output];"
+                 : [output] "=r" (out), "=a" (memVal)
                  : "a" (compare), // cmpxchgb constrains this to be in %al
-                   "q" (newVal),  // Needs to be byte-accessible
-                   "r" (&lock_)
+                   [newVal] "q" (newVal),  // Needs to be byte-accessible
+                   [lockPtr] "r" (&lock_)
                  : "memory", "flags");
     return out;
   }

diff --git a/folly/test/SmallLocksTest.cpp b/folly/test/SmallLocksTest.cpp
index e7cc04fc1e9c6deb7d0d36410c3596a3cbb98c1d..dedc476fef4d77dbcdcbcd9ac438fa92c6b05fe4 100644 (file)
--- a/folly/test/SmallLocksTest.cpp
+++ b/folly/test/SmallLocksTest.cpp
@@ -17,6 +17,7 @@
 #include "folly/SmallLocks.h"
 #include <cassert>
 #include <cstdio>
+#include <mutex>
 #include <string>
 #include <vector>
 #include <pthread.h>
@@ -102,6 +103,22 @@ void doPslTest() {
   }
 }
 
+struct TestClobber {
+  TestClobber() {
+    lock_.init();
+  }
+
+  void go() {
+    std::lock_guard<MicroSpinLock> g(lock_);
+    // This bug depends on gcc register allocation and is very sensitive. We
+    // have to use DCHECK instead of EXPECT_*.
+    DCHECK(!lock_.try_lock());
+  }
+
+ private:
+  MicroSpinLock lock_;
+};
+
 }
 
 TEST(SmallLocks, SpinLockCorrectness) {
@@ -140,3 +157,7 @@ TEST(SmallLocks, PicoSpinSigned) {
   }
   EXPECT_EQ(val.getData(), -8);
 }
+
+TEST(SmallLocks, RegClobber) {
+  TestClobber().go();
+}