i#6495: Handle invariant errors in x86 QEMU syscall templates (Dynamo…

…RIO#6718)

Handles various invariant errors seen in system call trace templates
collected on x86 QEMU.

Modifies syscall trace template file format to use the
TRACE_MARKER_TYPE_SYSCALL_TRACE_START and
TRACE_MARKER_TYPE_SYSCALL_TRACE_END markers to show start and end
respectively of each syscall trace template, instead of separating them
using a TRACE_MARKER_TYPE_SYSCALL marker. This makes it easier to write
invariant checks that also work for the syscall trace template file (in
addition to an actual trace file injected with trace templates). Also adds
cache line size and page size markers to the template, similar to the
context switch sequence template file.

Handles cases where there are a different number of read/write records
than expected by the decoder; after iret, variants of xrstor, variants
of xsaves, and prefetch instrs.

Relaxes the PC discontinuity check after hlt, and within two instrs of
sti (which enables interrupts, so there may be an interrupt shortly
after, as empirically seen in some QEMU syscall trace templates).

Makes other misc changes to make sure the syscall trace template file
passes the invariant checker: add thread exit (since we already have a
thread start), relaxation of various invariant checks.

Adds and implements the instr_is_xrstor API that identifies variants of
the xrstor opcode, and adds supervisor versions of xsave to
instr_is_xsave.

Adds unit tests for these new scenarios. Added a TODO to handle other
arch equivalent versions of these scenarios.

Adds a new flag `-abort_on_invariant_error` which is true by default, to
allow the user to instruct the invariant checker to continue past
invariant errors (using `-no_abort_on_invariant_error`). This is helpful
since there are still a few instances of some invariant errors in the
syscall trace template that are harder to generalize and fix/ignore.

Issue: DynamoRIO#6495

api/docs/release.dox

@@ -212,6 +212,11 @@ Further non-compatibility-affecting changes include:
    #dynamorio::drmemtrace::analysis_tool_t to allow the tool to make holistic
    adjustments to the interval snapshots after all have been generated, and before
    they are used for merging across shards (potentially), and printing the results.
+ - Added -abort_on_invariant_error flag that instructs the invariant checker drmemtrace
+   analysis tool to abort trace analysis when a trace invariant error is found. This
+   is set to true by default to match the existing behavior of the invariant checker.
+ - Added a new instr API instr_is_xrstor() that tells whether an instruction is any
+   variant of the x86 xrstor opcode.
 
 **************************************************
 <hr>

clients/drcachesim/analyzer_multi.cpp

@@ -175,7 +175,8 @@ analyzer_multi_t::create_invariant_checker()
     }
     return new invariant_checker_t(op_offline.get_value(), op_verbose.get_value(),
                                    op_test_mode_name.get_value(),
-                                   serial_schedule_file_.get(), cpu_schedule_file_.get());
+                                   serial_schedule_file_.get(), cpu_schedule_file_.get(),
+                                   op_abort_on_invariant_error.get_value());
 }
 
 template <>

clients/drcachesim/common/options.cpp

@@ -931,7 +931,8 @@ droption_t<std::string> op_syscall_template_file(
     "Path to the file that contains system call trace templates.",
     "Path to the file that contains system call trace templates. "
     "If set, system call traces will be injected from the file "
-    "into the resulting trace.");
+    "into the resulting trace. This is still experimental so the template file "
+    "format may change without backward compatibility.");
 
 // Record filter options.
 droption_t<uint64_t> op_filter_stop_timestamp(
@@ -976,5 +977,13 @@ droption_t<uint64_t> op_trim_after_timestamp(
     "Removes all records from the first TRACE_MARKER_TYPE_TIMESTAMP marker with "
     "timestamp larger than the specified value.");
 
+droption_t<bool> op_abort_on_invariant_error(
+    DROPTION_SCOPE_ALL, "abort_on_invariant_error", true,
+    "Abort invariant checker when a trace invariant error is found.",
+    "When set to true, the trace invariant checker analysis tool aborts when a trace "
+    "invariant error is found. Otherwise it prints the error and continues. Also, the "
+    "total invariant error count is printed at the end; a non-zero error count does not "
+    "affect the exit code of the analyzer.");
+
 } // namespace drmemtrace
 } // namespace dynamorio

clients/drcachesim/common/options.h

@@ -215,6 +215,7 @@ extern dynamorio::droption::droption_t<std::string> op_filter_trace_types;
 extern dynamorio::droption::droption_t<std::string> op_filter_marker_types;
 extern dynamorio::droption::droption_t<uint64_t> op_trim_before_timestamp;
 extern dynamorio::droption::droption_t<uint64_t> op_trim_after_timestamp;
+extern dynamorio::droption::droption_t<bool> op_abort_on_invariant_error;
 
 } // namespace drmemtrace
 } // namespace dynamorio

clients/drcachesim/common/trace_entry.h

@@ -940,9 +940,10 @@ typedef enum {
     OFFLINE_FILE_TYPE_BIMODAL_FILTERED_WARMUP = 0x2000,
     /**
      * Indicates an offline trace that contains trace templates for some system calls.
-     * The individual traces are separated by a #TRACE_MARKER_TYPE_SYSCALL marker which
-     * also specifies what system call the following trace belongs to. This file can be
-     * used with -syscall_template_file to raw2trace to create a
+     * The individual traces are enclosed within a pair of
+     * #TRACE_MARKER_TYPE_SYSCALL_TRACE_START and #TRACE_MARKER_TYPE_SYSCALL_TRACE_END
+     * markers which also specify what system call the contained trace belongs to. This
+     * file can be used with -syscall_template_file to raw2trace to create an
      * #OFFLINE_FILE_TYPE_KERNEL_SYSCALLS trace. See the sample file written by the
      * burst_syscall_inject.cpp test for more details on the expected format for the
      * system call template file.

clients/drcachesim/tests/burst_syscall_inject.cpp

@@ -1,5 +1,5 @@
 /* **********************************************************
- * Copyright (c) 2016-2023 Google, Inc.  All rights reserved.
+ * Copyright (c) 2016-2024 Google, Inc.  All rights reserved.
  * **********************************************************/
 
 /*
@@ -141,16 +141,22 @@ write_system_call_template(void *dr_context)
         trace_entry_t to_write = *reinterpret_cast<trace_entry_t *>(buf_at);
         write_trace_entry(writer, to_write);
     }
+    write_trace_entry(writer, make_marker(TRACE_MARKER_TYPE_CACHE_LINE_SIZE, 64));
+    write_trace_entry(writer, make_marker(TRACE_MARKER_TYPE_PAGE_SIZE, 4096));
 
     // Write the trace template for SYS_getpid.
-    write_trace_entry(writer, make_marker(TRACE_MARKER_TYPE_SYSCALL, SYS_getpid));
+    write_trace_entry(writer,
+                      make_marker(TRACE_MARKER_TYPE_SYSCALL_TRACE_START, SYS_getpid));
     // Just a random instruction.
     instr_in_getpid = XINST_CREATE_nop(dr_context);
     write_instr_entry(dr_context, writer, instr_in_getpid,
                       reinterpret_cast<app_pc>(PC_SYSCALL_GETPID));
+    write_trace_entry(writer,
+                      make_marker(TRACE_MARKER_TYPE_SYSCALL_TRACE_END, SYS_getpid));
 
     // Write the trace template for SYS_gettid.
-    write_trace_entry(writer, make_marker(TRACE_MARKER_TYPE_SYSCALL, SYS_gettid));
+    write_trace_entry(writer,
+                      make_marker(TRACE_MARKER_TYPE_SYSCALL_TRACE_START, SYS_gettid));
     // Just a random instruction.
 #ifdef X86
 #    define TEST_REG DR_REG_XDX
@@ -167,7 +173,14 @@ write_system_call_template(void *dr_context)
     write_trace_entry(
         writer,
         make_memref(READ_MEMADDR_GETTID, TRACE_TYPE_READ, opnd_size_in_bytes(OPSZ_PTR)));
+    write_trace_entry(writer,
+                      make_marker(TRACE_MARKER_TYPE_SYSCALL_TRACE_END, SYS_gettid));
 
+    // Write footer.
+    dynamorio::drmemtrace::trace_entry_t thread_exit = {
+        dynamorio::drmemtrace::TRACE_TYPE_THREAD_EXIT, 0, { /*tid=*/1 }
+    };
+    write_trace_entry(writer, thread_exit);
     write_trace_entry(writer, make_footer());
     std::cerr << "Done writing system call trace template\n";
     return syscall_trace_template_file;
@@ -406,7 +419,8 @@ test_main(int argc, const char *argv[])
     std::cerr << "Getting basic counts for system call trace template\n";
     basic_counts_t::counters_t template_counts = get_basic_counts(syscall_trace_template);
     if (!(template_counts.instrs == 2 && template_counts.encodings == 2 &&
-          template_counts.syscall_number_markers == 2)) {
+          // We only have trace start and end markers, no syscall number markers.
+          template_counts.syscall_number_markers == 0)) {
         std::cerr << "Unexpected counts in system call trace template: "
                   << syscall_trace_template << ": #instrs: " << template_counts.instrs
                   << ", #encodings: " << template_counts.encodings

clients/drcachesim/tests/invariant_checker_test.cpp

@@ -3165,6 +3165,160 @@ check_kernel_syscall_trace(void)
                          "Failed to catch nested syscall traces"))
             res = false;
     }
+#    ifdef X86
+    // TODO i#6495: Adapt this test to AArch64-equivalent scenarios.
+    {
+        instr_t *move1 = XINST_CREATE_move(GLOBAL_DCONTEXT, opnd_create_reg(REG1),
+                                           opnd_create_reg(REG2));
+        instr_t *iret = INSTR_CREATE_iret(GLOBAL_DCONTEXT);
+        instr_t *sti = INSTR_CREATE_sti(GLOBAL_DCONTEXT);
+        instr_t *nop1 = XINST_CREATE_nop(GLOBAL_DCONTEXT);
+        instr_t *nop2 = XINST_CREATE_nop(GLOBAL_DCONTEXT);
+        instr_t *xrstors = INSTR_CREATE_xrstors64(
+            GLOBAL_DCONTEXT,
+            opnd_create_base_disp(DR_REG_XCX, DR_REG_NULL, 0, 0, OPSZ_xsave));
+        instr_t *xsaves = INSTR_CREATE_xsaves64(
+            GLOBAL_DCONTEXT,
+            opnd_create_base_disp(DR_REG_XCX, DR_REG_NULL, 0, 0, OPSZ_xsave));
+        instr_t *hlt = INSTR_CREATE_hlt(GLOBAL_DCONTEXT);
+        instr_t *nop3 = XINST_CREATE_nop(GLOBAL_DCONTEXT);
+        instr_t *prefetch = INSTR_CREATE_prefetchnta(
+            GLOBAL_DCONTEXT,
+            opnd_create_base_disp(DR_REG_XCX, DR_REG_NULL, 0, 0, OPSZ_1));
+        instr_t *sysret = INSTR_CREATE_sysret(GLOBAL_DCONTEXT);
+        instr_t *nop4 = XINST_CREATE_nop(GLOBAL_DCONTEXT);
+        instr_t *sys1 = instr_clone(GLOBAL_DCONTEXT, sys);
+        instr_t *nop5 = XINST_CREATE_nop(GLOBAL_DCONTEXT);
+        instr_t *sys2 = instr_clone(GLOBAL_DCONTEXT, sys);
+        instr_t *nop6 = XINST_CREATE_nop(GLOBAL_DCONTEXT);
+        instrlist_t *ilist2 = instrlist_create(GLOBAL_DCONTEXT);
+        instrlist_append(ilist2, move1);
+        instrlist_append(ilist2, iret);
+        instrlist_append(ilist2, sti);
+        instrlist_append(ilist2, nop1);
+        instrlist_append(ilist2, nop2);
+        instrlist_append(ilist2, xrstors);
+        instrlist_append(ilist2, xsaves);
+        instrlist_append(ilist2, hlt);
+        instrlist_append(ilist2, nop3);
+        instrlist_append(ilist2, prefetch);
+        instrlist_append(ilist2, sysret);
+        instrlist_append(ilist2, nop4);
+        instrlist_append(ilist2, sys1);
+        instrlist_append(ilist2, nop5);
+        instrlist_append(ilist2, sys2);
+        instrlist_append(ilist2, nop6);
+        {
+            std::vector<memref_with_IR_t> memref_instr_vec = {
+                { gen_marker(TID_A, TRACE_MARKER_TYPE_FILETYPE,
+                             OFFLINE_FILE_TYPE_ENCODINGS |
+                                 OFFLINE_FILE_TYPE_SYSCALL_NUMBERS |
+                                 OFFLINE_FILE_TYPE_KERNEL_SYSCALLS),
+                  nullptr },
+                { gen_marker(TID_A, TRACE_MARKER_TYPE_CACHE_LINE_SIZE, 64), nullptr },
+                { gen_marker(TID_A, TRACE_MARKER_TYPE_PAGE_SIZE, 4096), nullptr },
+                { gen_instr(TID_A), sys1 },
+                { gen_marker(TID_A, TRACE_MARKER_TYPE_SYSCALL, 42), nullptr },
+                { gen_marker(TID_A, TRACE_MARKER_TYPE_SYSCALL_TRACE_START, 42), nullptr },
+                { gen_instr(TID_A), move1 },
+                { gen_instr(TID_A), iret },
+                // Multiple reads. Acceptable because of the prior iret.
+                { gen_data(TID_A, true, 42, 8), nullptr },
+                { gen_data(TID_A, true, 42, 8), nullptr },
+                { gen_data(TID_A, true, 42, 8), nullptr },
+                { gen_data(TID_A, true, 42, 8), nullptr },
+                { gen_instr(TID_A), sti },
+                { gen_instr(TID_A), nop1 },
+                // Missing nop2. Acceptable because of the recent sti.
+                { gen_instr(TID_A), xrstors },
+                // Multiple reads. Acceptable because of the prior xrstors.
+                { gen_data(TID_A, true, 42, 8), nullptr },
+                { gen_data(TID_A, true, 42, 8), nullptr },
+                { gen_data(TID_A, true, 42, 8), nullptr },
+                { gen_data(TID_A, true, 42, 8), nullptr },
+                { gen_instr(TID_A), xsaves },
+                // Multiple writes. Acceptable because of the prior xsaves.
+                { gen_data(TID_A, false, 42, 8), nullptr },
+                { gen_data(TID_A, false, 42, 8), nullptr },
+                { gen_data(TID_A, false, 42, 8), nullptr },
+                { gen_data(TID_A, false, 42, 8), nullptr },
+                { gen_instr(TID_A), hlt },
+                // Missing nop3. Acceptable because of the prior hlt.
+                { gen_instr(TID_A), prefetch },
+                // Missing reads. Acceptable because of the prior prefetch.
+                { gen_instr(TID_A), sysret },
+                { gen_marker(TID_A, TRACE_MARKER_TYPE_SYSCALL_TRACE_END, 42), nullptr },
+                // Continues after sys1.
+                { gen_instr(TID_A), nop5 },
+                { gen_instr(TID_A), sys2 },
+                { gen_marker(TID_A, TRACE_MARKER_TYPE_SYSCALL, 41), nullptr },
+                { gen_marker(TID_A, TRACE_MARKER_TYPE_SYSCALL_TRACE_START, 41), nullptr },
+                { gen_instr(TID_A), move1 },
+                { gen_marker(TID_A, TRACE_MARKER_TYPE_SYSCALL_TRACE_END, 41), nullptr },
+                // Continues after sys2.
+                { gen_instr(TID_A), nop6 },
+                { gen_exit(TID_A), nullptr }
+            };
+            auto memrefs = add_encodings_to_memrefs(ilist2, memref_instr_vec, BASE_ADDR);
+            if (!run_checker(memrefs, false))
+                res = false;
+        }
+        {
+            std::vector<memref_with_IR_t> memref_instr_vec = {
+                { gen_marker(TID_A, TRACE_MARKER_TYPE_FILETYPE,
+                             OFFLINE_FILE_TYPE_ENCODINGS |
+                                 OFFLINE_FILE_TYPE_SYSCALL_NUMBERS |
+                                 OFFLINE_FILE_TYPE_KERNEL_SYSCALLS),
+                  nullptr },
+                { gen_marker(TID_A, TRACE_MARKER_TYPE_CACHE_LINE_SIZE, 64), nullptr },
+                { gen_marker(TID_A, TRACE_MARKER_TYPE_PAGE_SIZE, 4096), nullptr },
+                { gen_instr(TID_A), sys1 },
+                { gen_marker(TID_A, TRACE_MARKER_TYPE_SYSCALL, 42), nullptr },
+                { gen_marker(TID_A, TRACE_MARKER_TYPE_SYSCALL_TRACE_START, 42), nullptr },
+                { gen_instr(TID_A), move1 },
+                { gen_marker(TID_A, TRACE_MARKER_TYPE_SYSCALL_TRACE_END, 42), nullptr },
+                // Missing instrs.
+                { gen_instr(TID_A), nop6 },
+                { gen_exit(TID_A), nullptr }
+            };
+            auto memrefs = add_encodings_to_memrefs(ilist2, memref_instr_vec, BASE_ADDR);
+            if (!run_checker(memrefs, true,
+                             { "Non-explicit control flow has no marker",
+                               /*tid=*/TID_A,
+                               /*ref_ordinal=*/9, /*last_timestamp=*/0,
+                               /*instrs_since_last_timestamp=*/3 },
+                             "Failed to catch discontinuity on return from syscall"))
+                res = false;
+        }
+        std::vector<memref_with_IR_t> memref_instr_vec = {
+            { gen_marker(TID_A, TRACE_MARKER_TYPE_FILETYPE,
+                         OFFLINE_FILE_TYPE_ENCODINGS | OFFLINE_FILE_TYPE_SYSCALL_NUMBERS |
+                             OFFLINE_FILE_TYPE_KERNEL_SYSCALLS),
+              nullptr },
+            { gen_marker(TID_A, TRACE_MARKER_TYPE_CACHE_LINE_SIZE, 64), nullptr },
+            { gen_marker(TID_A, TRACE_MARKER_TYPE_PAGE_SIZE, 4096), nullptr },
+            { gen_instr(TID_A), sys1 },
+            { gen_marker(TID_A, TRACE_MARKER_TYPE_SYSCALL, 42), nullptr },
+            { gen_marker(TID_A, TRACE_MARKER_TYPE_SYSCALL_TRACE_START, 42), nullptr },
+            { gen_instr(TID_A), move1 },
+            // Missing instrs.
+            { gen_instr(TID_A), sti },
+            { gen_marker(TID_A, TRACE_MARKER_TYPE_SYSCALL_TRACE_END, 42), nullptr },
+            { gen_instr(TID_A), nop5 },
+            { gen_exit(TID_A), nullptr }
+        };
+        {
+            auto memrefs = add_encodings_to_memrefs(ilist2, memref_instr_vec, BASE_ADDR);
+            if (!run_checker(memrefs, true,
+                             { "Non-explicit control flow has no marker",
+                               /*tid=*/TID_A,
+                               /*ref_ordinal=*/8, /*last_timestamp=*/0,
+                               /*instrs_since_last_timestamp=*/3 },
+                             "Failed to catch discontinuity inside syscall trace"))
+                res = false;
+        }
+    }
+#    endif
     return res;
 #endif
 }