i#5390: Full handling of emulation markers in drbbdup

Fixes several shortcomings in the initial attempt to use emulation
markers to support the drmgr emulation API in drbbdup, and fixes
related issues in drmemtrace when it uses drbbdup.

Adds missing emulation markers for a special instr for the last bbdup case (previously only the earlier cases were marked).

Removes emulation markers from the analysis copy in
drbbdup_extract_bb_copy() (otherwise drmemtrace sees them and
incorrectly disables elision).

Fixes the drmemtrace check for elision labels to use "where" except
when "app" is actually an exclusive store, to properly find the labels
and elide.

Enables the tools.drcacheoff.windows-invar test which now passes on
all platforms.

Updates the drbbdup-emul-test to cover the drbbdup changes.

Fixes #5390

clients/drcachesim/tracer/instru_offline.cpp

@@ -559,8 +559,12 @@ offline_instru_t::instrument_memref(void *drcontext, instrlist_t *ilist, instr_t
 {
     // Check whether we can elide this address.
     // Be sure to start with "app" and not "where" to handle post-instr insertion
-    // such as for exclusive stores.
-    for (instr_t *prev = instr_get_prev(app); prev != nullptr && !instr_is_app(prev);
+    // for exclusive stores -- but otherwise we want "where" for drbbdup's handling
+    // of block-final instrs.
+    instr_t *start = where;
+    if (instr_is_exclusive_store(app))
+        start = app;
+    for (instr_t *prev = instr_get_prev(start); prev != nullptr && !instr_is_app(prev);
          prev = instr_get_prev(prev)) {
         int elided_index;
         bool elided_is_store;

clients/drcachesim/tracer/raw2trace.cpp

@@ -944,10 +944,11 @@ raw2trace_t::get_next_entry(void *tls)
                                       sizeof(tdata->last_entry)))
             return nullptr;
     }
-    VPRINT(5, "[get_next_entry]: type=%d\n",
+    VPRINT(5, "[get_next_entry]: type=%d val=" HEX64_FORMAT_STRING "\n",
            // Some compilers think .addr.type is "int" while others think it's "unsigned
            // long".  We avoid dueling warnings by casting to int.
-           static_cast<int>(tdata->last_entry.addr.type));
+           static_cast<int>(tdata->last_entry.addr.type),
+           tdata->last_entry.combined_value);
     return &tdata->last_entry;
 }
 
@@ -971,6 +972,7 @@ void
 raw2trace_t::unread_last_entry(void *tls)
 {
     auto tdata = reinterpret_cast<raw2trace_thread_data_t *>(tls);
+    VPRINT(5, "Unreading last entry\n");
     if (tdata->last_entry_is_split) {
         VPRINT(4, "Unreading both parts of split entry at once\n");
         tdata->pre_read.push_back(tdata->last_split_first_entry);

clients/drcachesim/tracer/raw2trace.h

@@ -739,6 +739,7 @@ template <typename T> class trace_converter_t {
                 buf += sizeof(*entry);
             } else {
                 // We should see an instr entry first
+                impl()->log(3, "extra memref entry: %p\n", in_entry->addr.addr);
                 return "memref entry found outside of bb";
             }
         } else if (in_entry->pc.type == OFFLINE_TYPE_PC) {

ext/drbbdup/drbbdup.c

@@ -493,9 +493,16 @@ drbbdup_set_up_copies(void *drcontext, instrlist_t *bb, drbbdup_manager_t *manag
      * last. Again, this is to abide by DR rules.
      */
     last = instrlist_last(bb);
-    if (drbbdup_is_special_instr(last))
+    if (drbbdup_is_special_instr(last)) {
+        emulated_instr_t emulated_instr;
+        emulated_instr.size = sizeof(emulated_instr);
+        emulated_instr.pc = instr_get_app_pc(last);
+        emulated_instr.instr = instr_clone(drcontext, last); /* Freed by label. */
+        emulated_instr.flags = 0;
+        drmgr_insert_emulation_start(drcontext, bb, last, &emulated_instr);
         instrlist_meta_preinsert(bb, last, exit_label);
-    else
+        drmgr_insert_emulation_end(drcontext, bb, last);
+    } else
         instrlist_meta_postinsert(bb, last, exit_label);
 }
 
@@ -509,7 +516,6 @@ static dr_emit_flags_t
 drbbdup_do_duplication(hashtable_t *manager_table, void *drcontext, void *tag,
                        instrlist_t *bb, bool for_trace, bool translating)
 {
-
     drbbdup_manager_t *manager =
         (drbbdup_manager_t *)hashtable_lookup(manager_table, tag);
 
@@ -629,6 +635,37 @@ drbbdup_is_at_end(instr_t *check_instr)
     return false;
 }
 
+/* Returns true if at the start of the end of a bb version: if check_instr is
+ * the start emulation label for the inserted jump or the exit label.  If there
+ * are no emulation labels this is equivalent to drbbdup_is_at_end().
+ */
+static bool
+drbbdup_is_at_end_initial(instr_t *check_instr)
+{
+    /* We need to stop at the emulation start label so that drmgr will point
+     * there for drmgr_orig_app_instr_for_*().
+     */
+    if (!drmgr_is_emulation_start(check_instr))
+        return false;
+    instr_t *next_instr = instr_get_next(check_instr);
+    if (next_instr == NULL)
+        return false;
+
+    if (drbbdup_is_at_label(next_instr, DRBBDUP_LABEL_EXIT))
+        return true;
+
+    if (instr_is_cti(next_instr)) {
+        next_instr = instr_get_next(next_instr);
+        return drbbdup_is_at_label(next_instr, DRBBDUP_LABEL_START) ||
+            /* There may be an emulation endpoint label in between. */
+            (next_instr != NULL && instr_get_next(next_instr) != NULL &&
+             drmgr_is_emulation_end(next_instr) &&
+             drbbdup_is_at_label(instr_get_next(next_instr), DRBBDUP_LABEL_START));
+    }
+
+    return false;
+}
+
 static bool
 drbbdup_is_exit_jmp_emulation_marker(instr_t *check_instr)
 {
@@ -713,7 +750,11 @@ drbbdup_extract_bb_copy(void *drcontext, instrlist_t *bb, instr_t *start,
     ASSERT(instr_get_note(start) == (void *)DRBBDUP_LABEL_START,
            "start instruction should be a START label");
 
-    *post = drbbdup_next_end(start);
+    /* Use end_initial to avoid placing emulation markers in the list at all
+     * (no need since we have the real final instr, and the markers mess up
+     * things like drmemtrace elision).
+     */
+    *post = drbbdup_next_end_initial(start);
     ASSERT(*post != NULL, "end instruction cannot be NULL");
     ASSERT(!drbbdup_is_at_start(*post), "end cannot be at start");
 
@@ -1487,10 +1528,11 @@ drbbdup_instrument_dups(void *drcontext, void *tag, instrlist_t *bb, instr_t *in
         /* XXX i#4134: statistics -- insert code that tracks the number of times the
          * current case (pt->case_index) is executed.
          */
-    } else if (drbbdup_is_exit_jmp_emulation_marker(instr)) {
-        /* Ignore instruction: hide drbbdup's own markers. */
-    } else if (drbbdup_is_at_end(instr)) {
-        /* Handle last special instruction (if present). */
+    } else if (drbbdup_is_at_end_initial(instr)) {
+        /* Handle last special instruction (if present).
+         * We use drbbdup_is_at_end_initial() to ensure drmgr will point to the
+         * emulation data we setup for the exit label.
+         */
         if (is_last_special) {
             flags = drbbdup_instrument_instr(drcontext, tag, bb, last, instr, for_trace,
                                              translating, pt, manager);
@@ -1500,6 +1542,10 @@ drbbdup_instrument_dups(void *drcontext, void *tag, instrlist_t *bb, instr_t *in
             }
         }
         drreg_restore_all(drcontext, bb, instr);
+    } else if (drbbdup_is_at_end(instr) && !is_last_special) {
+        drreg_restore_all(drcontext, bb, instr);
+    } else if (drbbdup_is_at_end(instr) || drbbdup_is_exit_jmp_emulation_marker(instr)) {
+        /* Ignore instruction: hide drbbdup's own markers and the rest of the end. */
     } else if (pt->case_index == DRBBDUP_IGNORE_INDEX) {
         /* Ignore instruction. */
         ASSERT(drbbdup_is_special_instr(instr), "ignored instr should be cti or syscall");

suite/tests/CMakeLists.txt

@@ -3609,12 +3609,10 @@ if (BUILD_CLIENTS)
     torunonly_drcacheoff(windows-simple ${ci_shared_app}
       "-trace_after_instrs 20K -trace_for_instrs 5K -retrace_every_instrs 35K"
       "@-simulator_type@basic_counts" "")
-    if (OFF) # FIXME i#5390: Fix drbbdup hiding block-final jmps; then enable.
-      # Ensure the invariant checker handles window transitions.
-      torunonly_drcacheoff(windows-invar ${ci_shared_app}
-        "-trace_after_instrs 20K -trace_for_instrs 5K -retrace_every_instrs 35K"
-        "@-simulator_type@invariant_checker" "")
-    endif ()
+    # Ensure the invariant checker handles window transitions.
+    torunonly_drcacheoff(windows-invar ${ci_shared_app}
+      "-trace_after_instrs 20K -trace_for_instrs 5K -retrace_every_instrs 35K"
+      "@-simulator_type@invariant_checker" "")
     if (X86 AND X64 AND UNIX)
       torunonly_drcacheoff(windows-asm allasm_repstr
         "-trace_after_instrs 3 -trace_for_instrs 4 -retrace_every_instrs 4"

suite/tests/client-interface/drbbdup-emul-test.dll.c

@@ -38,6 +38,12 @@
 #include "drbbdup.h"
 #include "drutil.h"
 #include "drreg.h"
+#include <string.h>
+
+typedef struct _per_block_t {
+    bool saw_first;
+    bool saw_last;
+} per_block_t;
 
 /* Assume single threaded. */
 static uintptr_t encode_val = 3;
@@ -46,17 +52,26 @@ static uintptr_t encode_val = 3;
 static bool saw_movs;
 static bool saw_zero_iter_rep_string;
 #endif
+static bool has_rest_of_block_emul;
 
 static dr_emit_flags_t
 app2app_event(void *drcontext, void *tag, instrlist_t *bb, bool for_trace,
               bool translating)
 {
+#if VERBOSE
+    /* Useful for debugging. */
+    instrlist_disassemble(drcontext, tag, bb, STDERR);
+#endif
+
+    has_rest_of_block_emul = false;
+
     /* Test drutil rep string expansion interacting with drbbdup. */
     bool expanded;
     if (!drutil_expand_rep_string_ex(drcontext, bb, &expanded, NULL))
         DR_ASSERT(false);
     if (expanded) {
         /* We can't overlap ours with drutil's so bow out. */
+        has_rest_of_block_emul = true;
         return DR_EMIT_DEFAULT;
     }
     /* Test handling of DR_EMULATE_REST_OF_BLOCK by inserting it in the middle
@@ -83,6 +98,10 @@ app2app_event(void *drcontext, void *tag, instrlist_t *bb, bool for_trace,
     emulated_instr.instr = instr_clone(drcontext, mid);
     emulated_instr.flags = DR_EMULATE_REST_OF_BLOCK;
     drmgr_insert_emulation_start(drcontext, bb, mid, &emulated_instr);
+    /* XXX i#5400: We'd like to pass a user_data to the instrument event but drbbdup
+     * doesn't support that; instead we rely on being single-threaded.
+     */
+    has_rest_of_block_emul = true;
 
     return DR_EMIT_DEFAULT;
 }
@@ -117,17 +136,40 @@ look_for_zero_iters(reg_t xcx)
 }
 #endif
 
+static void
+analyze_case(void *drcontext, void *tag, instrlist_t *bb, uintptr_t encoding,
+             void *user_data, void *orig_analysis_data, void **analysis_data)
+{
+    per_block_t *per_block =
+        (per_block_t *)dr_thread_alloc(drcontext, sizeof(per_block_t));
+    memset(per_block, 0, sizeof(*per_block));
+    *analysis_data = per_block;
+}
+
+static void
+destroy_case_analysis(void *drcontext, uintptr_t encoding, void *user_data,
+                      void *orig_analysis_data, void *analysis_data)
+{
+    per_block_t *per_block = (per_block_t *)analysis_data;
+    CHECK(per_block->saw_first, "failed to see first instr");
+    /* If we added a rest-of-block emul it will hide the last instr. */
+    CHECK(per_block->saw_last || has_rest_of_block_emul, "failed to see last instr");
+    dr_thread_free(drcontext, analysis_data, sizeof(per_block_t));
+}
+
 static dr_emit_flags_t
 instrument_instr(void *drcontext, void *tag, instrlist_t *bb, instr_t *instr,
                  instr_t *where, bool for_trace, bool translating, uintptr_t encoding,
                  void *user_data, void *orig_analysis_data, void *analysis_data)
 {
-    bool is_first;
+    bool is_first, is_last;
     drbbdup_status_t res;
+    per_block_t *per_block = (per_block_t *)analysis_data;
 
     res = drbbdup_is_first_instr(drcontext, instr, &is_first);
     CHECK(res == DRBBDUP_SUCCESS, "failed to check whether instr is first");
     if (is_first) {
+        per_block->saw_first = true;
         /* Ensure DR_EMULATE_REST_OF_BLOCK didn't leak through. */
         const emulated_instr_t *emul_info;
         if (drmgr_in_emulation_region(drcontext, &emul_info)) {
@@ -176,10 +218,50 @@ instrument_instr(void *drcontext, void *tag, instrlist_t *bb, instr_t *instr,
      * instr so we can't compare emul to instr; instead we make sure the emul returned
      * is never a jump to a label.
      */
-    instr_t *emul = drmgr_orig_app_instr_for_fetch(drcontext);
-    CHECK(emul == NULL || !instr_is_ubr(emul) || !opnd_is_instr(instr_get_target(emul)),
+    instr_t *instr_fetch = drmgr_orig_app_instr_for_fetch(drcontext);
+    CHECK(instr_fetch == NULL || !instr_is_ubr(instr_fetch) ||
+              !opnd_is_instr(instr_get_target(instr_fetch)),
           "app instr should never be jump to label");
 
+    /* Ensure the drmgr emulation API works for the final special instr in the
+     * final case (as well as other cases).
+     */
+    instr_t *instr_operands = drmgr_orig_app_instr_for_operands(drcontext);
+#if VERBOSE
+    /* Useful for debugging */
+    dr_fprintf(STDERR, "%s: emul fetch=", __FUNCTION__);
+    if (instr_fetch == NULL)
+        dr_fprintf(STDERR, "<null>", __FUNCTION__);
+    else
+        instr_disassemble(drcontext, instr_fetch, STDERR);
+    dr_fprintf(STDERR, "  op=", __FUNCTION__);
+    if (instr_operands == NULL)
+        dr_fprintf(STDERR, "<null>", __FUNCTION__);
+    else
+        instr_disassemble(drcontext, instr_operands, STDERR);
+    dr_fprintf(STDERR, "  instr=", __FUNCTION__);
+    instr_disassemble(drcontext, instr, STDERR);
+    dr_fprintf(STDERR, "  where=", __FUNCTION__);
+    instr_disassemble(drcontext, where, STDERR);
+    dr_fprintf(STDERR, "\n");
+#endif
+    CHECK(instr_fetch != NULL || instr_operands != NULL || has_rest_of_block_emul ||
+              (instr_get_prev(where) != NULL &&
+               drmgr_is_emulation_start(instr_get_prev(where))),
+          "emul error");
+
+    res = drbbdup_is_last_instr(drcontext, instr, &is_last);
+    CHECK(res == DRBBDUP_SUCCESS, "failed to check whether instr is last");
+    if (is_last) {
+        per_block->saw_last = true;
+        if (!has_rest_of_block_emul) {
+            CHECK(instr_fetch != NULL || !instr_is_app(instr),
+                  "last instr hidden from emul");
+            CHECK(instr_operands != NULL || !instr_is_app(instr),
+                  "last instr hidden from emul");
+        }
+    }
+
     return DR_EMIT_DEFAULT;
 }
 
@@ -215,6 +297,8 @@ dr_init(client_id_t id)
     drbbdup_options_t opts = { 0 };
     opts.struct_size = sizeof(drbbdup_options_t);
     opts.set_up_bb_dups = set_up_bb_dups;
+    opts.analyze_case = analyze_case;
+    opts.destroy_case_analysis = destroy_case_analysis;
     opts.instrument_instr_ex = instrument_instr;
     opts.runtime_case_opnd = OPND_CREATE_ABSMEM(&encode_val, OPSZ_PTR);
     opts.non_default_case_limit = 3;