Skip to content

Commit

Permalink
Cleanup
Browse files Browse the repository at this point in the history
  • Loading branch information
@abhinav92003
abhinav92003 committed Dec 22, 2024
1 parent 6bcc33b commit b24d79a
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 29 deletions.
26 changes: 17 additions & 9 deletions clients/drcachesim/tests/decode_cache_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ namespace drmemtrace {

static constexpr addr_t TID_A = 1;
static constexpr offline_file_type_t ENCODING_FILE_TYPE =
static_cast<offline_file_type_t>(OFFLINE_FILE_VERSION_ENCODINGS);
static_cast<offline_file_type_t>(OFFLINE_FILE_TYPE_ENCODINGS);

class test_decode_info_t : public decode_info_base_t {
public:
Expand Down Expand Up @@ -113,11 +113,13 @@ check_decode_caching(void *drcontext, bool persist_instrs, bool use_module_mappe
test_decode_cache_t<instr_decode_info_t> decode_cache(
drcontext,
/*persist_decoded_instr=*/true, ilist_for_test_decode_cache);
decode_cache.init(ENCODING_FILE_TYPE, module_file_for_test_decode_cache, "");
std::string err =
decode_cache.init(ENCODING_FILE_TYPE, module_file_for_test_decode_cache, "");
if (err != "")
return err;
for (const memref_t &memref : memrefs) {
instr_decode_info_t *unused_cached_decode_info;
std::string err =
decode_cache.add_decode_info(memref.instr, unused_cached_decode_info);
err = decode_cache.add_decode_info(memref.instr, unused_cached_decode_info);
if (err != "")
return err;
}
Expand All @@ -139,8 +141,10 @@ check_decode_caching(void *drcontext, bool persist_instrs, bool use_module_mappe
test_decode_cache_t<test_decode_info_t> decode_cache(
drcontext,
/*persist_decoded_instrs=*/false, ilist_for_test_decode_cache);
decode_cache.init(ENCODING_FILE_TYPE, module_file_for_test_decode_cache, "");

std::string err =
decode_cache.init(ENCODING_FILE_TYPE, module_file_for_test_decode_cache, "");
if (err != "")
return err;
// Test: Lookup non-existing pc.
if (decode_cache.get_decode_info(
reinterpret_cast<app_pc>(memrefs[0].instr.addr)) != nullptr) {
Expand All @@ -150,8 +154,7 @@ check_decode_caching(void *drcontext, bool persist_instrs, bool use_module_mappe
test_decode_info_t *cached_decode_info;

// Test: Lookup existing pc.
std::string err =
decode_cache.add_decode_info(memrefs[0].instr, cached_decode_info);
err = decode_cache.add_decode_info(memrefs[0].instr, cached_decode_info);
if (err != "")
return err;
test_decode_info_t *decode_info_nop =
Expand Down Expand Up @@ -198,7 +201,8 @@ check_decode_caching(void *drcontext, bool persist_instrs, bool use_module_mappe
test_decode_info_t *decode_info_ipt = decode_cache.get_decode_info(
reinterpret_cast<app_pc>(memrefs[3].instr.addr));
if (decode_info_ipt == nullptr || decode_info_ipt != cached_decode_info ||
!decode_info_ipt->is_valid() || !decode_info_ipt->is_ipt_) {
!decode_info_ipt->is_valid() || !decode_info_ipt->is_ipt_ ||
decode_info_ipt->is_ret_) {
return "Unexpected test_decode_info_t for ipt instr";
}
decode_info_ret = decode_cache.get_decode_info(
Expand Down Expand Up @@ -234,13 +238,17 @@ check_missing_module_mapper_and_no_encoding(void *drcontext)
instr_decode_info_t dummy;
// Initialize to non-nullptr for the test.
instr_decode_info_t *cached_decode_info = &dummy;

// Missing init before add_decode_info.
std::string err = decode_cache.add_decode_info(instr.instr, cached_decode_info);
if (err == "") {
return "Expected error at add_decode_info but did not get any";
}
if (cached_decode_info != nullptr) {
return "Expected returned reference cached_decode_info to be nullptr";
}

// init for a filetype without encodings, with no module file path either.
err = decode_cache.init(
static_cast<offline_file_type_t>(OFFLINE_FILE_TYPE_SYSCALL_NUMBERS), "", "");
if (err == "") {
Expand Down
7 changes: 4 additions & 3 deletions clients/drcachesim/tests/opcode_mix_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ class test_opcode_mix_t : public opcode_mix_t {
public:
// Pass a non-nullptr instrlist_t if the module mapper must be used.
test_opcode_mix_t(instrlist_t *instrs)
: opcode_mix_t(/*module_file_path=*/"some_file", /*verbose=*/0,
: opcode_mix_t(/*module_file_path=*/(instrs == nullptr ? "" : "some_file"),
/*verbose=*/0,
/*alt_module_dir=*/"")
, instrs_(instrs)
{
Expand Down Expand Up @@ -102,9 +103,9 @@ check_opcode_mix(void *drcontext, bool use_module_mapper)
ilist_for_test = ilist;
} else {
memrefs = add_encodings_to_memrefs(ilist, memref_setup, BASE_ADDR);
// Set up the second nop memref to reuse the same encoding as the first nop.
memrefs[3].instr.encoding_is_new = false;
}
// Set up the second nop memref to reuse the same encoding as the first nop.
memrefs[3].instr.encoding_is_new = false;
test_opcode_mix_t opcode_mix(ilist_for_test);
opcode_mix.initialize();
void *shard_data = opcode_mix.parallel_shard_init_stream(0, nullptr, nullptr);
Expand Down
36 changes: 19 additions & 17 deletions clients/drcachesim/tools/common/decode_cache.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,11 @@ class decode_info_base_t {
instr_t *instr);

/**
* Indicates whether the decode info stored in this object is valid. It won't be
* valid if the object is default-constructed without a subsequent
* set_decode_info() call. When used with #decode_cache_t, this indicates
* whether an invalid instruction was observed at the processed instr record.
* Indicates whether set_decode_info() was invoked on the object with a valid
* decoded #instr_t, typically by #decode_cache_t. It won't be valid if the
* object is default-constructed without a subsequent set_decode_info() call.
* When used with #decode_cache_t, this indicates whether an invalid
* instruction was observed at the processed instr record.
*/
bool
is_valid() const;
Expand Down Expand Up @@ -102,7 +103,7 @@ class decode_info_base_t {
};

/**
* Decode info including the full decoded instr_t. This should be used with a
* Decode info including the full decoded #instr_t. This should be used with a
* #decode_cache_t constructed with \p persist_decoded_instrs_ set to
* true.
*/
Expand Down Expand Up @@ -238,15 +239,15 @@ template <class DecodeInfo> class decode_cache_t : public decode_cache_base_t {
/**
* Returns a pointer to the DecodeInfo available for the instruction at \p pc.
* Returns nullptr if no instruction is known at that \p pc. Returns the
* default-constructed DecodeInfo if there was a decoding error for the
* instruction.
* default-constructed DecodeInfo if an instr was seen at that \p pc but there
* was a decoding error for the instruction.
*
* Guaranteed to be non-nullptr if the prior add_decode_info for that pc
* returned no error.
* Guaranteed to be non-nullptr and valid if the prior add_decode_info() for
* that \p pc returned no error.
*
* When analyzing memref_t from a trace, it may be better to use
* add_decode_info() instead if it's possible that the instr at \p pc
* may have changed (e.g., JIT code).
* When analyzing #memref_t from a trace, it may be better to just use
* add_decode_info() instead (as it also returns the added DecodeInfo) if
* it's possible that the instr at \p pc may have changed (e.g., JIT code).
*/
DecodeInfo *
get_decode_info(app_pc pc)
Expand All @@ -259,6 +260,7 @@ template <class DecodeInfo> class decode_cache_t : public decode_cache_base_t {
}
/**
* Adds decode info for the given \p memref_instr if it is not yet recorded.
* or if it contains a new encoding.
*
* Uses the embedded encodings in the trace or, if init() was invoked with
* a module file path, the encodings from the instantiated
Expand Down Expand Up @@ -288,9 +290,9 @@ template <class DecodeInfo> class decode_cache_t : public decode_cache_base_t {
auto it_inserted = decode_cache_.emplace(trace_pc, DecodeInfo());
typename std::unordered_map<app_pc, DecodeInfo>::iterator info =
it_inserted.first;
bool exists = !it_inserted.second;
if (exists &&
// We can return the cached DecodeInfo if:
bool already_exists = !it_inserted.second;
if (already_exists &&
// We can return the existing cached DecodeInfo if:
// - we're using the module mapper, where we don't support the
// change-prone JIT encodings; or
// - we're using embedded encodings from the trace, and the
Expand All @@ -302,7 +304,7 @@ template <class DecodeInfo> class decode_cache_t : public decode_cache_base_t {
// hasn't changed.
cached_decode_info = &info->second;
return "";
} else if (exists) {
} else if (already_exists) {
// We may end up here if we're using the embedded encodings from
// the trace and now we have a new instr at trace_pc.
info->second = DecodeInfo();
Expand Down Expand Up @@ -382,11 +384,11 @@ template <class DecodeInfo> class decode_cache_t : public decode_cache_base_t {
dr_set_isa_mode(dcontext_, DR_ISA_REGDEPS, nullptr);
}

init_done_ = true;
if (!TESTANY(OFFLINE_FILE_TYPE_ENCODINGS, filetype) && module_file_path.empty()) {
return "Trace does not have embedded encodings, and no module_file_path "
"provided";
}
init_done_ = true;
if (module_file_path.empty())
return "";
return init_module_mapper(module_file_path, alt_module_dir);
Expand Down

0 comments on commit b24d79a

Please sign in to comment.