From 1a6e1dbff295e710c3b7e1ed6d1f9789b61a6e2a Mon Sep 17 00:00:00 2001 From: hogliux Date: Thu, 8 Jun 2017 16:45:49 +0100 Subject: [PATCH] Result of pointer arithmetic should not depend on if the pointer is being wrapped in a JUCE Atomic or not (breaking change: see https://github.com/WeAreROLI/JUCE/blob/develop/BREAKING-CHANGES.txt --- BREAKING-CHANGES.txt | 25 +++ modules/juce_core/memory/juce_Atomic.h | 179 +++++++++++++++------- modules/juce_core/threads/juce_Thread.cpp | 47 ++++-- 3 files changed, 181 insertions(+), 70 deletions(-) diff --git a/BREAKING-CHANGES.txt b/BREAKING-CHANGES.txt index c62a0e838b..10cb8ca884 100644 --- a/BREAKING-CHANGES.txt +++ b/BREAKING-CHANGES.txt @@ -1,6 +1,31 @@ JUCE breaking changes ===================== +Develop Branch +============== + +Change +------ +Pointer arithmetic on a pointer will have the same result regardless if it is wrapped in JUCE's Atomic class or not + +Possible Issues +--------------- +Any code using pointer arithmetic on Atomic will now have a different result leading to undefined behaviour or crashes. + +Workaround +---------- +Re-write your code in a way that it does not depend on your pointer being wrapped in JUCE's Atomic or not. See rationale. + +Rationale +--------- +Before this change, pointer arithmetic with JUCE's Atomic type would yield confusing results. For example, the following code would assert before this change: + +int* a; Atomic b; + +jassert (++a == ++b); + +Pointer a in the above code would be advanced by sizeof(int) whereas the JUCE's Atomic always advances it's underlying pointer by a single byte. The same is true for operator+=/operator-= and operator--. The difference in behaviour is confusing and unintuitive. Furthermore, this aligns JUCE's Atomic type with std::atomic. + Version 4.3.1 ============= diff --git a/modules/juce_core/memory/juce_Atomic.h b/modules/juce_core/memory/juce_Atomic.h index a1273f4d9e..eb6354d573 100644 --- a/modules/juce_core/memory/juce_Atomic.h +++ b/modules/juce_core/memory/juce_Atomic.h @@ -22,6 +22,10 @@ #pragma once +//============================================================================== +#ifndef DOXYGEN + template class AtomicBase; +#endif //============================================================================== /** @@ -31,26 +35,20 @@ There are methods to perform most of the basic atomic operations. */ template -class Atomic +class Atomic : public AtomicBase { public: + /** Resulting type when subtracting the underlying Type. */ + typedef typename AtomicBase::DiffType DiffType; + /** Creates a new value, initialised to zero. */ - inline Atomic() noexcept - : value (0) - { - } + inline Atomic() noexcept {} /** Creates a new value, with a given initial value. */ - inline explicit Atomic (const Type initialValue) noexcept - : value (initialValue) - { - } + inline explicit Atomic (const Type initialValue) noexcept : AtomicBase (initialValue) {} /** Copies another value (atomically). */ - inline Atomic (const Atomic& other) noexcept - : value (other.get()) - { - } + inline Atomic (const Atomic& other) noexcept : AtomicBase (other) {} /** Destructor. */ inline ~Atomic() noexcept @@ -60,25 +58,25 @@ public: } /** Atomically reads and returns the current value. */ - Type get() const noexcept; + inline Type get() const noexcept { return AtomicBase::get(); } /** Copies another value onto this one (atomically). */ - inline Atomic& operator= (const Atomic& other) noexcept { exchange (other.get()); return *this; } + inline Atomic& operator= (const Atomic& other) noexcept { AtomicBase::operator= (other); return *this; } /** Copies another value onto this one (atomically). */ - inline Atomic& operator= (const Type newValue) noexcept { exchange (newValue); return *this; } + inline Atomic& operator= (const Type newValue) noexcept { AtomicBase::operator= (newValue); return *this; } /** Atomically sets the current value. */ - void set (Type newValue) noexcept { exchange (newValue); } + inline void set (Type newValue) noexcept { exchange (newValue); } /** Atomically sets the current value, returning the value that was replaced. */ - Type exchange (Type value) noexcept; + inline Type exchange (Type v) noexcept { return AtomicBase::exchange (v); } /** Atomically adds a number to this value, returning the new value. */ - Type operator+= (Type amountToAdd) noexcept; + Type operator+= (DiffType amountToAdd) noexcept; /** Atomically subtracts a number from this value, returning the new value. */ - Type operator-= (Type amountToSubtract) noexcept; + Type operator-= (DiffType amountToSubtract) noexcept; /** Atomically increments this value, returning the new value. */ Type operator++() noexcept; @@ -107,7 +105,7 @@ public: the comparison failed and the value was left unchanged. @see compareAndSetValue */ - bool compareAndSetBool (Type newValue, Type valueToCompare) noexcept; + inline bool compareAndSetBool (Type newValue, Type valueToCompare) noexcept { return AtomicBase::compareAndSetBool (newValue, valueToCompare); } /** Atomically compares this value with a target value, and if it is equal, sets this to be equal to a new value. @@ -127,9 +125,36 @@ public: @returns the old value before it was changed. @see compareAndSetBool */ - Type compareAndSetValue (Type newValue, Type valueToCompare) noexcept; + inline Type compareAndSetValue (Type newValue, Type valueToCompare) noexcept { return AtomicBase::compareAndSetValue (newValue, valueToCompare); } /** Implements a memory read/write barrier. */ + static inline void memoryBarrier() noexcept { AtomicBase::memoryBarrier (); } +}; + +#ifndef DOXYGEN + +//============================================================================== +// Internal implementation follows +//============================================================================== +template struct DiffTypeHelper { typedef T Type; }; +template struct DiffTypeHelper { typedef std::ptrdiff_t Type; }; + +template +class AtomicBase +{ +public: + typedef typename DiffTypeHelper::Type DiffType; + + inline AtomicBase() noexcept : value (0) {} + inline explicit AtomicBase (const Type v) noexcept : value (v) {} + inline AtomicBase (const AtomicBase& other) noexcept : value (other.get()) {} + Type get() const noexcept; + inline AtomicBase& operator= (const AtomicBase& other) noexcept { exchange (other.get()); return *this; } + inline AtomicBase& operator= (const Type newValue) noexcept { exchange (newValue); return *this; } + void set (Type newValue) noexcept { exchange (newValue); } + Type exchange (Type) noexcept; + bool compareAndSetBool (Type, Type) noexcept; + Type compareAndSetValue (Type, Type) noexcept; static void memoryBarrier() noexcept; //============================================================================== @@ -145,7 +170,7 @@ public: */ volatile Type value; -private: +protected: template static inline Dest castTo (Source value) noexcept { union { Dest d; Source s; } u; u.s = value; return u.d; } @@ -175,6 +200,24 @@ private: } }; +//============================================================================== +// Specialisation for void* which does not include the pointer arithmetic +template <> +class Atomic : public AtomicBase +{ +public: + inline Atomic() noexcept {} + inline explicit Atomic (void* const initialValue) noexcept : AtomicBase (initialValue) {} + inline Atomic (const Atomic& other) noexcept : AtomicBase (other) {} + inline void* get() const noexcept { return AtomicBase::get(); } + inline Atomic& operator= (const Atomic& other) noexcept { AtomicBase::operator= (other); return *this; } + inline Atomic& operator= (void* const newValue) noexcept { AtomicBase::operator= (newValue); return *this; } + inline void set (void* newValue) noexcept { exchange (newValue); } + inline void* exchange (void* v) noexcept { return AtomicBase::exchange (v); } + inline bool compareAndSetBool (void* newValue, void* valueToCompare) noexcept { return AtomicBase::compareAndSetBool (newValue, valueToCompare); } + inline void* compareAndSetValue (void* newValue, void* valueToCompare) noexcept { return AtomicBase::compareAndSetValue (newValue, valueToCompare); } + static inline void memoryBarrier() noexcept { AtomicBase::memoryBarrier(); } +}; //============================================================================== /* @@ -302,9 +345,46 @@ private: #pragma warning (disable: 4311) // (truncation warning) #endif +template +struct AtomicIncrementDecrement +{ + static inline Type inc (AtomicBase& a) noexcept + { + #if JUCE_ATOMICS_MAC_LEGACY + return sizeof (Type) == 4 ? (Type) OSAtomicIncrement32Barrier ((volatile int32_t*) &a.value) + : (Type) OSAtomicIncrement64Barrier ((volatile int64_t*) &a.value); + #elif JUCE_ATOMICS_WINDOWS + return WindowsInterlockedHelpers::inc (&a.value); + #elif JUCE_ATOMICS_GCC + return sizeof (Type) == 4 ? (Type) __sync_add_and_fetch (& (a.value), (Type) 1) + : (Type) __sync_add_and_fetch ((int64_t*) & (a.value), 1); + #endif + } + + static inline Type dec (AtomicBase& a) noexcept + { + #if JUCE_ATOMICS_MAC_LEGACY + return sizeof (Type) == 4 ? (Type) OSAtomicDecrement32Barrier ((volatile int32_t*) &a.value) + : (Type) OSAtomicDecrement64Barrier ((volatile int64_t*) &a.value); + #elif JUCE_ATOMICS_WINDOWS + return WindowsInterlockedHelpers::dec (&a.value); + #elif JUCE_ATOMICS_GCC + return sizeof (Type) == 4 ? (Type) __sync_add_and_fetch (& (a.value), (Type) -1) + : (Type) __sync_add_and_fetch ((int64_t*) & (a.value), -1); + #endif + } +}; + +template +struct AtomicIncrementDecrement +{ + static inline Type* inc (Atomic& a) noexcept { return a.operator+= (1); } + static inline Type* dec (Atomic& a) noexcept { return a.operator-= (1); } +}; + //============================================================================== template -inline Type Atomic::get() const noexcept +inline Type AtomicBase::get() const noexcept { #if JUCE_ATOMICS_MAC_LEGACY return sizeof (Type) == 4 ? castFrom32Bit ((int32) OSAtomicAdd32Barrier ((int32_t) 0, (volatile int32_t*) &value)) @@ -318,7 +398,7 @@ inline Type Atomic::get() const noexcept } template -inline Type Atomic::exchange (const Type newValue) noexcept +inline Type AtomicBase::exchange (const Type newValue) noexcept { #if JUCE_ATOMICS_MAC_LEGACY || JUCE_ATOMICS_GCC Type currentVal = value; @@ -330,54 +410,34 @@ inline Type Atomic::exchange (const Type newValue) noexcept } template -inline Type Atomic::operator+= (const Type amountToAdd) noexcept +inline Type Atomic::operator+= (const DiffType amountToAdd) noexcept { + Type amount = (Type() + amountToAdd); + #if JUCE_ATOMICS_MAC_LEGACY - return sizeof (Type) == 4 ? (Type) OSAtomicAdd32Barrier ((int32_t) castTo32Bit (amountToAdd), (volatile int32_t*) &value) - : (Type) OSAtomicAdd64Barrier ((int64_t) amountToAdd, (volatile int64_t*) &value); + return sizeof (Type) == 4 ? (Type) OSAtomicAdd32Barrier ((int32_t) castTo32Bit (amount), (volatile int32_t*) &AtomicBase::value) + : (Type) OSAtomicAdd64Barrier ((int64_t) amount, (volatile int64_t*) &AtomicBase::value); #elif JUCE_ATOMICS_WINDOWS - return WindowsInterlockedHelpers::add (&value, amountToAdd); + return WindowsInterlockedHelpers::add (& (AtomicBase::value), amount); #elif JUCE_ATOMICS_GCC - return (Type) __sync_add_and_fetch (&value, amountToAdd); + return (Type) __sync_add_and_fetch (& (AtomicBase::value), amount); #endif } template -inline Type Atomic::operator-= (const Type amountToSubtract) noexcept +inline Type Atomic::operator-= (const DiffType amountToSubtract) noexcept { - return operator+= (negateValue (amountToSubtract)); + return operator+= (AtomicBase::negateValue (amountToSubtract)); } template -inline Type Atomic::operator++() noexcept -{ - #if JUCE_ATOMICS_MAC_LEGACY - return sizeof (Type) == 4 ? (Type) OSAtomicIncrement32Barrier ((volatile int32_t*) &value) - : (Type) OSAtomicIncrement64Barrier ((volatile int64_t*) &value); - #elif JUCE_ATOMICS_WINDOWS - return WindowsInterlockedHelpers::inc (&value); - #elif JUCE_ATOMICS_GCC - return sizeof (Type) == 4 ? (Type) __sync_add_and_fetch (&value, (Type) 1) - : (Type) __sync_add_and_fetch ((int64_t*) &value, 1); - #endif -} +inline Type Atomic::operator++() noexcept { return AtomicIncrementDecrement::inc (*this); } template -inline Type Atomic::operator--() noexcept -{ - #if JUCE_ATOMICS_MAC_LEGACY - return sizeof (Type) == 4 ? (Type) OSAtomicDecrement32Barrier ((volatile int32_t*) &value) - : (Type) OSAtomicDecrement64Barrier ((volatile int64_t*) &value); - #elif JUCE_ATOMICS_WINDOWS - return WindowsInterlockedHelpers::dec (&value); - #elif JUCE_ATOMICS_GCC - return sizeof (Type) == 4 ? (Type) __sync_add_and_fetch (&value, (Type) -1) - : (Type) __sync_add_and_fetch ((int64_t*) &value, -1); - #endif -} +inline Type Atomic::operator--() noexcept { return AtomicIncrementDecrement::dec (*this); } template -inline bool Atomic::compareAndSetBool (const Type newValue, const Type valueToCompare) noexcept +inline bool AtomicBase::compareAndSetBool (const Type newValue, const Type valueToCompare) noexcept { #if JUCE_ATOMICS_MAC_LEGACY return sizeof (Type) == 4 ? OSAtomicCompareAndSwap32Barrier ((int32_t) castTo32Bit (valueToCompare), (int32_t) castTo32Bit (newValue), (volatile int32_t*) &value) @@ -391,7 +451,7 @@ inline bool Atomic::compareAndSetBool (const Type newValue, const Type val } template -inline Type Atomic::compareAndSetValue (const Type newValue, const Type valueToCompare) noexcept +inline Type AtomicBase::compareAndSetValue (const Type newValue, const Type valueToCompare) noexcept { #if JUCE_ATOMICS_MAC_LEGACY for (;;) // Annoying workaround for only having a bool CAS operation.. @@ -413,7 +473,7 @@ inline Type Atomic::compareAndSetValue (const Type newValue, const Type va } template -inline void Atomic::memoryBarrier() noexcept +inline void AtomicBase::memoryBarrier() noexcept { #if JUCE_ATOMICS_MAC_LEGACY OSMemoryBarrier(); @@ -427,3 +487,4 @@ inline void Atomic::memoryBarrier() noexcept #if JUCE_MSVC #pragma warning (pop) #endif +#endif diff --git a/modules/juce_core/threads/juce_Thread.cpp b/modules/juce_core/threads/juce_Thread.cpp index 64f50a4630..c0e30413d4 100644 --- a/modules/juce_core/threads/juce_Thread.cpp +++ b/modules/juce_core/threads/juce_Thread.cpp @@ -308,8 +308,6 @@ public: AtomicTester ::testInteger (*this); beginTest ("Atomic long"); AtomicTester ::testInteger (*this); - beginTest ("Atomic void*"); - AtomicTester ::testInteger (*this); beginTest ("Atomic int*"); AtomicTester ::testInteger (*this); beginTest ("Atomic float"); @@ -322,6 +320,21 @@ public: beginTest ("Atomic double"); AtomicTester ::testFloat (*this); #endif + beginTest ("Atomic pointer increment/decrement"); + Atomic a (a2); int* b (a2); + expect (++a == ++b); + + { + beginTest ("Atomic void*"); + Atomic atomic; + void* c; + + atomic.set ((void*) 10); + c = (void*) 10; + + expect (atomic.value == c); + expect (atomic.get() == c); + } } template @@ -333,23 +346,35 @@ public: static void testInteger (UnitTest& test) { Atomic a, b; + Type c; + a.set ((Type) 10); - test.expect (a.value == (Type) 10); - test.expect (a.get() == (Type) 10); - a += (Type) 15; - test.expect (a.get() == (Type) 25); + c = (Type) 10; + + test.expect (a.value == c); + test.expect (a.get() == c); + + a += 15; + c += 15; + test.expect (a.get() == c); a.memoryBarrier(); - a -= (Type) 5; - test.expect (a.get() == (Type) 20); - test.expect (++a == (Type) 21); + + a -= 5; + c -= 5; + test.expect (a.get() == c); + + test.expect (++a == ++c); ++a; - test.expect (--a == (Type) 21); - test.expect (a.get() == (Type) 21); + ++c; + test.expect (--a == --c); + test.expect (a.get() == c); a.memoryBarrier(); testFloat (test); } + + static void testFloat (UnitTest& test) { Atomic a, b;