i#6466: Fix raw2trace missing encoding error for filtered traces

clients/drcachesim/tests/raw2trace_unit_tests.cpp

@@ -169,13 +169,13 @@ class archive_ostream_test_t : public archive_ostream_t {
 };
 
 offline_entry_t
-make_header(int version = OFFLINE_FILE_VERSION)
+make_header(int version = OFFLINE_FILE_VERSION, uint64_t additional_file_types = 0)
 {
     offline_entry_t entry;
     entry.extended.type = OFFLINE_TYPE_EXTENDED;
     entry.extended.ext = OFFLINE_EXT_TYPE_HEADER;
     entry.extended.valueA = OFFLINE_FILE_TYPE_DEFAULT | OFFLINE_FILE_TYPE_ENCODINGS |
-        OFFLINE_FILE_TYPE_SYSCALL_NUMBERS;
+        OFFLINE_FILE_TYPE_SYSCALL_NUMBERS | additional_file_types;
     entry.extended.valueB = version;
     return entry;
 }
@@ -2738,6 +2738,142 @@ test_is_maybe_blocking_syscall(void *drcontext)
     return true;
 }
 
+bool
+test_ifiltered(void *drcontext)
+{
+#ifdef X86
+    std::cerr << "\n===============\nTesting ifiltered trace\n";
+    // Our synthetic test first constructs a list of instructions to be encoded into
+    // a buffer for decoding by raw2trace.
+    instrlist_t *ilist = instrlist_create(drcontext);
+    // raw2trace doesn't like offsets of 0 so we shift with a nop.
+    instr_t *nop = XINST_CREATE_nop(drcontext);
+    instr_t *move1 =
+        XINST_CREATE_move(drcontext, opnd_create_reg(REG1), opnd_create_reg(REG2));
+    instr_t *move2 =
+        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(move1));
+    // Control flow in the test assumes that memaddr stores address to jcc.
+    instr_t *jmp = XINST_CREATE_jump_reg(drcontext, OPND_CREATE_MEMPTR(REG1, 0));
+    instr_t *move3 =
+        XINST_CREATE_move(drcontext, opnd_create_reg(REG1), opnd_create_reg(REG2));
+    instrlist_append(ilist, nop);
+    instrlist_append(ilist, move1);
+    instrlist_append(ilist, jmp);
+    instrlist_append(ilist, jcc);
+    instrlist_append(ilist, move2);
+    instrlist_append(ilist, move3);
+    size_t offs_nop = 0;
+    size_t offs_move1 = offs_nop + instr_length(drcontext, nop);
+    size_t offs_jmp = offs_move1 + instr_length(drcontext, move1);
+    int jmp_length = instr_length(drcontext, jmp);
+    size_t offs_jcc = offs_jmp + jmp_length;
+    size_t offs_move2 = offs_jcc + instr_length(drcontext, jcc);
+    size_t offs_move3 = offs_move2 + instr_length(drcontext, move2);
+
+    // Now we synthesize our raw trace itself, including a valid header sequence.
+    std::vector<offline_entry_t> raw;
+    raw.push_back(make_header(OFFLINE_FILE_VERSION, OFFLINE_FILE_TYPE_IFILTERED));
+    raw.push_back(make_tid());
+    raw.push_back(make_pid());
+    raw.push_back(make_line_size());
+    // First instance of the jmp instr is filtered out but its memref is not filtered
+    // out (indicated by the zero sized block), so no encoding will be emitted and
+    // it will not count towards the chunk instr count. But this will still be
+    // accumulated as a delayed branch.
+    raw.push_back(make_block(offs_jmp, 0));
+    raw.push_back(make_memref(42));
+    // Second accumulated delayed branch.
+    raw.push_back(make_block(offs_jcc, 1));
+    // At this point, the jmp and jcc are accumulated as delayed branches.
+    // When writing the delayed branches, we want to make sure we correctly track
+    // the index into decode_pcs. If we don't increment the index at ifiltered
+    // instrs, the decode pc of jmp will be accidentally used when recording the
+    // encoding emitted for jcc. This will cause the jmp encoding to not be emitted
+    // in the next entry because raw2trace incorrectly tracked that it had
+    // already emitted it.
+    raw.push_back(make_block(offs_move1, 1));
+    // Second instance of the jmp instr is not filtered out. Its encoding must be
+    // emitted by raw2trace, or else the reader (in memref_counter_t) will
+    // complain about a missing encoding.
+    raw.push_back(make_block(offs_jmp, 1));
+    // The memref is also not filtered out. We have a separate pc entry with
+    // zero instr count just before the memref.
+    raw.push_back(make_block(offs_jmp, 0));
+    raw.push_back(make_memref(42));
+    raw.push_back(make_block(offs_jcc, 1));
+    raw.push_back(make_block(offs_move2, 1));
+    // End of first chunk.
+    raw.push_back(make_block(offs_move3, 1));
+    raw.push_back(make_exit());
+
+    static const int CHUNK_INSTR_COUNT = 5;
+    std::vector<trace_entry_t> entries;
+    if (!run_raw2trace(drcontext, raw, ilist, entries, nullptr, CHUNK_INSTR_COUNT))
+        return false;
+    int idx = 0;
+    return (
+        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) &&
+        // jmp
+        // No encoding for the i-filtered instr with 0-instr count.
+        check_entry(entries, idx, TRACE_TYPE_INSTR_INDIRECT_JUMP, 0) &&
+        check_entry(entries, idx, TRACE_TYPE_READ, -1) &&
+        // jcc
+        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
+        // Since we cannot infer branch targets accurately for i-filtered traces, this
+        // has the generic conditional jump type (instead of the more specific
+        // TRACE_TYPE_INSTR_TAKEN_JUMP type).
+        check_entry(entries, idx, TRACE_TYPE_INSTR_CONDITIONAL_JUMP, -1) &&
+        // move1
+        check_entry(entries, idx, TRACE_TYPE_ENCODING, -1) &&
+        check_entry(entries, idx, TRACE_TYPE_INSTR, -1) &&
+        // jmp
+        // This has an encoding because the previous dynamic instance was actually
+        // i-filtered.
+        check_entry(entries, idx, TRACE_TYPE_ENCODING, -1) &&
+        // i-filtered instrs have separate pc entries: one for the instr itself which
+        // has the instr length, and another for the memref which has a zero length.
+        check_entry(entries, idx, TRACE_TYPE_INSTR_INDIRECT_JUMP, jmp_length) &&
+        check_entry(entries, idx, TRACE_TYPE_INSTR_INDIRECT_JUMP, 0) &&
+        check_entry(entries, idx, TRACE_TYPE_READ, -1) &&
+        // jcc. No encoding because it has already been emitted above.
+        // Since we cannot infer branch targets accurately for i-filtered traces, this
+        // has the generic conditional jump type (instead of the more specific
+        // TRACE_TYPE_INSTR_UNTAKEN_JUMP type).
+        check_entry(entries, idx, TRACE_TYPE_INSTR_CONDITIONAL_JUMP, -1) &&
+        // move2
+        check_entry(entries, idx, TRACE_TYPE_ENCODING, -1) &&
+        check_entry(entries, idx, TRACE_TYPE_INSTR, -1) &&
+        // Chunk ends since we've seen exactly 5 instrs.
+        check_entry(entries, idx, TRACE_TYPE_MARKER, TRACE_MARKER_TYPE_CHUNK_FOOTER) &&
+        check_entry(entries, idx, TRACE_TYPE_MARKER, TRACE_MARKER_TYPE_RECORD_ORDINAL) &&
+        check_entry(entries, idx, TRACE_TYPE_MARKER, TRACE_MARKER_TYPE_TIMESTAMP) &&
+        check_entry(entries, idx, TRACE_TYPE_MARKER, TRACE_MARKER_TYPE_CPU_ID) &&
+        // move3
+        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));
+#else
+    // This test requires a CTI (so that it gets accumulated as a delayed branch) that
+    // also reads from memory (so that it's possible to have a case with a zero-sized PC
+    // entry in the raw trace). AArchXX does not have such an instr.
+    return true;
+#endif
+}
+
 int
 test_main(int argc, const char *argv[])
 {
@@ -2756,7 +2892,7 @@ test_main(int argc, const char *argv[])
         !test_midrseq_end(drcontext) || !test_xfer_modoffs(drcontext) ||
         !test_xfer_absolute(drcontext) || !test_branch_decoration(drcontext) ||
         !test_stats_timestamp_instr_count(drcontext) ||
-        !test_is_maybe_blocking_syscall(drcontext))
+        !test_is_maybe_blocking_syscall(drcontext) || !test_ifiltered(drcontext))
         return 1;
     return 0;
 }

clients/drcachesim/tracer/raw2trace.cpp

@@ -3161,19 +3161,19 @@ raw2trace_t::write(raw2trace_thread_data_t *tdata, const trace_entry_t *start,
                 start = it;
                 DEBUG_ASSERT(tdata->cur_chunk_instr_count == 0);
             }
-            if (type_is_instr(static_cast<trace_type_t>(it->type)) &&
-                // Do not count PC-only i-filtered instrs.
-                it->size > 0) {
-                accumulate_to_statistic(tdata,
-                                        RAW2TRACE_STAT_FINAL_TRACE_INSTRUCTION_COUNT, 1);
-                ++tdata->cur_chunk_instr_count;
+            if (type_is_instr(static_cast<trace_type_t>(it->type))) {
                 ++instr_ordinal;
-                if (TESTANY(OFFLINE_FILE_TYPE_ENCODINGS, tdata->file_type) &&
-                    // We don't want encodings for the PC-only i-filtered entries.
-                    it->size > 0 && instr_ordinal >= static_cast<int>(decode_pcs_size)) {
-                    tdata->error = "decode_pcs is missing entries for written "
-                                   "instructions";
-                    return false;
+                // Do not count PC-only i-filtered instrs.
+                if (it->size > 0) {
+                    accumulate_to_statistic(
+                        tdata, RAW2TRACE_STAT_FINAL_TRACE_INSTRUCTION_COUNT, 1);
+                    ++tdata->cur_chunk_instr_count;
+                    if (TESTANY(OFFLINE_FILE_TYPE_ENCODINGS, tdata->file_type) &&
+                        instr_ordinal >= static_cast<int>(decode_pcs_size)) {
+                        tdata->error = "decode_pcs is missing entries for written "
+                                       "instructions";
+                        return false;
+                    }
                 }
             }
             // Check for missing encodings after possibly opening a new chunk.