Skip to content

Commit

Permalink
i#4014 dr$sim phys: Fix 2 crashes (#5541)
Browse files Browse the repository at this point in the history
Fixes two bugs causing crashes with -use_physical:

+ Records the drcontext used for hashtable creation to ensure the same
  one is used at destruction as a different thread can call the exit
  event.

+ Orders the drmemtrace thread exit event before the drmodtrack one to
  ensure drmodtrack access during the final thread buffer output is safe.

These fixes were manually tested on large multi-threaded applications
where these two crashes showed up before and disappear with the fixes.

Issue: #4014
  • Loading branch information
derekbruening authored Jun 22, 2022
1 parent 1152e64 commit 4be6b6d
Show file tree
Hide file tree
Showing 4 changed files with 16 additions and 5 deletions.
2 changes: 1 addition & 1 deletion clients/drcachesim/tracer/instru_offline.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ offline_instru_t::get_entry_addr(void *drcontext, byte *buf_ptr) const
{
offline_entry_t *entry = (offline_entry_t *)buf_ptr;
if (entry->addr.type == OFFLINE_TYPE_PC) {
// XXX i#4014: Use caching to avoid looup for last queried modbase.
// XXX i#4014: Use caching to avoid lookup for last queried modbase.
app_pc modbase;
if (drmodtrack_lookup_pc_from_index(drcontext, entry->pc.modidx, &modbase) !=
DRCOVLIB_SUCCESS)
Expand Down
10 changes: 7 additions & 3 deletions clients/drcachesim/tracer/physaddr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ physaddr_t::physaddr_t()
, cache_idx_(0)
, fd_(-1)
, v2p_(nullptr)
, drcontext_(nullptr)
, count_(0)
, num_hit_cache_(0)
, num_hit_table_(0)
Expand Down Expand Up @@ -95,7 +96,7 @@ physaddr_t::~physaddr_t()
num_hit_cache_, num_hit_table_, num_miss_);
}
if (v2p_ != nullptr)
dr_hashtable_destroy(dr_get_current_drcontext(), v2p_);
dr_hashtable_destroy(drcontext_, v2p_);
#endif
}

Expand Down Expand Up @@ -150,7 +151,10 @@ physaddr_t::init()
// loads compared to the data inlined into the array here, and higher
// resize thresholds are also slower.
// With the setup here, the hashtable lookup is no longer the bottleneck.
v2p_ = dr_hashtable_create(dr_get_current_drcontext(), V2P_INITIAL_BITS, 20,
// We record the context so we can pass the same one in our destructor, which
// might be called from a different thread.
drcontext_ = dr_get_current_drcontext();
v2p_ = dr_hashtable_create(drcontext_, V2P_INITIAL_BITS, 20,
/*synch=*/false, nullptr);

// We avoid std::ostringstream to avoid malloc use for static linking.
Expand Down Expand Up @@ -249,7 +253,7 @@ physaddr_t::virtual2physical(void *drcontext, addr_t virt, OUT addr_t *phys,
NOTIFY(3, "v2p: %p => entry " HEX64_FORMAT_STRING " @ offs " INT64_FORMAT_STRING "\n",
vpage, entry, offs);
if (!TESTALL(PAGEMAP_VALID, entry) || TESTANY(PAGEMAP_SWAP, entry)) {
NOTIFY(1, "v2p failure: entry %p is invalid for %p\n", vpage);
NOTIFY(1, "v2p failure: entry %p is invalid for %p\n", entry, vpage);
return false;
}
addr_t ppage = (addr_t)((entry & PAGEMAP_PFN) << page_bits_);
Expand Down
2 changes: 2 additions & 0 deletions clients/drcachesim/tracer/physaddr.h
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@ class physaddr_t {
// The drcontainers hashtable is too slow due to the extra dereferences:
// we need an open-addressed table.
void *v2p_;
// We must pass the same context to free as we used to allocate.
void *drcontext_;
static constexpr addr_t PAGE_INVALID = (addr_t)-1;
// With hashtable_t nullptr is how non-existence is shown, so we store
// an actual 0 address (can happen for physical) as this sentinel.
Expand Down
7 changes: 6 additions & 1 deletion clients/drcachesim/tracer/tracer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3280,8 +3280,13 @@ drmemtrace_client_main(client_id_t id, int argc, const char *argv[])
#ifdef UNIX
dr_register_fork_init_event(fork_init);
#endif
/* We need our thread exit event to run *before* drmodtrack's as we may
* need to translate physical addresses for the thread's final buffer.
*/
drmgr_priority_t pri_thread_exit = { sizeof(drmgr_priority_t), "", nullptr, nullptr,
-100 };
if (!drmgr_register_thread_init_event(event_thread_init) ||
!drmgr_register_thread_exit_event(event_thread_exit))
!drmgr_register_thread_exit_event_ex(event_thread_exit, &pri_thread_exit))
DR_ASSERT(false);

instrumentation_init();
Expand Down

0 comments on commit 4be6b6d

Please sign in to comment.