Skip to content

Commit

Permalink
Also include decode_pc in set_decode_info calls
Browse files Browse the repository at this point in the history
  • Loading branch information
@abhinav92003
abhinav92003 committed Jan 6, 2025
1 parent 23dca98 commit 7ebe54f
Show file tree
Hide file tree
Showing 7 changed files with 151 additions and 66 deletions.
103 changes: 79 additions & 24 deletions clients/drcachesim/tests/decode_cache_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,23 +53,55 @@ class test_decode_info_t : public decode_info_base_t {
bool is_interrupt_ = false;
bool decode_info_set_ = false;

static bool expect_decoded_instr_;

private:
void
set_decode_info_derived(void *dcontext,
const dynamorio::drmemtrace::_memref_instr_t &memref_instr,
instr_t *instr) override
instr_t *instr, app_pc decode_pc) override
{
// decode_cache_t should call set_decode_info only one time per object.
assert(!decode_info_set_);
is_nop_ = instr_is_nop(instr);
is_ret_ = instr_is_return(instr);
is_interrupt_ = instr_is_interrupt(instr);
if (decode_info_set_) {
std::cerr << "decode_cache_t should call set_decode_info only one time per "
"object\n";
assert(false);
}
instr_t my_decoded_instr;
instr_init(dcontext, &my_decoded_instr);
app_pc next_pc = decode_from_copy(dcontext, decode_pc,
reinterpret_cast<app_pc>(memref_instr.addr),
&my_decoded_instr);
assert(next_pc != nullptr && instr_valid(&my_decoded_instr));
if (expect_decoded_instr_) {
if (instr == nullptr) {
std::cerr << "Expected to see a decoded instr_t\n";
assert(false);
}
if (!instr_same(instr, &my_decoded_instr)) {
std::cerr << "Expected provided decoded instr_t and self decoded instr_t "
"to be the same\n";
assert(false);
}
} else {
if (instr != nullptr) {
std::cerr << "Expected to see a null decoded instr\n";
assert(false);
}
}

is_nop_ = instr_is_nop(&my_decoded_instr);
is_ret_ = instr_is_return(&my_decoded_instr);
is_interrupt_ = instr_is_interrupt(&my_decoded_instr);
decode_info_set_ = true;

instr_free(dcontext, &my_decoded_instr);
}
};
bool test_decode_info_t::expect_decoded_instr_ = true;

std::string
check_decode_caching(void *drcontext, bool persist_instrs, bool use_module_mapper)
check_decode_caching(void *drcontext, bool use_module_mapper, bool include_decoded_instr,
bool persist_decoded_instr)
{
static constexpr addr_t BASE_ADDR = 0x123450;
instr_t *nop = XINST_CREATE_nop(drcontext);
Expand Down Expand Up @@ -105,13 +137,15 @@ check_decode_caching(void *drcontext, bool persist_instrs, bool use_module_mappe
return "Unexpected valid default-constructed decode info";
}

if (persist_instrs) {
if (persist_decoded_instr) {
// This test mode needs the decoded instr_t.
assert(include_decoded_instr);
// These are tests to verify the operation of instr_decode_info_t: that it stores
// the instr_t correctly.
// Tests for instr_decode_cache_t are done when persist_instrs = false (see
// Tests for instr_decode_cache_t are done when persist_decoded_instr = false (see
// the else part below).
test_decode_cache_t<instr_decode_info_t> decode_cache(
drcontext,
drcontext, include_decoded_instr,
/*persist_decoded_instr=*/true, ilist_for_test_decode_cache);
std::string err =
decode_cache.init(ENCODING_FILE_TYPE, module_file_for_test_decode_cache, "");
Expand All @@ -136,11 +170,12 @@ check_decode_caching(void *drcontext, bool persist_instrs, bool use_module_mappe
return "Unexpected instr_decode_info_t for ret instr";
}
} else {
test_decode_info_t::expect_decoded_instr_ = include_decoded_instr;
// These are tests to verify the operation of instr_decode_cache_t: that it caches
// decode info correctly.
test_decode_cache_t<test_decode_info_t> decode_cache(
drcontext,
/*persist_decoded_instrs=*/false, ilist_for_test_decode_cache);
drcontext, include_decoded_instr,
/*persist_decoded_instr=*/false, ilist_for_test_decode_cache);
std::string err =
decode_cache.init(ENCODING_FILE_TYPE, module_file_for_test_decode_cache, "");
if (err != "")
Expand Down Expand Up @@ -225,8 +260,9 @@ check_decode_caching(void *drcontext, bool persist_instrs, bool use_module_mappe
}
}
instrlist_clear_and_destroy(drcontext, ilist);
std::cerr << "check_decode_caching with persist_instrs: " << persist_instrs
<< ", use_module_mapper: " << use_module_mapper << " passed\n";
std::cerr << "check_decode_caching with use_module_mapper: " << use_module_mapper
<< ", include_decoded_instr: " << include_decoded_instr
<< ", persist_decoded_instr: " << persist_decoded_instr << " passed\n";
return "";
}

Expand All @@ -235,8 +271,9 @@ check_missing_module_mapper_and_no_encoding(void *drcontext)
{
memref_t instr = gen_instr(TID_A);
test_decode_cache_t<instr_decode_info_t> decode_cache(
drcontext,
/*persist_decoded_instr=*/true, /*ilist_for_test_module_mapper=*/nullptr);
drcontext, /*include_decoded_instr=*/true,
/*persist_decoded_instr=*/true,
/*ilist_for_test_module_mapper=*/nullptr);
instr_decode_info_t dummy;
// Initialize to non-nullptr for the test.
instr_decode_info_t *cached_decode_info = &dummy;
Expand Down Expand Up @@ -264,29 +301,47 @@ int
test_main(int argc, const char *argv[])
{
void *drcontext = dr_standalone_init();
std::string err = check_decode_caching(drcontext, /*persist_instrs=*/false,
/*use_module_mapper=*/false);
std::string err = check_decode_caching(drcontext, /*use_module_mapper=*/false,
/*include_decoded_instr=*/false,
/*persist_decoded_instr=*/false);
if (err != "") {
std::cerr << err << "\n";
exit(1);
}
err = check_decode_caching(drcontext, /*persist_instrs=*/true,
/*use_module_mapper=*/false);
err = check_decode_caching(drcontext, /*use_module_mapper=*/false,
/*include_decoded_instr=*/true,
/*persist_decoded_instr=*/false);
if (err != "") {
std::cerr << err << "\n";
exit(1);
}
err = check_decode_caching(drcontext, /*use_module_mapper=*/false,
/*include_decoded_instr=*/true,
/*persist_decoded_instr=*/true);
if (err != "") {
std::cerr << err << "\n";
exit(1);
}
#ifndef WINDOWS
// TODO i#5960: Enable these tests after the test-only Windows issue is
// fixed.
err = check_decode_caching(drcontext, /*persist_instrs=*/false,
/*use_module_mapper=*/true);
err = check_decode_caching(drcontext, /*use_module_mapper=*/true,
/*include_decoded_instr=*/false,
/*persist_decoded_instr=*/false);
if (err != "") {
std::cerr << err << "\n";
exit(1);
}
err = check_decode_caching(drcontext, /*use_module_mapper=*/true,
/*include_decoded_instr=*/true,
/*persist_decoded_instr=*/false);
if (err != "") {
std::cerr << err << "\n";
exit(1);
}
err = check_decode_caching(drcontext, /*persist_instrs=*/true,
/*use_module_mapper=*/true);
err = check_decode_caching(drcontext, /*use_module_mapper=*/true,
/*include_decoded_instr=*/true,
/*persist_decoded_instr=*/true);
if (err != "") {
std::cerr << err << "\n";
exit(1);
Expand Down
6 changes: 3 additions & 3 deletions clients/drcachesim/tools/common/decode_cache.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@ namespace drmemtrace {
void
decode_info_base_t::set_decode_info(
void *dcontext, const dynamorio::drmemtrace::_memref_instr_t &memref_instr,
instr_t *instr)
instr_t *instr, app_pc decode_pc)
{
set_decode_info_derived(dcontext, memref_instr, instr);
set_decode_info_derived(dcontext, memref_instr, instr, decode_pc);
is_valid_ = true;
}

Expand Down Expand Up @@ -137,7 +137,7 @@ decode_cache_base_t::find_mapped_trace_address(app_pc trace_pc, app_pc &decode_p
void
instr_decode_info_t::set_decode_info_derived(
void *dcontext, const dynamorio::drmemtrace::_memref_instr_t &memref_instr,
instr_t *instr)
instr_t *instr, app_pc decode_pc)
{
dcontext_ = dcontext;
instr_ = instr;
Expand Down
98 changes: 63 additions & 35 deletions clients/drcachesim/tools/common/decode_cache.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,17 +57,25 @@ class decode_info_base_t {
public:
/**
* Sets the decode info for the provided \p instr which was allocated using the
* provided \p dcontext for the provided \p memref_instr. This is done using
* the set_decode_info_derived() provided by the derived class, which also takes
* ownership of the provided #instr_t if used with a
* #dynamorio::drmemtrace::decode_cache_t object constructed with
* \p persist_decoded_instrs_ set to true. Additionally, this does other required
* bookkeeping.
* provided \p dcontext for the provided \p memref_instr, decoded from raw bytes
* at the provided address in \p decode_pc which are valid only for this call.
*
* This invokes the set_decode_info_derived() provided by the derived class, and
* additionally does other required bookkeeping.
*
* set_decode_info_derived() is expected to take ownership of the provided \p instr
* if used with a #dynamorio::drmemtrace::decode_cache_t object constructed with
* \p persist_decoded_instr_ set to true.
*
* The provided \p instr will be nullptr if the
* #dynamorio::drmemtrace::decode_cache_t was constructed with
* \p include_decoded_instr_ set to false, which is useful when the user needs only
* the \p decode_pc to perform the actual decoding themselves.
*/
void
set_decode_info(void *dcontext,
const dynamorio::drmemtrace::_memref_instr_t &memref_instr,
instr_t *instr);
instr_t *instr, app_pc decode_pc);

/**
* Indicates whether set_decode_info() was invoked on the object with a valid
Expand All @@ -84,31 +92,37 @@ class decode_info_base_t {
/**
* Sets the decoding info fields as required by the derived class, based on the
* provided #instr_t which was allocated using the provided opaque \p dcontext
* for the provided \p memref_instr. Derived classes must implement this virtual
* function. Note that this cannot be invoked directly as it is private, but
* only through set_decode_info() which does other required bookkeeping.
* for the provided \p memref_instr, decoded from raw bytes at the provided
* address in \p decode_pc which are valid only for this call. Derived classes
* must implement this virtual function. Note that this cannot be invoked
* directly as it is private, but only through set_decode_info() which does
* other required bookkeeping.
*
* This is meant for use with #dynamorio::drmemtrace::decode_cache_t, which will
* invoke set_decode_info() for each new decoded instruction.
*
* The responsibility for invoking instr_destroy() on the provided \p instr
* lies with this #decode_info_base_t object, unless
* lies with this #decode_info_base_t object, unless the
* #dynamorio::drmemtrace::decode_cache_t was constructed with
* \p persist_decoded_instrs_ set to false, in which case no heap allocation
* \p persist_decoded_instr_ set to false, in which case no heap allocation
* takes place.
*
* The provided \p instr will be nullptr if the
* #dynamorio::drmemtrace::decode_cache_t was constructed with
* \p include_decoded_instr_ set to false.
*/
virtual void
set_decode_info_derived(void *dcontext,
const dynamorio::drmemtrace::_memref_instr_t &memref_instr,
instr_t *instr) = 0;
instr_t *instr, app_pc decode_pc) = 0;

bool is_valid_ = false;
};

/**
* Decode info including the full decoded #instr_t. This should be used with a
* #dynamorio::drmemtrace::decode_cache_t constructed with
* \p persist_decoded_instrs_ set to true.
* \p persist_decoded_instr_ set to true.
*/
class instr_decode_info_t : public decode_info_base_t {
public:
Expand All @@ -120,7 +134,7 @@ class instr_decode_info_t : public decode_info_base_t {
void
set_decode_info_derived(void *dcontext,
const dynamorio::drmemtrace::_memref_instr_t &memref_instr,
instr_t *instr) override;
instr_t *instr, app_pc decode_pc) override;

// This is owned by the enclosing instr_decode_info_t object and will be
// instr_destroy()-ed in the destructor.
Expand Down Expand Up @@ -224,7 +238,7 @@ class decode_cache_base_t {
* This class handles the heavy lifting of actually producing the decoded #instr_t. The
* decoded #instr_t may be made to persist beyond the set_decode_info() calls by
* constructing the #dynamorio::drmemtrace::decode_cache_t object with
* \p persist_decoded_instrs_ set to true.
* \p persist_decoded_instr_ set to true.
*
* Usage note: after constructing an object, init() must be called.
*/
Expand All @@ -233,9 +247,14 @@ template <class DecodeInfo> class decode_cache_t : public decode_cache_base_t {
"DecodeInfo not derived from decode_info_base_t");

public:
decode_cache_t(void *dcontext, bool persist_decoded_instrs)
decode_cache_t(void *dcontext, bool include_decoded_instr, bool persist_decoded_instr)
: dcontext_(dcontext)
, persist_decoded_instrs_(persist_decoded_instrs) {};
, include_decoded_instr_(include_decoded_instr)
, persist_decoded_instr_(persist_decoded_instr)
{
// Cannot persist the decoded instr if it is not requested.
assert(!persist_decoded_instr_ || include_decoded_instr_);
};
virtual ~decode_cache_t()
{
}
Expand Down Expand Up @@ -322,15 +341,8 @@ template <class DecodeInfo> class decode_cache_t : public decode_cache_base_t {
info->second = DecodeInfo();
}
cached_decode_info = &info->second;
instr_t *instr = nullptr;
instr_noalloc_t noalloc;
if (persist_decoded_instrs_) {
instr = instr_create(dcontext_);
} else {
instr_noalloc_init(dcontext_, &noalloc);
instr = instr_from_noalloc(&noalloc);
}

// Get address for the instr encoding.
app_pc decode_pc;
if (!use_module_mapper_) {
decode_pc = const_cast<app_pc>(memref_instr.encoding);
Expand All @@ -340,15 +352,28 @@ template <class DecodeInfo> class decode_cache_t : public decode_cache_base_t {
if (err != "")
return err;
}
app_pc next_pc = decode_from_copy(dcontext_, decode_pc, trace_pc, instr);
if (next_pc == nullptr || !instr_valid(instr)) {
if (persist_decoded_instrs_) {
instr_destroy(dcontext_, instr);

// Optionally decode the instruction.
instr_t *instr = nullptr;
instr_noalloc_t noalloc;
if (include_decoded_instr_) {
if (persist_decoded_instr_) {
instr = instr_create(dcontext_);
} else {
instr_noalloc_init(dcontext_, &noalloc);
instr = instr_from_noalloc(&noalloc);
}

app_pc next_pc = decode_from_copy(dcontext_, decode_pc, trace_pc, instr);
if (next_pc == nullptr || !instr_valid(instr)) {
if (persist_decoded_instr_) {
instr_destroy(dcontext_, instr);
}
return "decode_from_copy failed";
}
return "decode_from_copy failed";
}
// Also sets is_valid_.
info->second.set_decode_info(dcontext_, memref_instr, instr);
info->second.set_decode_info(dcontext_, memref_instr, instr, decode_pc);
return "";
}

Expand Down Expand Up @@ -432,7 +457,8 @@ template <class DecodeInfo> class decode_cache_t : public decode_cache_base_t {
std::unordered_map<app_pc, DecodeInfo> decode_cache_;
void *dcontext_ = nullptr;
std::mutex dcontext_mutex_;
bool persist_decoded_instrs_ = false;
bool include_decoded_instr_ = false;
bool persist_decoded_instr_ = false;

// Describes whether init() was invoked.
// This helps in detecting cases where a module mapper was not specified
Expand All @@ -449,9 +475,11 @@ class test_decode_cache_t : public decode_cache_t<DecodeInfo> {
public:
// The ilist arg is required only for testing the module_mapper_t
// decoding strategy.
test_decode_cache_t(void *dcontext, bool persist_decoded_instrs,
test_decode_cache_t(void *dcontext, bool include_decoded_instr,
bool persist_decoded_instr,
instrlist_t *ilist_for_test_module_mapper = nullptr)
: decode_cache_t<DecodeInfo>(dcontext, persist_decoded_instrs)
: decode_cache_t<DecodeInfo>(dcontext, include_decoded_instr,
persist_decoded_instr)
, dcontext_(dcontext)
, ilist_for_test_module_mapper_(ilist_for_test_module_mapper)
{
Expand Down
Loading

0 comments on commit 7ebe54f

Please sign in to comment.