diff --git a/clients/drcachesim/tests/decode_cache_test.cpp b/clients/drcachesim/tests/decode_cache_test.cpp index 7d02ca6b6c2..da81a4fa5ec 100644 --- a/clients/drcachesim/tests/decode_cache_test.cpp +++ b/clients/drcachesim/tests/decode_cache_test.cpp @@ -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(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); @@ -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 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, ""); @@ -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 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 != "") @@ -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 ""; } @@ -235,8 +271,9 @@ check_missing_module_mapper_and_no_encoding(void *drcontext) { memref_t instr = gen_instr(TID_A); test_decode_cache_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; @@ -264,14 +301,23 @@ 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); @@ -279,14 +325,23 @@ test_main(int argc, const char *argv[]) #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); diff --git a/clients/drcachesim/tools/common/decode_cache.cpp b/clients/drcachesim/tools/common/decode_cache.cpp index 0f297af7f53..a312f7e9d77 100644 --- a/clients/drcachesim/tools/common/decode_cache.cpp +++ b/clients/drcachesim/tools/common/decode_cache.cpp @@ -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; } @@ -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; diff --git a/clients/drcachesim/tools/common/decode_cache.h b/clients/drcachesim/tools/common/decode_cache.h index a637dfac9fd..1072b527492 100644 --- a/clients/drcachesim/tools/common/decode_cache.h +++ b/clients/drcachesim/tools/common/decode_cache.h @@ -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 @@ -84,23 +92,29 @@ 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; }; @@ -108,7 +122,7 @@ class decode_info_base_t { /** * 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: @@ -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. @@ -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. */ @@ -233,9 +247,14 @@ template 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() { } @@ -322,15 +341,8 @@ template 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(memref_instr.encoding); @@ -340,15 +352,28 @@ template 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 ""; } @@ -432,7 +457,8 @@ template class decode_cache_t : public decode_cache_base_t { std::unordered_map 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 @@ -449,9 +475,11 @@ class test_decode_cache_t : public decode_cache_t { 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(dcontext, persist_decoded_instrs) + : decode_cache_t(dcontext, include_decoded_instr, + persist_decoded_instr) , dcontext_(dcontext) , ilist_for_test_module_mapper_(ilist_for_test_module_mapper) { diff --git a/clients/drcachesim/tools/invariant_checker.cpp b/clients/drcachesim/tools/invariant_checker.cpp index e24ab905e8e..e31fcc10e99 100644 --- a/clients/drcachesim/tools/invariant_checker.cpp +++ b/clients/drcachesim/tools/invariant_checker.cpp @@ -1670,7 +1670,7 @@ invariant_checker_t::check_regdeps_invariants(per_shard_t *shard, const memref_t void invariant_checker_t::per_shard_t::decoding_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) { is_prefetch_ = instr_is_prefetch(instr); opcode_ = instr_get_opcode(instr); @@ -1711,6 +1711,7 @@ invariant_checker_t::make_and_init_decode_cache(per_shard_t *shard) shard->decode_cache_ = std::unique_ptr>( new decode_cache_t( drcontext_, + /*include_decoded_instr=*/true, /*persist_decoded_instrs=*/false)); shard->error_ = shard->decode_cache_->init(shard->file_type_); if (shard->error_ != "") diff --git a/clients/drcachesim/tools/invariant_checker.h b/clients/drcachesim/tools/invariant_checker.h index 1b217d55e29..19305e3bb6c 100644 --- a/clients/drcachesim/tools/invariant_checker.h +++ b/clients/drcachesim/tools/invariant_checker.h @@ -160,7 +160,7 @@ class invariant_checker_t : public analysis_tool_t { 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; }; struct instr_info_t { memref_t memref = {}; diff --git a/clients/drcachesim/tools/opcode_mix.cpp b/clients/drcachesim/tools/opcode_mix.cpp index 07b84529cc8..96bd508e480 100644 --- a/clients/drcachesim/tools/opcode_mix.cpp +++ b/clients/drcachesim/tools/opcode_mix.cpp @@ -86,6 +86,7 @@ opcode_mix_t::make_decode_cache(shard_data_t *shard, void *dcontext) { shard->decode_cache = std::unique_ptr>( new decode_cache_t(dcontext, + /*include_decoded_instr=*/true, /*persist_decoded_instrs=*/false)); } @@ -430,7 +431,7 @@ opcode_mix_t::release_interval_snapshot(interval_state_snapshot_t *interval_snap void opcode_mix_t::opcode_data_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) { opcode_ = instr_get_opcode(instr); category_ = instr_get_category(instr); diff --git a/clients/drcachesim/tools/opcode_mix.h b/clients/drcachesim/tools/opcode_mix.h index e5bef81f6df..1467ad2fa88 100644 --- a/clients/drcachesim/tools/opcode_mix.h +++ b/clients/drcachesim/tools/opcode_mix.h @@ -129,7 +129,7 @@ class opcode_mix_t : public analysis_tool_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; }; class snapshot_t : public interval_state_snapshot_t {