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 detected (GCC, Clang, MS C and DEC C on
    Itanium.)

  - Lock-based: If DONT_USE_AIO_INTRINSICS is defined or intrinsics are
    not detected, 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 detected.

  - 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 ...

  "-DDONT_USE_AIO_INTRINSICS:Bool=On" is the command line CMake
  configuration option.

Author: bscottm

CMakeLists.txt

@@ -226,6 +226,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)

README-CMake.md

@@ -517,25 +517,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
@@ -547,8 +544,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
@@ -559,6 +555,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.
     ```
 
@@ -575,17 +582,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
@@ -594,9 +601,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
@@ -630,6 +637,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...]
     ```
 

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

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()

makefile

@@ -2110,7 +2110,7 @@ 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}  ${AIO_CCDEFS}
+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 \

scp.c

@@ -284,7 +284,6 @@
 #define UPDATE_SIM_TIME                                         \
     if (1) {                                                    \
         int32 _x;                                               \
-        AIO_LOCK;                                               \
         if (sim_clock_queue == QUEUE_LIST_END)                  \
             _x = noqueue_time;                                  \
         else                                                    \
@@ -295,7 +294,6 @@
             noqueue_time = sim_interval;                        \
         else                                                    \
             sim_clock_queue->time = sim_interval;               \
-        AIO_UNLOCK;                                             \
         }                                                       \
     else                                                        \
         (void)0
@@ -378,43 +376,73 @@ pthread_mutex_t sim_tmxr_poll_lock = PTHREAD_MUTEX_INITIALIZER;
 pthread_cond_t sim_tmxr_poll_cond  = PTHREAD_COND_INITIALIZER;
 int32 sim_tmxr_poll_count;
 pthread_t sim_asynch_main_threadid;
-UNIT * volatile sim_asynch_queue;
+sim_unit_atomic_t 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 sim_unit_atomic_t aio_queue_worklist()
+{
+    UNIT *q;
+
+    do {
+        /* Atomically load, wash-rinse-repeat if there is thread interference */
+        q = unit_ptr_load_atomic(&sim_asynch_queue);
+    } while (!unit_ptr_cmpxchg(&sim_asynch_queue, QUEUE_LIST_END, 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)
+{
+    UNIT *q;
+
+    do {
+        q = unit_ptr_load_atomic(&sim_asynch_queue);
+        unit->a_next = (UNIT *) q;                           /* Mark as on list */
+    } while (!unit_ptr_cmpxchg(&sim_asynch_queue, unit, 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()) {
+    sim_unit_atomic_t 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_ptr_load_atomic(&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 +451,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);
@@ -2808,7 +2833,7 @@ sim_quiet = sim_switches & SWMASK ('Q');                /* -q means quiet */
 sim_on_inherit = sim_switches & SWMASK ('O');           /* -o means inherit on state */
 
 sim_init_sock ();                                       /* init socket capabilities */
-AIO_INIT;                                               /* init Asynch I/O */
+aio_initialization();                                   /* init Asynch I/O */
 sim_finit ();                                           /* init fio package */
 sim_disk_init ();                                       /* init disk package */
 sim_tape_init ();                                       /* init tape package */
@@ -2982,7 +3007,7 @@ sim_set_logoff (0, NULL);                               /* close log */
 sim_set_notelnet (0, NULL);                             /* close Telnet */
 vid_close_all ();                                       /* close video */
 sim_ttclose ();                                         /* close console */
-AIO_CLEANUP;                                            /* Asynch I/O */
+aio_termination ();                                     /* Asynch I/O */
 sim_cleanup_sock ();                                    /* cleanup sockets */
 fclose (stdnul);                                        /* close bit bucket file handle */
 free (targv);                                           /* release any argv copy that was made */
@@ -7038,17 +7063,20 @@ 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) {
-        if ((dptr = find_dev_from_unit (uptr)) != NULL) {
+    UNIT *p_unit;
+    for (p_unit = unit_ptr_load_atomic(&sim_asynch_queue);
+         p_unit != QUEUE_LIST_END;
+         p_unit = unit_ptr_load_atomic(&p_unit->a_next)) {
+        if ((dptr = find_dev_from_unit (p_unit)) != NULL) {
             fprintf (st, "  %s", sim_dname (dptr));
             if (dptr->numunits > 1) fprintf (st, " unit %d",
-                (int32) (uptr - dptr->units));
+                (int32) (p_unit - dptr->units));
             }
         else fprintf (st, "  Unknown");
-        fprintf (st, " event delay %d\n", uptr->a_event_time);
+        fprintf (st, " event delay %d\n", p_unit->a_event_time);
         }
     }
 fprintf (st, "asynch latency: %d nanoseconds\n", sim_asynch_latency);
@@ -13660,7 +13688,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
@@ -13745,7 +13774,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)
@@ -14656,15 +14685,15 @@ for (hblock = astrings; (htext = *hblock) != NULL; hblock++) {
                 }
             excluded = FALSE;
             if (*start == '?') {                /* Conditional topic? */
-                size_t n = 0;
+                size_t m = 0;
                 start++;
                 while (sim_isdigit (*start))    /* Get param # */
-                    n += (n * 10) + (*start++ - '0');
-                if (!*start || *start == '\n'|| n == 0 || n >= VSMAX)
+                    m += (m * 10) + (*start++ - '0');
+                if (!*start || *start == '\n'|| m == 0 || n >= VSMAX)
                     FAIL (SCPE_ARG, Invalid parameter number, start);
-                while (n > vsnum)               /* Get arg pointer if not cached */
+                while (m > vsnum)               /* Get arg pointer if not cached */
                     vstrings[vsnum++] = va_arg (ap, char *);
-                end = vstrings[n-1];            /* Check for True */
+                end = vstrings[m-1];            /* Check for True */
                 if (!end || !(sim_toupper (*end) == 'T' || *end == '1')) {
                     excluded = TRUE;            /* False, skip topic this time */
                     if (*htext)