Merge r181305 - 8-bit version of weakCompareAndSwap() can cause an in…

…finite loop.

https://webkit.org/b/142513>

Reviewed by Filip Pizlo.

Source/JavaScriptCore:

Added a test that exercises the 8-bit CAS from multiple threads.  The threads
will contend to set bits in a large array of bytes using the CAS function.

* API/tests/CompareAndSwapTest.cpp: Added.
(Bitmap::Bitmap):
(Bitmap::numBits):
(Bitmap::clearAll):
(Bitmap::concurrentTestAndSet):
(setBitThreadFunc):
(testCompareAndSwap):
* API/tests/testapi.c:
(main):
* JavaScriptCore.vcxproj/testapi/testapi.vcxproj:
* JavaScriptCore.vcxproj/testapi/testapi.vcxproj.filters:
* JavaScriptCore.xcodeproj/project.pbxproj:

Source/WTF:

Presently, Bitmap::concurrentTestAndSet() uses the 8-bit version of
weakCompareAndSwap() (which compares and swaps an uint8_t value).
Bitmap::concurrentTestAndSet() has a loop that checks if a bit in the
byte of interest has been set.  If not, it will call the 8-bit CAS
function to set the bit.

Under the covers, for ARM, the 8-bit CAS function actually works with a
32-bit CAS.  The 8-bit CAS will first fetch the 32-bit value in memory
that should contain the 8-bit value, and check if it contains the
expected byte.  If the value in memory doesn't have the expected byte,
it will return early to its caller.  The expectation is that the caller
will reload the byte from memory and call the 8-bit CAS again.

Unfortunately, this code path that returns early does not have a
compiler fence.  Without a compiler fence, the C++ compiler can
optimize away the reloading of the expected byte value, leaving it
unchanged.  As a result, we'll have a infinite loop here that checks a
value that will never change, and the loop will not terminate until the
value changes.

The fix is to eliminate the early return check in the 8-bit CAS, and
have it always call down to the 32-bit CAS.  The 32-bit CAS has a
compiler fence which will prevent this issue.

* wtf/Atomics.h:
(WTF::weakCompareAndSwap):

Source/JavaScriptCore/API/tests/CompareAndSwapTest.cpp

@@ -0,0 +1,118 @@
+/*
+ * Copyright (C) 2015 Apple Inc. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
+ * THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include "config.h"
+
+#include <stdio.h>
+#include <thread>
+#include <wtf/Atomics.h>
+
+// Regression test for webkit.org/b/142513
+extern "C" void testCompareAndSwap();
+
+class Bitmap {
+public:
+    Bitmap() { clearAll(); }
+
+    inline void clearAll();
+    inline bool concurrentTestAndSet(size_t n);
+    inline size_t numBits() const { return words * wordSize; }
+
+private:
+    static const size_t Size = 4096*10;
+
+    static const unsigned wordSize = sizeof(uint8_t) * 8;
+    static const unsigned words = (Size + wordSize - 1) / wordSize;
+    static const uint8_t one = 1;
+
+    uint8_t bits[words];
+};
+
+inline void Bitmap::clearAll()
+{
+    memset(&bits, 0, sizeof(bits));
+}
+
+inline bool Bitmap::concurrentTestAndSet(size_t n)
+{
+    uint8_t mask = one << (n % wordSize);
+    size_t index = n / wordSize;
+    uint8_t* wordPtr = &bits[index];
+    uint8_t oldValue;
+    do {
+        oldValue = *wordPtr;
+        if (oldValue & mask)
+            return true;
+    } while (!WTF::weakCompareAndSwap(wordPtr, oldValue, oldValue | mask));
+    return false;
+}
+
+struct Data {
+    Bitmap* bitmap;
+    int id;
+    int numThreads;
+};
+
+static void* setBitThreadFunc(void* p)
+{
+    Data* data = reinterpret_cast<Data*>(p);
+    Bitmap* bitmap = data->bitmap;
+    size_t numBits = bitmap->numBits();
+
+    // The computed start index here is heuristic that seems to maximize (anecdotally)
+    // the chance for the CAS issue to manifest.
+    size_t start = (numBits * (data->numThreads - data->id)) / data->numThreads;
+
+    printf("   started Thread %d\n", data->id);
+    for (size_t i = start; i < numBits; i++)
+        while (!bitmap->concurrentTestAndSet(i)) { }
+    for (size_t i = 0; i < start; i++)
+        while (!bitmap->concurrentTestAndSet(i)) { }
+
+    printf("   finished Thread %d\n", data->id);
+    pthread_exit(nullptr);
+}
+
+void testCompareAndSwap()
+{
+    Bitmap bitmap;
+    const int numThreads = 5;
+    pthread_t threadIDs[numThreads];
+    Data data[numThreads];
+
+    printf("Starting %d threads for CompareAndSwap test.  Test should complete without hanging.\n", numThreads);
+    for (int i = 0; i < numThreads; i++) {
+        data[i].bitmap = &bitmap;
+        data[i].id = i;
+        data[i].numThreads = numThreads;
+        pthread_create(&threadIDs[i], 0, &setBitThreadFunc, &data[i]);
+    }
+
+    printf("Waiting for %d threads to join\n", numThreads);
+    for (int i = 0; i < numThreads; i++)
+        pthread_join(threadIDs[i], nullptr);
+
+    printf("PASS: CompareAndSwap test completed without a hang\n");
+}

Source/JavaScriptCore/API/tests/testapi.c

@@ -46,6 +46,7 @@
 #if JSC_OBJC_API_ENABLED
 void testObjectiveCAPI(void);
 #endif
+void testCompareAndSwap();
 
 extern void JSSynchronousGarbageCollectForDebugging(JSContextRef);
 
@@ -1174,6 +1175,8 @@ int main(int argc, char* argv[])
     ::SetErrorMode(0);
 #endif
 
+    testCompareAndSwap();
+
 #if JSC_OBJC_API_ENABLED
     testObjectiveCAPI();
 #endif

Source/JavaScriptCore/ChangeLog

@@ -1,3 +1,26 @@
+2015-03-09  Mark Lam  <mark.lam@apple.com>
+
+        8-bit version of weakCompareAndSwap() can cause an infinite loop.
+        https://webkit.org/b/142513>
+
+        Reviewed by Filip Pizlo.
+
+        Added a test that exercises the 8-bit CAS from multiple threads.  The threads
+        will contend to set bits in a large array of bytes using the CAS function.
+
+        * API/tests/CompareAndSwapTest.cpp: Added.
+        (Bitmap::Bitmap):
+        (Bitmap::numBits):
+        (Bitmap::clearAll):
+        (Bitmap::concurrentTestAndSet):
+        (setBitThreadFunc):
+        (testCompareAndSwap):
+        * API/tests/testapi.c:
+        (main):
+        * JavaScriptCore.vcxproj/testapi/testapi.vcxproj:
+        * JavaScriptCore.vcxproj/testapi/testapi.vcxproj.filters:
+        * JavaScriptCore.xcodeproj/project.pbxproj:
+
 2014-12-04  Oliver Hunt  <oliver@apple.com>
 
         Serialization of MapData object provides unsafe access to internal types

Source/WTF/ChangeLog

@@ -1,3 +1,37 @@
+2015-03-09  Mark Lam  <mark.lam@apple.com>
+
+        8-bit version of weakCompareAndSwap() can cause an infinite loop.
+        https://webkit.org/b/142513>
+
+        Reviewed by Filip Pizlo.
+
+        Presently, Bitmap::concurrentTestAndSet() uses the 8-bit version of
+        weakCompareAndSwap() (which compares and swaps an uint8_t value).
+        Bitmap::concurrentTestAndSet() has a loop that checks if a bit in the
+        byte of interest has been set.  If not, it will call the 8-bit CAS
+        function to set the bit.
+
+        Under the covers, for ARM, the 8-bit CAS function actually works with a
+        32-bit CAS.  The 8-bit CAS will first fetch the 32-bit value in memory
+        that should contain the 8-bit value, and check if it contains the
+        expected byte.  If the value in memory doesn't have the expected byte,
+        it will return early to its caller.  The expectation is that the caller
+        will reload the byte from memory and call the 8-bit CAS again.
+
+        Unfortunately, this code path that returns early does not have a
+        compiler fence.  Without a compiler fence, the C++ compiler can
+        optimize away the reloading of the expected byte value, leaving it
+        unchanged.  As a result, we'll have a infinite loop here that checks a
+        value that will never change, and the loop will not terminate until the
+        value changes.
+
+        The fix is to eliminate the early return check in the 8-bit CAS, and
+        have it always call down to the 32-bit CAS.  The 32-bit CAS has a
+        compiler fence which will prevent this issue.
+
+        * wtf/Atomics.h:
+        (WTF::weakCompareAndSwap):
+
 2014-10-22  Byungseon Shin  <sun.shin@lge.com>
 
         String(new Date(Mar 30 2014 01:00:00)) is wrong in CET

Source/WTF/wtf/Atomics.h

@@ -292,16 +292,24 @@ inline bool weakCompareAndSwap(uint8_t* location, uint8_t expected, uint8_t newV
     unsigned* alignedLocation = bitwise_cast<unsigned*>(alignedLocationValue);
     // Make sure that this load is always issued and never optimized away.
     unsigned oldAlignedValue = *const_cast<volatile unsigned*>(alignedLocation);
-    union {
-        uint8_t bytes[sizeof(unsigned)];
-        unsigned word;
-    } u;
-    u.word = oldAlignedValue;
-    if (u.bytes[locationOffset] != expected)
-        return false;
-    u.bytes[locationOffset] = newValue;
-    unsigned newAlignedValue = u.word;
-    return weakCompareAndSwap(alignedLocation, oldAlignedValue, newAlignedValue);
+
+    struct Splicer {
+        static unsigned splice(unsigned value, uint8_t byte, uintptr_t byteIndex)
+        {
+            union {
+                unsigned word;
+                uint8_t bytes[sizeof(unsigned)];
+            } u;
+            u.word = value;
+            u.bytes[byteIndex] = byte;
+            return u.word;
+        }
+    };
+
+    unsigned expectedAlignedValue = Splicer::splice(oldAlignedValue, expected, locationOffset);
+    unsigned newAlignedValue = Splicer::splice(oldAlignedValue, newValue, locationOffset);
+
+    return weakCompareAndSwap(alignedLocation, expectedAlignedValue, newAlignedValue);
 #endif
 #else
     UNUSED_PARAM(location);