SCP: Fix scp_asynch_check cross-thread interference

Address a Clang/LLVM sanitizer warning that scp_asynch_check is written by both the AIO and main threads, one thread potentially clobbering the other's value. The "scp_asynch_check = 0" at scp.c:239 is where the thread sanitizer detects the issue. This check is supposed to cause the main thread to process I/O updates on the next AIO_CHECK_EVENT code's execution. To preserve that behavior, AIO_CHECK_EVENT now executes AIO_UPDATE_QUEUE when either sim_asynch_check decrements below 0 or there is work to be done on the AIO queue (sim_asynch_queue != QUEUE_HEAD.) Code refactoring: - Eliminate the asymmetry between the lock-based (mutex) and lock-free implementations. - Lock-free: AIO_ILOCK/AIO_IUNLOCK do not reacquire sim_asynch_lock when compiler intrinsics are present (GCC, Clang, MS C and DEC C on Itanium.) - Lock-based: If DONT_USE_AIO_INTRINSICS is defined, the AIO implementation becomes lock-based via mutexes and AIO_ILOCK/- AIO_IUNLOCK recursively acquire/release sim_asynch_lock. - AIO defaults to the lock-based implementation if compiler intrinsics are not available. - GCC, Clang: Prefer the __atomic intrinsics over the deprecated __sync intrinsics. The __sync intrinics still exist for older GCC compilers. - sim_asynch_lock is a recursive mutex for both lock-based and lock-free implementations. Eliminates implementation asymmetry. - AIO_CHECK_EVENT invokes AIO_ILOCK and AIO_IUNLOCK so that the lock-based code cannot alter sim_asynch_queue when checking for pending I/O work. - sim_debug_io_lock: Debug output serialization lock. Previously, sim_asynch_lock was semantically overloaded to serialize output from _sim_debug_write_flush. This lock provides better semantic clarity. - New builder script flag to disable AIO lock-free, force AIO lock-based code: - cmake-builder.ps1 -noaiointrinsics ... - cmake-builder.sh -no-aio-intrinics ...
open-simh · Jan 16, 2024 · ff11516 · ff11516
1 parent c077c22
commit ff11516
Show file tree

Hide file tree

Showing 7 changed files with 290 additions and 147 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -228,6 +228,9 @@ option(SIMH_PACKAGE_SUFFIX
 option(MAC_UNIVERSAL
        "macOS universal binary flag: TRUE -> build universal binaries, FALSE -> don't."
        ${MAC_UNIVERSAL_OPTVAL})
+option(DONT_USE_AIO_INTRINSICS
+       "Don't use compiler/platform intrinsics for AIO, revert to lock-based AIO"
+       FALSE)
 
 # Places where CMake should look for dependent package configuration fragments and artifacts:
 set(SIMH_PREFIX_PATH_LIST)

diff --git a/README-CMake.md b/README-CMake.md
@@ -511,25 +511,22 @@ or video support.
 
     # List the supported command line flags:
     $ cmake/cmake-builder.sh --help
-    Configure and build simh simulators on Linux and *nix-like platforms.
+    ** cmake version 3.18.4
 
-    Subdirectories:
-    cmake/build-unix:  Makefile-based build simulators
-    cmake/build-ninja: Ninja build-based simulators
+    CMake suite maintained and supported by Kitware (kitware.com/cmake).
+    Configure and build simh simulators on Linux and *nix-like platforms.
 
-    Options:
-    --------
+    Compile/Build options:
+    ----------------------
     --clean (-x)      Remove the build subdirectory
     --generate (-g)   Generate the build environment, don't compile/build
     --parallel (-p)   Enable build parallelism (parallel builds)
-    --nonetwork       Build simulators without network support
-    --novideo         Build simulators without video support
     --notest          Do not execute 'ctest' test cases
     --noinstall       Do not install SIMH simulators.
     --testonly        Do not build, execute the 'ctest' test cases
     --installonly     Do not build, install the SIMH simulators
 
-    --flavor (-f)     Specifies the build flavor. Valid flavors are:
+    --flavor (-f)     [Required] Specifies the build flavor. Valid flavors are:
                         unix
                         ninja
                         xcode
@@ -541,8 +538,7 @@ or video support.
     --config (-c)     Specifies the build configuration: 'Release' or 'Debug'
 
     --target          Build a specific simulator or simulators. Separate multiple
-                      targets by separating with a comma,
-                      e.g. "--target pdp8,pdp11,vax750,altairz80,3b2"
+                      targets with a comma, e.g. "--target pdp8,pdp11,vax750,altairz80,3b2"
     --lto             Enable Link Time Optimization (LTO) in Release builds
     --debugWall       Enable maximal warnings in Debug builds
     --cppcheck        Enable cppcheck static code analysis rules
@@ -553,6 +549,17 @@ or video support.
 
     --verbose         Turn on verbose build output
 
+    SIMH feature control options:
+    -----------------------------
+    --nonetwork       Build simulators without network support
+    --novideo         Build simulators without video support
+    --no-aio-intrinsics
+                      Do not use compiler/platform intrinsics to implement AIO
+                      functions (aka "lock-free" AIO), reverts to lock-based AIO
+                      if threading libraries are detected.
+
+    Other options:
+    --------------
     --help (-h)       Print this help.
     ```
 
@@ -569,17 +576,17 @@ or video support.
     PS C:\...\open-simh> Get-Help -deatailed cmake\cmake-builder.ps1
 
     NAME
-        C:\Users\bsm21317\play\open-simh\cmake\cmake-builder.ps1
+        C:\...\play\open-simh\cmake\cmake-builder.ps1
 
     SYNOPSIS
         Configure and build SIMH's dependencies and simulators using the Microsoft Visual
         Studio C compiler or MinGW-W64-based gcc compiler.
 
 
     SYNTAX
-        C:\Users\bsm21317\play\open-simh\cmake\cmake-builder.ps1 [[-flavor] <String>] [[-config] <String>] [[-cpack_suffix] <String>] [[-target] <String>]
-        [-clean] [-help] [-nonetwork] [-novideo] [-notest] [-noinstall] [-parallel] [-generate] [-regenerate] [-testonly] [-installOnly] [-windeprecation]
-        [-package] [-lto] [-debugWall] [-cppcheck] [<CommonParameters>]
+        C:\...\play\open-simh\cmake\cmake-builder.ps1 [[-flavor] <String>] [[-config] <String>] [[-cpack_suffix] <String>] [[-target]
+        <String[]>] [-clean] [-help] [-nonetwork] [-novideo] [-noaioinstrinsics] [-notest] [-noinstall] [-parallel] [-generate] [-testonly]
+        [-installOnly] [-windeprecation] [-lto] [-debugWall] [-cppcheck] [<CommonParameters>]
 
 
     DESCRIPTION
@@ -588,9 +595,9 @@ or video support.
 
         1. Configure and generate the build environment selected by '-flavor' option.
         2. Build missing runtime dependencies and the simulator suite with the compiler
-          configuration selected by the '-config' option. The "Release" configuration
-          generates optimized executables; the "Debug" configuration generates
-          development executables with debugger information.
+           configuration selected by the '-config' option. The "Release" configuration
+           generates optimized executables; the "Debug" configuration generates
+           development executables with debugger information.
         3. Test the simulators
 
         There is an install phase that can be invoked separately as part of the SIMH
@@ -624,6 +631,9 @@ or video support.
             mingw-make      MinGW GCC/mingw32-make
             mingw-ninja     MinGW GCC/ninja
 
+        -config <String>
+            The target build configuration. Valid values: "Release" and "Debug"
+
     [...truncated for brevity...]
     ```
 

diff --git a/cmake/cmake-builder.ps1 b/cmake/cmake-builder.ps1
@@ -115,6 +115,11 @@ param (
     [Parameter(Mandatory=$false)]
     [switch] $novideo        = $false,
 
+    ## Compile the SIMH simulator without AIO instrinsics ("lock-free" AIO),
+    ## using lock-based AIO via thread mutexes instead.
+    [Parameter(Mandatory=$false)]
+    [switch] $noaiointrinsics = $false,
+
     ## Disable the build's tests.
     [Parameter(Mandatory=$false)]
     [switch] $notest         = $false,
@@ -411,6 +416,10 @@ if (($scriptPhases -contains "generate") -or ($scriptPhases -contains "build"))
     {
       $generateArgs += @("-DWITH_VIDEO:Bool=Off")
     }
+    if ($noaiointrinsics)
+    { 
+      $generateArgs += @("-DDONT_USE_AIO_INTRINSICS:Bool=On")
+    }
     if ($lto)
     {
         $generateArgs += @("-DRELEASE_LTO:Bool=On")

diff --git a/cmake/cmake-builder.sh b/cmake/cmake-builder.sh
@@ -7,17 +7,11 @@ showHelp()
     cat <<EOF
 Configure and build simh simulators on Linux and *nix-like platforms.
 
-Subdirectories:
-cmake/build-unix:  Makefile-based build simulators
-cmake/build-ninja: Ninja build-based simulators
-
-Options:
---------
+Compile/Build options:
+----------------------
 --clean (-x)      Remove the build subdirectory
 --generate (-g)   Generate the build environment, don't compile/build
 --parallel (-p)   Enable build parallelism (parallel builds)
---nonetwork       Build simulators without network support
---novideo         Build simulators without video support
 --notest          Do not execute 'ctest' test cases
 --noinstall       Do not install SIMH simulators.
 --testonly        Do not build, execute the 'ctest' test cases
@@ -46,6 +40,17 @@ Options:
 
 --verbose         Turn on verbose build output
 
+SIMH feature control options:
+-----------------------------
+--nonetwork       Build simulators without network support
+--novideo         Build simulators without video support
+--no-aio-intrinsics
+                  Do not use compiler/platform intrinsics to implement AIO
+                  functions (aka "lock-free" AIO), reverts to lock-based AIO
+                  if threading libraries are detected.
+
+Other options:
+--------------
 --help (-h)       Print this help.
 EOF
 
@@ -152,6 +157,7 @@ fi
 
 longopts=clean,help,flavor:,config:,nonetwork,novideo,notest,parallel,generate,testonly
 longopts=${longopts},noinstall,installonly,verbose,target:,lto,debugWall,cppcheck,cpack_suffix:
+longopts=${longopts},no-aio-intrinsics
 
 ARGS=$(${getopt_prog} --longoptions $longopts --options xhf:c:pg -- "$@")
 if [ $? -ne 0 ] ; then
@@ -219,6 +225,10 @@ while true; do
             generateArgs="${generateArgs} -DWITH_VIDEO:Bool=Off"
             shift
             ;;
+        --no-aio-intrinsics)
+            generateArgs="${generateArgs} -DDONT_USE_AIO_INTRINSICS:Bool=On"
+            shift
+            ;;
         --notest)
             notest=yes
             shift

diff --git a/cmake/pthreads-dep.cmake b/cmake/pthreads-dep.cmake
@@ -69,6 +69,10 @@ if (WITH_ASYNC)
     else ()
         target_compile_definitions(thread_lib INTERFACE DONT_USE_READER_THREAD)
     endif ()
+
+    if (DONT_USE_AIO_INTRINSICS)
+      target_compile_definitions(thread_lib INTERFACE DONT_USE_AIO_INTRINSICS)
+    endif ()
 else (WITH_ASYNC)
     target_compile_definitions(thread_lib INTERFACE DONT_USE_READER_THREAD)
     set(THREADING_PKG_STATUS "asynchronous I/O disabled.")

diff --git a/scp.c b/scp.c
@@ -382,37 +382,68 @@ int32 sim_asynch_check;
 int32 sim_asynch_latency = 4000;      /* 4 usec interrupt latency */
 int32 sim_asynch_inst_latency = 20;   /* assume 5 mip simulator */
 
+/* Debug flush mutex to serialize debug output. */
+pthread_mutex_t sim_debug_io_lock = PTHREAD_MUTEX_INITIALIZER;
+
+/* aio_queue_worklist: Grab the current queue and replace sim_asynch_queue with
+ * the empty queue.
+ *
+ * Returns the UNIT worklist to which sim_asynch_queue previously pointed.
+ */
+static SIM_INLINE volatile UNIT *aio_queue_worklist()
+{
+    volatile UNIT *q;
+
+    do {
+        /* Atomically load, wash-rinse-repeat if there is thread interference */
+        q = sim_sync_load_pointer((void * volatile *) &sim_asynch_queue);
+    } while (!sim_sync_cmpxchg((void * volatile *) &sim_asynch_queue, QUEUE_LIST_END, (void *) q));
+
+    return q;
+}
+
+/* aio_enqueue_unit: Atomically add a UNIT to the sim_asynch_queue list.
+ */
+static SIM_INLINE void aio_enqueue_unit(UNIT *unit)
+{
+    volatile UNIT *q;
+
+    do {
+        q = sim_sync_load_pointer((void * volatile *) &sim_asynch_queue);
+        unit->a_next = (UNIT *) q;                           /* Mark as on list */
+    } while (!sim_sync_cmpxchg((void * volatile *) &sim_asynch_queue, unit, (void *) q));
+}
+
 int sim_aio_update_queue (void)
 {
 int migrated = 0;
 
 AIO_ILOCK;
-if (AIO_QUEUE_VAL != QUEUE_LIST_END) {  /* List !Empty */
-    UNIT *q, *uptr;
+if (!AIO_QUEUE_EMPTY()) {
+    volatile UNIT *q;
+    UNIT *uptr;
     int32 a_event_time;
-    do {                                /* Grab current queue */
-        q = AIO_QUEUE_VAL;
-        } while (q != AIO_QUEUE_SET(QUEUE_LIST_END, q));
-    while (q != QUEUE_LIST_END) {       /* List !Empty */
-        sim_debug (SIM_DBG_AIO_QUEUE, &sim_scp_dev, "Migrating Asynch event for %s after %d %s\n", sim_uname(q), q->a_event_time, sim_vm_interval_units);
+    for (q = aio_queue_worklist(); q != QUEUE_LIST_END; /* empty */) {
+        uptr = (UNIT *) q;
+        sim_debug (SIM_DBG_AIO_QUEUE, &sim_scp_dev, "Migrating Asynch event for %s after %d %s\n",
+                   sim_uname(uptr), uptr->a_event_time, sim_vm_interval_units);
         ++migrated;
-        uptr = q;
         q = q->a_next;
-        uptr->a_next = NULL;        /* hygiene */
+        uptr->a_next = NULL;           /* hygiene */
         if (uptr->a_activate_call != &sim_activate_notbefore) {
-            a_event_time = uptr->a_event_time-((sim_asynch_inst_latency+1)/2);
+            a_event_time = uptr->a_event_time - ((sim_asynch_inst_latency + 1) / 2);
             if (a_event_time < 0)
                 a_event_time = 0;
             }
         else
             a_event_time = uptr->a_event_time;
-        AIO_IUNLOCK;
+
         uptr->a_activate_call (uptr, a_event_time);
+
         if (uptr->a_check_completion) {
             sim_debug (SIM_DBG_AIO_QUEUE, &sim_scp_dev, "Calling Completion Check for asynch event on %s\n", sim_uname(uptr));
             uptr->a_check_completion (uptr);
             }
-        AIO_ILOCK;
         }
     }
 AIO_IUNLOCK;
@@ -421,22 +452,19 @@ return migrated;
 
 void sim_aio_activate (ACTIVATE_API caller, UNIT *uptr, int32 event_time)
 {
-AIO_ILOCK;
 sim_debug (SIM_DBG_AIO_QUEUE, &sim_scp_dev, "Queueing Asynch event for %s after %d %s\n", sim_uname(uptr), event_time, sim_vm_interval_units);
-if (uptr->a_next) {
+
+AIO_ILOCK;
+if (NULL != uptr->a_next) {
     uptr->a_activate_call = sim_activate_abs;
     }
 else {
-    UNIT *q;
     uptr->a_event_time = event_time;
     uptr->a_activate_call = caller;
-    do {
-        q = AIO_QUEUE_VAL;
-        uptr->a_next = q;                               /* Mark as on list */
-        } while (q != AIO_QUEUE_SET(uptr, q));
+    aio_enqueue_unit(uptr);
     }
 AIO_IUNLOCK;
-sim_asynch_check = 0;                             /* try to force check */
+
 if (sim_idle_wait) {
     sim_debug (TIMER_DBG_IDLE, &sim_timer_dev, "waking due to event on %s after %d %s\n", sim_uname(uptr), event_time, sim_vm_interval_units);
     pthread_cond_signal (&sim_asynch_wake);
@@ -7019,7 +7047,7 @@ sim_show_clock_queues (st, dnotused, unotused, flag, cptr);
 pthread_mutex_lock (&sim_asynch_lock);
 sim_mfile = &buf;
 fprintf (st, "asynchronous pending event queue\n");
-if (sim_asynch_queue == QUEUE_LIST_END)
+if (AIO_QUEUE_EMPTY())
     fprintf (st, "  Empty\n");
 else {
     for (uptr = sim_asynch_queue; uptr != QUEUE_LIST_END; uptr = uptr->a_next) {
@@ -13620,7 +13648,8 @@ if (sim_deb_switches & SWMASK ('F')) {              /* filtering disabled? */
         _debug_fwrite (buf, len);                   /* output now. */
     return;                                         /* done */
     }
-AIO_LOCK;
+
+AIO_DEBUG_IO_ACTIVE;
 if (debug_line_offset + len + 1 > debug_line_bufsize) {
     debug_line_bufsize += MAX(1024, debug_line_offset + len + 1);
     debug_line_buf = (char *)realloc (debug_line_buf, debug_line_bufsize);
@@ -13688,7 +13717,7 @@ while ((eol = strchr (debug_line_buf, '\n')) || flush) {
         memmove (debug_line_buf, eol + 1, debug_line_offset);
     debug_line_buf[debug_line_offset] = '\0';
     }
-AIO_UNLOCK;
+AIO_DEBUG_IO_DONE;
 }
 
 static void _sim_debug_write (const char *buf, size_t len)