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 assignment 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 sim_asynch_queue != QUEUE_HEAD, i.e., there is work to be done in the queue. Code refactoring: - Convert preprocessor macro code to inline functions unless impractical. - 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. sim_asynch_lock was semantically overloaded to serialize output from _sim_debug_write_flush. This lock provides better semantic clarity. - Removed sim_asynch_check. It is no longer necessary. - 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 · Feb 24, 2024 · 314192d · 314192d
1 parent c47e933
commit 314192d
Show file tree

Hide file tree

Showing 9 changed files with 304 additions and 160 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -229,6 +229,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/PDP10/CMakeLists.txt b/PDP10/CMakeLists.txt
@@ -98,6 +98,7 @@ add_simulator(pdp10-ka
     FEATURE_INT64
     FEATURE_VIDEO
     FEATURE_DISPLAY
+    USES_AIO
     LABEL PDP10
     PKG_FAMILY pdp10_family
     TEST ka10)
@@ -139,6 +140,7 @@ add_simulator(pdp10-ki
     FEATURE_INT64
     FEATURE_VIDEO
     FEATURE_DISPLAY
+    USES_AIO
     LABEL PDP10
     PKG_FAMILY pdp10_family
     TEST ki10)
@@ -171,6 +173,7 @@ add_simulator(pdp10-kl
     DEFINES
         KL=1
     FEATURE_INT64
+    USES_AIO
     LABEL PDP10
     PKG_FAMILY pdp10_family
     TEST kl10)
@@ -198,6 +201,7 @@ add_simulator(pdp10-ks
     DEFINES
         KS=1
     FEATURE_INT64
+    USES_AIO
     LABEL PDP10
     PKG_FAMILY pdp10_family
     TEST ks10)

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.sh b/cmake/cmake-builder.sh
@@ -7,8 +7,7 @@ showHelp()
     cat <<EOF
 Configure and build simh simulators on Linux and *nix-like platforms.
 
--Compile/Build options:
------------------------
+Compile/Build options:
 --clean (-x)      Remove the build subdirectory
 --generate (-g)   Generate the build environment, don't compile/build
 --cache           '--generate' and show CMake's variable cache

diff --git a/cmake/pthreads-dep.cmake b/cmake/pthreads-dep.cmake
@@ -70,7 +70,11 @@ if (WITH_ASYNC)
     else ()
         set(AIO_FLAGS DONT_USE_READER_THREAD)
     endif ()
-else()
+
+    if (DONT_USE_AIO_INTRINSICS)
+      target_compile_definitions(thread_lib INTERFACE DONT_USE_AIO_INTRINSICS)
+    endif ()
+else ()
     target_compile_definitions(thread_lib INTERFACE DONT_USE_READER_THREAD)
     set(THREADING_PKG_STATUS "asynchronous I/O disabled.")
 endif()
diff --git a/makefile b/makefile
@@ -2071,7 +2071,8 @@ KA10 = ${KA10D}/kx10_cpu.c ${KA10D}/kx10_sys.c ${KA10D}/kx10_df.c \
 	${KA10D}/ka10_ai.c ${KA10D}/ka10_iii.c ${KA10D}/kx10_disk.c \
 	${KA10D}/ka10_pclk.c ${KA10D}/ka10_tv.c \
 	${DISPLAYL} ${DISPLAY340} ${DISPLAYIII}
-KA10_OPT = -DKA=1 -DUSE_INT64 -I ${KA10D} -DUSE_SIM_CARD ${NETWORK_OPT} ${DISPLAY_OPT} ${KA10_DISPLAY_OPT}
+KA10_OPT = -DKA=1 -DUSE_INT64 -I ${KA10D} -DUSE_SIM_CARD ${NETWORK_OPT} ${DISPLAY_OPT} ${KA10_DISPLAY_OPT} \
+	   ${AIO_CCDEFS}
 ifneq (${PANDA_LIGHTS},)
 # ONLY for Panda display.
 KA10_OPT += -DPANDA_LIGHTS
@@ -2091,7 +2092,8 @@ KI10 = ${KI10D}/kx10_cpu.c ${KI10D}/kx10_sys.c ${KI10D}/kx10_df.c \
 	${KI10D}/kx10_cp.c ${KI10D}/kx10_tu.c ${KI10D}/kx10_rs.c \
 	${KI10D}/kx10_imp.c ${KI10D}/kx10_dpy.c ${KI10D}/kx10_disk.c \
 	${DISPLAYL} ${DISPLAY340}
-KI10_OPT = -DKI=1 -DUSE_INT64 -I ${KI10D} -DUSE_SIM_CARD ${NETWORK_OPT} ${DISPLAY_OPT} ${KI10_DISPLAY_OPT}
+KI10_OPT = -DKI=1 -DUSE_INT64 -I ${KI10D} -DUSE_SIM_CARD ${NETWORK_OPT} ${DISPLAY_OPT} ${KI10_DISPLAY_OPT} \
+	   ${AIO_CCDEFS}
 ifneq (${PANDA_LIGHTS},)
 # ONLY for Panda display.
 KI10_OPT += -DPANDA_LIGHTS
@@ -2107,15 +2109,15 @@ KL10 =  ${KL10D}/kx10_cpu.c ${KL10D}/kx10_sys.c ${KL10D}/kx10_df.c \
     ${KL10D}/kx10_rp.c ${KL10D}/kx10_tu.c ${KL10D}/kx10_rs.c \
     ${KL10D}/kx10_imp.c ${KL10D}/kl10_fe.c ${KL10D}/ka10_pd.c \
     ${KL10D}/ka10_ch10.c ${KL10D}/kl10_nia.c ${KL10D}/kx10_disk.c
-KL10_OPT = -DKL=1 -DUSE_INT64 -I $(KL10D) -DUSE_SIM_CARD ${NETWORK_OPT} 
+KL10_OPT = -DKL=1 -DUSE_INT64 -I $(KL10D) -DUSE_SIM_CARD ${NETWORK_OPT} ${AIO_CCDEFS}
 
 KS10D = ${SIMHD}/PDP10
 KS10 = ${KS10D}/kx10_cpu.c ${KS10D}/kx10_sys.c ${KS10D}/kx10_disk.c \
 	${KS10D}/ks10_cty.c ${KS10D}/ks10_uba.c ${KS10D}/kx10_rh.c \
 	${KS10D}/kx10_rp.c ${KS10D}/kx10_tu.c ${KS10D}/ks10_dz.c \
 	${KS10D}/ks10_tcu.c ${KS10D}/ks10_lp.c ${KS10D}/ks10_ch11.c \
 	${KS10D}/ks10_kmc.c ${KS10D}/ks10_dup.c ${KS10D}/kx10_imp.c
-KS10_OPT = -DKS=1 -DUSE_INT64 -I $(KS10D) -I $(PDP11D) ${NETWORK_OPT}
+KS10_OPT = -DKS=1 -DUSE_INT64 -I $(KS10D) -I $(PDP11D) ${NETWORK_OPT} ${AIO_CCDEFS}
 
 ATT3B2D = ${SIMHD}/3B2
 ATT3B2M400 = ${ATT3B2D}/3b2_cpu.c ${ATT3B2D}/3b2_sys.c \

diff --git a/scp.c b/scp.c
@@ -380,41 +380,71 @@ int32 sim_tmxr_poll_count;
 pthread_t sim_asynch_main_threadid;
 UNIT * volatile sim_asynch_queue;
 t_bool sim_asynch_enabled = TRUE;
-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;
@@ -423,22 +453,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);
@@ -7036,7 +7063,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) {
@@ -13653,7 +13680,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) {
     /* realloc(NULL, size) == malloc(size). Initialize the malloc()-ed space. Only
        need to test debug_line_buf since SIMH allocates both buffers at the same
@@ -13738,7 +13766,7 @@ while (NULL != (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)