From b24d79acb3b343474206608c278018d2d03f094f Mon Sep 17 00:00:00 2001 From: Abhinav Anil Sharma Date: Sun, 22 Dec 2024 13:29:14 -0500 Subject: [PATCH] Cleanup --- .../drcachesim/tests/decode_cache_test.cpp | 26 +++++++++----- clients/drcachesim/tests/opcode_mix_test.cpp | 7 ++-- .../drcachesim/tools/common/decode_cache.h | 36 ++++++++++--------- 3 files changed, 40 insertions(+), 29 deletions(-) diff --git a/clients/drcachesim/tests/decode_cache_test.cpp b/clients/drcachesim/tests/decode_cache_test.cpp index 5ccecaf87a2..d04cf6b4f42 100644 --- a/clients/drcachesim/tests/decode_cache_test.cpp +++ b/clients/drcachesim/tests/decode_cache_test.cpp @@ -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_VERSION_ENCODINGS); + static_cast(OFFLINE_FILE_TYPE_ENCODINGS); class test_decode_info_t : public decode_info_base_t { public: @@ -113,11 +113,13 @@ check_decode_caching(void *drcontext, bool persist_instrs, bool use_module_mappe test_decode_cache_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; } @@ -139,8 +141,10 @@ check_decode_caching(void *drcontext, bool persist_instrs, bool use_module_mappe test_decode_cache_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(memrefs[0].instr.addr)) != nullptr) { @@ -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 = @@ -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(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( @@ -234,6 +238,8 @@ 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"; @@ -241,6 +247,8 @@ check_missing_module_mapper_and_no_encoding(void *drcontext) 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_SYSCALL_NUMBERS), "", ""); if (err == "") { diff --git a/clients/drcachesim/tests/opcode_mix_test.cpp b/clients/drcachesim/tests/opcode_mix_test.cpp index da600ce1485..cafde96739c 100644 --- a/clients/drcachesim/tests/opcode_mix_test.cpp +++ b/clients/drcachesim/tests/opcode_mix_test.cpp @@ -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) { @@ -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); diff --git a/clients/drcachesim/tools/common/decode_cache.h b/clients/drcachesim/tools/common/decode_cache.h index 8b1b6742c42..3125d9f1e35 100644 --- a/clients/drcachesim/tools/common/decode_cache.h +++ b/clients/drcachesim/tools/common/decode_cache.h @@ -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; @@ -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. */ @@ -238,15 +239,15 @@ template 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) @@ -259,6 +260,7 @@ template 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 @@ -288,9 +290,9 @@ template class decode_cache_t : public decode_cache_base_t { auto it_inserted = decode_cache_.emplace(trace_pc, DecodeInfo()); typename std::unordered_map::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 @@ -302,7 +304,7 @@ template 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(); @@ -382,11 +384,11 @@ template 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);