Commit db56587b authored by bsimmers's avatar bsimmers Committed by Jordan DeLong

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
parent ab4ee5fa
...@@ -111,12 +111,13 @@ struct MicroSpinLock { ...@@ -111,12 +111,13 @@ struct MicroSpinLock {
*/ */
bool cas(uint8_t compare, uint8_t newVal) { bool cas(uint8_t compare, uint8_t newVal) {
bool out; bool out;
asm volatile("lock; cmpxchgb %2, (%3);" bool memVal; // only set if the cmpxchg fails
"setz %0;" asm volatile("lock; cmpxchgb %[newVal], (%[lockPtr]);"
: "=r" (out) "setz %[output];"
: [output] "=r" (out), "=a" (memVal)
: "a" (compare), // cmpxchgb constrains this to be in %al : "a" (compare), // cmpxchgb constrains this to be in %al
"q" (newVal), // Needs to be byte-accessible [newVal] "q" (newVal), // Needs to be byte-accessible
"r" (&lock_) [lockPtr] "r" (&lock_)
: "memory", "flags"); : "memory", "flags");
return out; return out;
} }
......
...@@ -17,6 +17,7 @@ ...@@ -17,6 +17,7 @@
#include "folly/SmallLocks.h" #include "folly/SmallLocks.h"
#include <cassert> #include <cassert>
#include <cstdio> #include <cstdio>
#include <mutex>
#include <string> #include <string>
#include <vector> #include <vector>
#include <pthread.h> #include <pthread.h>
...@@ -102,6 +103,22 @@ void doPslTest() { ...@@ -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) { TEST(SmallLocks, SpinLockCorrectness) {
...@@ -140,3 +157,7 @@ TEST(SmallLocks, PicoSpinSigned) { ...@@ -140,3 +157,7 @@ TEST(SmallLocks, PicoSpinSigned) {
} }
EXPECT_EQ(val.getData(), -8); EXPECT_EQ(val.getData(), -8);
} }
TEST(SmallLocks, RegClobber) {
TestClobber().go();
}
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment