i#2708 trace discontinuity: add test, fix bugs found by test (#2747)

Extends the trace_invariants test from #2638 to ensure there's no
discontinuity in control flow not indicated by a branch or a kernel xfer
marker.

Adds an online trace_invariants test.

Switches the trace_invariants test to run pthreads.ptsig on UNIX, which is
marked un-FLAKY, to test both threads and signals, and on winxfer on
Windows.

Fixes several issues found by this new test:
+ Adds TRACE_TYPE_INSTR_SYSENTER to mark the PC discontinuity from
  OP_sysenter.
+ Adds proper handling of a mid-bb fault in offline trace conversion by
  looking ahead after each memref to see whether there's a marker.
+ #2011 follow-up: fixes the zero-iter code from 2772b0b which it turns out
  only worked for offline traces.  For online, the top-of-bb instr is
  jecxz, so the instr type and size were wrong.
+ Fixes pipe write splitting to avoid separating an instr from its bundle
  entries.
+ Avoids a marker for a thread init kernel xfer event on Windows.

Fixes #2708

clients/drcachesim/CMakeLists.txt

@@ -86,7 +86,7 @@ add_library(raw2trace STATIC
 configure_DynamoRIO_standalone(raw2trace)
 target_link_libraries(raw2trace drfrontendlib)
 
-add_executable(drcachesim
+set(drcachesim_srcs
   launcher.cpp
   analyzer.cpp
   analyzer_multi.cpp
@@ -99,6 +99,11 @@ add_executable(drcachesim
   tracer/instru.cpp
   tracer/instru_online.cpp
   )
+if (DEBUG)
+  # We include the invariants analyzer for testing.
+  set(drcachesim_srcs ${drcachesim_srcs} tests/trace_invariants.cpp)
+endif ()
+add_executable(drcachesim ${drcachesim_srcs})
 # In order to embed raw2trace we need to be standalone:
 configure_DynamoRIO_standalone(drcachesim)
 # Link in our tools:

clients/drcachesim/analyzer_multi.cpp

@@ -42,6 +42,9 @@
 #include "reader/ipc_reader.h"
 #include "tracer/raw2trace_directory.h"
 #include "tracer/raw2trace.h"
+#ifdef DEBUG
+# include "tests/trace_invariants.h"
+#endif
 
 analyzer_multi_t::analyzer_multi_t()
 {
@@ -123,6 +126,18 @@ analyzer_multi_t::create_analysis_tools()
     if (tools[0] == NULL)
         return false;
     num_tools = 1;
+#ifdef DEBUG
+    if (op_test_mode.get_value()) {
+        tools[1] = new trace_invariants_t(op_offline.get_value(), op_verbose.get_value());
+        if (tools[1] != NULL && !*tools[1]) {
+            delete tools[1];
+            tools[1] = NULL;
+        }
+        if (tools[1] == NULL)
+            return false;
+        num_tools = 2;
+    }
+#endif
     return true;
 }
 

clients/drcachesim/common/options.cpp

@@ -230,6 +230,12 @@ droption_t<unsigned int> op_verbose
 (DROPTION_SCOPE_ALL, "verbose", 0, 0, 64, "Verbosity level",
  "Verbosity level for notifications.");
 
+#ifdef DEBUG
+droption_t<bool> op_test_mode
+(DROPTION_SCOPE_ALL, "test_mode", false, "Run sanity tests",
+ "Run extra analyses for sanity checks on the trace.");
+#endif
+
 droption_t<std::string> op_dr_root
 (DROPTION_SCOPE_FRONTEND, "dr", "", "Path to DynamoRIO root directory",
  "Specifies the path of the DynamoRIO root directory.");

clients/drcachesim/common/options.h

@@ -86,6 +86,9 @@ extern droption_t<unsigned int> op_TLB_L2_assoc;
 extern droption_t<std::string> op_TLB_replace_policy;
 extern droption_t<std::string> op_simulator_type;
 extern droption_t<unsigned int> op_verbose;
+#ifdef DEBUG
+extern droption_t<bool> op_test_mode;
+#endif
 extern droption_t<std::string> op_dr_root;
 extern droption_t<bool> op_dr_debug;
 extern droption_t<std::string> op_dr_ops;

clients/drcachesim/common/trace_entry.h

@@ -146,6 +146,11 @@ typedef enum {
     // An internal value used for online traces and turned by reader_t into
     // either TRACE_TYPE_INSTR or TRACE_TYPE_INSTR_NO_FETCH.
     TRACE_TYPE_INSTR_MAYBE_FETCH,
+
+    // We separate out the x86 sysenter instruction as it has a hardcoded
+    // return point that shows up as a discontinuity in the user mode program
+    // counter execution sequence.
+    TRACE_TYPE_INSTR_SYSENTER,
 } trace_type_t;
 
 // The sub-type for TRACE_TYPE_MARKER.
@@ -169,11 +174,12 @@ typedef enum {
 extern const char * const trace_type_names[];
 
 // Returns whether the type represents an instruction fetch.
-// Deliberately excludes TRACE_TYPE_INSTR_NO_FETCH.
+// Deliberately excludes TRACE_TYPE_INSTR_NO_FETCH and TRACE_TYPE_INSTR_BUNDLE.
 static inline bool
 type_is_instr(const trace_type_t type)
 {
-    return (type >= TRACE_TYPE_INSTR && type <= TRACE_TYPE_INSTR_BUNDLE);
+    return (type >= TRACE_TYPE_INSTR && type <= TRACE_TYPE_INSTR_RETURN) ||
+        type == TRACE_TYPE_INSTR_SYSENTER;
 }
 
 static inline bool

clients/drcachesim/reader/reader.cpp

@@ -31,7 +31,6 @@
  */
 
 #include <assert.h>
-#include <map>
 #include "reader.h"
 #include "../common/memref.h"
 #include "../common/utils.h"
@@ -117,6 +116,7 @@ reader_t::operator++()
         case TRACE_TYPE_INSTR_DIRECT_CALL:
         case TRACE_TYPE_INSTR_INDIRECT_CALL:
         case TRACE_TYPE_INSTR_RETURN:
+        case TRACE_TYPE_INSTR_SYSENTER:
         case TRACE_TYPE_INSTR_NO_FETCH:
             have_memref = true;
             assert(cur_tid != 0 && cur_pid != 0);
@@ -141,6 +141,7 @@ reader_t::operator++()
             have_memref = true;
             // The trace stream always has the instr fetch first, which we
             // use to compute the starting PC for the subsequent instructions.
+            assert(type_is_instr(cur_ref.instr.type));
             cur_ref.instr.size = input_entry->length[bundle_idx++];
             cur_pc = next_pc;
             cur_ref.instr.addr = cur_pc;

clients/drcachesim/tests/drcachesim-invariants.templatex

@@ -0,0 +1,2 @@
+.*
+Trace invariant checks passed

clients/drcachesim/tests/trace_invariants.cpp

@@ -35,9 +35,11 @@
 #include <iostream>
 #include <string.h>
 
-trace_invariants_t::trace_invariants_t()
+trace_invariants_t::trace_invariants_t(bool offline, unsigned int verbose) :
+    knob_offline(offline), knob_verbose(verbose)
 {
     memset(&prev_instr, 0, sizeof(prev_instr));
+    memset(&prev_marker, 0, sizeof(prev_marker));
 }
 
 trace_invariants_t::~trace_invariants_t()
@@ -47,16 +49,53 @@ trace_invariants_t::~trace_invariants_t()
 bool
 trace_invariants_t::process_memref(const memref_t &memref)
 {
+    if (memref.exit.type == TRACE_TYPE_THREAD_EXIT)
+        thread_exited[memref.exit.tid] = true;
     if (type_is_instr(memref.instr.type) ||
         memref.instr.type == TRACE_TYPE_PREFETCH_INSTR ||
         memref.instr.type == TRACE_TYPE_INSTR_NO_FETCH) {
+        if (knob_verbose >= 3) {
+            std::cerr << "::" << memref.data.pid << ":" << memref.data.tid << ":: " <<
+                " @" << (void *)memref.instr.addr <<
+                ((memref.instr.type == TRACE_TYPE_INSTR_NO_FETCH) ? " non-fetched" : "")
+                << " instr x" << memref.instr.size << "\n";
+        }
         // Invariant: offline traces guarantee that a branch target must immediately
         // follow the branch w/ no intervening trace switch.
-        if (type_is_instr_branch(prev_instr.instr.type)) {
-            assert(prev_instr.instr.tid == memref.instr.tid);
+        if (knob_offline && type_is_instr_branch(prev_instr.instr.type)) {
+            assert(prev_instr.instr.tid == memref.instr.tid ||
+                   // For limited-window traces a thread might exit after a branch.
+                   thread_exited[prev_instr.instr.tid]);
+        }
+        // Invariant: non-explicit control flow (i.e., kernel-mediated) is indicated
+        // by markers.
+        if (prev_instr.instr.addr != 0/*first*/ &&
+            prev_instr.instr.tid == memref.instr.tid &&
+            !type_is_instr_branch(prev_instr.instr.type)) {
+            assert(// Regular fall-through.
+                   (prev_instr.instr.addr + prev_instr.instr.size == memref.instr.addr) ||
+                   // String loop.
+                   (prev_instr.instr.addr == memref.instr.addr &&
+                    memref.instr.type == TRACE_TYPE_INSTR_NO_FETCH) ||
+                   // Kernel-mediated.
+                   (prev_marker.instr.tid == memref.instr.tid &&
+                    (prev_marker.marker.marker_type == TRACE_MARKER_TYPE_KERNEL_EVENT ||
+                     prev_marker.marker.marker_type == TRACE_MARKER_TYPE_KERNEL_XFER)) ||
+                   prev_instr.instr.type == TRACE_TYPE_INSTR_SYSENTER);
         }
         prev_instr = memref;
     }
+    if (memref.marker.type == TRACE_TYPE_MARKER) {
+        if (knob_verbose >= 3) {
+            std::cerr << "::" << memref.data.pid << ":" << memref.data.tid << ":: " <<
+                "marker type " << memref.marker.marker_type <<
+                " value " << memref.marker.marker_value << "\n";
+        }
+        prev_marker = memref;
+        // Clear prev_instr to avoid a branch-gap failure above for things like
+        // wow64 call* NtContinue syscall.
+        memset(&prev_instr, 0, sizeof(prev_instr));
+    }
     return true;
 }
 

clients/drcachesim/tests/trace_invariants.h

@@ -38,17 +38,22 @@
 
 #include "../analysis_tool.h"
 #include "../common/memref.h"
+#include <unordered_map>
 
 class trace_invariants_t : public analysis_tool_t
 {
  public:
-    trace_invariants_t();
+    trace_invariants_t(bool offline = true, unsigned int verbose = 0);
     virtual ~trace_invariants_t();
     virtual bool process_memref(const memref_t &memref);
     virtual bool print_results();
 
  protected:
+    bool knob_offline;
+    unsigned int knob_verbose;
     memref_t prev_instr;
+    memref_t prev_marker;
+    std::unordered_map<memref_tid_t,bool> thread_exited;
 };
 
 #endif /* _TRACE_INVARIANTS_H_ */

clients/drcachesim/tools/histogram_launcher.cpp

@@ -101,7 +101,7 @@ _tmain(int argc, const TCHAR *targv[])
                               op_verbose.get_value());
     std::vector<analysis_tool_t*> tools;
     tools.push_back(tool1);
-    trace_invariants_t tool2;
+    trace_invariants_t tool2(true/*offline*/, op_verbose.get_value());
     if (op_test_mode.get_value()) {
         // We use this launcher to run tests as well:
         tools.push_back(&tool2);

clients/drcachesim/tracer/instru.cpp

@@ -54,6 +54,10 @@ instru_t::instr_to_instr_type(instr_t *instr, bool repstr_expanded)
         return TRACE_TYPE_INSTR_INDIRECT_JUMP;
     if (instr_is_cbr(instr))
         return TRACE_TYPE_INSTR_CONDITIONAL_JUMP;
+#ifdef X86
+    if (instr_get_opcode(instr) == OP_sysenter)
+        return TRACE_TYPE_INSTR_SYSENTER;
+#endif
     // i#2051: to satisfy both cache and core simulators we mark subsequent iters
     // of string loops as TRACE_TYPE_INSTR_NO_FETCH, converted from this
     // TRACE_TYPE_INSTR_MAYBE_FETCH by reader_t (since online traces would need

clients/drcachesim/tracer/instru_offline.cpp

@@ -387,8 +387,10 @@ offline_instru_t::instrument_instr(void *drcontext, void *tag, void **bb_field,
         if ((ptr_uint_t)*bb_field > MAX_INSTR_COUNT)
             return adjust;
         pc = dr_fragment_app_pc(tag);
-    } else
+    } else {
+        // XXX: For repstr do we want tag insted of skipping rep prefix?
         pc = instr_get_app_pc(app);
+    }
     adjust += insert_save_pc(drcontext, ilist, where, reg_ptr, reg_tmp, adjust,
                            pc, memref_needs_full_info ? 1 : (uint)(ptr_uint_t)*bb_field);
     if (!memref_needs_full_info)

clients/drcachesim/tracer/instru_online.cpp

@@ -283,6 +283,7 @@ online_instru_t::instrument_memref(void *drcontext, instrlist_t *ilist, instr_t
         insert_save_type_and_size(drcontext, ilist, where, reg_ptr, reg_tmp,
                                   TRACE_TYPE_INSTR, 0, adjust);
         insert_save_pc(drcontext, ilist, where, reg_ptr, reg_tmp,
+                       // XXX: For repstr do we want tag insted of skipping rep prefix?
                        instr_get_app_pc(app), adjust);
         adjust += sizeof(trace_entry_t);
     }
@@ -310,11 +311,18 @@ online_instru_t::instrument_instr(void *drcontext, void *tag, void **bb_field,
                                   instr_t *app)
 {
     bool repstr_expanded = *bb_field != 0; // Avoid cl warning C4800.
+    app_pc pc = repstr_expanded ? dr_fragment_app_pc(tag) : instr_get_app_pc(app);
+    // To handle zero-iter repstr loops this routine is called at the top of the bb
+    // where "app" is jecxz so we have to hardcode the rep str type and get length
+    // from the tag.
+    ushort type = repstr_expanded ? TRACE_TYPE_INSTR_MAYBE_FETCH :
+        instr_to_instr_type(app, repstr_expanded);
+    ushort size = repstr_expanded ?
+        (ushort)decode_sizeof(drcontext, pc, NULL _IF_X86_64(NULL)) :
+        (ushort)instr_length(drcontext, app);
     insert_save_type_and_size(drcontext, ilist, where, reg_ptr, reg_tmp,
-                              instr_to_instr_type(app, repstr_expanded),
-                              (ushort)instr_length(drcontext, app), adjust);
-    insert_save_pc(drcontext, ilist, where, reg_ptr, reg_tmp,
-                   instr_get_app_pc(app), adjust);
+                              type, size, adjust);
+    insert_save_pc(drcontext, ilist, where, reg_ptr, reg_tmp, pc, adjust);
     return (adjust + sizeof(trace_entry_t));
 }
 

clients/drcachesim/tracer/raw2trace.cpp

@@ -277,28 +277,37 @@ raw2trace_t::unmap_modules(void)
  * Disassembly to fill in instr and memref entries
  */
 
+#define FAULT_INTERRUPTED_BB "INTERRUPTED"
+
+// Returns FAULT_INTERRUPTED_BB if a fault occurred on this memref.
+// Any other non-empty string is a fatal error.
 std::string
 raw2trace_t::append_memref(INOUT trace_entry_t **buf_in, uint tidx, instr_t *instr,
                            opnd_t ref, bool write)
 {
     trace_entry_t *buf = *buf_in;
-    offline_entry_t in_entry;
-    if (!thread_files[tidx]->read((char*)&in_entry, sizeof(in_entry)))
+    // To avoid having to backtrack later, we read 2 ahead to see whether this memref
+    // faulted.  There's a footer so this should always succeed.
+    offline_entry_t in_entry[2];
+    if (!thread_files[tidx]->read((char*)in_entry, sizeof(in_entry)))
         return "Trace ends mid-block";
-    if (in_entry.addr.type != OFFLINE_TYPE_MEMREF &&
-        in_entry.addr.type != OFFLINE_TYPE_MEMREF_HIGH) {
+    if (in_entry[0].addr.type != OFFLINE_TYPE_MEMREF &&
+        in_entry[0].addr.type != OFFLINE_TYPE_MEMREF_HIGH) {
         // This happens when there are predicated memrefs in the bb, or for a
         // zero-iter rep string loop.  For predicated memrefs,
         // they could be earlier, so "instr" may not itself be predicated.
         // XXX i#2015: if there are multiple predicated memrefs, our instr vs
         // data stream may not be in the correct order here.
         VPRINT(4, "Missing memref from predication or 0-iter repstr (next type is 0x"
-               ZHEX64_FORMAT_STRING ")\n", in_entry.combined_value);
-        // Put back the entry.
+               ZHEX64_FORMAT_STRING ")\n", in_entry[0].combined_value);
+        // Put back both entries.
         thread_files[tidx]->seekg(-(std::streamoff)sizeof(in_entry),
                                   thread_files[tidx]->cur);
         return "";
     }
+    // Put back the 2nd entry.
+    thread_files[tidx]->seekg(-(std::streamoff)sizeof(in_entry[0]),
+                              thread_files[tidx]->cur);
     if (instr_is_prefetch(instr)) {
         buf->type = instru_t::instr_to_prefetch_type(instr);
         buf->size = 1;
@@ -313,9 +322,16 @@ raw2trace_t::append_memref(INOUT trace_entry_t **buf_in, uint tidx, instr_t *ins
         buf->size = (ushort) opnd_size_in_bytes(opnd_get_size(ref));
     }
     // We take the full value, to handle low or high.
-    buf->addr = (addr_t) in_entry.combined_value;
+    buf->addr = (addr_t) in_entry[0].combined_value;
     VPRINT(4, "Appended memref to " PFX "\n", (ptr_uint_t)buf->addr);
     *buf_in = ++buf;
+    if (in_entry[1].extended.type == OFFLINE_TYPE_EXTENDED &&
+        in_entry[1].extended.ext == OFFLINE_EXT_TYPE_MARKER &&
+        in_entry[1].extended.valueB == TRACE_MARKER_TYPE_KERNEL_EVENT) {
+        // A signal/exception interrupted the bb after the memref.
+        VPRINT(4, "Signal/exception interrupted the bb\n");
+        return FAULT_INTERRUPTED_BB;
+    }
     return "";
 }
 
@@ -341,6 +357,7 @@ raw2trace_t::append_bb_entries(uint tidx, offline_entry_t *in_entry, OUT bool *h
                (ptr_uint_t)in_entry->pc.modoffs, modvec[in_entry->pc.modidx].path);
     }
     bool skip_icache = false;
+    bool truncated = false; // Whether a fault ended the bb early.
     if (instr_count == 0) {
         // L0 filtering adds a PC entry with a count of 0 prior to each memref.
         skip_icache = true;
@@ -350,7 +367,7 @@ raw2trace_t::append_bb_entries(uint tidx, offline_entry_t *in_entry, OUT bool *h
             instrs_are_separate = true;
     }
     CHECK(!instrs_are_separate || instr_count == 1, "cannot mix 0-count and >1-count");
-    for (uint i = 0; i < instr_count; ++i) {
+    for (uint i = 0; !truncated && i < instr_count; ++i) {
         trace_entry_t *buf = buf_start;
         app_pc orig_pc = decode_pc - modvec[in_entry->pc.modidx].map_base +
             modvec[in_entry->pc.modidx].orig_base;
@@ -403,19 +420,25 @@ raw2trace_t::append_bb_entries(uint tidx, offline_entry_t *in_entry, OUT bool *h
         if ((!instrs_are_separate || skip_icache) &&
             // Rule out OP_lea.
             (instr_reads_memory(instr) || instr_writes_memory(instr))) {
-            for (int i = 0; i < instr_num_srcs(instr); i++) {
-                if (opnd_is_memory_reference(instr_get_src(instr, i))) {
+            for (int j = 0; j < instr_num_srcs(instr); j++) {
+                if (opnd_is_memory_reference(instr_get_src(instr, j))) {
                     std::string error = append_memref(&buf, tidx, instr,
-                                                      instr_get_src(instr, i), false);
-                    if (!error.empty())
+                                                      instr_get_src(instr, j), false);
+                    if (error == FAULT_INTERRUPTED_BB) {
+                        truncated = true;
+                        break;
+                    } else if (!error.empty())
                         return error;
                 }
             }
-            for (int i = 0; i < instr_num_dsts(instr); i++) {
-                if (opnd_is_memory_reference(instr_get_dst(instr, i))) {
+            for (int j = 0; !truncated && j < instr_num_dsts(instr); j++) {
+                if (opnd_is_memory_reference(instr_get_dst(instr, j))) {
                     std::string error = append_memref(&buf, tidx, instr,
-                                                      instr_get_dst(instr, i), true);
-                    if (!error.empty())
+                                                      instr_get_dst(instr, j), true);
+                    if (error == FAULT_INTERRUPTED_BB) {
+                        truncated = true;
+                        break;
+                    } else if (!error.empty())
                         return error;
                 }
             }