Bug 1146817 - Correct order of arguments to _InterlockedCompareExchange in AtomicOperations. Align test data. Add alignment assertions to x86 and x64. r=me
authorLars T Hansen <lhansen@mozilla.com>
Mon, 04 Sep 2017 16:04:56 +0200
changeset 428361 cd4cb832958672a9b3d068d5f59cc946a2fe66f3
parent 428360 8822dc7f2d3ccc881625676aee1a907365494af4
child 428362 9b9159079eea2092d5914e8f263135a79e92cfe5
push id7761
push userjlund@mozilla.com
push dateFri, 15 Sep 2017 00:19:52 +0000
treeherdermozilla-beta@c38455951db4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersme
bugs1146817
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1146817 - Correct order of arguments to _InterlockedCompareExchange in AtomicOperations. Align test data. Add alignment assertions to x86 and x64. r=me
js/src/jit/x86-shared/AtomicOperations-x86-shared-gcc.h
js/src/jit/x86-shared/AtomicOperations-x86-shared-msvc.h
js/src/jsapi-tests/testAtomicOperations.cpp
--- a/js/src/jit/x86-shared/AtomicOperations-x86-shared-gcc.h
+++ b/js/src/jit/x86-shared/AtomicOperations-x86-shared-gcc.h
@@ -79,107 +79,124 @@ js::jit::AtomicOperations::isLockfree8()
 
 inline void
 js::jit::AtomicOperations::fenceSeqCst()
 {
     __atomic_thread_fence(__ATOMIC_SEQ_CST);
 }
 
 template<typename T>
+static bool
+IsAligned(const T* addr)
+{
+    return (uintptr_t(addr) & (sizeof(T) - 1)) == 0;
+}
+
+template<typename T>
 inline T
 js::jit::AtomicOperations::loadSeqCst(T* addr)
 {
     static_assert(sizeof(T) <= 8, "atomics supported up to 8 bytes only");
     MOZ_ASSERT_IF(sizeof(T) == 8, isLockfree8());
+    MOZ_ASSERT(IsAligned(addr));
     T v;
     __atomic_load(addr, &v, __ATOMIC_SEQ_CST);
     return v;
 }
 
 template<typename T>
 inline void
 js::jit::AtomicOperations::storeSeqCst(T* addr, T val)
 {
     static_assert(sizeof(T) <= 8, "atomics supported up to 8 bytes only");
     MOZ_ASSERT_IF(sizeof(T) == 8, isLockfree8());
+    MOZ_ASSERT(IsAligned(addr));
     __atomic_store(addr, &val, __ATOMIC_SEQ_CST);
 }
 
 template<typename T>
 inline T
 js::jit::AtomicOperations::exchangeSeqCst(T* addr, T val)
 {
     static_assert(sizeof(T) <= 8, "atomics supported up to 8 bytes only");
     MOZ_ASSERT_IF(sizeof(T) == 8, isLockfree8());
+    MOZ_ASSERT(IsAligned(addr));
     T v;
     __atomic_exchange(addr, &val, &v, __ATOMIC_SEQ_CST);
     return v;
 }
 
 template<typename T>
 inline T
 js::jit::AtomicOperations::compareExchangeSeqCst(T* addr, T oldval, T newval)
 {
     static_assert(sizeof(T) <= 8, "atomics supported up to 8 bytes only");
     MOZ_ASSERT_IF(sizeof(T) == 8, isLockfree8());
+    MOZ_ASSERT(IsAligned(addr));
     __atomic_compare_exchange(addr, &oldval, &newval, false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST);
     return oldval;
 }
 
 template<typename T>
 inline T
 js::jit::AtomicOperations::fetchAddSeqCst(T* addr, T val)
 {
     static_assert(sizeof(T) <= 8, "atomics supported up to 8 bytes only");
     MOZ_ASSERT_IF(sizeof(T) == 8, isLockfree8());
+    MOZ_ASSERT(IsAligned(addr));
     return __atomic_fetch_add(addr, val, __ATOMIC_SEQ_CST);
 }
 
 template<typename T>
 inline T
 js::jit::AtomicOperations::fetchSubSeqCst(T* addr, T val)
 {
     static_assert(sizeof(T) <= 8, "atomics supported up to 8 bytes only");
     MOZ_ASSERT_IF(sizeof(T) == 8, isLockfree8());
+    MOZ_ASSERT(IsAligned(addr));
     return __atomic_fetch_sub(addr, val, __ATOMIC_SEQ_CST);
 }
 
 template<typename T>
 inline T
 js::jit::AtomicOperations::fetchAndSeqCst(T* addr, T val)
 {
     static_assert(sizeof(T) <= 8, "atomics supported up to 8 bytes only");
     MOZ_ASSERT_IF(sizeof(T) == 8, isLockfree8());
+    MOZ_ASSERT(IsAligned(addr));
     return __atomic_fetch_and(addr, val, __ATOMIC_SEQ_CST);
 }
 
 template<typename T>
 inline T
 js::jit::AtomicOperations::fetchOrSeqCst(T* addr, T val)
 {
     static_assert(sizeof(T) <= 8, "atomics supported up to 8 bytes only");
     MOZ_ASSERT_IF(sizeof(T) == 8, isLockfree8());
+    MOZ_ASSERT(IsAligned(addr));
     return __atomic_fetch_or(addr, val, __ATOMIC_SEQ_CST);
 }
 
 template<typename T>
 inline T
 js::jit::AtomicOperations::fetchXorSeqCst(T* addr, T val)
 {
     static_assert(sizeof(T) <= 8, "atomics supported up to 8 bytes only");
     MOZ_ASSERT_IF(sizeof(T) == 8, isLockfree8());
+    MOZ_ASSERT(IsAligned(addr));
     return __atomic_fetch_xor(addr, val, __ATOMIC_SEQ_CST);
 }
 
 template<typename T>
 inline T
 js::jit::AtomicOperations::loadSafeWhenRacy(T* addr)
 {
     static_assert(sizeof(T) <= 8, "atomics supported up to 8 bytes only");
     MOZ_ASSERT_IF(sizeof(T) == 8, isLockfree8());
+    MOZ_ASSERT(IsAligned(addr));
     T v;
     __atomic_load(addr, &v, __ATOMIC_RELAXED);
     return v;
 }
 
 namespace js { namespace jit {
 
 template<>
@@ -194,16 +211,17 @@ js::jit::AtomicOperations::loadSafeWhenR
 } }
 
 template<typename T>
 inline void
 js::jit::AtomicOperations::storeSafeWhenRacy(T* addr, T val)
 {
     static_assert(sizeof(T) <= 8, "atomics supported up to 8 bytes only");
     MOZ_ASSERT_IF(sizeof(T) == 8, isLockfree8());
+    MOZ_ASSERT(IsAligned(addr));
     __atomic_store(addr, &val, __ATOMIC_RELAXED);
 }
 
 namespace js { namespace jit {
 
 template<>
 inline void
 js::jit::AtomicOperations::storeSafeWhenRacy(uint8_clamped* addr, uint8_clamped val)
--- a/js/src/jit/x86-shared/AtomicOperations-x86-shared-msvc.h
+++ b/js/src/jit/x86-shared/AtomicOperations-x86-shared-msvc.h
@@ -29,16 +29,19 @@
 // higher level of function which uses the intrinsic when possible (8, 16, and
 // 32-bit operations, and 64-bit operations on 64-bit systems) and otherwise
 // falls back on CMPXCHG8B for 64-bit operations on 32-bit systems.  We could be
 // using those functions in many cases here (though not all).  I have not done
 // so because (a) I don't yet know how far back those functions are supported
 // and (b) I expect we'll end up dropping into assembler here eventually so as
 // to guarantee that the C++ compiler won't optimize the code.
 
+// Note, _InterlockedCompareExchange takes the *new* value as the second argument
+// and the *comparand* (expected old value) as the third argument.
+
 inline bool
 js::jit::AtomicOperations::hasAtomic8()
 {
     return true;
 }
 
 inline bool
 js::jit::AtomicOperations::isLockfree8()
@@ -57,33 +60,42 @@ js::jit::AtomicOperations::isLockfree8()
 inline void
 js::jit::AtomicOperations::fenceSeqCst()
 {
     _ReadWriteBarrier();
     _mm_mfence();
 }
 
 template<typename T>
+static bool
+IsAligned(const T* addr)
+{
+    return (uintptr_t(addr) & (sizeof(T) - 1)) == 0;
+}
+
+template<typename T>
 inline T
 js::jit::AtomicOperations::loadSeqCst(T* addr)
 {
     static_assert(sizeof(T) <= 8, "atomics supported up to 8 bytes only");
+    MOZ_ASSERT(IsAligned(addr));
     _ReadWriteBarrier();
     T v = *addr;
     _ReadWriteBarrier();
     return v;
 }
 
 #ifdef _M_IX86
 namespace js { namespace jit {
 
 # define MSC_LOADOP(T)                      \
     template<>                              \
     inline T                                \
     AtomicOperations::loadSeqCst(T* addr) { \
+        MOZ_ASSERT(IsAligned(addr));        \
         _ReadWriteBarrier();                \
         return (T)_InterlockedCompareExchange64((__int64 volatile*)addr, 0, 0); \
     }
 
 MSC_LOADOP(int64_t)
 MSC_LOADOP(uint64_t)
 
 # undef MSC_LOADOP
@@ -91,34 +103,36 @@ MSC_LOADOP(uint64_t)
 } }
 #endif // _M_IX86
 
 template<typename T>
 inline void
 js::jit::AtomicOperations::storeSeqCst(T* addr, T val)
 {
     static_assert(sizeof(T) <= 8, "atomics supported up to 8 bytes only");
+    MOZ_ASSERT(IsAligned(addr));
     _ReadWriteBarrier();
     *addr = val;
     fenceSeqCst();
 }
 
 #ifdef _M_IX86
 namespace js { namespace jit {
 
 # define MSC_STOREOP(T)                              \
     template<>                                      \
     inline void                                     \
     AtomicOperations::storeSeqCst(T* addr, T val) { \
+        MOZ_ASSERT(IsAligned(addr));                \
         _ReadWriteBarrier();                        \
         T oldval = *addr;                           \
         for (;;) {                                  \
             T nextval = (T)_InterlockedCompareExchange64((__int64 volatile*)addr, \
-                                                         (__int64)oldval,         \
-                                                         (__int64)val);           \
+                                                         (__int64)val,            \
+                                                         (__int64)oldval);        \
             if (nextval == oldval)                  \
                 break;                              \
             oldval = nextval;                       \
         }                                           \
         _ReadWriteBarrier();                        \
     }
 
 MSC_STOREOP(int64_t)
@@ -128,30 +142,32 @@ MSC_STOREOP(uint64_t)
 
 } }
 #endif // _M_IX86
 
 #define MSC_EXCHANGEOP(T, U, xchgop)                            \
     template<> inline T                                         \
     AtomicOperations::exchangeSeqCst(T* addr, T val) {          \
         static_assert(sizeof(T) <= 8, "atomics supported up to 8 bytes only"); \
+        MOZ_ASSERT(IsAligned(addr));                            \
         return (T)xchgop((U volatile*)addr, (U)val);            \
     }
 
 #ifdef _M_IX86
 # define MSC_EXCHANGEOP_CAS(T)                                       \
     template<> inline T                                              \
     AtomicOperations::exchangeSeqCst(T* addr, T val) {               \
         static_assert(sizeof(T) == 8, "8-byte variant");             \
+        MOZ_ASSERT(IsAligned(addr));                                 \
         _ReadWriteBarrier();                                         \
         T oldval = *addr;                                            \
         for (;;) {                                                   \
             T nextval = (T)_InterlockedCompareExchange64((__int64 volatile*)addr, \
-                                                         (__int64)oldval,         \
-                                                         (__int64)val);           \
+                                                         (__int64)val,            \
+                                                         (__int64)oldval);        \
             if (nextval == oldval)                                   \
                 break;                                               \
             oldval = nextval;                                        \
         }                                                            \
         _ReadWriteBarrier();                                         \
         return oldval;                                               \
     }
 #endif // _M_IX86
@@ -177,16 +193,17 @@ MSC_EXCHANGEOP(uint64_t, __int64, _Inter
 
 #undef MSC_EXCHANGEOP
 #undef MSC_EXCHANGEOP_CAS
 
 #define MSC_CAS(T, U, cmpxchg)                                          \
     template<> inline T                                                 \
     AtomicOperations::compareExchangeSeqCst(T* addr, T oldval, T newval) { \
         static_assert(sizeof(T) <= 8, "atomics supported up to 8 bytes only"); \
+        MOZ_ASSERT(IsAligned(addr));                                    \
         return (T)cmpxchg((U volatile*)addr, (U)newval, (U)oldval);     \
     }
 
 namespace js { namespace jit {
 
 MSC_CAS(int8_t, char, _InterlockedCompareExchange8)
 MSC_CAS(uint8_t, char, _InterlockedCompareExchange8)
 MSC_CAS(int16_t, short, _InterlockedCompareExchange16)
@@ -200,36 +217,38 @@ MSC_CAS(uint64_t, __int64, _InterlockedC
 
 #undef MSC_CAS
 
 #define MSC_FETCHADDOP(T, U, xadd)                                      \
     template<> inline T                                                 \
     AtomicOperations::fetchAddSeqCst(T* addr, T val) {                  \
         static_assert(sizeof(T) <= 8,                                   \
                       "atomics supported up to 8 bytes only");          \
+        MOZ_ASSERT(IsAligned(addr));                                    \
         return (T)xadd((U volatile*)addr, (U)val);                      \
     }                                                                   \
 
 #define MSC_FETCHSUBOP(T)                                            \
     template<> inline T                                              \
     AtomicOperations::fetchSubSeqCst(T* addr, T val) {               \
         return fetchAddSeqCst(addr, (T)(0-val));                     \
     }
 
 #ifdef _M_IX86
 # define MSC_FETCHADDOP_CAS(T)                                       \
     template<> inline T                                              \
     AtomicOperations::fetchAddSeqCst(T* addr, T val) {               \
         static_assert(sizeof(T) == 8, "8-byte variant");             \
+        MOZ_ASSERT(IsAligned(addr));                                 \
         _ReadWriteBarrier();                                         \
         T oldval = *addr;                                            \
         for (;;) {                                                   \
             T nextval = (T)_InterlockedCompareExchange64((__int64 volatile*)addr, \
-                                                         (__int64)oldval, \
-                                                         (__int64)(oldval + val)); \
+                                                         (__int64)(oldval + val), \
+                                                         (__int64)oldval);        \
             if (nextval == oldval)                                   \
                 break;                                               \
             oldval = nextval;                                        \
         }                                                            \
         _ReadWriteBarrier();                                         \
         return oldval;                                               \
     }
 #endif // _M_IX86
@@ -266,38 +285,40 @@ MSC_FETCHSUBOP(uint64_t)
 #undef MSC_FETCHADDOP_CAS
 #undef MSC_FETCHSUBOP
 
 #define MSC_FETCHBITOPX(T, U, name, op)                                 \
     template<> inline T                                                 \
     AtomicOperations::name(T* addr, T val) {                            \
         static_assert(sizeof(T) <= 8,                                   \
                       "atomics supported up to 8 bytes only");          \
+        MOZ_ASSERT(IsAligned(addr));                                    \
         return (T)op((U volatile*)addr, (U)val);                        \
     }
 
 #define MSC_FETCHBITOP(T, U, andop, orop, xorop)                        \
     MSC_FETCHBITOPX(T, U, fetchAndSeqCst, andop)                        \
     MSC_FETCHBITOPX(T, U, fetchOrSeqCst, orop)                          \
     MSC_FETCHBITOPX(T, U, fetchXorSeqCst, xorop)
 
 #ifdef _M_IX86
 # define AND_OP &
 # define OR_OP |
 # define XOR_OP ^
 # define MSC_FETCHBITOPX_CAS(T, name, OP)                            \
     template<> inline T                                              \
     AtomicOperations::name(T* addr, T val) {                         \
         static_assert(sizeof(T) == 8, "8-byte variant");             \
+        MOZ_ASSERT(IsAligned(addr));                                 \
         _ReadWriteBarrier();                                         \
         T oldval = *addr;                                            \
         for (;;) {                                                   \
-            T nextval = (T)_InterlockedCompareExchange64((__int64 volatile*)addr, \
-                                                         (__int64)oldval, \
-                                                         (__int64)(oldval OP val)); \
+            T nextval = (T)_InterlockedCompareExchange64((__int64 volatile*)addr,  \
+                                                         (__int64)(oldval OP val), \
+                                                         (__int64)oldval);         \
             if (nextval == oldval)                                   \
                 break;                                               \
             oldval = nextval;                                        \
         }                                                            \
         _ReadWriteBarrier();                                         \
         return oldval;                                               \
     }
 
@@ -331,24 +352,26 @@ MSC_FETCHBITOP(uint64_t, __int64, _Inter
 #undef MSC_FETCHBITOPX
 #undef MSC_FETCHBITOP_CAS
 #undef MSC_FETCHBITOP
 
 template<typename T>
 inline T
 js::jit::AtomicOperations::loadSafeWhenRacy(T* addr)
 {
+    MOZ_ASSERT(IsAligned(addr));
     return *addr;
 }
 
 #ifdef _M_IX86
 # define MSC_RACYLOADOP(T)                        \
     template<>                                    \
     inline T                                      \
     AtomicOperations::loadSafeWhenRacy(T* addr) { \
+        MOZ_ASSERT(IsAligned(addr));              \
         return (T)_InterlockedCompareExchange64((__int64 volatile*)addr, 0, 0); \
     }
 
 namespace js { namespace jit {
 
 MSC_RACYLOADOP(int64_t)
 MSC_RACYLOADOP(uint64_t)
 
@@ -356,31 +379,33 @@ MSC_RACYLOADOP(uint64_t)
 
 # undef MSC_RACYLOADOP
 #endif // _M_IX86
 
 template<typename T>
 inline void
 js::jit::AtomicOperations::storeSafeWhenRacy(T* addr, T val)
 {
+    MOZ_ASSERT(IsAligned(addr));
     *addr = val;
 }
 
 #ifdef _M_IX86
 namespace js { namespace jit {
 
 # define MSC_RACYSTOREOP(T)                               \
     template<>                                            \
     inline void                                           \
     AtomicOperations::storeSafeWhenRacy(T* addr, T val) { \
+        MOZ_ASSERT(IsAligned(addr));                      \
         T oldval = *addr;                                 \
         for (;;) {                                        \
             T nextval = (T)_InterlockedCompareExchange64((__int64 volatile*)addr, \
-                                                         (__int64)oldval,         \
-                                                         (__int64)val);           \
+                                                         (__int64)val,            \
+                                                         (__int64)oldval);        \
             if (nextval == oldval)                        \
                 break;                                    \
             oldval = nextval;                             \
         }                                                 \
     }
 
 MSC_RACYSTOREOP(int64_t)
 MSC_RACYSTOREOP(uint64_t)
--- a/js/src/jsapi-tests/testAtomicOperations.cpp
+++ b/js/src/jsapi-tests/testAtomicOperations.cpp
@@ -1,15 +1,16 @@
 /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-
 * vim: set ts=8 sts=4 et sw=4 tw=99:
 */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+#include "mozilla/Alignment.h"
 #include "mozilla/Assertions.h"
 
 #include "jit/AtomicOperations.h"
 #include "jsapi-tests/tests.h"
 #include "vm/SharedMem.h"
 #include "wasm/WasmJS.h"
 
 using namespace js;
@@ -78,20 +79,21 @@ END_TEST(testAtomicFence)
 //////////////////////////////////////////////////////////////////////
 //
 // Memory access primitives
 
 // These tests for the atomic load and store primitives ascertain that the
 // primitives are defined and that they load and store the values they should,
 // but not that the primitives are actually atomic wrt to the memory subsystem.
 
-// Memory for testing atomics
+// Memory for testing atomics.  This must be aligned to the natural alignment of
+// the type we're testing; for now, use 8-byte alignment for all.
 
-static uint8_t atomicMem[8];
-static uint8_t atomicMem2[8];
+MOZ_ALIGNED_DECL(static uint8_t atomicMem[8], 8);
+MOZ_ALIGNED_DECL(static uint8_t atomicMem2[8], 8);
 
 // T is the primitive type we're testing, and A and B are references to constant
 // bindings holding values of that type.
 //
 // No bytes of A and B should be 0 or FF.  A+B and A-B must not overflow.
 
 #define ATOMIC_TESTS(T, A, B) \
     T* q = (T*)hidePointerValue((void*)atomicMem);                      \