Merge branch 'tsan-warnings'

Fix a harmless warning from thread sanitizer and make wxAtomicInc() more
useful.

See #22417.

build/aclocal/atomic_builtins.m4

@@ -6,16 +6,13 @@ AC_DEFUN([WX_ATOMIC_BUILTINS],
 [
   AC_REQUIRE([AC_PROG_CC])
   if test -n "$GCC"; then
-    AC_MSG_CHECKING([for __sync_fetch_and_add and __sync_sub_and_fetch builtins])
+    AC_MSG_CHECKING([for __sync_xxx_and_fetch builtins])
     AC_CACHE_VAL(wx_cv_cc_gcc_atomic_builtins, [
       AC_TRY_LINK(
         [],
         [
           unsigned int value=0;
-          /* wxAtomicInc doesn't use return value here */
+          volatile unsigned int r1 = __sync_add_and_fetch(&value, 2);
-          __sync_fetch_and_add(&value, 2);
-          __sync_sub_and_fetch(&value, 1);
-          /* but wxAtomicDec does, so mimic that: */
           volatile unsigned int r2 = __sync_sub_and_fetch(&value, 1);
         ],
         wx_cv_cc_gcc_atomic_builtins=yes,

configure

@@ -25176,8 +25176,8 @@ fi
 
 
   if test -n "$GCC"; then
-    { $as_echo "$as_me:${as_lineno-$LINENO}: checking for __sync_fetch_and_add and __sync_sub_and_fetch builtins" >&5
+    { $as_echo "$as_me:${as_lineno-$LINENO}: checking for __sync_xxx_and_fetch builtins" >&5
-$as_echo_n "checking for __sync_fetch_and_add and __sync_sub_and_fetch builtins... " >&6; }
+$as_echo_n "checking for __sync_xxx_and_fetch builtins... " >&6; }
     if ${wx_cv_cc_gcc_atomic_builtins+:} false; then :
   $as_echo_n "(cached) " >&6
 else
@@ -25190,10 +25190,7 @@ main ()
 {
 
           unsigned int value=0;
-          /* wxAtomicInc doesn't use return value here */
+          volatile unsigned int r1 = __sync_add_and_fetch(&value, 2);
-          __sync_fetch_and_add(&value, 2);
-          __sync_sub_and_fetch(&value, 1);
-          /* but wxAtomicDec does, so mimic that: */
           volatile unsigned int r2 = __sync_sub_and_fetch(&value, 1);
 
   ;

include/wx/atomic.h

@@ -17,10 +17,7 @@
 // get the value of wxUSE_THREADS configuration flag
 #include "wx/defs.h"
 
-// constraints on the various functions:
+// these functions return the new value, after the operation
-//  - wxAtomicDec must return a zero value if the value is zero once
-//  decremented else it must return any non-zero value (the true value is OK
-//  but not necessary).
 
 #if wxUSE_THREADS
 
@@ -31,9 +28,9 @@
 //     http://bugs.mysql.com/bug.php?id=28456
 //     http://golubenco.org/blog/atomic-operations/
 
-inline void wxAtomicInc (wxUint32 &value)
+inline wxUint32 wxAtomicInc (wxUint32 &value)
 {
-    __sync_fetch_and_add(&value, 1);
+    return __sync_add_and_fetch(&value, 1);
 }
 
 inline wxUint32 wxAtomicDec (wxUint32 &value)
@@ -47,9 +44,9 @@ inline wxUint32 wxAtomicDec (wxUint32 &value)
 // include standard Windows headers
 #include "wx/msw/wrapwin.h"
 
-inline void wxAtomicInc (wxUint32 &value)
+inline wxUint32 wxAtomicInc (wxUint32 &value)
 {
-    InterlockedIncrement ((LONG*)&value);
+    return InterlockedIncrement ((LONG*)&value);
 }
 
 inline wxUint32 wxAtomicDec (wxUint32 &value)
@@ -60,9 +57,9 @@ inline wxUint32 wxAtomicDec (wxUint32 &value)
 #elif defined(__DARWIN__)
 
 #include "libkern/OSAtomic.h"
-inline void wxAtomicInc (wxUint32 &value)
+inline wxUint32 wxAtomicInc (wxUint32 &value)
 {
-    OSAtomicIncrement32 ((int32_t*)&value);
+    return OSAtomicIncrement32 ((int32_t*)&value);
 }
 
 inline wxUint32 wxAtomicDec (wxUint32 &value)
@@ -76,7 +73,7 @@ inline wxUint32 wxAtomicDec (wxUint32 &value)
 
 inline void wxAtomicInc (wxUint32 &value)
 {
-    atomic_add_32 ((uint32_t*)&value, 1);
+    return atomic_add_32_nv ((uint32_t*)&value, 1);
 }
 
 inline wxUint32 wxAtomicDec (wxUint32 &value)
@@ -94,7 +91,7 @@ inline wxUint32 wxAtomicDec (wxUint32 &value)
 #else // else of wxUSE_THREADS
 // if no threads are used we can safely use simple ++/--
 
-inline void wxAtomicInc (wxUint32 &value) { ++value; }
+inline wxUint32 wxAtomicInc (wxUint32 &value) { return ++value; }
 inline wxUint32 wxAtomicDec (wxUint32 &value) { return --value; }
 
 #endif // !wxUSE_THREADS
@@ -120,10 +117,10 @@ public:
 
     wxAtomicInt32& operator=(wxInt32 v) { m_value = v; return *this; }
 
-    void Inc()
+    wxInt32 Inc()
     {
         wxCriticalSectionLocker lock(m_locker);
-        ++m_value;
+        return ++m_value;
     }
 
     wxInt32 Dec()
@@ -137,14 +134,14 @@ private:
     wxCriticalSection m_locker;
 };
 
-inline void wxAtomicInc(wxAtomicInt32 &value) { value.Inc(); }
+inline wxInt32 wxAtomicInc(wxAtomicInt32 &value) { return value.Inc(); }
 inline wxInt32 wxAtomicDec(wxAtomicInt32 &value) { return value.Dec(); }
 
 #else // !wxNEEDS_GENERIC_ATOMIC_OPS
 
 #define wxHAS_ATOMIC_OPS
 
-inline void wxAtomicInc(wxInt32 &value) { wxAtomicInc((wxUint32&)value); }
+inline wxInt32 wxAtomicInc(wxInt32 &value) { return wxAtomicInc((wxUint32&)value); }
 inline wxInt32 wxAtomicDec(wxInt32 &value) { return wxAtomicDec((wxUint32&)value); }
 
 typedef wxInt32 wxAtomicInt32;

interface/wx/atomic.h

@@ -16,21 +16,27 @@
 /**
     This function increments @a value in an atomic manner.
 
+    @note It is recommended to use @c std::atomic available in C++11 and later
+    instead of this function in any new code.
+
     Whenever possible wxWidgets provides an efficient, CPU-specific,
     implementation of this function. If such implementation is available, the
     symbol wxHAS_ATOMIC_OPS is defined. Otherwise this function still exists
     but is implemented in a generic way using a critical section which can be
     prohibitively expensive for use in performance-sensitive code.
 
+    Returns the new value after the increment (the return value is only
+    available since wxWidgets 3.1.7, this function doesn't return anything in
+    previous versions of the library).
+
     @header{wx/atomic.h}
 */
-void wxAtomicInc(wxAtomicInt& value);
+wxInt32 wxAtomicInc(wxAtomicInt& value);
 
 /**
     This function decrements value in an atomic manner.
 
-    Returns 0 if value is 0 after decrement or any non-zero value (not
+    Returns the new value after decrementing it.
-    necessarily equal to the value of the variable) otherwise.
 
     @see wxAtomicInc
 

src/common/init.cpp

@@ -28,7 +28,7 @@
 #endif
 
 #include "wx/init.h"
-#include "wx/thread.h"
+#include "wx/atomic.h"
 
 #include "wx/scopedptr.h"
 #include "wx/except.h"
@@ -130,21 +130,20 @@ static inline void Use(void *) { }
 static struct InitData
 {
     InitData()
+        : nInitCount(0)
     {
-        nInitCount = 0;
-
 #if wxUSE_UNICODE
         argc = argcOrig = 0;
         // argv = argvOrig = NULL; -- not even really needed
 #endif // wxUSE_UNICODE
     }
 
-    // critical section protecting this struct
-    wxCRIT_SECT_DECLARE_MEMBER(csInit);
-
     // number of times wxInitialize() was called minus the number of times
     // wxUninitialize() was
-    size_t nInitCount;
+    //
+    // it is atomic to allow more than one thread to call wxInitialize() but
+    // only one of them to actually initialize the library
+    wxAtomicInt nInitCount;
 
 #if wxUSE_UNICODE
     int argc;
@@ -530,9 +529,7 @@ bool wxInitialize()
 
 bool wxInitialize(int& argc, wxChar **argv)
 {
-    wxCRIT_SECT_LOCKER(lockInit, gs_initData.csInit);
+    if ( wxAtomicInc(gs_initData.nInitCount) != 1 )
-
-    if ( gs_initData.nInitCount++ )
     {
         // already initialized
         return true;
@@ -544,9 +541,7 @@ bool wxInitialize(int& argc, wxChar **argv)
 #if wxUSE_UNICODE
 bool wxInitialize(int& argc, char **argv)
 {
-    wxCRIT_SECT_LOCKER(lockInit, gs_initData.csInit);
+    if ( wxAtomicInc(gs_initData.nInitCount) != 1 )
-
-    if ( gs_initData.nInitCount++ )
     {
         // already initialized
         return true;
@@ -558,10 +553,8 @@ bool wxInitialize(int& argc, char **argv)
 
 void wxUninitialize()
 {
-    wxCRIT_SECT_LOCKER(lockInit, gs_initData.csInit);
+    if ( wxAtomicDec(gs_initData.nInitCount) != 0 )
+        return;
 
-    if ( --gs_initData.nInitCount == 0 )
-    {
     wxEntryCleanup();
 }
-}

tests/thread/atomic.cpp

@@ -20,30 +20,22 @@
 #include "wx/thread.h"
 #include "wx/dynarray.h"
 #include "wx/log.h"
+#include "wx/vector.h"
 
-WX_DEFINE_ARRAY_PTR(wxThread *, wxArrayThread);
+typedef wxVector<wxThread*> wxArrayThread;
 
 // ----------------------------------------------------------------------------
 // constants
 // ----------------------------------------------------------------------------
 
-// number of times to run the loops: the code takes too long to run if we use
+static const wxInt32 ITERATIONS_NUM = 10000;
-// the bigger value with generic atomic operations implementation
-#ifdef wxHAS_ATOMIC_OPS
-    static const wxInt32 ITERATIONS_NUM = 10000000;
-#else
-    static const wxInt32 ITERATIONS_NUM = 1000;
-#endif
 
 // ----------------------------------------------------------------------------
-// test class
+// test helper thread
 // ----------------------------------------------------------------------------
 
-class AtomicTestCase : public CppUnit::TestCase
+namespace
 {
-public:
-    AtomicTestCase() { }
-
     enum ETestType
     {
         IncAndDecMixed,
@@ -51,7 +43,6 @@ public:
         DecOnly
     };
 
-private:
     class MyThread : public wxThread
     {
     public:
@@ -65,34 +56,13 @@ private:
         wxAtomicInt &m_operateOn;
         ETestType m_testType;
     };
+} // anonymous namespace
 
-    CPPUNIT_TEST_SUITE( AtomicTestCase );
+// ----------------------------------------------------------------------------
-        CPPUNIT_TEST( TestNoThread );
+// the tests themselves
-        CPPUNIT_TEST( TestDecReturn );
+// ----------------------------------------------------------------------------
-        CPPUNIT_TEST( TestTwoThreadsMix );
-        CPPUNIT_TEST( TestTenThreadsMix );
-        CPPUNIT_TEST( TestTwoThreadsSeparate );
-        CPPUNIT_TEST( TestTenThreadsSeparate );
-    CPPUNIT_TEST_SUITE_END();
 
-    void TestNoThread();
+TEST_CASE("Atomic::NoThread", "[atomic]")
-    void TestDecReturn();
-    void TestTenThreadsMix() { TestWithThreads(10, IncAndDecMixed); }
-    void TestTwoThreadsMix() { TestWithThreads(2, IncAndDecMixed); }
-    void TestTenThreadsSeparate() { TestWithThreads(10, IncOnly); }
-    void TestTwoThreadsSeparate() { TestWithThreads(2, IncOnly); }
-    void TestWithThreads(int count, ETestType testtype);
-
-    wxDECLARE_NO_COPY_CLASS(AtomicTestCase);
-};
-
-// register in the unnamed registry so that these tests are run by default
-CPPUNIT_TEST_SUITE_REGISTRATION( AtomicTestCase );
-
-// also include in its own registry so that these tests can be run alone
-CPPUNIT_TEST_SUITE_NAMED_REGISTRATION( AtomicTestCase, "AtomicTestCase" );
-
-void AtomicTestCase::TestNoThread()
 {
     wxAtomicInt int1 = 0,
                 int2 = 0;
@@ -103,23 +73,30 @@ void AtomicTestCase::TestNoThread()
         wxAtomicDec(int2);
     }
 
-    CPPUNIT_ASSERT( int1 == ITERATIONS_NUM );
+    CHECK( int1 == ITERATIONS_NUM );
-    CPPUNIT_ASSERT( int2 == -ITERATIONS_NUM );
+    CHECK( int2 == -ITERATIONS_NUM );
 }
 
-void AtomicTestCase::TestDecReturn()
+TEST_CASE("Atomic::ReturnValue", "[atomic]")
 {
     wxAtomicInt i(0);
-    wxAtomicInc(i);
+    REQUIRE( wxAtomicInc(i) == 1 );
-    wxAtomicInc(i);
+    REQUIRE( wxAtomicInc(i) == 2 );
-    CPPUNIT_ASSERT( i == 2 );
 
-    CPPUNIT_ASSERT( wxAtomicDec(i) > 0 );
+    REQUIRE( wxAtomicDec(i) == 1 );
-    CPPUNIT_ASSERT( wxAtomicDec(i) == 0 );
+    REQUIRE( wxAtomicDec(i) == 0 );
 }
 
-void AtomicTestCase::TestWithThreads(int count, ETestType testType)
+TEST_CASE("Atomic::WithThreads", "[atomic]")
 {
+    int count;
+    ETestType testType;
+
+    SECTION( "2 threads using inc and dec") { count =  2; testType = IncAndDecMixed; }
+    SECTION("10 threads using inc and dec") { count = 10; testType = IncAndDecMixed; }
+    SECTION( "2 threads using inc or dec" ) { count =  2; testType = IncOnly; }
+    SECTION("10 threads using inc or dec" ) { count = 10; testType = IncOnly; }
+
     wxAtomicInt    int1=0;
 
     wxArrayThread  threads;
@@ -146,7 +123,7 @@ void AtomicTestCase::TestWithThreads(int count, ETestType testType)
             delete thread;
         }
         else
-            threads.Add(thread);
+            threads.push_back(thread);
     }
 
     for ( i = 0; i < count; ++i )
@@ -158,16 +135,16 @@ void AtomicTestCase::TestWithThreads(int count, ETestType testType)
     for ( i = 0; i < count; ++i )
     {
         // each thread should return 0, else it detected some problem
-        CPPUNIT_ASSERT (threads[i]->Wait() == (wxThread::ExitCode)0);
+        CHECK (threads[i]->Wait() == (wxThread::ExitCode)0);
         delete threads[i];
     }
 
-    CPPUNIT_ASSERT( int1 == 0 );
+    CHECK( int1 == 0 );
 }
 
 // ----------------------------------------------------------------------------
 
-void *AtomicTestCase::MyThread::Entry()
+void *MyThread::Entry()
 {
     wxInt32 negativeValuesSeen = 0;
 
@@ -175,19 +152,17 @@ void *AtomicTestCase::MyThread::Entry()
     {
         switch ( m_testType )
         {
-            case AtomicTestCase::IncAndDecMixed:
+            case IncAndDecMixed:
                 wxAtomicInc(m_operateOn);
-                wxAtomicDec(m_operateOn);
+                if ( wxAtomicDec(m_operateOn) < 0 )
-
-                if (m_operateOn < 0)
                     ++negativeValuesSeen;
                 break;
 
-            case AtomicTestCase::IncOnly:
+            case IncOnly:
                 wxAtomicInc(m_operateOn);
                 break;
 
-            case AtomicTestCase::DecOnly:
+            case DecOnly:
                 wxAtomicDec(m_operateOn);
                 break;
         }