Reviewer requests:

+ Remove documented guarantee that branches are delayed
  (but the impl remains; added a comment about waiting
  for users to update before changing that, if we do)
+ Added comments about some issues in signal PC continuity
  checks that #5912 should address
+ Added missing clear of last_indirect_target_
+ Use last_insr_in_cur_context_ instead of prev_instr_
+ Multiple comment wording and variable name and test parameter updates
+ Added an untaken-before-signal raw2trace unit test
+ Added an assert on no non-branch instrs being delayed

clients/drcachesim/common/trace_entry.h

@@ -83,7 +83,7 @@ typedef enum {
      * conditional branches use either #TRACE_TYPE_INSTR_TAKEN_JUMP or
      * #TRACE_TYPE_INSTR_UNTAKEN_JUMP and that the target of indirect branches is in a
      * marker of type #TRACE_MARKER_TYPE_BRANCH_TARGET prior to the indirect branch
-     * instrution entry itself.  This only applies to offline traces whose instructions
+     * instruction entry itself.  This only applies to offline traces whose instructions
      * are not filtered; online traces, and i-filtered offline traces, even at this
      * version, do not contain this information.
      */
@@ -525,7 +525,7 @@ typedef enum {
 
     /**
      * This marker is present just before each indirect branch instruction in offline
-     * un-instruction-filtered traces.  The marker value holds the actual target of the
+     * non-i-filtered traces.  The marker value holds the actual target of the
      * branch.
      */
     TRACE_MARKER_TYPE_BRANCH_TARGET,

clients/drcachesim/docs/drcachesim.dox.in

@@ -1384,17 +1384,6 @@ behavior for rseq aborts):
      2212820     1648447:     1249326 ifetch       5 byte(s) @ 0x00007f34066a8b37 4c 8b 54 24 48       mov    0x48(%rsp), %r10
 ```
 
-Offline traces further guarantee that a branch target instruction
-entry in a trace must immediately follow the branch instruction with
-no intervening thread switch.  This allows a core simulator to
-identify the target of a branch by looking at the subsequent trace
-entry in most cases, though as mentioned the target should be
-available from the taken/untaken type plus the decoding and indirect
-branch marker.  This guarantee does not hold when a kernel event such
-as a signal is delivered immediately after a branch; however, each
-marker indicating such a kernel transfer includes the interrupted PC,
-explicitly providing the branch target, for all but rseq aborts.
-
 Filtered traces (filtered via -L0_filter) include the dynamic
 (pre-filtered) per-thread instruction count in a
 #dynamorio::drmemtrace::TRACE_MARKER_TYPE_INSTRUCTION_COUNT marker at

clients/drcachesim/tests/raw2trace_unit_tests.cpp

@@ -1983,7 +1983,7 @@ test_branch_decoration(void *drcontext)
     // We focus on signals, rseq rollbacks to branches, and terminal branches here.
     bool res = true;
     {
-        // Branch before signal.
+        // Taken branch before signal.
         instrlist_t *ilist = instrlist_create(drcontext);
         instr_t *nop1 = XINST_CREATE_nop(drcontext); // Avoid offset of 0.
         instr_t *move =
@@ -2033,6 +2033,56 @@ test_branch_decoration(void *drcontext)
             check_entry(entries, idx, TRACE_TYPE_THREAD_EXIT, -1) &&
             check_entry(entries, idx, TRACE_TYPE_FOOTER, -1);
     }
+    {
+        // Untaken branch before signal.
+        instrlist_t *ilist = instrlist_create(drcontext);
+        instr_t *nop1 = XINST_CREATE_nop(drcontext); // Avoid offset of 0.
+        instr_t *move =
+            XINST_CREATE_move(drcontext, opnd_create_reg(REG1), opnd_create_reg(REG2));
+        instr_t *jcc =
+            XINST_CREATE_jump_cond(drcontext, DR_PRED_EQ, opnd_create_instr(move));
+        instr_t *nop2 = XINST_CREATE_nop(drcontext);
+        instrlist_append(ilist, nop1);
+        instrlist_append(ilist, jcc);
+        instrlist_append(ilist, nop2);
+        instrlist_append(ilist, move);
+        size_t offs_nop1 = 0;
+        size_t offs_jcc = offs_nop1 + instr_length(drcontext, nop1);
+        size_t offs_nop2 = offs_jcc + instr_length(drcontext, jcc);
+
+        std::vector<offline_entry_t> raw;
+        raw.push_back(make_header());
+        raw.push_back(make_tid());
+        raw.push_back(make_pid());
+        raw.push_back(make_line_size());
+        raw.push_back(make_block(offs_jcc, 1));
+        raw.push_back(make_marker(TRACE_MARKER_TYPE_KERNEL_EVENT, offs_nop2));
+        raw.push_back(make_exit());
+
+        std::vector<trace_entry_t> entries;
+        if (!run_raw2trace(drcontext, raw, ilist, entries))
+            return false;
+        int idx = 0;
+        res = res && check_entry(entries, idx, TRACE_TYPE_HEADER, -1) &&
+            check_entry(entries, idx, TRACE_TYPE_MARKER, TRACE_MARKER_TYPE_VERSION) &&
+            check_entry(entries, idx, TRACE_TYPE_MARKER, TRACE_MARKER_TYPE_FILETYPE) &&
+            check_entry(entries, idx, TRACE_TYPE_THREAD, -1) &&
+            check_entry(entries, idx, TRACE_TYPE_PID, -1) &&
+            check_entry(entries, idx, TRACE_TYPE_MARKER,
+                        TRACE_MARKER_TYPE_CACHE_LINE_SIZE) &&
+            check_entry(entries, idx, TRACE_TYPE_MARKER,
+                        TRACE_MARKER_TYPE_CHUNK_INSTR_COUNT) &&
+            check_entry(entries, idx, TRACE_TYPE_ENCODING, -1) &&
+#ifdef X86_32
+            // An extra encoding entry is needed.
+            check_entry(entries, idx, TRACE_TYPE_ENCODING, -1) &&
+#endif
+            check_entry(entries, idx, TRACE_TYPE_INSTR_UNTAKEN_JUMP, -1, offs_jcc) &&
+            check_entry(entries, idx, TRACE_TYPE_MARKER, TRACE_MARKER_TYPE_KERNEL_EVENT,
+                        offs_nop2) &&
+            check_entry(entries, idx, TRACE_TYPE_THREAD_EXIT, -1) &&
+            check_entry(entries, idx, TRACE_TYPE_FOOTER, -1);
+    }
     {
         // Untaken branch at and of rseq rollback.
         instrlist_t *ilist = instrlist_create(drcontext);

clients/drcachesim/tools/invariant_checker.cpp

@@ -295,7 +295,7 @@ invariant_checker_t::parallel_shard_memref(void *shard_data, const memref_t &mem
     }
     if (memref.marker.type == TRACE_TYPE_MARKER &&
         memref.marker.marker_type == TRACE_MARKER_TYPE_VERSION) {
-        shard->version_ = memref.marker.marker_value;
+        shard->trace_version_ = memref.marker.marker_value;
         report_if_false(shard,
                         shard->stream == nullptr ||
                             memref.marker.marker_value == shard->stream->get_version(),
@@ -502,10 +502,14 @@ invariant_checker_t::parallel_shard_memref(void *shard_data, const memref_t &mem
                 "Branch target not immediately after branch");
         }
         // Invariant: non-explicit control flow (i.e., kernel-mediated) is indicated
-        // by markers.
+        // by markers.  Checks are relaxed if a kernel event occurred but we pass
+        // last_instr_in_cur_context_ to avoid any confusion.
+        // XXX i#5912: For adding checks across every part of a signal handler should
+        // we change this to last_instr_in_cur_context_ and remove/adjust the
+        // check relaxations inside check_for_pc_discontinuity()?
         const std::string non_explicit_flow_violation_msg = check_for_pc_discontinuity(
-            shard, memref, shard->prev_instr_, memref.instr.addr, cur_instr_decoded,
-            expect_encoding, /*at_kernel_event=*/false);
+            shard, memref, shard->last_instr_in_cur_context_, memref.instr.addr,
+            cur_instr_decoded, expect_encoding, /*at_kernel_event=*/false);
         report_if_false(shard, non_explicit_flow_violation_msg.empty(),
                         non_explicit_flow_violation_msg);
 
@@ -696,21 +700,27 @@ invariant_checker_t::parallel_shard_memref(void *shard_data, const memref_t &mem
         memref.marker.marker_type == TRACE_MARKER_TYPE_BRANCH_TARGET) {
         shard->last_branch_marker_value_ = memref.marker.marker_value;
     }
-    if (knob_offline_ && shard->version_ >= TRACE_ENTRY_VERSION_BRANCH_INFO &&
-        type_is_instr_branch(memref.instr.type) &&
-        // I-filtered traces don't mark branch targets.
-        !TESTANY(OFFLINE_FILE_TYPE_FILTERED | OFFLINE_FILE_TYPE_IFILTERED,
-                 shard->file_type_)) {
-        report_if_false(shard, memref.instr.type != TRACE_TYPE_INSTR_CONDITIONAL_JUMP,
-                        "The CONDITIONAL_JUMP type is deprecated and should not appear");
-        if (!type_is_instr_direct_branch(memref.instr.type)) {
-            shard->last_indirect_target_ = shard->last_branch_marker_value_;
-            report_if_false(shard,
-                            shard->last_indirect_target_ != 0 &&
-                                shard->prev_entry_.marker.type == TRACE_TYPE_MARKER &&
-                                shard->prev_entry_.marker.marker_type ==
-                                    TRACE_MARKER_TYPE_BRANCH_TARGET,
-                            "Indirect branches must be preceded by their targets");
+    if (knob_offline_ && shard->trace_version_ >= TRACE_ENTRY_VERSION_BRANCH_INFO) {
+        if (type_is_instr_branch(memref.instr.type) &&
+            // I-filtered traces don't mark branch targets.
+            !TESTANY(OFFLINE_FILE_TYPE_FILTERED | OFFLINE_FILE_TYPE_IFILTERED,
+                     shard->file_type_)) {
+            report_if_false(
+                shard, memref.instr.type != TRACE_TYPE_INSTR_CONDITIONAL_JUMP,
+                "The CONDITIONAL_JUMP type is deprecated and should not appear");
+            if (!type_is_instr_direct_branch(memref.instr.type)) {
+                shard->last_indirect_target_ = shard->last_branch_marker_value_;
+                report_if_false(shard,
+                                shard->last_indirect_target_ != 0 &&
+                                    shard->prev_entry_.marker.type == TRACE_TYPE_MARKER &&
+                                    shard->prev_entry_.marker.marker_type ==
+                                        TRACE_MARKER_TYPE_BRANCH_TARGET,
+                                "Indirect branches must be preceded by their targets");
+            }
+        }
+        if (!type_is_instr_branch(memref.instr.type) ||
+            type_is_instr_direct_branch(memref.instr.type)) {
+            shard->last_indirect_target_ = 0;
         }
     }
 
@@ -917,15 +927,15 @@ invariant_checker_t::check_for_pc_discontinuity(
     // Check for all valid transitions except taken branches. We consider taken
     // branches later so that we can provide a different message for those
     // invariant violations.
-    bool fall_through_ok = !type_is_instr_branch(prev_instr.instr.type) ||
+    bool fall_through_allowed = !type_is_instr_branch(prev_instr.instr.type) ||
         prev_instr.instr.type == TRACE_TYPE_INSTR_CONDITIONAL_JUMP ||
         prev_instr.instr.type == TRACE_TYPE_INSTR_UNTAKEN_JUMP;
     const bool valid_nonbranch_flow =
         // Filtered.
         TESTANY(OFFLINE_FILE_TYPE_FILTERED | OFFLINE_FILE_TYPE_IFILTERED,
                 shard->file_type_) ||
         // Regular fall-through.
-        (fall_through_ok && prev_instr_trace_pc + prev_instr.instr.size == cur_pc) ||
+        (fall_through_allowed && prev_instr_trace_pc + prev_instr.instr.size == cur_pc) ||
         // String loop.
         (prev_instr_trace_pc == cur_pc &&
          (memref.instr.type == TRACE_TYPE_INSTR_NO_FETCH ||
@@ -938,6 +948,9 @@ invariant_checker_t::check_for_pc_discontinuity(
         // same instruction.
         (prev_instr_trace_pc == cur_pc && at_kernel_event) ||
         // Kernel-mediated, but we can't tell if we had a thread swap.
+        // TODO i#5912: Isn't this relaxation too loose since we really only want
+        // to relax if a kernel event happened immediately prior, while prev_xfer_marker_
+        // could be many records back?
         (shard->prev_xfer_marker_.instr.tid != 0 && !at_kernel_event &&
          (shard->prev_xfer_marker_.marker.marker_type == TRACE_MARKER_TYPE_KERNEL_EVENT ||
           shard->prev_xfer_marker_.marker.marker_type == TRACE_MARKER_TYPE_KERNEL_XFER ||
@@ -949,7 +962,8 @@ invariant_checker_t::check_for_pc_discontinuity(
         // Check if the type is a branch instruction and there is a branch target
         // mismatch.
         if (type_is_instr_branch(prev_instr.instr.type)) {
-            if (knob_offline_ && shard->version_ >= TRACE_ENTRY_VERSION_BRANCH_INFO) {
+            if (knob_offline_ &&
+                shard->trace_version_ >= TRACE_ENTRY_VERSION_BRANCH_INFO) {
                 // We have precise branch target info.
                 if (prev_instr.instr.type == TRACE_TYPE_INSTR_UNTAKEN_JUMP) {
                     branch_target = prev_instr_trace_pc + prev_instr.instr.size;

clients/drcachesim/tools/invariant_checker.h

@@ -127,9 +127,9 @@ class invariant_checker_t : public analysis_tool_t {
         // We treat 0 as a sentinel; thus we do not support a trace deliberately
         // jumping to 0 and handling the fault.
         addr_t last_indirect_target_ = 0;
-        // We need an "on-deck" value to handle consecutive indirect branches.
+        // We need a dedicated variable to handle consecutive indirect branches.
         addr_t last_branch_marker_value_ = 0;
-        uintptr_t version_ = 0;
+        uintptr_t trace_version_ = 0;
 #ifdef UNIX
         // We keep track of some state per nested signal depth.
         struct signal_context {