SCP: Fix scp_asynch_check cross-thread interference

Fix 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. "scp_asynch_check = 0" 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: - AIO_ILOCK/AIO_IUNLOCK: As documented in sim_defs.h, these two macros are empty in the lock-free code case. Minor performance improvement. - AIO_QUEUE_VAL and AIO_QUEUE_SET are inlined static functions and directly use compiler intrinsics for the lock-free implementation, when available. - GCC/Clang: Use the __atomic intrinsics. __sync instrinsics have been deprecated since the C++11 standard's publication. The __sync intrinsics are still available and provide a fallback if GCC or Clang is really so old that the __atomic intrinsics are unavailable (insert Wallace Shawn's iconic "That's totally inconceivable!" here.) - AIO_QUEUE_VAL: Leverages the processor's read fence when available. Ensures that sim_asynch_queue's value is consistenly read across cores and threads in the lock-free implementation for platforms without total store ordering or strong consistency guarantees (i.e., anything other than Intel and SPARC.) - AIO_QUEUE_SET: Compare-exchange to set sim_asynch_queue's head. The return value is a boolean (1/0) to reflect the success (1) or failure (0) of the compare-exchange. - AIO_CHECK_EVENT invokes AIO_ILOCK and AIO_IUNLOCK so that the lock-based code cannot alter sim_asynch_queue when checking to see if there is pending I/O work to be done. sim_asynch_lock is a recursive mutex in the lock-based case, so AIO_ILOCK/AIO_IUNLOCK will succeed for the lock-based implementation. - 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 · Dec 27, 2023 · 117a996 · 117a996
1 parent c077c22
commit 117a996
Show file tree

Hide file tree

Showing 7 changed files with 247 additions and 58 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
@@ -387,12 +387,12 @@ int sim_aio_update_queue (void)
 int migrated = 0;
 
 AIO_ILOCK;
-if (AIO_QUEUE_VAL != QUEUE_LIST_END) {  /* List !Empty */
+if (AIO_QUEUE_VAL() != QUEUE_LIST_END) {  /* List !Empty */
     UNIT *q, *uptr;
     int32 a_event_time;
     do {                                /* Grab current queue */
-        q = AIO_QUEUE_VAL;
-        } while (q != AIO_QUEUE_SET(QUEUE_LIST_END, q));
+        q = AIO_QUEUE_VAL();
+        } while (!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);
         ++migrated;
@@ -406,13 +406,16 @@ if (AIO_QUEUE_VAL != QUEUE_LIST_END) {  /* List !Empty */
             }
         else
             a_event_time = uptr->a_event_time;
-        AIO_IUNLOCK;
+        /* Note: Commented out and not deleted. So far, SIMH doesn't appear to
+         * attempt to reacquire the simh_asynch_lock mutex across threads.
+         * Reacquiring the mutex would potentially cause a deadlock across threads. */
+        /*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_ILOCK;*/
         }
     }
 AIO_IUNLOCK;
@@ -431,12 +434,11 @@ else {
     uptr->a_event_time = event_time;
     uptr->a_activate_call = caller;
     do {
-        q = AIO_QUEUE_VAL;
+        q = AIO_QUEUE_VAL();
         uptr->a_next = q;                               /* Mark as on list */
-        } while (q != AIO_QUEUE_SET(uptr, q));
+        } while (!AIO_QUEUE_SET(uptr, q));
     }
 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);