From c4de70903761fabeaecf1b2ed32452ed149b8b01 Mon Sep 17 00:00:00 2001 From: Derek Bruening Date: Wed, 9 Mar 2022 01:47:21 -0500 Subject: [PATCH] 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 | 8 +- clients/drcachesim/tracer/raw2trace.cpp | 6 +- clients/drcachesim/tracer/raw2trace.h | 1 + ext/drbbdup/drbbdup.c | 62 +++++++++++-- suite/tests/CMakeLists.txt | 10 +-- .../client-interface/drbbdup-emul-test.dll.c | 90 ++++++++++++++++++- 6 files changed, 156 insertions(+), 21 deletions(-) diff --git a/clients/drcachesim/tracer/instru_offline.cpp b/clients/drcachesim/tracer/instru_offline.cpp index a8411b8c71b..15d2ba2fde6 100644 --- a/clients/drcachesim/tracer/instru_offline.cpp +++ b/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; diff --git a/clients/drcachesim/tracer/raw2trace.cpp b/clients/drcachesim/tracer/raw2trace.cpp index 793c4f774ad..7eba9d41b87 100644 --- a/clients/drcachesim/tracer/raw2trace.cpp +++ b/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(tdata->last_entry.addr.type)); + static_cast(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(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); diff --git a/clients/drcachesim/tracer/raw2trace.h b/clients/drcachesim/tracer/raw2trace.h index de2d2bb66ed..986fa7015ee 100644 --- a/clients/drcachesim/tracer/raw2trace.h +++ b/clients/drcachesim/tracer/raw2trace.h @@ -739,6 +739,7 @@ template 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) { diff --git a/ext/drbbdup/drbbdup.c b/ext/drbbdup/drbbdup.c index 9f573086c07..b096133a546 100644 --- a/ext/drbbdup/drbbdup.c +++ b/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"); diff --git a/suite/tests/CMakeLists.txt b/suite/tests/CMakeLists.txt index 4a47ff4f860..723d651bc71 100644 --- a/suite/tests/CMakeLists.txt +++ b/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" diff --git a/suite/tests/client-interface/drbbdup-emul-test.dll.c b/suite/tests/client-interface/drbbdup-emul-test.dll.c index 334d1a29612..9cb3864585f 100644 --- a/suite/tests/client-interface/drbbdup-emul-test.dll.c +++ b/suite/tests/client-interface/drbbdup-emul-test.dll.c @@ -38,6 +38,12 @@ #include "drbbdup.h" #include "drutil.h" #include "drreg.h" +#include + +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, "", __FUNCTION__); + else + instr_disassemble(drcontext, instr_fetch, STDERR); + dr_fprintf(STDERR, " op=", __FUNCTION__); + if (instr_operands == NULL) + dr_fprintf(STDERR, "", __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;