From 55f425ef5020149631693baca2533151e6a79c0d Mon Sep 17 00:00:00 2001 From: Abhinav Anil Sharma Date: Mon, 19 Oct 2020 21:18:56 -0400 Subject: [PATCH 01/22] i#1369: Use synch flush callback to enable drcachesim tracing. DR translates a fault in the code cache to a fault at the corresponding application address. This is done using ilist reconstruction for the fragment where the fault occurred. But, this does not work as expected when the DR client changes instrumentation during execution; currently, drcachesim does this to enable tracing after -trace_after_instrs. The reconstructed basic block gets the new instrumentation whereas the one in code cache has the old one. This causes issues during fault handling. In the current drcachesim case, it appears as though a meta-instr has faulted because the reconstructed ilist has a meta-instr at the code cache fault pc. This issue may manifest differently if the basic block with the new instrumentation is smaller than the old one (unlike the drcachesim 'meta-instr faulted' case) and the faulting address lies beyond the end of the new instrumented basic block. We may see an ASSERT_NOT_REACHED due to the ilist walk ending before the faulting code cache pc was found in the reconstructed ilist. In the existing code, drcachesim attempts to avoid this by flushing old fragments using dr_unlink_flush_region after it switches to the tracing instrumentation. However, due to the flush being asynch, there's a race and the flush does not complete in time. This PR adds support for a callback in the synchronous dr_flush_region API. The callback is executed after the flush but before the threads are resumed. Using the dr_flush_region callback to change drcachesim instrumentation ensures that old instrumentation is not applied after the flush and the new one is not applied before. Issue: #1369 --- api/samples/cbr.c | 4 +- clients/drcachesim/tracer/tracer.cpp | 55 +++++++++++++++++++--------- core/arch/clean_call_opt_shared.c | 3 +- core/dispatch.c | 3 +- core/fragment.c | 13 +++++-- core/fragment.h | 2 +- core/lib/instrument.c | 8 ++-- core/lib/instrument_api.h | 5 ++- core/unix/os.c | 3 +- core/win32/callback.c | 27 +++++++++----- ext/drwrap/drwrap.c | 2 +- 11 files changed, 85 insertions(+), 40 deletions(-) diff --git a/api/samples/cbr.c b/api/samples/cbr.c index 9d27e9f1d82..baad9af7381 100644 --- a/api/samples/cbr.c +++ b/api/samples/cbr.c @@ -251,7 +251,7 @@ at_taken(app_pc src, app_pc targ) /* Since the flush will remove the fragment we're already in, * redirect execution to the target address. */ - dr_flush_region(src, 1); + dr_flush_region(src, 1, NULL); dr_get_mcontext(drcontext, &mcontext); mcontext.pc = targ; dr_redirect_execution(&mcontext); @@ -280,7 +280,7 @@ at_not_taken(app_pc src, app_pc fall) /* Since the flush will remove the fragment we're already in, * redirect execution to the fallthrough address. */ - dr_flush_region(src, 1); + dr_flush_region(src, 1, NULL); dr_get_mcontext(drcontext, &mcontext); mcontext.pc = fall; dr_redirect_execution(&mcontext); diff --git a/clients/drcachesim/tracer/tracer.cpp b/clients/drcachesim/tracer/tracer.cpp index 30251cf04d1..a88880d082e 100644 --- a/clients/drcachesim/tracer/tracer.cpp +++ b/clients/drcachesim/tracer/tracer.cpp @@ -1316,8 +1316,9 @@ event_kernel_xfer(void *drcontext, const dr_kernel_xfer_info_t *info) */ static uint64 instr_count; -static volatile bool tracing_enabled; -static void *enable_tracing_lock; +static bool tracing_enabled; +static volatile bool tracing_scheduled; +static void *schedule_tracing_lock; #ifdef X86_64 # define DELAYED_CHECK_INLINED 1 @@ -1345,7 +1346,7 @@ enable_delay_instrumentation() if (!drmgr_register_bb_instrumentation_event( event_delay_bb_analysis, event_delay_app_instruction, &memtrace_pri)) DR_ASSERT(false); - enable_tracing_lock = dr_mutex_create(); + schedule_tracing_lock = dr_mutex_create(); } static void @@ -1358,8 +1359,8 @@ disable_delay_instrumentation() static void exit_delay_instrumentation() { - if (enable_tracing_lock != NULL) - dr_mutex_destroy(enable_tracing_lock); + if (schedule_tracing_lock != NULL) + dr_mutex_destroy(schedule_tracing_lock); #ifdef DELAYED_CHECK_INLINED drx_exit(); #endif @@ -1379,28 +1380,46 @@ enable_tracing_instrumentation() } static void -hit_instr_count_threshold() +change_instrumentation_callback() +{ + NOTIFY(0, "Hit delay threshold: enabling tracing.\n"); + disable_delay_instrumentation(); + enable_tracing_instrumentation(); +} + +static void +hit_instr_count_threshold(app_pc next_pc) { bool do_flush = false; - dr_mutex_lock(enable_tracing_lock); - if (!tracing_enabled) { // Already came here? - NOTIFY(0, "Hit delay threshold: enabling tracing.\n"); - disable_delay_instrumentation(); - enable_tracing_instrumentation(); + dr_mutex_lock(schedule_tracing_lock); + if (!tracing_scheduled) { do_flush = true; + tracing_scheduled = true; } - dr_mutex_unlock(enable_tracing_lock); - if (do_flush && !dr_unlink_flush_region(NULL, ~0UL)) + dr_mutex_unlock(schedule_tracing_lock); + + if (do_flush) { + if (!dr_flush_region(NULL, ~0UL, change_instrumentation_callback)) + DR_ASSERT(false); + + dr_mcontext_t mcontext = { + sizeof(mcontext), + DR_MC_ALL, + }; + dr_get_mcontext(dr_get_current_drcontext(), &mcontext); + mcontext.pc = next_pc; + dr_redirect_execution(&mcontext); DR_ASSERT(false); + } } #ifndef DELAYED_CHECK_INLINED static void -check_instr_count_threshold(uint incby) +check_instr_count_threshold(uint incby, app_pc next_pc) { instr_count += incby; if (instr_count > op_trace_after_instrs.get_value()) - hit_instr_count_threshold(); + hit_instr_count_threshold(next_pc); } #endif @@ -1450,7 +1469,8 @@ event_delay_app_instruction(void *drcontext, void *tag, instrlist_t *bb, instr_t } MINSERT(bb, instr, INSTR_CREATE_jcc(drcontext, OP_jl, opnd_create_instr(skip_call))); dr_insert_clean_call(drcontext, bb, instr, (void *)hit_instr_count_threshold, - false /*fpstate */, 0); + false /*fpstate */, 1, + OPND_CREATE_INTPTR((ptr_uint_t)instr_get_app_pc(instr))); MINSERT(bb, instr, skip_call); if (scratch != DR_REG_NULL) { if (drreg_unreserve_register(drcontext, bb, instr, scratch) != DRREG_SUCCESS) @@ -1463,7 +1483,8 @@ event_delay_app_instruction(void *drcontext, void *tag, instrlist_t *bb, instr_t // XXX: drx_insert_counter_update doesn't support 64-bit, and there's no // XINST_CREATE_load_8bytes. For now we pay the cost of a clean call every time. dr_insert_clean_call(drcontext, bb, instr, (void *)check_instr_count_threshold, - false /*fpstate */, 1, OPND_CREATE_INT32(num_instrs)); + false /*fpstate */, 2, OPND_CREATE_INT32(num_instrs), + OPND_CREATE_INTPTR((ptr_uint_t)instr_get_app_pc(instr))); #endif return DR_EMIT_DEFAULT; } diff --git a/core/arch/clean_call_opt_shared.c b/core/arch/clean_call_opt_shared.c index 43440a42ff0..73721515077 100644 --- a/core/arch/clean_call_opt_shared.c +++ b/core/arch/clean_call_opt_shared.c @@ -213,7 +213,8 @@ decode_callee_instr(dcontext_t *dcontext, callee_info_t *ci, app_pc instr_pc) instr = instr_create(GLOBAL_DCONTEXT); instrlist_append(ilist, instr); ci->num_instrs++; - TRY_EXCEPT(dcontext, { next_pc = decode(GLOBAL_DCONTEXT, instr_pc, instr); }, + TRY_EXCEPT((dcontext_t *)dr_get_current_drcontext(), + { next_pc = decode(GLOBAL_DCONTEXT, instr_pc, instr); }, { /* EXCEPT */ LOG(THREAD, LOG_CLEANCALL, 2, "CLEANCALL: crash on decoding callee instruction at: " PFX "\n", diff --git a/core/dispatch.c b/core/dispatch.c index 6bb55e23320..4e796f3b0a5 100644 --- a/core/dispatch.c +++ b/core/dispatch.c @@ -902,7 +902,8 @@ dispatch_enter_dynamorio(dcontext_t *dcontext) app_pc begin = (app_pc)dcontext->local_state->spill_space.r2; app_pc end = (app_pc)dcontext->local_state->spill_space.r3; dcontext->next_tag = (app_pc)dcontext->local_state->spill_space.r4; - flush_fragments_from_region(dcontext, begin, end - begin, true); + flush_fragments_from_region(dcontext, begin, end - begin, true, + NULL /*flush_completion_callback*/); } #endif diff --git a/core/fragment.c b/core/fragment.c index 39c38d170d5..e622f62b6ce 100644 --- a/core/fragment.c +++ b/core/fragment.c @@ -5659,12 +5659,14 @@ process_client_flush_requests(dcontext_t *dcontext, dcontext_t *alloc_dcontext, * that we can inform the client right away, it might be nice to use * the more performant regular flush when possible. */ flush_fragments_from_region(dcontext, iter->start, iter->size, - true /*force synchall*/); + true /*force synchall*/, + NULL /*flush_completion_callback*/); (*iter->flush_callback)(iter->flush_id); } else { /* do a regular flush */ flush_fragments_from_region(dcontext, iter->start, iter->size, - false /*don't force synchall*/); + false /*don't force synchall*/, + NULL /*flush_completion_callback*/); } } HEAP_TYPE_FREE(alloc_dcontext, iter, client_flush_req_t, ACCT_CLIENT, @@ -6881,10 +6883,11 @@ flush_fragments_and_remove_region(dcontext_t *dcontext, app_pc base, size_t size /* Flushes fragments from the region without any changes to the exec list. * Does not free futures and caller can't be holding the initexit lock. + * Invokes the given callback after flushing and before resuming threads. * FIXME - add argument parameters (free futures etc.) as needed. */ void flush_fragments_from_region(dcontext_t *dcontext, app_pc base, size_t size, - bool force_synchall) + bool force_synchall, void (*flush_completion_callback)()) { /* we pass false to flush_fragments_in_region_start() below for owning the initexit * lock */ @@ -6895,6 +6898,10 @@ flush_fragments_from_region(dcontext_t *dcontext, app_pc base, size_t size, flush_fragments_in_region_start(dcontext, base, size, false /*don't own initexit*/, false /*don't free futures*/, false /*exec valid*/, force_synchall _IF_DGCDIAG(NULL)); + if (flush_completion_callback != NULL) { + (*flush_completion_callback)(); + } + flush_fragments_in_region_finish(dcontext, false); } diff --git a/core/fragment.h b/core/fragment.h index 6a41ab208e8..f17a2c80b3b 100644 --- a/core/fragment.h +++ b/core/fragment.h @@ -1141,7 +1141,7 @@ flush_fragments_and_remove_region(dcontext_t *dcontext, app_pc base, size_t size void flush_fragments_from_region(dcontext_t *dcontext, app_pc base, size_t size, - bool force_synchall); + bool force_synchall, void (*flush_completion_callback)()); void flush_fragments_custom_list(dcontext_t *dcontext, fragment_t *list, diff --git a/core/lib/instrument.c b/core/lib/instrument.c index 1dd4edfedcf..54004d21cea 100644 --- a/core/lib/instrument.c +++ b/core/lib/instrument.c @@ -7174,7 +7174,7 @@ DR_API * to be !couldbelinking (xref PR 199115, 227619). Caller must use * dr_redirect_execution() to return to the cache. */ bool -dr_flush_region(app_pc start, size_t size) +dr_flush_region(app_pc start, size_t size, void (*flush_completion_callback)()) { dcontext_t *dcontext = get_thread_private_dcontext(); CLIENT_ASSERT(!standalone_library, "API not supported in standalone mode"); @@ -7207,7 +7207,8 @@ dr_flush_region(app_pc start, size_t size) if (!executable_vm_area_executed_from(start, start + size)) return true; - flush_fragments_from_region(dcontext, start, size, true /*force synchall*/); + flush_fragments_from_region(dcontext, start, size, true /*force synchall*/, + flush_completion_callback); return true; } @@ -7259,7 +7260,8 @@ dr_unlink_flush_region(app_pc start, size_t size) if (!executable_vm_area_executed_from(start, start + size)) return true; - flush_fragments_from_region(dcontext, start, size, false /*don't force synchall*/); + flush_fragments_from_region(dcontext, start, size, false /*don't force synchall*/, + NULL /*flush_completion_callback*/); return true; } diff --git a/core/lib/instrument_api.h b/core/lib/instrument_api.h index 0dd4b883a9d..870eea77c34 100644 --- a/core/lib/instrument_api.h +++ b/core/lib/instrument_api.h @@ -6228,11 +6228,14 @@ DR_API * \note Use \p size == 1 to flush fragments containing the instruction at address * \p start. A flush of \p size == 0 is not allowed. * + * \note Use flush_completion_callback to specify logic to be executed after the flush + * and before the threads are resumed. + * * \note As currently implemented, dr_delay_flush_region() with no completion callback * routine specified can be substantially more performant. */ bool -dr_flush_region(app_pc start, size_t size); +dr_flush_region(app_pc start, size_t size, void (*flush_completion_callback)()); /* FIXME - get rid of the no locks requirement by making event callbacks !couldbelinking * and no dr locks (see PR 227619) so that client locks owned by this thread can't block diff --git a/core/unix/os.c b/core/unix/os.c index acfa4f7e2d5..4aaee6aee03 100644 --- a/core/unix/os.c +++ b/core/unix/os.c @@ -7655,7 +7655,8 @@ pre_system_call(dcontext_t *dcontext) * use synch to ensure other threads see the * new code. */ - false /*don't force synchall*/); + false /*don't force synchall*/, + NULL /*flush_completion_callback*/); break; } # endif /* ARM */ diff --git a/core/win32/callback.c b/core/win32/callback.c index 3e3be8640f0..4cacc21881e 100644 --- a/core/win32/callback.c +++ b/core/win32/callback.c @@ -3908,14 +3908,16 @@ intercept_nt_continue(CONTEXT *cxt, int flag) d_r_debug_register[0] = (app_pc)cxt->Dr0; flush_fragments_from_region(dcontext, d_r_debug_register[0], 1 /* size */, - false /*don't force synchall*/); + false /*don't force synchall*/, + NULL /*flush_completion_callback*/); } } else { /* Disable debug register. */ if (d_r_debug_register[0] != NULL) { flush_fragments_from_region(dcontext, d_r_debug_register[0], 1 /* size */, - false /*don't force synchall*/); + false /*don't force synchall*/, + NULL /*flush_completion_callback*/); d_r_debug_register[0] = NULL; } } @@ -3924,14 +3926,16 @@ intercept_nt_continue(CONTEXT *cxt, int flag) d_r_debug_register[1] = (app_pc)cxt->Dr1; flush_fragments_from_region(dcontext, d_r_debug_register[1], 1 /* size */, - false /*don't force synchall*/); + false /*don't force synchall*/, + NULL /*flush_completion_callback*/); } } else { /* Disable debug register. */ if (d_r_debug_register[1] != NULL) { flush_fragments_from_region(dcontext, d_r_debug_register[1], 1 /* size */, - false /*don't force synchall*/); + false /*don't force synchall*/, + NULL /*flush_completion_callback*/); d_r_debug_register[1] = NULL; } } @@ -3940,14 +3944,16 @@ intercept_nt_continue(CONTEXT *cxt, int flag) d_r_debug_register[2] = (app_pc)cxt->Dr2; flush_fragments_from_region(dcontext, d_r_debug_register[2], 1 /* size */, - false /*don't force synchall*/); + false /*don't force synchall*/, + NULL /*flush_completion_callback*/); } } else { /* Disable debug register. */ if (d_r_debug_register[2] != NULL) { flush_fragments_from_region(dcontext, d_r_debug_register[2], 1 /* size */, - false /*don't force synchall*/); + false /*don't force synchall*/, + NULL /*flush_completion_callback*/); d_r_debug_register[2] = NULL; } } @@ -3956,14 +3962,16 @@ intercept_nt_continue(CONTEXT *cxt, int flag) d_r_debug_register[3] = (app_pc)cxt->Dr3; flush_fragments_from_region(dcontext, d_r_debug_register[3], 1 /* size */, - false /*don't force synchall*/); + false /*don't force synchall*/, + NULL /*flush_completion_callback*/); } } else { /* Disable debug register. */ if (d_r_debug_register[3] != NULL) { flush_fragments_from_region(dcontext, d_r_debug_register[3], 1 /* size */, - false /*don't force synchall*/); + false /*don't force synchall*/, + NULL /*flush_completion_callback*/); d_r_debug_register[3] = NULL; } } @@ -5874,7 +5882,8 @@ intercept_exception(app_state_at_intercept_t *state) */ flush_fragments_from_region(dcontext, dcontext->next_tag, 1 /* size */, - false /*don't force synchall*/); + false /*don't force synchall*/, + NULL /*flush_completion_callback*/); /* Sets a field so that build_bb_ilist knows when to stop. */ dcontext->single_step_addr = dcontext->next_tag; LOG(THREAD, LOG_ASYNCH, 2, diff --git a/ext/drwrap/drwrap.c b/ext/drwrap/drwrap.c index 9d45af1faa3..513fd652aeb 100644 --- a/ext/drwrap/drwrap.c +++ b/ext/drwrap/drwrap.c @@ -1754,7 +1754,7 @@ drwrap_mark_retaddr_for_instru(void *drcontext, app_pc decorated_pc, dr_recurlock_unlock(wrap_lock); } dr_atomic_add_stat_return_sum(&drwrap_stats.flush_count, 1); - dr_flush_region(retaddr, 1); + dr_flush_region(retaddr, 1, NULL); /* now we are guaranteed no thread is inside the fragment */ /* another thread may have done a racy competing flush: should be fine */ dr_rwlock_read_lock(post_call_rwlock); From e2e89bbd73012c7f8d170982c653cf97c6015890 Mon Sep 17 00:00:00 2001 From: Abhinav Anil Sharma Date: Tue, 20 Oct 2020 13:30:27 -0400 Subject: [PATCH 02/22] Fix args to dr_flush_region call-site in events.dll.c test --- suite/tests/client-interface/events.dll.c | 3 ++- suite/tests/client-interface/flush.dll.c | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/suite/tests/client-interface/events.dll.c b/suite/tests/client-interface/events.dll.c index d711f79d179..163126131d2 100644 --- a/suite/tests/client-interface/events.dll.c +++ b/suite/tests/client-interface/events.dll.c @@ -507,7 +507,8 @@ exception_event1(void *dcontext, dr_exception_t *excpt) dr_fprintf(STDERR, "unregister failed!\n"); /* ensure we get our deletion events */ - dr_flush_region((app_pc)excpt->record->ExceptionAddress, 1); + dr_flush_region((app_pc)excpt->record->ExceptionAddress, 1, + NULL /*flush_completion_callback*/); return true; } diff --git a/suite/tests/client-interface/flush.dll.c b/suite/tests/client-interface/flush.dll.c index 75d4cb9541d..c0ea7dd417e 100644 --- a/suite/tests/client-interface/flush.dll.c +++ b/suite/tests/client-interface/flush.dll.c @@ -196,7 +196,7 @@ callback(void *tag, app_pc next_pc) dr_delay_flush_region((app_pc)tag - 20, 30, callback_count, flush_event); dr_get_mcontext(dr_get_current_drcontext(), &mcontext); mcontext.pc = next_pc; - dr_flush_region(tag, 1); + dr_flush_region(tag, 1, NULL /*flush_completion_callback*/); dr_redirect_execution(&mcontext); *(volatile uint *)NULL = 0; /* ASSERT_NOT_REACHED() */ } else if (use_unlink) { From 5342bf627d4b771daa4862885645c97be07423bd Mon Sep 17 00:00:00 2001 From: Abhinav Anil Sharma Date: Tue, 20 Oct 2020 18:24:37 -0400 Subject: [PATCH 03/22] Load stolen reg with TLS base address before entering reset exit stub. Without this, some threads segfault due to incorrect value in the stolen reg. --- core/synch.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/core/synch.c b/core/synch.c index 22f9f655249..2a3d027ab42 100644 --- a/core/synch.c +++ b/core/synch.c @@ -1749,6 +1749,10 @@ translate_from_synchall_to_dispatch(thread_record_t *tr, thread_synch_state_t sy arch_mcontext_reset_stolen_reg(dcontext, mc); } }); + IF_AARCHXX({ + set_stolen_reg_val(mc, (reg_t)os_get_dr_tls_base(dcontext)); + IF_ARM(ASSERT_NOT_TESTED()); + }); /* We send all threads, regardless of whether was in DR or not, to * re-interp from translated cxt, to avoid having to handle stale * local state problems if we simply resumed. From 7561b942cb905d8a1f7737592573e58c036c1b60 Mon Sep 17 00:00:00 2001 From: Abhinav Anil Sharma Date: Tue, 20 Oct 2020 18:52:11 -0400 Subject: [PATCH 04/22] Add callback function to dr_flush_region call in existing test for flush. --- suite/tests/client-interface/flush.dll.c | 8 +++++++- suite/tests/client-interface/flush.template | 2 ++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/suite/tests/client-interface/flush.dll.c b/suite/tests/client-interface/flush.dll.c index c0ea7dd417e..9976bd0e426 100644 --- a/suite/tests/client-interface/flush.dll.c +++ b/suite/tests/client-interface/flush.dll.c @@ -177,6 +177,12 @@ flush_event(int flush_id) dr_fprintf(STDERR, "Flush completion id=%d\n", flush_id); } +static void +synch_flush_completion_callback() +{ + dr_fprintf(STDERR, "in synch_flush_completion_callback\n"); +} + static void callback(void *tag, app_pc next_pc) { @@ -196,7 +202,7 @@ callback(void *tag, app_pc next_pc) dr_delay_flush_region((app_pc)tag - 20, 30, callback_count, flush_event); dr_get_mcontext(dr_get_current_drcontext(), &mcontext); mcontext.pc = next_pc; - dr_flush_region(tag, 1, NULL /*flush_completion_callback*/); + dr_flush_region(tag, 1, synch_flush_completion_callback); dr_redirect_execution(&mcontext); *(volatile uint *)NULL = 0; /* ASSERT_NOT_REACHED() */ } else if (use_unlink) { diff --git a/suite/tests/client-interface/flush.template b/suite/tests/client-interface/flush.template index 58c059415b7..f2c7dd224d1 100644 --- a/suite/tests/client-interface/flush.template +++ b/suite/tests/client-interface/flush.template @@ -10,8 +10,10 @@ kernel_xfer_event: type 9 Flush completion id=400 #else options =@& +in synch_flush_completion_callback kernel_xfer_event: type 9 Flush completion id=200 +in synch_flush_completion_callback kernel_xfer_event: type 9 Flush completion id=400 #endif From 3d3f3184c4069e096c06f4570e9edcfbaa99f28b Mon Sep 17 00:00:00 2001 From: Abhinav Anil Sharma Date: Tue, 20 Oct 2020 21:10:38 -0400 Subject: [PATCH 05/22] Fix args to dr_flush_region call in cbr.dll.c test. --- suite/tests/client-interface/cbr3.dll.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/suite/tests/client-interface/cbr3.dll.c b/suite/tests/client-interface/cbr3.dll.c index ba56c591743..b0946591b75 100644 --- a/suite/tests/client-interface/cbr3.dll.c +++ b/suite/tests/client-interface/cbr3.dll.c @@ -230,7 +230,7 @@ at_taken(app_pc src, app_pc targ) * time it is executed. Since the flush will remove the fragment * we're already in, redirect execution to the target. */ - dr_flush_region(src, 1); + dr_flush_region(src, 1, NULL /*flush_completion_callback*/); dr_get_mcontext(drcontext, &mcontext); mcontext.pc = targ; dr_redirect_execution(&mcontext); @@ -259,7 +259,7 @@ at_not_taken(app_pc src, app_pc fall) * time it is executed. Since the flush will remove the fragment * we're already in, redirect execution to the target. */ - dr_flush_region(src, 1); + dr_flush_region(src, 1, NULL /*flush_completion_callback*/); dr_get_mcontext(drcontext, &mcontext); mcontext.pc = fall; dr_redirect_execution(&mcontext); From 497996530030a605e3385671ba78819434ec7e84 Mon Sep 17 00:00:00 2001 From: Abhinav Anil Sharma Date: Tue, 20 Oct 2020 21:20:49 -0400 Subject: [PATCH 06/22] Document dr_flush_region signature change in release doc. This is a source compatibility change and will require change in client source code. The added documentation describes it as such. --- api/docs/release.dox | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/api/docs/release.dox b/api/docs/release.dox index 7614d50a632..2d586522ffb 100644 --- a/api/docs/release.dox +++ b/api/docs/release.dox @@ -141,6 +141,10 @@ changes: member fields of classes are now following a consistent style with an underscore suffix. References to renamed fields will need to be updated. + - Changed the signature of the dr_flush_region() instrument API, in order to + support a callback function which is executed after completion of flush but + before threads are resumed. Existing client source will need to be modified + to pass a NULL value for the flush_completion_callback arg. The changes between version \DR_VERSION and 8.0.0 include the following minor compatibility changes: From 6df5dbf0ddf2c2806fda69395924d40218fa047a Mon Sep 17 00:00:00 2001 From: Abhinav Anil Sharma Date: Tue, 20 Oct 2020 21:24:41 -0400 Subject: [PATCH 07/22] Update issue number for inlining instr counter increment in drcachesim. --- clients/drcachesim/tracer/tracer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clients/drcachesim/tracer/tracer.cpp b/clients/drcachesim/tracer/tracer.cpp index a88880d082e..d8e9e353135 100644 --- a/clients/drcachesim/tracer/tracer.cpp +++ b/clients/drcachesim/tracer/tracer.cpp @@ -1323,7 +1323,7 @@ static void *schedule_tracing_lock; #ifdef X86_64 # define DELAYED_CHECK_INLINED 1 #else -// XXX: we don't have the inlining implemented yet. +// XXX, i#4487: we don't have the inlining implemented yet. #endif static dr_emit_flags_t From 333da5bfbd8416b4f285daa2e390bd4547dcd0b6 Mon Sep 17 00:00:00 2001 From: Abhinav Anil Sharma Date: Tue, 20 Oct 2020 22:06:06 -0400 Subject: [PATCH 08/22] Revert change made in TRY_EXCEPT in clean_call_opt_shared.c While working on some draft changes, the existing TRY_EXCEPT caused an ASSERT failure due to the passed dcontext not being the current thread's. But it seems to have gone away due to changes made since. --- core/arch/clean_call_opt_shared.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/core/arch/clean_call_opt_shared.c b/core/arch/clean_call_opt_shared.c index 73721515077..43440a42ff0 100644 --- a/core/arch/clean_call_opt_shared.c +++ b/core/arch/clean_call_opt_shared.c @@ -213,8 +213,7 @@ decode_callee_instr(dcontext_t *dcontext, callee_info_t *ci, app_pc instr_pc) instr = instr_create(GLOBAL_DCONTEXT); instrlist_append(ilist, instr); ci->num_instrs++; - TRY_EXCEPT((dcontext_t *)dr_get_current_drcontext(), - { next_pc = decode(GLOBAL_DCONTEXT, instr_pc, instr); }, + TRY_EXCEPT(dcontext, { next_pc = decode(GLOBAL_DCONTEXT, instr_pc, instr); }, { /* EXCEPT */ LOG(THREAD, LOG_CLEANCALL, 2, "CLEANCALL: crash on decoding callee instruction at: " PFX "\n", From 71f273f22d2798af4bc6229ebc72413ac0e9a524 Mon Sep 17 00:00:00 2001 From: Abhinav Anil Sharma Date: Thu, 22 Oct 2020 01:10:25 -0400 Subject: [PATCH 09/22] Add missing callback invocation for early return cases of dr_flush_region. --- core/lib/instrument.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/core/lib/instrument.c b/core/lib/instrument.c index 54004d21cea..b232dfbc148 100644 --- a/core/lib/instrument.c +++ b/core/lib/instrument.c @@ -7201,11 +7201,15 @@ dr_flush_region(app_pc start, size_t size, void (*flush_completion_callback)()) CLIENT_ASSERT(size != 0, "dr_flush_region: 0 is invalid size for flush"); /* release build check of requirements, as many as possible at least */ - if (size == 0 || is_couldbelinking(dcontext)) + if (size == 0 || is_couldbelinking(dcontext)) { + (*flush_completion_callback)(); return false; + } - if (!executable_vm_area_executed_from(start, start + size)) + if (!executable_vm_area_executed_from(start, start + size)) { + (*flush_completion_callback)(); return true; + } flush_fragments_from_region(dcontext, start, size, true /*force synchall*/, flush_completion_callback); From 69c3f8900bd01a1e7a839841aba7e07e142eaabc Mon Sep 17 00:00:00 2001 From: Abhinav Anil Sharma Date: Thu, 22 Oct 2020 17:07:30 -0400 Subject: [PATCH 10/22] Fix reset tests so that they actually test the complete reset path. Currently, the tests perform reset at a given fragment count. That count is reached much before the child thread is created. When there's just one thread, the complete reset path is not invoked. To fix this, we cannot simply change the -reset_at_fragment_count value, as the ideal value is prone to change without us noticing. Instead, we perform reset at a given thread count. Verified that linux.thread-reset and linux.clone-reset actually crash with a SIGSEGV on AArch64 without the stolen reg restore in core/synch.c --- api/docs/release.dox | 2 ++ core/dynamo.c | 8 ++++++++ core/fragment.c | 11 ---------- core/options.c | 4 ++-- core/optionsx.h | 6 +++--- core/synch.c | 2 ++ suite/tests/CMakeLists.txt | 6 ++++-- suite/tests/linux/thread.c | 36 +++++++++++++++++++-------------- suite/tests/linux/thread.expect | 14 +++++++++++++ 9 files changed, 56 insertions(+), 33 deletions(-) diff --git a/api/docs/release.dox b/api/docs/release.dox index 2d586522ffb..643faebf4b5 100644 --- a/api/docs/release.dox +++ b/api/docs/release.dox @@ -145,6 +145,8 @@ changes: support a callback function which is executed after completion of flush but before threads are resumed. Existing client source will need to be modified to pass a NULL value for the flush_completion_callback arg. + - Replaces the -reset_at_fragment_count option with -reset_at_thread_count. This + was required to make the Linux reset tests more robust. The changes between version \DR_VERSION and 8.0.0 include the following minor compatibility changes: diff --git a/core/dynamo.c b/core/dynamo.c index e9992c72570..8f1217bc1b2 100644 --- a/core/dynamo.c +++ b/core/dynamo.c @@ -2184,6 +2184,14 @@ add_thread(IF_WINDOWS_ELSE_NP(HANDLE hthread, process_id_t pid), thread_id_t tid RSTATS_ADD_PEAK(num_threads, 1); RSTATS_INC(num_threads_created); num_known_threads++; + DOSTATS({ + if (d_r_stats != NULL && + (uint)GLOBAL_STAT(num_threads_created) == + INTERNAL_OPTION(reset_at_thread_count)) { + ASSERT(INTERNAL_OPTION(reset_at_thread_count) != 0); + schedule_reset(RESET_ALL); + } + }); d_r_mutex_unlock(&all_threads_lock); } diff --git a/core/fragment.c b/core/fragment.c index e622f62b6ce..1ccda408f2b 100644 --- a/core/fragment.c +++ b/core/fragment.c @@ -2403,17 +2403,6 @@ fragment_create(dcontext_t *dcontext, app_pc tag, int body_size, int direct_exit if (!fragment_lookup_deleted(dcontext, tag) && !TEST(FRAG_COARSE_GRAIN, flags)) STATS_INC(num_unique_fragments); }); - /* FIXME: make fragment count a release-build stat so we can do this in - * release builds - */ - DOSTATS({ - if (d_r_stats != NULL && - (uint)GLOBAL_STAT(num_fragments) == - INTERNAL_OPTION(reset_at_fragment_count)) { - ASSERT(INTERNAL_OPTION(reset_at_fragment_count) != 0); - schedule_reset(RESET_ALL); - } - }); DODEBUG({ if ((uint)GLOBAL_STAT(num_fragments) == INTERNAL_OPTION(log_at_fragment_count)) { /* we started at loglevel 1 and now we raise to the requested level */ diff --git a/core/options.c b/core/options.c index a0496bb862a..0fbafeacffb 100644 --- a/core/options.c +++ b/core/options.c @@ -1502,8 +1502,8 @@ check_option_compatibility_helper(int recurse_count) changed_options = true; } # ifdef EXPOSE_INTERNAL_OPTIONS - else if (INTERNAL_OPTION(reset_at_fragment_count)) { - USAGE_ERROR("-reset_at_fragment_count requires -enable_reset, enabling"); + else if (INTERNAL_OPTION(reset_at_thread_count)) { + USAGE_ERROR("-reset_at_thread_count requires -enable_reset, enabling"); dynamo_options.enable_reset = true; changed_options = true; } diff --git a/core/optionsx.h b/core/optionsx.h index 7a5198607ba..3e158ce3762 100644 --- a/core/optionsx.h +++ b/core/optionsx.h @@ -136,7 +136,7 @@ #define DISABLE_RESET(prefix) \ { \ (prefix)->enable_reset = false; \ - IF_INTERNAL((prefix)->reset_at_fragment_count = 0;) \ + IF_INTERNAL((prefix)->reset_at_thread_count = 0;) \ (prefix)->reset_at_nth_thread = 0; \ (prefix)->reset_at_switch_to_os_at_vmm_limit = false; \ (prefix)->reset_at_vmm_percent_free_limit = 0; \ @@ -1492,8 +1492,8 @@ OPTION_COMMAND(bool, enable_reset, IF_X86_ELSE(true, false), "enable_reset", "separate persistent memory from non-persistent for resets", STATIC, OP_PCACHE_NOP) -OPTION_DEFAULT_INTERNAL(uint, reset_at_fragment_count, 0, - "reset all caches at a certain fragment count") +OPTION_DEFAULT_INTERNAL(uint, reset_at_thread_count, 0, + "reset all caches when thread count reaches given value") OPTION(uint, reset_at_nth_thread, "reset all caches when the nth thread is explicitly created") /* FIXME - is potentially using up all the os allocation leaving nothing for the diff --git a/core/synch.c b/core/synch.c index 2a3d027ab42..ab4b4dce9f2 100644 --- a/core/synch.c +++ b/core/synch.c @@ -1751,6 +1751,8 @@ translate_from_synchall_to_dispatch(thread_record_t *tr, thread_synch_state_t sy }); IF_AARCHXX({ set_stolen_reg_val(mc, (reg_t)os_get_dr_tls_base(dcontext)); + // XXX: This path is tested by linux.thread-reset and linux.clone-reset. + // We just haven't run those on ARM yet. IF_ARM(ASSERT_NOT_TESTED()); }); /* We send all threads, regardless of whether was in DR or not, to diff --git a/suite/tests/CMakeLists.txt b/suite/tests/CMakeLists.txt index 387b3b45632..b9a1c9faddb 100644 --- a/suite/tests/CMakeLists.txt +++ b/suite/tests/CMakeLists.txt @@ -4069,10 +4069,12 @@ if (UNIX) #tobuild(linux.vfork-fib linux/vfork-fib.c) # runs of other builds with custom DR options + # XXX: Using -reset_at_thread_count 2 causes an ASSERT failure, so we use + # -reset_at_thread_count 3 instead. torunonly(linux.thread-reset linux.thread linux/thread.c - "-enable_reset -reset_at_fragment_count 100" "") + "-enable_reset -reset_at_thread_count 3" "") torunonly(linux.clone-reset linux.clone linux/clone.c - "-enable_reset -reset_at_fragment_count 100" "") + "-enable_reset -reset_at_thread_count 3" "") tobuild(pthreads.pthreads pthreads/pthreads.c) tobuild(pthreads.pthreads_exit pthreads/pthreads_exit.c) tobuild(pthreads.ptsig pthreads/ptsig.c) diff --git a/suite/tests/linux/thread.c b/suite/tests/linux/thread.c index 18596e3ebe8..2d5236bb19a 100644 --- a/suite/tests/linux/thread.c +++ b/suite/tests/linux/thread.c @@ -83,22 +83,28 @@ static struct timespec sleeptime; int main() { - sleeptime.tv_sec = 0; - sleeptime.tv_nsec = 10 * 1000 * 1000; /* 10ms */ - - child_exit = false; - child_done = false; - child = create_thread(run, NULL, &stack); - assert(child > -1); - - /* waste some time */ - nanosleep(&sleeptime, NULL); - - child_exit = true; - /* we want deterministic printf ordering */ - while (!child_done) + // We need to create multiple threads to verify correctness of reset, + // which requires more than one threads. + // XXX: Using -reset_at_thread_count 2 causes an ASSERT failure, so we + // use 3 threads for now. + for (int i = 0; i < 2; i++) { + sleeptime.tv_sec = 0; + sleeptime.tv_nsec = 10 * 1000 * 1000; /* 10ms */ + + child_exit = false; + child_done = false; + child = create_thread(run, NULL, &stack); + assert(child > -1); + + /* waste some time */ nanosleep(&sleeptime, NULL); - delete_thread(child, stack); + + child_exit = true; + /* we want deterministic printf ordering */ + while (!child_done) + nanosleep(&sleeptime, NULL); + delete_thread(child, stack); + } } /* Procedure executed by sideline threads diff --git a/suite/tests/linux/thread.expect b/suite/tests/linux/thread.expect index 242de3e9226..7932fefc5bf 100644 --- a/suite/tests/linux/thread.expect +++ b/suite/tests/linux/thread.expect @@ -12,3 +12,17 @@ i = 25000000 Sideline thread finished Waiting for child to exit Child has exited +Sideline thread started +i = 2500000 +i = 5000000 +i = 7500000 +i = 10000000 +i = 12500000 +i = 15000000 +i = 17500000 +i = 20000000 +i = 22500000 +i = 25000000 +Sideline thread finished +Waiting for child to exit +Child has exited From 2889982045f3908b5375956cc2efff6bd6f13ef7 Mon Sep 17 00:00:00 2001 From: Abhinav Anil Sharma Date: Thu, 22 Oct 2020 21:32:41 -0400 Subject: [PATCH 11/22] Add back reset_at_fragment_count option that was removed earlier. This option is still useful for other manual debugging use cases. --- api/docs/release.dox | 5 +++-- core/fragment.c | 11 +++++++++++ core/options.c | 6 ++++-- core/optionsx.h | 3 +++ 4 files changed, 21 insertions(+), 4 deletions(-) diff --git a/api/docs/release.dox b/api/docs/release.dox index 643faebf4b5..ddd0105eade 100644 --- a/api/docs/release.dox +++ b/api/docs/release.dox @@ -145,8 +145,6 @@ changes: support a callback function which is executed after completion of flush but before threads are resumed. Existing client source will need to be modified to pass a NULL value for the flush_completion_callback arg. - - Replaces the -reset_at_fragment_count option with -reset_at_thread_count. This - was required to make the Linux reset tests more robust. The changes between version \DR_VERSION and 8.0.0 include the following minor compatibility changes: @@ -179,6 +177,9 @@ Further non-compatibility-affecting changes include: - Added opnd_create_immed_double(), opnd_get_immed_double() and opnd_is_immed_double() to enable the creation and handling of double precision floating-point operands. + - Added -reset_at_thread_count option to allow triggering reset when created thread + count reaches the given value. This option is currently used to make Linux reset + tests more robust. **************************************************
diff --git a/core/fragment.c b/core/fragment.c index 1ccda408f2b..e622f62b6ce 100644 --- a/core/fragment.c +++ b/core/fragment.c @@ -2403,6 +2403,17 @@ fragment_create(dcontext_t *dcontext, app_pc tag, int body_size, int direct_exit if (!fragment_lookup_deleted(dcontext, tag) && !TEST(FRAG_COARSE_GRAIN, flags)) STATS_INC(num_unique_fragments); }); + /* FIXME: make fragment count a release-build stat so we can do this in + * release builds + */ + DOSTATS({ + if (d_r_stats != NULL && + (uint)GLOBAL_STAT(num_fragments) == + INTERNAL_OPTION(reset_at_fragment_count)) { + ASSERT(INTERNAL_OPTION(reset_at_fragment_count) != 0); + schedule_reset(RESET_ALL); + } + }); DODEBUG({ if ((uint)GLOBAL_STAT(num_fragments) == INTERNAL_OPTION(log_at_fragment_count)) { /* we started at loglevel 1 and now we raise to the requested level */ diff --git a/core/options.c b/core/options.c index 0fbafeacffb..e5eecbb7848 100644 --- a/core/options.c +++ b/core/options.c @@ -1502,8 +1502,10 @@ check_option_compatibility_helper(int recurse_count) changed_options = true; } # ifdef EXPOSE_INTERNAL_OPTIONS - else if (INTERNAL_OPTION(reset_at_thread_count)) { - USAGE_ERROR("-reset_at_thread_count requires -enable_reset, enabling"); + else if (INTERNAL_OPTION(reset_at_thread_count) || + INTERNAL_OPTION(reset_at_fragment_count)) { + USAGE_ERROR("-reset_at_thread_count and -reset_at_fragment_count require " + "-enable_reset, enabling"); dynamo_options.enable_reset = true; changed_options = true; } diff --git a/core/optionsx.h b/core/optionsx.h index 3e158ce3762..2c5cf0b4025 100644 --- a/core/optionsx.h +++ b/core/optionsx.h @@ -136,6 +136,7 @@ #define DISABLE_RESET(prefix) \ { \ (prefix)->enable_reset = false; \ + IF_INTERNAL((prefix)->reset_at_fragment_count = 0;) \ IF_INTERNAL((prefix)->reset_at_thread_count = 0;) \ (prefix)->reset_at_nth_thread = 0; \ (prefix)->reset_at_switch_to_os_at_vmm_limit = false; \ @@ -1492,6 +1493,8 @@ OPTION_COMMAND(bool, enable_reset, IF_X86_ELSE(true, false), "enable_reset", "separate persistent memory from non-persistent for resets", STATIC, OP_PCACHE_NOP) +OPTION_DEFAULT_INTERNAL(uint, reset_at_fragment_count, 0, + "reset all caches at a certain fragment count") OPTION_DEFAULT_INTERNAL(uint, reset_at_thread_count, 0, "reset all caches when thread count reaches given value") OPTION(uint, reset_at_nth_thread, From 8b2f7b96218b705b1b12ba1b4265d58f2e7c062e Mon Sep 17 00:00:00 2001 From: Abhinav Anil Sharma Date: Tue, 27 Oct 2020 18:42:16 -0400 Subject: [PATCH 12/22] Preserve source compatibility by creating new API for flush with callback. Added dr_flush_region_ex that also invokes the callback. The existing dr_flush_region simply invokes dr_flush_region_ex with a NULL callback. --- api/docs/release.dox | 4 ---- api/samples/cbr.c | 4 ++-- clients/drcachesim/tracer/tracer.cpp | 2 +- core/lib/instrument.c | 15 ++++++++++++--- core/lib/instrument_api.h | 9 +++++++-- ext/drwrap/drwrap.c | 2 +- suite/tests/client-interface/cbr3.dll.c | 4 ++-- suite/tests/client-interface/events.dll.c | 3 +-- suite/tests/client-interface/flush.dll.c | 2 +- 9 files changed, 27 insertions(+), 18 deletions(-) diff --git a/api/docs/release.dox b/api/docs/release.dox index ddd0105eade..42fbabe219f 100644 --- a/api/docs/release.dox +++ b/api/docs/release.dox @@ -141,10 +141,6 @@ changes: member fields of classes are now following a consistent style with an underscore suffix. References to renamed fields will need to be updated. - - Changed the signature of the dr_flush_region() instrument API, in order to - support a callback function which is executed after completion of flush but - before threads are resumed. Existing client source will need to be modified - to pass a NULL value for the flush_completion_callback arg. The changes between version \DR_VERSION and 8.0.0 include the following minor compatibility changes: diff --git a/api/samples/cbr.c b/api/samples/cbr.c index baad9af7381..9d27e9f1d82 100644 --- a/api/samples/cbr.c +++ b/api/samples/cbr.c @@ -251,7 +251,7 @@ at_taken(app_pc src, app_pc targ) /* Since the flush will remove the fragment we're already in, * redirect execution to the target address. */ - dr_flush_region(src, 1, NULL); + dr_flush_region(src, 1); dr_get_mcontext(drcontext, &mcontext); mcontext.pc = targ; dr_redirect_execution(&mcontext); @@ -280,7 +280,7 @@ at_not_taken(app_pc src, app_pc fall) /* Since the flush will remove the fragment we're already in, * redirect execution to the fallthrough address. */ - dr_flush_region(src, 1, NULL); + dr_flush_region(src, 1); dr_get_mcontext(drcontext, &mcontext); mcontext.pc = fall; dr_redirect_execution(&mcontext); diff --git a/clients/drcachesim/tracer/tracer.cpp b/clients/drcachesim/tracer/tracer.cpp index d8e9e353135..6ca9ab5634f 100644 --- a/clients/drcachesim/tracer/tracer.cpp +++ b/clients/drcachesim/tracer/tracer.cpp @@ -1399,7 +1399,7 @@ hit_instr_count_threshold(app_pc next_pc) dr_mutex_unlock(schedule_tracing_lock); if (do_flush) { - if (!dr_flush_region(NULL, ~0UL, change_instrumentation_callback)) + if (!dr_flush_region_ex(NULL, ~0UL, change_instrumentation_callback)) DR_ASSERT(false); dr_mcontext_t mcontext = { diff --git a/core/lib/instrument.c b/core/lib/instrument.c index b232dfbc148..375f626688a 100644 --- a/core/lib/instrument.c +++ b/core/lib/instrument.c @@ -7171,10 +7171,11 @@ DR_API /* Flush all fragments that contain code from the region [start, start+size). * Uses a synchall flush to guarantee that no execution occurs out of the fragments * flushed once this returns. Requires caller to be holding no locks (dr or client) and - * to be !couldbelinking (xref PR 199115, 227619). Caller must use + * to be !couldbelinking (xref PR 199115, 227619). Invokes the given callback after the + * flush completes and before threads are resumed. Caller must use * dr_redirect_execution() to return to the cache. */ bool -dr_flush_region(app_pc start, size_t size, void (*flush_completion_callback)()) +dr_flush_region_ex(app_pc start, size_t size, void (*flush_completion_callback)()) { dcontext_t *dcontext = get_thread_private_dcontext(); CLIENT_ASSERT(!standalone_library, "API not supported in standalone mode"); @@ -7198,7 +7199,7 @@ dr_flush_region(app_pc start, size_t size, void (*flush_completion_callback)()) "dr_flush_region: caller owns a client " "lock or was called from an event callback that doesn't support " "calling this routine; see header file for restrictions."); - CLIENT_ASSERT(size != 0, "dr_flush_region: 0 is invalid size for flush"); + CLIENT_ASSERT(size != 0, "dr_flush_region_ex: 0 is invalid size for flush"); /* release build check of requirements, as many as possible at least */ if (size == 0 || is_couldbelinking(dcontext)) { @@ -7217,6 +7218,14 @@ dr_flush_region(app_pc start, size_t size, void (*flush_completion_callback)()) return true; } +DR_API +/* Equivalent to dr_flush_region_ex, without the callback. */ +bool +dr_flush_region(app_pc start, size_t size) +{ + return dr_flush_region_ex(start,size, NULL /*flush_completion_callback*/); +} + DR_API /* Flush all fragments that contain code from the region [start, start+size). * Uses an unlink flush which guarantees that no thread will enter a fragment that was diff --git a/core/lib/instrument_api.h b/core/lib/instrument_api.h index 870eea77c34..3ec7cdfbb55 100644 --- a/core/lib/instrument_api.h +++ b/core/lib/instrument_api.h @@ -6229,13 +6229,18 @@ DR_API * \p start. A flush of \p size == 0 is not allowed. * * \note Use flush_completion_callback to specify logic to be executed after the flush - * and before the threads are resumed. + * and before the threads are resumed. Use NULL if not needed. * * \note As currently implemented, dr_delay_flush_region() with no completion callback * routine specified can be substantially more performant. */ bool -dr_flush_region(app_pc start, size_t size, void (*flush_completion_callback)()); +dr_flush_region_ex(app_pc start, size_t size, void (*flush_completion_callback)()); + +DR_API +/** Equivalent to dr_flush_region_ex(start, size, NULL). */ +bool +dr_flush_region(app_pc start, size_t size); /* FIXME - get rid of the no locks requirement by making event callbacks !couldbelinking * and no dr locks (see PR 227619) so that client locks owned by this thread can't block diff --git a/ext/drwrap/drwrap.c b/ext/drwrap/drwrap.c index 513fd652aeb..9d45af1faa3 100644 --- a/ext/drwrap/drwrap.c +++ b/ext/drwrap/drwrap.c @@ -1754,7 +1754,7 @@ drwrap_mark_retaddr_for_instru(void *drcontext, app_pc decorated_pc, dr_recurlock_unlock(wrap_lock); } dr_atomic_add_stat_return_sum(&drwrap_stats.flush_count, 1); - dr_flush_region(retaddr, 1, NULL); + dr_flush_region(retaddr, 1); /* now we are guaranteed no thread is inside the fragment */ /* another thread may have done a racy competing flush: should be fine */ dr_rwlock_read_lock(post_call_rwlock); diff --git a/suite/tests/client-interface/cbr3.dll.c b/suite/tests/client-interface/cbr3.dll.c index b0946591b75..ba56c591743 100644 --- a/suite/tests/client-interface/cbr3.dll.c +++ b/suite/tests/client-interface/cbr3.dll.c @@ -230,7 +230,7 @@ at_taken(app_pc src, app_pc targ) * time it is executed. Since the flush will remove the fragment * we're already in, redirect execution to the target. */ - dr_flush_region(src, 1, NULL /*flush_completion_callback*/); + dr_flush_region(src, 1); dr_get_mcontext(drcontext, &mcontext); mcontext.pc = targ; dr_redirect_execution(&mcontext); @@ -259,7 +259,7 @@ at_not_taken(app_pc src, app_pc fall) * time it is executed. Since the flush will remove the fragment * we're already in, redirect execution to the target. */ - dr_flush_region(src, 1, NULL /*flush_completion_callback*/); + dr_flush_region(src, 1); dr_get_mcontext(drcontext, &mcontext); mcontext.pc = fall; dr_redirect_execution(&mcontext); diff --git a/suite/tests/client-interface/events.dll.c b/suite/tests/client-interface/events.dll.c index 163126131d2..d711f79d179 100644 --- a/suite/tests/client-interface/events.dll.c +++ b/suite/tests/client-interface/events.dll.c @@ -507,8 +507,7 @@ exception_event1(void *dcontext, dr_exception_t *excpt) dr_fprintf(STDERR, "unregister failed!\n"); /* ensure we get our deletion events */ - dr_flush_region((app_pc)excpt->record->ExceptionAddress, 1, - NULL /*flush_completion_callback*/); + dr_flush_region((app_pc)excpt->record->ExceptionAddress, 1); return true; } diff --git a/suite/tests/client-interface/flush.dll.c b/suite/tests/client-interface/flush.dll.c index 9976bd0e426..f6664e1890a 100644 --- a/suite/tests/client-interface/flush.dll.c +++ b/suite/tests/client-interface/flush.dll.c @@ -202,7 +202,7 @@ callback(void *tag, app_pc next_pc) dr_delay_flush_region((app_pc)tag - 20, 30, callback_count, flush_event); dr_get_mcontext(dr_get_current_drcontext(), &mcontext); mcontext.pc = next_pc; - dr_flush_region(tag, 1, synch_flush_completion_callback); + dr_flush_region_ex(tag, 1, synch_flush_completion_callback); dr_redirect_execution(&mcontext); *(volatile uint *)NULL = 0; /* ASSERT_NOT_REACHED() */ } else if (use_unlink) { From a2514e7c6209e182e281823fed1e62918ad7e9f1 Mon Sep 17 00:00:00 2001 From: Abhinav Anil Sharma Date: Tue, 27 Oct 2020 18:55:42 -0400 Subject: [PATCH 13/22] Rename new option to reset_at_created_thread_count. This is to clearly differentiate from reset_at_nth_thread, which checks the existing number of active threads. --- api/docs/release.dox | 6 +++--- core/dynamo.c | 4 ++-- core/options.c | 12 ++++++++---- core/optionsx.h | 7 ++++--- suite/tests/CMakeLists.txt | 8 ++++---- suite/tests/linux/thread.c | 4 ++-- 6 files changed, 23 insertions(+), 18 deletions(-) diff --git a/api/docs/release.dox b/api/docs/release.dox index 42fbabe219f..b7153b3193a 100644 --- a/api/docs/release.dox +++ b/api/docs/release.dox @@ -173,9 +173,9 @@ Further non-compatibility-affecting changes include: - Added opnd_create_immed_double(), opnd_get_immed_double() and opnd_is_immed_double() to enable the creation and handling of double precision floating-point operands. - - Added -reset_at_thread_count option to allow triggering reset when created thread - count reaches the given value. This option is currently used to make Linux reset - tests more robust. + - Added -reset_at_created_thread_count option to allow triggering reset when + created thread count reaches the given value. This option is currently used to + make Linux reset tests more robust. **************************************************
diff --git a/core/dynamo.c b/core/dynamo.c index 8f1217bc1b2..cc5b50521d8 100644 --- a/core/dynamo.c +++ b/core/dynamo.c @@ -2187,8 +2187,8 @@ add_thread(IF_WINDOWS_ELSE_NP(HANDLE hthread, process_id_t pid), thread_id_t tid DOSTATS({ if (d_r_stats != NULL && (uint)GLOBAL_STAT(num_threads_created) == - INTERNAL_OPTION(reset_at_thread_count)) { - ASSERT(INTERNAL_OPTION(reset_at_thread_count) != 0); + INTERNAL_OPTION(reset_at_created_thread_count)) { + ASSERT(INTERNAL_OPTION(reset_at_created_thread_count) != 0); schedule_reset(RESET_ALL); } }); diff --git a/core/options.c b/core/options.c index e5eecbb7848..9a935ff7bc1 100644 --- a/core/options.c +++ b/core/options.c @@ -1502,10 +1502,14 @@ check_option_compatibility_helper(int recurse_count) changed_options = true; } # ifdef EXPOSE_INTERNAL_OPTIONS - else if (INTERNAL_OPTION(reset_at_thread_count) || - INTERNAL_OPTION(reset_at_fragment_count)) { - USAGE_ERROR("-reset_at_thread_count and -reset_at_fragment_count require " - "-enable_reset, enabling"); + else if (INTERNAL_OPTION(reset_at_fragment_count)) { + USAGE_ERROR("-reset_at_fragment_count requires -enable_reset, enabling"); + dynamo_options.enable_reset = true; + changed_options = true; + } + else if (INTERNAL_OPTION(reset_at_created_thread_count)) { + USAGE_ERROR("-reset_at_created_thread_count requires -enable_reset, " + "enabling"); dynamo_options.enable_reset = true; changed_options = true; } diff --git a/core/optionsx.h b/core/optionsx.h index 2c5cf0b4025..4778e4571de 100644 --- a/core/optionsx.h +++ b/core/optionsx.h @@ -137,7 +137,8 @@ { \ (prefix)->enable_reset = false; \ IF_INTERNAL((prefix)->reset_at_fragment_count = 0;) \ - IF_INTERNAL((prefix)->reset_at_thread_count = 0;) \ + IF_INTERNAL( \ + (prefix)->reset_at_created_thread_count = 0;) \ (prefix)->reset_at_nth_thread = 0; \ (prefix)->reset_at_switch_to_os_at_vmm_limit = false; \ (prefix)->reset_at_vmm_percent_free_limit = 0; \ @@ -1495,8 +1496,8 @@ OPTION_COMMAND(bool, enable_reset, IF_X86_ELSE(true, false), "enable_reset", OPTION_DEFAULT_INTERNAL(uint, reset_at_fragment_count, 0, "reset all caches at a certain fragment count") -OPTION_DEFAULT_INTERNAL(uint, reset_at_thread_count, 0, - "reset all caches when thread count reaches given value") +OPTION_DEFAULT_INTERNAL(uint, reset_at_created_thread_count, 0, + "reset all caches when created thread count reaches given value") OPTION(uint, reset_at_nth_thread, "reset all caches when the nth thread is explicitly created") /* FIXME - is potentially using up all the os allocation leaving nothing for the diff --git a/suite/tests/CMakeLists.txt b/suite/tests/CMakeLists.txt index b9a1c9faddb..d658c4a93bd 100644 --- a/suite/tests/CMakeLists.txt +++ b/suite/tests/CMakeLists.txt @@ -4069,12 +4069,12 @@ if (UNIX) #tobuild(linux.vfork-fib linux/vfork-fib.c) # runs of other builds with custom DR options - # XXX: Using -reset_at_thread_count 2 causes an ASSERT failure, so we use - # -reset_at_thread_count 3 instead. + # XXX: Using -reset_at_created_thread_count 2 causes an ASSERT failure, so + # we use -reset_at_created_thread_count 3 instead. torunonly(linux.thread-reset linux.thread linux/thread.c - "-enable_reset -reset_at_thread_count 3" "") + "-enable_reset -reset_at_created_thread_count 3" "") torunonly(linux.clone-reset linux.clone linux/clone.c - "-enable_reset -reset_at_thread_count 3" "") + "-enable_reset -reset_at_created_thread_count 3" "") tobuild(pthreads.pthreads pthreads/pthreads.c) tobuild(pthreads.pthreads_exit pthreads/pthreads_exit.c) tobuild(pthreads.ptsig pthreads/ptsig.c) diff --git a/suite/tests/linux/thread.c b/suite/tests/linux/thread.c index 2d5236bb19a..c05a0bd5725 100644 --- a/suite/tests/linux/thread.c +++ b/suite/tests/linux/thread.c @@ -85,8 +85,8 @@ main() { // We need to create multiple threads to verify correctness of reset, // which requires more than one threads. - // XXX: Using -reset_at_thread_count 2 causes an ASSERT failure, so we - // use 3 threads for now. + // XXX: Using -reset_at_created_thread_count 2 causes an ASSERT failure, + // so we use 3 threads for now. for (int i = 0; i < 2; i++) { sleeptime.tv_sec = 0; sleeptime.tv_nsec = 10 * 1000 * 1000; /* 10ms */ From 0b375a744c7bc5dca8638daaa55f5a8013b3a4cb Mon Sep 17 00:00:00 2001 From: Abhinav Anil Sharma Date: Tue, 27 Oct 2020 18:57:36 -0400 Subject: [PATCH 14/22] Minor style fixes. --- clients/drcachesim/tracer/tracer.cpp | 2 +- suite/tests/linux/thread.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/clients/drcachesim/tracer/tracer.cpp b/clients/drcachesim/tracer/tracer.cpp index 6ca9ab5634f..17888ea45a1 100644 --- a/clients/drcachesim/tracer/tracer.cpp +++ b/clients/drcachesim/tracer/tracer.cpp @@ -1323,7 +1323,7 @@ static void *schedule_tracing_lock; #ifdef X86_64 # define DELAYED_CHECK_INLINED 1 #else -// XXX, i#4487: we don't have the inlining implemented yet. +// XXX i#4487: we don't have the inlining implemented yet. #endif static dr_emit_flags_t diff --git a/suite/tests/linux/thread.c b/suite/tests/linux/thread.c index c05a0bd5725..b902d0e987d 100644 --- a/suite/tests/linux/thread.c +++ b/suite/tests/linux/thread.c @@ -84,7 +84,7 @@ int main() { // We need to create multiple threads to verify correctness of reset, - // which requires more than one threads. + // which requires more than one thread. // XXX: Using -reset_at_created_thread_count 2 causes an ASSERT failure, // so we use 3 threads for now. for (int i = 0; i < 2; i++) { From f9d64ac3bfa78e1d65b5f059d46be140c214c6e8 Mon Sep 17 00:00:00 2001 From: Abhinav Anil Sharma Date: Tue, 27 Oct 2020 21:35:42 -0400 Subject: [PATCH 15/22] Add user_data arg to synch flush callback function. --- clients/drcachesim/tracer/tracer.cpp | 5 +- core/dispatch.c | 3 +- core/fragment.c | 18 +++--- core/fragment.h | 4 +- core/lib/instrument.c | 10 +-- core/lib/instrument_api.h | 3 +- core/win32/callback.c | 72 ++++++++++----------- suite/tests/client-interface/flush.dll.c | 7 +- suite/tests/client-interface/flush.template | 4 +- 9 files changed, 68 insertions(+), 58 deletions(-) diff --git a/clients/drcachesim/tracer/tracer.cpp b/clients/drcachesim/tracer/tracer.cpp index 17888ea45a1..1f30ca3671f 100644 --- a/clients/drcachesim/tracer/tracer.cpp +++ b/clients/drcachesim/tracer/tracer.cpp @@ -1380,7 +1380,7 @@ enable_tracing_instrumentation() } static void -change_instrumentation_callback() +change_instrumentation_callback(void *unused_user_data) { NOTIFY(0, "Hit delay threshold: enabling tracing.\n"); disable_delay_instrumentation(); @@ -1399,7 +1399,8 @@ hit_instr_count_threshold(app_pc next_pc) dr_mutex_unlock(schedule_tracing_lock); if (do_flush) { - if (!dr_flush_region_ex(NULL, ~0UL, change_instrumentation_callback)) + if (!dr_flush_region_ex(NULL, ~0UL, change_instrumentation_callback, + NULL /*user_data*/)) DR_ASSERT(false); dr_mcontext_t mcontext = { diff --git a/core/dispatch.c b/core/dispatch.c index 4e796f3b0a5..6051f55b38d 100644 --- a/core/dispatch.c +++ b/core/dispatch.c @@ -903,7 +903,8 @@ dispatch_enter_dynamorio(dcontext_t *dcontext) app_pc end = (app_pc)dcontext->local_state->spill_space.r3; dcontext->next_tag = (app_pc)dcontext->local_state->spill_space.r4; flush_fragments_from_region(dcontext, begin, end - begin, true, - NULL /*flush_completion_callback*/); + NULL /*flush_completion_callback*/, + NULL /*user_data*/); } #endif diff --git a/core/fragment.c b/core/fragment.c index e622f62b6ce..d6ff5716774 100644 --- a/core/fragment.c +++ b/core/fragment.c @@ -5658,15 +5658,15 @@ process_client_flush_requests(dcontext_t *dcontext, dcontext_t *alloc_dcontext, /* FIXME - for implementation simplicity we do a synch-all flush so * that we can inform the client right away, it might be nice to use * the more performant regular flush when possible. */ - flush_fragments_from_region(dcontext, iter->start, iter->size, - true /*force synchall*/, - NULL /*flush_completion_callback*/); + flush_fragments_from_region( + dcontext, iter->start, iter->size, true /*force synchall*/, + NULL /*flush_completion_callback*/, NULL /*user_data*/); (*iter->flush_callback)(iter->flush_id); } else { /* do a regular flush */ - flush_fragments_from_region(dcontext, iter->start, iter->size, - false /*don't force synchall*/, - NULL /*flush_completion_callback*/); + flush_fragments_from_region( + dcontext, iter->start, iter->size, false /*don't force synchall*/, + NULL /*flush_completion_callback*/, NULL /*user_data*/); } } HEAP_TYPE_FREE(alloc_dcontext, iter, client_flush_req_t, ACCT_CLIENT, @@ -6887,7 +6887,9 @@ flush_fragments_and_remove_region(dcontext_t *dcontext, app_pc base, size_t size * FIXME - add argument parameters (free futures etc.) as needed. */ void flush_fragments_from_region(dcontext_t *dcontext, app_pc base, size_t size, - bool force_synchall, void (*flush_completion_callback)()) + bool force_synchall, + void (*flush_completion_callback)(void *user_data), + void *user_data) { /* we pass false to flush_fragments_in_region_start() below for owning the initexit * lock */ @@ -6899,7 +6901,7 @@ flush_fragments_from_region(dcontext_t *dcontext, app_pc base, size_t size, false /*don't free futures*/, false /*exec valid*/, force_synchall _IF_DGCDIAG(NULL)); if (flush_completion_callback != NULL) { - (*flush_completion_callback)(); + (*flush_completion_callback)(user_data); } flush_fragments_in_region_finish(dcontext, false); diff --git a/core/fragment.h b/core/fragment.h index f17a2c80b3b..80dfa62f9cb 100644 --- a/core/fragment.h +++ b/core/fragment.h @@ -1141,7 +1141,9 @@ flush_fragments_and_remove_region(dcontext_t *dcontext, app_pc base, size_t size void flush_fragments_from_region(dcontext_t *dcontext, app_pc base, size_t size, - bool force_synchall, void (*flush_completion_callback)()); + bool force_synchall, + void (*flush_completion_callback)(void *user_data), + void *user_data); void flush_fragments_custom_list(dcontext_t *dcontext, fragment_t *list, diff --git a/core/lib/instrument.c b/core/lib/instrument.c index 375f626688a..4a890820a10 100644 --- a/core/lib/instrument.c +++ b/core/lib/instrument.c @@ -7175,7 +7175,8 @@ DR_API * flush completes and before threads are resumed. Caller must use * dr_redirect_execution() to return to the cache. */ bool -dr_flush_region_ex(app_pc start, size_t size, void (*flush_completion_callback)()) +dr_flush_region_ex(app_pc start, size_t size, void (*flush_completion_callback)(), + void *user_data) { dcontext_t *dcontext = get_thread_private_dcontext(); CLIENT_ASSERT(!standalone_library, "API not supported in standalone mode"); @@ -7213,7 +7214,7 @@ dr_flush_region_ex(app_pc start, size_t size, void (*flush_completion_callback)( } flush_fragments_from_region(dcontext, start, size, true /*force synchall*/, - flush_completion_callback); + flush_completion_callback, user_data); return true; } @@ -7223,7 +7224,8 @@ DR_API bool dr_flush_region(app_pc start, size_t size) { - return dr_flush_region_ex(start,size, NULL /*flush_completion_callback*/); + return dr_flush_region_ex(start, size, NULL /*flush_completion_callback*/, + NULL /*user_data*/); } DR_API @@ -7274,7 +7276,7 @@ dr_unlink_flush_region(app_pc start, size_t size) return true; flush_fragments_from_region(dcontext, start, size, false /*don't force synchall*/, - NULL /*flush_completion_callback*/); + NULL /*flush_completion_callback*/, NULL /*user_data*/); return true; } diff --git a/core/lib/instrument_api.h b/core/lib/instrument_api.h index 3ec7cdfbb55..77e332f74fe 100644 --- a/core/lib/instrument_api.h +++ b/core/lib/instrument_api.h @@ -6235,7 +6235,8 @@ DR_API * routine specified can be substantially more performant. */ bool -dr_flush_region_ex(app_pc start, size_t size, void (*flush_completion_callback)()); +dr_flush_region_ex(app_pc start, size_t size, + void (*flush_completion_callback)(void *user_data), void *user_data); DR_API /** Equivalent to dr_flush_region_ex(start, size, NULL). */ diff --git a/core/win32/callback.c b/core/win32/callback.c index 4cacc21881e..20d1b2fdf8d 100644 --- a/core/win32/callback.c +++ b/core/win32/callback.c @@ -3906,72 +3906,72 @@ intercept_nt_continue(CONTEXT *cxt, int flag) /* Flush only when debug register value changes. */ if (d_r_debug_register[0] != (app_pc)cxt->Dr0) { d_r_debug_register[0] = (app_pc)cxt->Dr0; - flush_fragments_from_region(dcontext, d_r_debug_register[0], - 1 /* size */, - false /*don't force synchall*/, - NULL /*flush_completion_callback*/); + flush_fragments_from_region( + dcontext, d_r_debug_register[0], 1 /* size */, + false /*don't force synchall*/, + NULL /*flush_completion_callback*/, NULL /*user_data*/); } } else { /* Disable debug register. */ if (d_r_debug_register[0] != NULL) { - flush_fragments_from_region(dcontext, d_r_debug_register[0], - 1 /* size */, - false /*don't force synchall*/, - NULL /*flush_completion_callback*/); + flush_fragments_from_region( + dcontext, d_r_debug_register[0], 1 /* size */, + false /*don't force synchall*/, + NULL /*flush_completion_callback*/, NULL /*user_data*/); d_r_debug_register[0] = NULL; } } if (TESTANY(cxt->Dr7, DEBUG_REGISTERS_FLAG_ENABLE_DR1)) { if (d_r_debug_register[1] != (app_pc)cxt->Dr1) { d_r_debug_register[1] = (app_pc)cxt->Dr1; - flush_fragments_from_region(dcontext, d_r_debug_register[1], - 1 /* size */, - false /*don't force synchall*/, - NULL /*flush_completion_callback*/); + flush_fragments_from_region( + dcontext, d_r_debug_register[1], 1 /* size */, + false /*don't force synchall*/, + NULL /*flush_completion_callback*/, NULL /*user_data*/); } } else { /* Disable debug register. */ if (d_r_debug_register[1] != NULL) { - flush_fragments_from_region(dcontext, d_r_debug_register[1], - 1 /* size */, - false /*don't force synchall*/, - NULL /*flush_completion_callback*/); + flush_fragments_from_region( + dcontext, d_r_debug_register[1], 1 /* size */, + false /*don't force synchall*/, + NULL /*flush_completion_callback*/, NULL /*user_data*/); d_r_debug_register[1] = NULL; } } if (TESTANY(cxt->Dr7, DEBUG_REGISTERS_FLAG_ENABLE_DR2)) { if (d_r_debug_register[2] != (app_pc)cxt->Dr2) { d_r_debug_register[2] = (app_pc)cxt->Dr2; - flush_fragments_from_region(dcontext, d_r_debug_register[2], - 1 /* size */, - false /*don't force synchall*/, - NULL /*flush_completion_callback*/); + flush_fragments_from_region( + dcontext, d_r_debug_register[2], 1 /* size */, + false /*don't force synchall*/, + NULL /*flush_completion_callback*/, NULL /*user_data*/); } } else { /* Disable debug register. */ if (d_r_debug_register[2] != NULL) { - flush_fragments_from_region(dcontext, d_r_debug_register[2], - 1 /* size */, - false /*don't force synchall*/, - NULL /*flush_completion_callback*/); + flush_fragments_from_region( + dcontext, d_r_debug_register[2], 1 /* size */, + false /*don't force synchall*/, + NULL /*flush_completion_callback*/, NULL /*user_data*/); d_r_debug_register[2] = NULL; } } if (TESTANY(cxt->Dr7, DEBUG_REGISTERS_FLAG_ENABLE_DR3)) { if (d_r_debug_register[3] != (app_pc)cxt->Dr3) { d_r_debug_register[3] = (app_pc)cxt->Dr3; - flush_fragments_from_region(dcontext, d_r_debug_register[3], - 1 /* size */, - false /*don't force synchall*/, - NULL /*flush_completion_callback*/); + flush_fragments_from_region( + dcontext, d_r_debug_register[3], 1 /* size */, + false /*don't force synchall*/, + NULL /*flush_completion_callback*/, NULL /*user_data*/); } } else { /* Disable debug register. */ if (d_r_debug_register[3] != NULL) { - flush_fragments_from_region(dcontext, d_r_debug_register[3], - 1 /* size */, - false /*don't force synchall*/, - NULL /*flush_completion_callback*/); + flush_fragments_from_region( + dcontext, d_r_debug_register[3], 1 /* size */, + false /*don't force synchall*/, + NULL /*flush_completion_callback*/, NULL /*user_data*/); d_r_debug_register[3] = NULL; } } @@ -5880,10 +5880,10 @@ intercept_exception(app_state_at_intercept_t *state) /* Deletes fragment starting at single step exception * if it exists. */ - flush_fragments_from_region(dcontext, dcontext->next_tag, - 1 /* size */, - false /*don't force synchall*/, - NULL /*flush_completion_callback*/); + flush_fragments_from_region( + dcontext, dcontext->next_tag, 1 /* size */, + false /*don't force synchall*/, + NULL /*flush_completion_callback*/, NULL /*user_data*/); /* Sets a field so that build_bb_ilist knows when to stop. */ dcontext->single_step_addr = dcontext->next_tag; LOG(THREAD, LOG_ASYNCH, 2, diff --git a/suite/tests/client-interface/flush.dll.c b/suite/tests/client-interface/flush.dll.c index f6664e1890a..4c33a003f21 100644 --- a/suite/tests/client-interface/flush.dll.c +++ b/suite/tests/client-interface/flush.dll.c @@ -178,9 +178,10 @@ flush_event(int flush_id) } static void -synch_flush_completion_callback() +synch_flush_completion_callback(void *user_data) { - dr_fprintf(STDERR, "in synch_flush_completion_callback\n"); + dr_fprintf(STDERR, "in synch_flush_completion_callback, user_data=%d\n", + *(int *)user_data); } static void @@ -202,7 +203,7 @@ callback(void *tag, app_pc next_pc) dr_delay_flush_region((app_pc)tag - 20, 30, callback_count, flush_event); dr_get_mcontext(dr_get_current_drcontext(), &mcontext); mcontext.pc = next_pc; - dr_flush_region_ex(tag, 1, synch_flush_completion_callback); + dr_flush_region_ex(tag, 1, synch_flush_completion_callback, &callback_count); dr_redirect_execution(&mcontext); *(volatile uint *)NULL = 0; /* ASSERT_NOT_REACHED() */ } else if (use_unlink) { diff --git a/suite/tests/client-interface/flush.template b/suite/tests/client-interface/flush.template index f2c7dd224d1..41f15ba31f0 100644 --- a/suite/tests/client-interface/flush.template +++ b/suite/tests/client-interface/flush.template @@ -10,10 +10,10 @@ kernel_xfer_event: type 9 Flush completion id=400 #else options =@& -in synch_flush_completion_callback +in synch_flush_completion_callback, user_data=200 kernel_xfer_event: type 9 Flush completion id=200 -in synch_flush_completion_callback +in synch_flush_completion_callback, user_data=400 kernel_xfer_event: type 9 Flush completion id=400 #endif From 8459b6cd150a482edf7bb3f60bcf5db43fd8d6f9 Mon Sep 17 00:00:00 2001 From: Abhinav Anil Sharma Date: Tue, 27 Oct 2020 22:06:53 -0400 Subject: [PATCH 16/22] Optimise mcontext initialisation by avoiding excessive zeroing. --- api/samples/cbr.c | 14 ++--- clients/drcachesim/tracer/tracer.cpp | 7 +-- ext/drbbdup/drbbdup.c | 7 +-- suite/tests/client-interface/avx512ctx.dll.c | 7 +-- suite/tests/client-interface/cbr3.dll.c | 14 ++--- suite/tests/client-interface/cleancall.dll.c | 56 ++++++++----------- .../client-interface/drreg-end-restore.dll.c | 7 +-- suite/tests/client-interface/events.dll.c | 8 +-- suite/tests/client-interface/flush.dll.c | 11 ++-- suite/tests/client-interface/syscall.dll.c | 7 +-- 10 files changed, 59 insertions(+), 79 deletions(-) diff --git a/api/samples/cbr.c b/api/samples/cbr.c index 9d27e9f1d82..b74effa39dc 100644 --- a/api/samples/cbr.c +++ b/api/samples/cbr.c @@ -232,10 +232,9 @@ insert(hash_table_t table, app_pc addr, cbr_state_t state) static void at_taken(app_pc src, app_pc targ) { - dr_mcontext_t mcontext = { - sizeof(mcontext), - DR_MC_ALL, - }; + dr_mcontext_t mcontext; + mcontext.size = sizeof(mcontext); + mcontext.flags = DR_MC_ALL; void *drcontext = dr_get_current_drcontext(); /* @@ -261,10 +260,9 @@ at_taken(app_pc src, app_pc targ) static void at_not_taken(app_pc src, app_pc fall) { - dr_mcontext_t mcontext = { - sizeof(mcontext), - DR_MC_ALL, - }; + dr_mcontext_t mcontext; + mcontext.size = sizeof(mcontext); + mcontext.flags = DR_MC_ALL; void *drcontext = dr_get_current_drcontext(); /* diff --git a/clients/drcachesim/tracer/tracer.cpp b/clients/drcachesim/tracer/tracer.cpp index 1f30ca3671f..151dd88be9f 100644 --- a/clients/drcachesim/tracer/tracer.cpp +++ b/clients/drcachesim/tracer/tracer.cpp @@ -1403,10 +1403,9 @@ hit_instr_count_threshold(app_pc next_pc) NULL /*user_data*/)) DR_ASSERT(false); - dr_mcontext_t mcontext = { - sizeof(mcontext), - DR_MC_ALL, - }; + dr_mcontext_t mcontext; + mcontext.size = sizeof(mcontext); + mcontext.flags = DR_MC_ALL; dr_get_mcontext(dr_get_current_drcontext(), &mcontext); mcontext.pc = next_pc; dr_redirect_execution(&mcontext); diff --git a/ext/drbbdup/drbbdup.c b/ext/drbbdup/drbbdup.c index 348af37bbdf..f345618420b 100644 --- a/ext/drbbdup/drbbdup.c +++ b/ext/drbbdup/drbbdup.c @@ -1211,10 +1211,9 @@ drbbdup_handle_new_case() (drbbdup_per_thread *)drmgr_get_tls_field(drcontext, tls_idx); /* Must use DR_MC_ALL due to dr_redirect_execution. */ - dr_mcontext_t mcontext = { - sizeof(mcontext), - DR_MC_ALL, - }; + dr_mcontext_t mcontext; + mcontext.size = sizeof(mcontext); + mcontext.flags = DR_MC_ALL; dr_get_mcontext(drcontext, &mcontext); /* Scratch register holds the tag. */ diff --git a/suite/tests/client-interface/avx512ctx.dll.c b/suite/tests/client-interface/avx512ctx.dll.c index 61d48c32b97..9ab3725ab01 100644 --- a/suite/tests/client-interface/avx512ctx.dll.c +++ b/suite/tests/client-interface/avx512ctx.dll.c @@ -86,10 +86,9 @@ read_avx512_state() dr_fprintf(STDERR, "Reading application state\n"); void *drcontext = dr_get_current_drcontext(); - dr_mcontext_t mcontext = { - sizeof(mcontext), - DR_MC_ALL, - }; + dr_mcontext_t mcontext; + mcontext.size = sizeof(mcontext); + mcontext.flags = DR_MC_ALL; dr_get_mcontext(drcontext, &mcontext); bool get_reg_value_ok = true; diff --git a/suite/tests/client-interface/cbr3.dll.c b/suite/tests/client-interface/cbr3.dll.c index ba56c591743..3380dcaaf2f 100644 --- a/suite/tests/client-interface/cbr3.dll.c +++ b/suite/tests/client-interface/cbr3.dll.c @@ -210,10 +210,9 @@ insert(hash_table_t table, app_pc addr, cbr_state_t state) static void at_taken(app_pc src, app_pc targ) { - dr_mcontext_t mcontext = { - sizeof(mcontext), - DR_MC_ALL, - }; + dr_mcontext_t mcontext; + mcontext.size = sizeof(mcontext); + mcontext.flags = DR_MC_ALL; void *drcontext = dr_get_current_drcontext(); /* @@ -239,10 +238,9 @@ at_taken(app_pc src, app_pc targ) static void at_not_taken(app_pc src, app_pc fall) { - dr_mcontext_t mcontext = { - sizeof(mcontext), - DR_MC_ALL, - }; + dr_mcontext_t mcontext; + mcontext.size = sizeof(mcontext); + mcontext.flags = DR_MC_ALL; void *drcontext = dr_get_current_drcontext(); /* diff --git a/suite/tests/client-interface/cleancall.dll.c b/suite/tests/client-interface/cleancall.dll.c index b95407d8f09..ab25a27b4e7 100644 --- a/suite/tests/client-interface/cleancall.dll.c +++ b/suite/tests/client-interface/cleancall.dll.c @@ -59,10 +59,9 @@ set_gpr() { check_stack_alignment(); void *drcontext = dr_get_current_drcontext(); - dr_mcontext_t mcontext = { - sizeof(mcontext), - DR_MC_ALL, - }; + dr_mcontext_t mcontext; + mcontext.size = sizeof(mcontext); + mcontext.flags = DR_MC_ALL; dr_get_mcontext(drcontext, &mcontext); reg_get_value_ex(DR_REG_XAX, &mcontext, orig_reg_val_buf); reg_get_value_ex(DR_REG_XAX, &mcontext, new_reg_val_buf); @@ -80,10 +79,9 @@ check_gpr() { check_stack_alignment(); void *drcontext = dr_get_current_drcontext(); - dr_mcontext_t mcontext = { - sizeof(mcontext), - DR_MC_ALL, - }; + dr_mcontext_t mcontext; + mcontext.size = sizeof(mcontext); + mcontext.flags = DR_MC_ALL; dr_get_mcontext(drcontext, &mcontext); reg_get_value_ex(DR_REG_XAX, &mcontext, new_reg_val_buf); print_error_on_fail(new_reg_val_buf[0] == 0x75); @@ -99,10 +97,9 @@ set_xmm() { check_stack_alignment(); void *drcontext = dr_get_current_drcontext(); - dr_mcontext_t mcontext = { - sizeof(mcontext), - DR_MC_ALL, - }; + dr_mcontext_t mcontext; + mcontext.size = sizeof(mcontext); + mcontext.flags = DR_MC_ALL; dr_get_mcontext(drcontext, &mcontext); reg_get_value_ex(DR_REG_XMM0, &mcontext, orig_reg_val_buf); reg_get_value_ex(DR_REG_XMM0, &mcontext, new_reg_val_buf); @@ -120,10 +117,9 @@ check_xmm() { check_stack_alignment(); void *drcontext = dr_get_current_drcontext(); - dr_mcontext_t mcontext = { - sizeof(mcontext), - DR_MC_ALL, - }; + dr_mcontext_t mcontext; + mcontext.size = sizeof(mcontext); + mcontext.flags = DR_MC_ALL; dr_get_mcontext(drcontext, &mcontext); reg_get_value_ex(DR_REG_XMM0, &mcontext, new_reg_val_buf); print_error_on_fail(new_reg_val_buf[0] == 0x77); @@ -139,10 +135,9 @@ set_ymm() { check_stack_alignment(); void *drcontext = dr_get_current_drcontext(); - dr_mcontext_t mcontext = { - sizeof(mcontext), - DR_MC_ALL, - }; + dr_mcontext_t mcontext; + mcontext.size = sizeof(mcontext); + mcontext.flags = DR_MC_ALL; dr_get_mcontext(drcontext, &mcontext); reg_get_value_ex(DR_REG_YMM0, &mcontext, orig_reg_val_buf); reg_get_value_ex(DR_REG_YMM0, &mcontext, new_reg_val_buf); @@ -162,10 +157,9 @@ check_ymm() { check_stack_alignment(); void *drcontext = dr_get_current_drcontext(); - dr_mcontext_t mcontext = { - sizeof(mcontext), - DR_MC_ALL, - }; + dr_mcontext_t mcontext; + mcontext.size = sizeof(mcontext); + mcontext.flags = DR_MC_ALL; dr_get_mcontext(drcontext, &mcontext); reg_get_value_ex(DR_REG_YMM0, &mcontext, new_reg_val_buf); print_error_on_fail(new_reg_val_buf[0] == 0x77); @@ -184,10 +178,9 @@ set_zmm() { check_stack_alignment(); void *drcontext = dr_get_current_drcontext(); - dr_mcontext_t mcontext = { - sizeof(mcontext), - DR_MC_ALL, - }; + dr_mcontext_t mcontext; + mcontext.size = sizeof(mcontext); + mcontext.flags = DR_MC_ALL; dr_get_mcontext(drcontext, &mcontext); reg_get_value_ex(DR_REG_ZMM0, &mcontext, orig_reg_val_buf); reg_get_value_ex(DR_REG_ZMM0, &mcontext, new_reg_val_buf); @@ -210,10 +203,9 @@ check_zmm() { check_stack_alignment(); void *drcontext = dr_get_current_drcontext(); - dr_mcontext_t mcontext = { - sizeof(mcontext), - DR_MC_ALL, - }; + dr_mcontext_t mcontext; + mcontext.size = sizeof(mcontext); + mcontext.flags = DR_MC_ALL; dr_get_mcontext(drcontext, &mcontext); reg_get_value_ex(DR_REG_ZMM0, &mcontext, new_reg_val_buf); print_error_on_fail(new_reg_val_buf[0] == 0x77); diff --git a/suite/tests/client-interface/drreg-end-restore.dll.c b/suite/tests/client-interface/drreg-end-restore.dll.c index 807b78dc4cb..3f0d0fa21e9 100644 --- a/suite/tests/client-interface/drreg-end-restore.dll.c +++ b/suite/tests/client-interface/drreg-end-restore.dll.c @@ -129,10 +129,9 @@ event_bb_analysis(void *drcontext, void *tag, instrlist_t *bb, bool for_trace, static reg_t get_val_from_ctx(void *drcontext, reg_id_t reg_id) { - dr_mcontext_t mcontext = { - sizeof(mcontext), - DR_MC_ALL, - }; + dr_mcontext_t mcontext; + mcontext.size = sizeof(mcontext); + mcontext.flags = DR_MC_ALL; dr_get_mcontext(drcontext, &mcontext); return reg_get_value(reg_id, &mcontext); } diff --git a/suite/tests/client-interface/events.dll.c b/suite/tests/client-interface/events.dll.c index d711f79d179..593831caaa4 100644 --- a/suite/tests/client-interface/events.dll.c +++ b/suite/tests/client-interface/events.dll.c @@ -452,7 +452,8 @@ kernel_xfer_event2(void *drcontext, const dr_kernel_xfer_info_t *info) if (info->type == DR_XFER_CLIENT_REDIRECT) { /* Test for exception event redirect. */ ASSERT(info->source_mcontext != NULL); - dr_mcontext_t mc = { sizeof(mc) }; + dr_mcontext_t mc; + mc.size = sizeof(mc); mc.flags = DR_MC_CONTROL; bool ok = dr_get_mcontext(drcontext, &mc); ASSERT(ok); @@ -469,10 +470,7 @@ static bool exception_event_redirect(void *dcontext, dr_exception_t *excpt) { app_pc addr; - dr_mcontext_t mcontext = { - sizeof(mcontext), - DR_MC_ALL, - }; + dr_mcontext_t mcontext; module_data_t *data = dr_lookup_module_by_name("client." EVENTS ".exe"); dr_fprintf(STDERR, "exception event redirect\n"); if (data == NULL) { diff --git a/suite/tests/client-interface/flush.dll.c b/suite/tests/client-interface/flush.dll.c index 4c33a003f21..f8153997ddd 100644 --- a/suite/tests/client-interface/flush.dll.c +++ b/suite/tests/client-interface/flush.dll.c @@ -195,11 +195,9 @@ callback(void *tag, app_pc next_pc) if (callback_count % 100 == 0) { if (callback_count % 200 == 0) { /* For windows test dr_flush_region() half the time */ - dr_mcontext_t mcontext = { - sizeof(mcontext), - DR_MC_ALL, - }; - + dr_mcontext_t mcontext; + mcontext.size = sizeof(mcontext); + mcontext.flags = DR_MC_ALL; dr_delay_flush_region((app_pc)tag - 20, 30, callback_count, flush_event); dr_get_mcontext(dr_get_current_drcontext(), &mcontext); mcontext.pc = next_pc; @@ -292,7 +290,8 @@ kernel_xfer_event(void *drcontext, const dr_kernel_xfer_info_t *info) /* Test kernel xfer on dr_redirect_execution */ dr_fprintf(STDERR, "%s: type %d\n", __FUNCTION__, info->type); ASSERT(info->source_mcontext != NULL); - dr_mcontext_t mc = { sizeof(mc) }; + dr_mcontext_t mc; + mc.size = sizeof(mc); mc.flags = DR_MC_CONTROL; bool ok = dr_get_mcontext(drcontext, &mc); ASSERT(ok); diff --git a/suite/tests/client-interface/syscall.dll.c b/suite/tests/client-interface/syscall.dll.c index 307239167c4..01620c48d5a 100644 --- a/suite/tests/client-interface/syscall.dll.c +++ b/suite/tests/client-interface/syscall.dll.c @@ -47,10 +47,9 @@ static void at_syscall() { if (monitoring) { - dr_mcontext_t mcontext = { - sizeof(mcontext), - DR_MC_ALL, - }; + dr_mcontext_t mcontext; + mcontext.size = sizeof(mcontext); + mcontext.flags = DR_MC_ALL; void *drcontext = dr_get_current_drcontext(); dr_get_mcontext(drcontext, &mcontext); dr_fprintf(STDERR, PFX "\n", mcontext.xax); From 32fd3890937f190d71924bad0aa631c2c0eb156a Mon Sep 17 00:00:00 2001 From: Abhinav Anil Sharma Date: Tue, 27 Oct 2020 22:28:14 -0400 Subject: [PATCH 17/22] Add XXX comments to describe future work issues. --- api/docs/release.dox | 5 +++-- core/synch.c | 1 + core/unix/signal.c | 1 + suite/tests/CMakeLists.txt | 6 ++++-- suite/tests/linux/thread.c | 4 ++-- 5 files changed, 11 insertions(+), 6 deletions(-) diff --git a/api/docs/release.dox b/api/docs/release.dox index b7153b3193a..dbd1a8254c6 100644 --- a/api/docs/release.dox +++ b/api/docs/release.dox @@ -174,8 +174,9 @@ Further non-compatibility-affecting changes include: opnd_is_immed_double() to enable the creation and handling of double precision floating-point operands. - Added -reset_at_created_thread_count option to allow triggering reset when - created thread count reaches the given value. This option is currently used to - make Linux reset tests more robust. + created thread count reaches the given value. This is unlike + reset_at_nth_thread which checks the existing thread count. This option is + currently used to make Linux reset tests more robust. **************************************************
diff --git a/core/synch.c b/core/synch.c index ab4b4dce9f2..0b7882e3320 100644 --- a/core/synch.c +++ b/core/synch.c @@ -1750,6 +1750,7 @@ translate_from_synchall_to_dispatch(thread_record_t *tr, thread_synch_state_t sy } }); IF_AARCHXX({ + // XXX i#4495: Consider saving stolen reg's application value. set_stolen_reg_val(mc, (reg_t)os_get_dr_tls_base(dcontext)); // XXX: This path is tested by linux.thread-reset and linux.clone-reset. // We just haven't run those on ARM yet. diff --git a/core/unix/signal.c b/core/unix/signal.c index 00b113925b8..fbcccaf3409 100644 --- a/core/unix/signal.c +++ b/core/unix/signal.c @@ -3423,6 +3423,7 @@ transfer_from_sig_handler_to_fcache_return(dcontext_t *dcontext, kernel_ucontext * from DR's handler. */ ASSERT(get_sigcxt_stolen_reg(sc) != (reg_t)*get_dr_tls_base_addr()); + // XXX i#4495: Consider saving stolen reg's application value. set_sigcxt_stolen_reg(sc, (reg_t)*get_dr_tls_base_addr()); # ifndef AARCH64 /* We're going to our fcache_return gencode which uses DEFAULT_ISA_MODE */ diff --git a/suite/tests/CMakeLists.txt b/suite/tests/CMakeLists.txt index d658c4a93bd..8ef7a6243c5 100644 --- a/suite/tests/CMakeLists.txt +++ b/suite/tests/CMakeLists.txt @@ -4069,8 +4069,10 @@ if (UNIX) #tobuild(linux.vfork-fib linux/vfork-fib.c) # runs of other builds with custom DR options - # XXX: Using -reset_at_created_thread_count 2 causes an ASSERT failure, so - # we use -reset_at_created_thread_count 3 instead. + # XXX i#4496: Using -reset_at_created_thread_count 2 causes an ASSERT + # failure, so we use -reset_at_created_thread_count 3 instead. When this + # is fixed, consider replacing with -reset_at_nth_thread 2 and maybe + # remove the reset_at_created_thread_count flag from DR options. torunonly(linux.thread-reset linux.thread linux/thread.c "-enable_reset -reset_at_created_thread_count 3" "") torunonly(linux.clone-reset linux.clone linux/clone.c diff --git a/suite/tests/linux/thread.c b/suite/tests/linux/thread.c index b902d0e987d..f60c963aa5c 100644 --- a/suite/tests/linux/thread.c +++ b/suite/tests/linux/thread.c @@ -85,8 +85,8 @@ main() { // We need to create multiple threads to verify correctness of reset, // which requires more than one thread. - // XXX: Using -reset_at_created_thread_count 2 causes an ASSERT failure, - // so we use 3 threads for now. + // XXX i#4496: Using -reset_at_created_thread_count 2 causes an ASSERT + // failure, so we use 3 total threads for now. for (int i = 0; i < 2; i++) { sleeptime.tv_sec = 0; sleeptime.tv_nsec = 10 * 1000 * 1000; /* 10ms */ From 9af7c48be098302a2f224e08d0569f5df6eb01c7 Mon Sep 17 00:00:00 2001 From: Abhinav Anil Sharma Date: Tue, 27 Oct 2020 23:43:00 -0400 Subject: [PATCH 18/22] Fix signature of defined dr_flush_region_ex. --- core/lib/instrument.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/lib/instrument.c b/core/lib/instrument.c index 4a890820a10..45ea3b671b0 100644 --- a/core/lib/instrument.c +++ b/core/lib/instrument.c @@ -7175,8 +7175,8 @@ DR_API * flush completes and before threads are resumed. Caller must use * dr_redirect_execution() to return to the cache. */ bool -dr_flush_region_ex(app_pc start, size_t size, void (*flush_completion_callback)(), - void *user_data) +dr_flush_region_ex(app_pc start, size_t size, + void (*flush_completion_callback)(void *user_data), void *user_data) { dcontext_t *dcontext = get_thread_private_dcontext(); CLIENT_ASSERT(!standalone_library, "API not supported in standalone mode"); From 47c81722b39b3cdbae0f26232cbfeb67334cbae7 Mon Sep 17 00:00:00 2001 From: Abhinav Anil Sharma Date: Tue, 27 Oct 2020 23:53:41 -0400 Subject: [PATCH 19/22] Add missing user_data to early-return invocations of callback. --- core/lib/instrument.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/lib/instrument.c b/core/lib/instrument.c index 45ea3b671b0..9216ca0b1dd 100644 --- a/core/lib/instrument.c +++ b/core/lib/instrument.c @@ -7204,12 +7204,12 @@ dr_flush_region_ex(app_pc start, size_t size, /* release build check of requirements, as many as possible at least */ if (size == 0 || is_couldbelinking(dcontext)) { - (*flush_completion_callback)(); + (*flush_completion_callback)(user_data); return false; } if (!executable_vm_area_executed_from(start, start + size)) { - (*flush_completion_callback)(); + (*flush_completion_callback)(user_data); return true; } From b2ebbea8dd1ad5c4520247c97c44fcdf5d5bc51d Mon Sep 17 00:00:00 2001 From: Abhinav Anil Sharma Date: Wed, 28 Oct 2020 01:29:53 -0400 Subject: [PATCH 20/22] Fix formatting and a callsite with missing arg. --- core/options.c | 3 +-- core/optionsx.h | 35 +++++++++++++++++------------------ core/unix/os.c | 3 ++- 3 files changed, 20 insertions(+), 21 deletions(-) diff --git a/core/options.c b/core/options.c index 9a935ff7bc1..c508cdb06a8 100644 --- a/core/options.c +++ b/core/options.c @@ -1506,8 +1506,7 @@ check_option_compatibility_helper(int recurse_count) USAGE_ERROR("-reset_at_fragment_count requires -enable_reset, enabling"); dynamo_options.enable_reset = true; changed_options = true; - } - else if (INTERNAL_OPTION(reset_at_created_thread_count)) { + } else if (INTERNAL_OPTION(reset_at_created_thread_count)) { USAGE_ERROR("-reset_at_created_thread_count requires -enable_reset, " "enabling"); dynamo_options.enable_reset = true; diff --git a/core/optionsx.h b/core/optionsx.h index 4778e4571de..61ea18f38b7 100644 --- a/core/optionsx.h +++ b/core/optionsx.h @@ -133,24 +133,23 @@ PC_OPTION_DEFAULT_INTERNAL(type, name, 0, description) /* option helper macros */ -#define DISABLE_RESET(prefix) \ - { \ - (prefix)->enable_reset = false; \ - IF_INTERNAL((prefix)->reset_at_fragment_count = 0;) \ - IF_INTERNAL( \ - (prefix)->reset_at_created_thread_count = 0;) \ - (prefix)->reset_at_nth_thread = 0; \ - (prefix)->reset_at_switch_to_os_at_vmm_limit = false; \ - (prefix)->reset_at_vmm_percent_free_limit = 0; \ - (prefix)->reset_at_vmm_free_limit = 0; \ - (prefix)->reset_at_vmm_full = false; \ - (prefix)->reset_at_commit_percent_free_limit = 0; \ - (prefix)->reset_at_commit_free_limit = 0; \ - (prefix)->reset_every_nth_pending = 0; \ - (prefix)->reset_at_nth_bb_unit = 0; \ - (prefix)->reset_at_nth_trace_unit = 0; \ - (prefix)->reset_every_nth_bb_unit = 0; \ - (prefix)->reset_every_nth_trace_unit = 0; \ +#define DISABLE_RESET(prefix) \ + { \ + (prefix)->enable_reset = false; \ + IF_INTERNAL((prefix)->reset_at_fragment_count = 0;) \ + IF_INTERNAL((prefix)->reset_at_created_thread_count = 0;) \ + (prefix)->reset_at_nth_thread = 0; \ + (prefix)->reset_at_switch_to_os_at_vmm_limit = false; \ + (prefix)->reset_at_vmm_percent_free_limit = 0; \ + (prefix)->reset_at_vmm_free_limit = 0; \ + (prefix)->reset_at_vmm_full = false; \ + (prefix)->reset_at_commit_percent_free_limit = 0; \ + (prefix)->reset_at_commit_free_limit = 0; \ + (prefix)->reset_every_nth_pending = 0; \ + (prefix)->reset_at_nth_bb_unit = 0; \ + (prefix)->reset_at_nth_trace_unit = 0; \ + (prefix)->reset_every_nth_bb_unit = 0; \ + (prefix)->reset_every_nth_trace_unit = 0; \ } #define REENABLE_RESET(prefix) \ { \ diff --git a/core/unix/os.c b/core/unix/os.c index 4aaee6aee03..af9923beca9 100644 --- a/core/unix/os.c +++ b/core/unix/os.c @@ -7656,7 +7656,8 @@ pre_system_call(dcontext_t *dcontext) * new code. */ false /*don't force synchall*/, - NULL /*flush_completion_callback*/); + NULL /*flush_completion_callback*/, + NULL /*user_data*/); break; } # endif /* ARM */ From f4211417028cb73e37825307e180d64daf7e75a2 Mon Sep 17 00:00:00 2001 From: Abhinav Anil Sharma Date: Fri, 30 Oct 2020 00:24:39 -0400 Subject: [PATCH 21/22] Revert changes related to i#4497 that were separated into a different PR. --- api/docs/release.dox | 4 ---- core/dynamo.c | 8 -------- core/options.c | 5 ----- core/optionsx.h | 35 +++++++++++++++----------------- core/synch.c | 7 ------- core/unix/signal.c | 1 - suite/tests/CMakeLists.txt | 8 ++------ suite/tests/linux/thread.c | 36 ++++++++++++++------------------- suite/tests/linux/thread.expect | 14 ------------- 9 files changed, 33 insertions(+), 85 deletions(-) diff --git a/api/docs/release.dox b/api/docs/release.dox index dbd1a8254c6..7614d50a632 100644 --- a/api/docs/release.dox +++ b/api/docs/release.dox @@ -173,10 +173,6 @@ Further non-compatibility-affecting changes include: - Added opnd_create_immed_double(), opnd_get_immed_double() and opnd_is_immed_double() to enable the creation and handling of double precision floating-point operands. - - Added -reset_at_created_thread_count option to allow triggering reset when - created thread count reaches the given value. This is unlike - reset_at_nth_thread which checks the existing thread count. This option is - currently used to make Linux reset tests more robust. **************************************************
diff --git a/core/dynamo.c b/core/dynamo.c index cc5b50521d8..e9992c72570 100644 --- a/core/dynamo.c +++ b/core/dynamo.c @@ -2184,14 +2184,6 @@ add_thread(IF_WINDOWS_ELSE_NP(HANDLE hthread, process_id_t pid), thread_id_t tid RSTATS_ADD_PEAK(num_threads, 1); RSTATS_INC(num_threads_created); num_known_threads++; - DOSTATS({ - if (d_r_stats != NULL && - (uint)GLOBAL_STAT(num_threads_created) == - INTERNAL_OPTION(reset_at_created_thread_count)) { - ASSERT(INTERNAL_OPTION(reset_at_created_thread_count) != 0); - schedule_reset(RESET_ALL); - } - }); d_r_mutex_unlock(&all_threads_lock); } diff --git a/core/options.c b/core/options.c index c508cdb06a8..a0496bb862a 100644 --- a/core/options.c +++ b/core/options.c @@ -1506,11 +1506,6 @@ check_option_compatibility_helper(int recurse_count) USAGE_ERROR("-reset_at_fragment_count requires -enable_reset, enabling"); dynamo_options.enable_reset = true; changed_options = true; - } else if (INTERNAL_OPTION(reset_at_created_thread_count)) { - USAGE_ERROR("-reset_at_created_thread_count requires -enable_reset, " - "enabling"); - dynamo_options.enable_reset = true; - changed_options = true; } # endif /* EXPOSE_INTERNAL_OPTIONS */ else if (DYNAMO_OPTION(reset_at_switch_to_os_at_vmm_limit)) { diff --git a/core/optionsx.h b/core/optionsx.h index 61ea18f38b7..7a5198607ba 100644 --- a/core/optionsx.h +++ b/core/optionsx.h @@ -133,23 +133,22 @@ PC_OPTION_DEFAULT_INTERNAL(type, name, 0, description) /* option helper macros */ -#define DISABLE_RESET(prefix) \ - { \ - (prefix)->enable_reset = false; \ - IF_INTERNAL((prefix)->reset_at_fragment_count = 0;) \ - IF_INTERNAL((prefix)->reset_at_created_thread_count = 0;) \ - (prefix)->reset_at_nth_thread = 0; \ - (prefix)->reset_at_switch_to_os_at_vmm_limit = false; \ - (prefix)->reset_at_vmm_percent_free_limit = 0; \ - (prefix)->reset_at_vmm_free_limit = 0; \ - (prefix)->reset_at_vmm_full = false; \ - (prefix)->reset_at_commit_percent_free_limit = 0; \ - (prefix)->reset_at_commit_free_limit = 0; \ - (prefix)->reset_every_nth_pending = 0; \ - (prefix)->reset_at_nth_bb_unit = 0; \ - (prefix)->reset_at_nth_trace_unit = 0; \ - (prefix)->reset_every_nth_bb_unit = 0; \ - (prefix)->reset_every_nth_trace_unit = 0; \ +#define DISABLE_RESET(prefix) \ + { \ + (prefix)->enable_reset = false; \ + IF_INTERNAL((prefix)->reset_at_fragment_count = 0;) \ + (prefix)->reset_at_nth_thread = 0; \ + (prefix)->reset_at_switch_to_os_at_vmm_limit = false; \ + (prefix)->reset_at_vmm_percent_free_limit = 0; \ + (prefix)->reset_at_vmm_free_limit = 0; \ + (prefix)->reset_at_vmm_full = false; \ + (prefix)->reset_at_commit_percent_free_limit = 0; \ + (prefix)->reset_at_commit_free_limit = 0; \ + (prefix)->reset_every_nth_pending = 0; \ + (prefix)->reset_at_nth_bb_unit = 0; \ + (prefix)->reset_at_nth_trace_unit = 0; \ + (prefix)->reset_every_nth_bb_unit = 0; \ + (prefix)->reset_every_nth_trace_unit = 0; \ } #define REENABLE_RESET(prefix) \ { \ @@ -1495,8 +1494,6 @@ OPTION_COMMAND(bool, enable_reset, IF_X86_ELSE(true, false), "enable_reset", OPTION_DEFAULT_INTERNAL(uint, reset_at_fragment_count, 0, "reset all caches at a certain fragment count") -OPTION_DEFAULT_INTERNAL(uint, reset_at_created_thread_count, 0, - "reset all caches when created thread count reaches given value") OPTION(uint, reset_at_nth_thread, "reset all caches when the nth thread is explicitly created") /* FIXME - is potentially using up all the os allocation leaving nothing for the diff --git a/core/synch.c b/core/synch.c index 0b7882e3320..22f9f655249 100644 --- a/core/synch.c +++ b/core/synch.c @@ -1749,13 +1749,6 @@ translate_from_synchall_to_dispatch(thread_record_t *tr, thread_synch_state_t sy arch_mcontext_reset_stolen_reg(dcontext, mc); } }); - IF_AARCHXX({ - // XXX i#4495: Consider saving stolen reg's application value. - set_stolen_reg_val(mc, (reg_t)os_get_dr_tls_base(dcontext)); - // XXX: This path is tested by linux.thread-reset and linux.clone-reset. - // We just haven't run those on ARM yet. - IF_ARM(ASSERT_NOT_TESTED()); - }); /* We send all threads, regardless of whether was in DR or not, to * re-interp from translated cxt, to avoid having to handle stale * local state problems if we simply resumed. diff --git a/core/unix/signal.c b/core/unix/signal.c index fbcccaf3409..00b113925b8 100644 --- a/core/unix/signal.c +++ b/core/unix/signal.c @@ -3423,7 +3423,6 @@ transfer_from_sig_handler_to_fcache_return(dcontext_t *dcontext, kernel_ucontext * from DR's handler. */ ASSERT(get_sigcxt_stolen_reg(sc) != (reg_t)*get_dr_tls_base_addr()); - // XXX i#4495: Consider saving stolen reg's application value. set_sigcxt_stolen_reg(sc, (reg_t)*get_dr_tls_base_addr()); # ifndef AARCH64 /* We're going to our fcache_return gencode which uses DEFAULT_ISA_MODE */ diff --git a/suite/tests/CMakeLists.txt b/suite/tests/CMakeLists.txt index 8ef7a6243c5..387b3b45632 100644 --- a/suite/tests/CMakeLists.txt +++ b/suite/tests/CMakeLists.txt @@ -4069,14 +4069,10 @@ if (UNIX) #tobuild(linux.vfork-fib linux/vfork-fib.c) # runs of other builds with custom DR options - # XXX i#4496: Using -reset_at_created_thread_count 2 causes an ASSERT - # failure, so we use -reset_at_created_thread_count 3 instead. When this - # is fixed, consider replacing with -reset_at_nth_thread 2 and maybe - # remove the reset_at_created_thread_count flag from DR options. torunonly(linux.thread-reset linux.thread linux/thread.c - "-enable_reset -reset_at_created_thread_count 3" "") + "-enable_reset -reset_at_fragment_count 100" "") torunonly(linux.clone-reset linux.clone linux/clone.c - "-enable_reset -reset_at_created_thread_count 3" "") + "-enable_reset -reset_at_fragment_count 100" "") tobuild(pthreads.pthreads pthreads/pthreads.c) tobuild(pthreads.pthreads_exit pthreads/pthreads_exit.c) tobuild(pthreads.ptsig pthreads/ptsig.c) diff --git a/suite/tests/linux/thread.c b/suite/tests/linux/thread.c index f60c963aa5c..18596e3ebe8 100644 --- a/suite/tests/linux/thread.c +++ b/suite/tests/linux/thread.c @@ -83,28 +83,22 @@ static struct timespec sleeptime; int main() { - // We need to create multiple threads to verify correctness of reset, - // which requires more than one thread. - // XXX i#4496: Using -reset_at_created_thread_count 2 causes an ASSERT - // failure, so we use 3 total threads for now. - for (int i = 0; i < 2; i++) { - sleeptime.tv_sec = 0; - sleeptime.tv_nsec = 10 * 1000 * 1000; /* 10ms */ - - child_exit = false; - child_done = false; - child = create_thread(run, NULL, &stack); - assert(child > -1); - - /* waste some time */ - nanosleep(&sleeptime, NULL); + sleeptime.tv_sec = 0; + sleeptime.tv_nsec = 10 * 1000 * 1000; /* 10ms */ - child_exit = true; - /* we want deterministic printf ordering */ - while (!child_done) - nanosleep(&sleeptime, NULL); - delete_thread(child, stack); - } + child_exit = false; + child_done = false; + child = create_thread(run, NULL, &stack); + assert(child > -1); + + /* waste some time */ + nanosleep(&sleeptime, NULL); + + child_exit = true; + /* we want deterministic printf ordering */ + while (!child_done) + nanosleep(&sleeptime, NULL); + delete_thread(child, stack); } /* Procedure executed by sideline threads diff --git a/suite/tests/linux/thread.expect b/suite/tests/linux/thread.expect index 7932fefc5bf..242de3e9226 100644 --- a/suite/tests/linux/thread.expect +++ b/suite/tests/linux/thread.expect @@ -12,17 +12,3 @@ i = 25000000 Sideline thread finished Waiting for child to exit Child has exited -Sideline thread started -i = 2500000 -i = 5000000 -i = 7500000 -i = 10000000 -i = 12500000 -i = 15000000 -i = 17500000 -i = 20000000 -i = 22500000 -i = 25000000 -Sideline thread finished -Waiting for child to exit -Child has exited From be2e3aa10b807e7182fefda311942f9502a2a876 Mon Sep 17 00:00:00 2001 From: Abhinav Anil Sharma Date: Fri, 30 Oct 2020 00:53:20 -0400 Subject: [PATCH 22/22] Comment, TODO and documentation changes. --- api/docs/bt.dox | 12 +++++++----- api/docs/release.dox | 3 +++ suite/tests/CMakeLists.txt | 4 ++++ 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/api/docs/bt.dox b/api/docs/bt.dox index 788d894009c..bb8ca09d65c 100644 --- a/api/docs/bt.dox +++ b/api/docs/bt.dox @@ -981,15 +981,17 @@ dr_insert_cbr_instrumentation() \subsection sec_adaptive Dynamic Instrumentation DynamoRIO allows a client to dynamically adjust its instrumentation -by providing routines to flush all cached fragments corresponding to -an application code region: +by providing a routine to flush all cached fragments corresponding to +an application code region and register (or unregister) instrumentation +event callbacks: \code -dr_flush_region() -dr_unlink_flush_region() -dr_delay_flush_region() +dr_flush_region_ex() \endcode +The client should provide a callback to this routine, that unregisters +old instrumentation event callbacks, and registers new ones. + In order to directly modify the instrumentation on a particular fragment (as opposed to replacing instrumentation on all copies of fragments corresponding to particular application code), DynamoRIO also supports diff --git a/api/docs/release.dox b/api/docs/release.dox index 7614d50a632..4488ad0e5ec 100644 --- a/api/docs/release.dox +++ b/api/docs/release.dox @@ -173,6 +173,9 @@ Further non-compatibility-affecting changes include: - Added opnd_create_immed_double(), opnd_get_immed_double() and opnd_is_immed_double() to enable the creation and handling of double precision floating-point operands. + - Added dr_flush_region_ex API that accepts a callback to be executed after synch + flush but before the threads are resumed. The existing dr_flush_region API + is modified to invoke dr_flush_region_ex with a NULL callback. **************************************************
diff --git a/suite/tests/CMakeLists.txt b/suite/tests/CMakeLists.txt index 387b3b45632..ba31135f4ad 100644 --- a/suite/tests/CMakeLists.txt +++ b/suite/tests/CMakeLists.txt @@ -4069,6 +4069,10 @@ if (UNIX) #tobuild(linux.vfork-fib linux/vfork-fib.c) # runs of other builds with custom DR options + # TODO i#4497: The following two tests need to trigger reset when multiple + # threads are active. But, the current -reset_at_fragment_count value does + # not ensure that. We need to make these tests more robust, perhaps by + # using a different trigger like -reset_at_nth_thread (pending i#4496). torunonly(linux.thread-reset linux.thread linux/thread.c "-enable_reset -reset_at_fragment_count 100" "") torunonly(linux.clone-reset linux.clone linux/clone.c