i#7470: Fix order of unreading split trace entries (#7469)

Fixes the order in which split trace entries such as
TRACE_MARKER_TYPE_SPLIT_VALUE are unread in unread_last_entry.
TRACE_MARKER_TYPE_SPLIT_VALUE must be at the front of the pre_read deque
so that it is returned _before_ the actual marker, in the same order it
is read from the raw trace.

Adds a raw2trace_unit_test that fails with "SPLIT_VALUE marker is not
adjacent to 2nd entry" due to the previously incorrect order of the
split_value marker wrt the actual marker in the deque. The error was
demonstrated using a large enough kernel_event marker value. This was
not seen on a real trace probably because kernel_event marker values do
not need the additional marker usually.

There is an additional bug where raw2trace does not fully take into
account the fact that some markers may be preceded by the
TRACE_MARKER_TYPE_SPLIT_VALUE marker. Added a TODO for now.

Issue: #7470

Author: abhinav92003

clients/drcachesim/tests/raw2trace_unit_tests.cpp

@@ -1,5 +1,5 @@
 /* **********************************************************
- * Copyright (c) 2021-2023 Google, Inc.  All rights reserved.
+ * Copyright (c) 2021-2025 Google, Inc.  All rights reserved.
  * **********************************************************/
 
 /*
@@ -441,15 +441,27 @@ test_branch_delays(void *drcontext)
         XINST_CREATE_move(drcontext, opnd_create_reg(REG1), opnd_create_reg(REG2));
     instr_t *jmp = XINST_CREATE_jump(drcontext, opnd_create_instr(move));
     instr_t *jcc = XINST_CREATE_jump_cond(drcontext, DR_PRED_EQ, opnd_create_instr(jmp));
+    instr_t *jump_reg = XINST_CREATE_jump_reg(drcontext, opnd_create_reg(REG1));
+    instr_t *move2 =
+        XINST_CREATE_move(drcontext, opnd_create_reg(REG1), opnd_create_reg(REG2));
     instrlist_append(ilist, nop);
     instrlist_append(ilist, jcc);
     instrlist_append(ilist, jmp);
     instrlist_append(ilist, move);
+    instrlist_append(ilist, jump_reg);
+    instrlist_append(ilist, move2);
     size_t offs_nop = 0;
     size_t offs_jz = offs_nop + instr_length(drcontext, nop);
     size_t offs_jmp = offs_jz + instr_length(drcontext, jcc);
     size_t offs_mov = offs_jmp + instr_length(drcontext, jmp);
+    size_t offs_jump_reg = offs_mov + instr_length(drcontext, move);
+    size_t offs_mov2 = offs_jump_reg + instr_length(drcontext, jump_reg);
 
+#ifdef X64
+    constexpr uint64_t kernel_event_value = 0x9abc12345678;
+#else
+    constexpr uint32_t kernel_event_value = 0x12345678;
+#endif
     // Now we synthesize our raw trace itself, including a valid header sequence.
     std::vector<offline_entry_t> raw;
     raw.push_back(make_header());
@@ -461,6 +473,13 @@ test_branch_delays(void *drcontext)
     raw.push_back(make_core());
     raw.push_back(make_block(offs_jmp, 1));
     raw.push_back(make_block(offs_mov, 1));
+    raw.push_back(make_block(offs_jump_reg, 1));
+#ifdef X64
+    raw.push_back(make_marker(TRACE_MARKER_TYPE_SPLIT_VALUE, (kernel_event_value >> 32)));
+#endif
+    raw.push_back(
+        make_marker(TRACE_MARKER_TYPE_KERNEL_EVENT, (kernel_event_value & 0xffffffff)));
+    raw.push_back(make_block(offs_mov2, 1));
     raw.push_back(make_exit());
 
     std::vector<trace_entry_t> entries;
@@ -493,6 +512,23 @@ test_branch_delays(void *drcontext)
         check_entry(entries, idx, TRACE_TYPE_INSTR_DIRECT_JUMP, -1) &&
         check_entry(entries, idx, TRACE_TYPE_ENCODING, -1) &&
         check_entry(entries, idx, TRACE_TYPE_INSTR, -1) &&
+        check_entry(entries, idx, TRACE_TYPE_ENCODING, -1) &&
+#ifdef X64
+        // TODO i#7470: This is a bug. When looking for the kernel_event marker,
+        // raw2trace doesn't look for the ones preceded by the split_value marker
+        // because the address was too large for a single marker. Some other marker
+        // related logic also needs to expect the split_value marker.
+        check_entry(entries, idx, TRACE_TYPE_MARKER, TRACE_MARKER_TYPE_BRANCH_TARGET,
+                    offs_mov2) &&
+#else
+        check_entry(entries, idx, TRACE_TYPE_MARKER, TRACE_MARKER_TYPE_BRANCH_TARGET,
+                    kernel_event_value) &&
+#endif
+        check_entry(entries, idx, TRACE_TYPE_INSTR_INDIRECT_JUMP, -1) &&
+        check_entry(entries, idx, TRACE_TYPE_MARKER, TRACE_MARKER_TYPE_KERNEL_EVENT,
+                    kernel_event_value) &&
+        check_entry(entries, idx, TRACE_TYPE_ENCODING, -1) &&
+        check_entry(entries, idx, TRACE_TYPE_INSTR, -1) &&
         check_entry(entries, idx, TRACE_TYPE_THREAD_EXIT, -1) &&
         check_entry(entries, idx, TRACE_TYPE_FOOTER, -1));
 }

clients/drcachesim/tracer/raw2trace.cpp

@@ -2794,12 +2794,12 @@ void
 raw2trace_t::unread_last_entry(raw2trace_thread_data_t *tdata)
 {
     VPRINT(5, "Unreading last entry\n");
+    tdata->pre_read.push_front(tdata->last_entry);
     if (tdata->last_entry_is_split) {
         VPRINT(4, "Unreading both parts of split entry at once\n");
         tdata->pre_read.push_front(tdata->last_split_first_entry);
         tdata->last_entry_is_split = false;
     }
-    tdata->pre_read.push_front(tdata->last_entry);
 }
 
 void