i#5784: Fix false positives in invariant checker

clients/drcachesim/tests/invariant_checker_test.cpp

@@ -246,6 +246,38 @@ check_kernel_xfer()
         if (!run_checker(memrefs, false))
             return false;
     }
+    // Signal before any instr in the trace.
+    {
+        std::vector<memref_t> memrefs = {
+            // No instr in the beginning here.
+            gen_marker(1, TRACE_MARKER_TYPE_KERNEL_EVENT, 2),
+            gen_instr(1, 101),
+            // XXX: This marker value is actually not guaranteed, yet the checker
+            // requires it and the view tool prints it.
+            gen_marker(1, TRACE_MARKER_TYPE_KERNEL_XFER, 102),
+            gen_instr(1, 2),
+        };
+        if (!run_checker(memrefs, false))
+            return false;
+    }
+    // Nested signals without any intervening instr.
+    {
+        std::vector<memref_t> memrefs = {
+            gen_instr(1, 1),
+            gen_marker(1, TRACE_MARKER_TYPE_KERNEL_EVENT, 2),
+            // No intervening instr here.
+            gen_marker(1, TRACE_MARKER_TYPE_KERNEL_EVENT, 101),
+            gen_instr(1, 201),
+            // XXX: This marker value is actually not guaranteed, yet the checker
+            // requires it and the view tool prints it.
+            gen_marker(1, TRACE_MARKER_TYPE_KERNEL_XFER, 202),
+            gen_instr(1, 101),
+            gen_marker(1, TRACE_MARKER_TYPE_KERNEL_XFER, 102),
+            gen_instr(1, 2),
+        };
+        if (!run_checker(memrefs, false))
+            return false;
+    }
     // Fail to return to recorded interruption point.
     {
         std::vector<memref_t> memrefs = {

clients/drcachesim/tools/invariant_checker.cpp

@@ -414,16 +414,19 @@ invariant_checker_t::parallel_shard_memref(void *shard_data, const memref_t &mem
                   // resumption point.
                   shard->pre_signal_instr_.top().instr.type ==
                       TRACE_TYPE_INSTR_SYSENTER) &&
-                 (memref.instr.addr == shard->pre_signal_instr_.top().instr.addr ||
-                  // Asynch will go to the subsequent instr.
-                  memref.instr.addr ==
-                      shard->pre_signal_instr_.top().instr.addr +
-                          shard->pre_signal_instr_.top().instr.size ||
-                  // Too hard to figure out branch targets.  We have the
-                  // prev_xfer_int_pc_ though.
-                  type_is_instr_branch(shard->pre_signal_instr_.top().instr.type) ||
-                  shard->pre_signal_instr_.top().instr.type ==
-                      TRACE_TYPE_INSTR_SYSENTER)) ||
+                 (
+                     // Skip pre_signal_instr_ check if there was no such instr.
+                     shard->pre_signal_instr_.top().instr.addr == 0 ||
+                     memref.instr.addr == shard->pre_signal_instr_.top().instr.addr ||
+                     // Asynch will go to the subsequent instr.
+                     memref.instr.addr ==
+                         shard->pre_signal_instr_.top().instr.addr +
+                             shard->pre_signal_instr_.top().instr.size ||
+                     // Too hard to figure out branch targets.  We have the
+                     // prev_xfer_int_pc_ though.
+                     type_is_instr_branch(shard->pre_signal_instr_.top().instr.type) ||
+                     shard->pre_signal_instr_.top().instr.type ==
+                         TRACE_TYPE_INSTR_SYSENTER)) ||
                     // Nested signal.  XXX: This only works for our annotated test
                     // signal_invariants where we know shard->app_handler_pc_.
                     memref.instr.addr == shard->app_handler_pc_ ||
@@ -438,6 +441,7 @@ invariant_checker_t::parallel_shard_memref(void *shard_data, const memref_t &mem
         }
 #endif
         shard->prev_instr_ = memref;
+        shard->saw_kernel_xfer_after_prev_instr_ = false;
         // Clear prev_xfer_marker_ on an instr (not a memref which could come between an
         // instr and a kernel-mediated far-away instr) to ensure it's *immediately*
         // prior (i#3937).
@@ -483,8 +487,19 @@ invariant_checker_t::parallel_shard_memref(void *shard_data, const memref_t &mem
                         "Kernel event marker value missing");
         if (memref.marker.marker_type == TRACE_MARKER_TYPE_KERNEL_EVENT &&
             // Give up on back-to-back signals.
-            shard->prev_xfer_marker_.marker.marker_type != TRACE_MARKER_TYPE_KERNEL_XFER)
-            shard->pre_signal_instr_.push(shard->prev_instr_);
+            shard->prev_xfer_marker_.marker.marker_type !=
+                TRACE_MARKER_TYPE_KERNEL_XFER) {
+            if (shard->saw_kernel_xfer_after_prev_instr_) {
+                // We have nested signals without an intervening app instr.
+                // Push an empty instr to mean that this shouldn't be used.
+                shard->pre_signal_instr_.push({});
+            } else {
+                shard->saw_kernel_xfer_after_prev_instr_ = true;
+                // If there was a kernel xfer marker at the very beginning
+                // of the trace, we may still push an empty instr here.
+                shard->pre_signal_instr_.push(shard->prev_instr_);
+            }
+        }
 #endif
         shard->prev_xfer_marker_ = memref;
         shard->last_xfer_marker_ = memref;

clients/drcachesim/tools/invariant_checker.h

@@ -97,6 +97,7 @@ class invariant_checker_t : public analysis_tool_t {
         int instrs_until_interrupt_ = -1;
         int memrefs_until_interrupt_ = -1;
 #endif
+        bool saw_kernel_xfer_after_prev_instr_ = false;
         bool saw_timestamp_but_no_instr_ = false;
         bool found_cache_line_size_marker_ = false;
         bool found_instr_count_marker_ = false;