Skip to content

Commit

Permalink
Reviewer suggested changes
Browse files Browse the repository at this point in the history
  • Loading branch information
@abhinav92003
abhinav92003 committed Dec 19, 2024
1 parent 2b301b5 commit 4157f23
Show file tree
Hide file tree
Showing 6 changed files with 99 additions and 95 deletions.
2 changes: 2 additions & 0 deletions clients/drcachesim/CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,8 @@ add_exported_library(drmemtrace_invariant_checker STATIC tools/invariant_checker
add_exported_library(drmemtrace_schedule_stats STATIC tools/schedule_stats.cpp)
add_exported_library(drmemtrace_decode_cache STATIC
tools/common/decode_cache.cpp
# XXX: Possibly create a library for raw2trace_shared, to avoid
# multiple build overhead.
tracer/raw2trace_shared.cpp)
add_exported_library(drmemtrace_schedule_file STATIC common/schedule_file.cpp)
add_exported_library(drmemtrace_mutex_dbg_owned STATIC common/mutex_dbg_owned.cpp)
Expand Down
16 changes: 8 additions & 8 deletions clients/drcachesim/tests/decode_cache_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,17 +48,17 @@ static constexpr offline_file_type_t ENCODING_FILE_TYPE =

class test_decode_info_t : public decode_info_base_t {
public:
bool is_nop = false;
bool is_ret = false;
bool is_nop_ = false;
bool is_ret_ = false;

private:
void
set_decode_info_derived(void *dcontext,
const dynamorio::drmemtrace::_memref_instr_t &memref_instr,
instr_t *instr) override
{
is_nop = instr_is_nop(instr);
is_ret = instr_is_return(instr);
is_nop_ = instr_is_nop(instr);
is_ret_ = instr_is_return(instr);
}
};

Expand Down Expand Up @@ -89,9 +89,9 @@ check_decode_caching(void *drcontext, bool persist_instrs, bool use_module_mappe
module_file_for_test_decode_cache = "some_mod_file";
} 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[2].instr.encoding_is_new = false;
}
// Set up the second nop memref to reuse the same encoding as the first nop.
memrefs[2].instr.encoding_is_new = false;

test_decode_info_t test_decode_info;
if (test_decode_info.is_valid()) {
Expand Down Expand Up @@ -141,7 +141,7 @@ check_decode_caching(void *drcontext, bool persist_instrs, bool use_module_mappe
test_decode_info_t *decode_info_nop =
decode_cache.get_decode_info(reinterpret_cast<app_pc>(memrefs[0].instr.addr));
if (decode_info_nop == nullptr || !decode_info_nop->is_valid() ||
!decode_info_nop->is_nop) {
!decode_info_nop->is_nop_) {
return "Unexpected test_decode_info_t for nop instr";
}
err = decode_cache.add_decode_info(memrefs[1].instr);
Expand All @@ -150,7 +150,7 @@ check_decode_caching(void *drcontext, bool persist_instrs, bool use_module_mappe
test_decode_info_t *decode_info_ret =
decode_cache.get_decode_info(reinterpret_cast<app_pc>(memrefs[1].instr.addr));
if (decode_info_ret == nullptr || !decode_info_ret->is_valid() ||
!decode_info_ret->is_ret) {
!decode_info_ret->is_ret_) {
return "Unexpected test_decode_info_t for ret instr";
}
err = decode_cache.add_decode_info(memrefs[2].instr);
Expand Down
102 changes: 59 additions & 43 deletions clients/drcachesim/tools/common/decode_cache.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,9 @@ class decode_info_base_t {
/**
* 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. Additionally,
* the set_decode_info_derived() provided by the derived class, which also takes
* ownership of the provided #instr_t if used with a #decode_cache_t object
* constructed with \p persist_decoded_instrs_ set to true. Additionally,
* this does other required bookkeeping.
*/
void
Expand All @@ -69,8 +71,8 @@ class decode_info_base_t {
/**
* 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 \p decode_cache_t, this indicates
* that an invalid instruction was observed at some pc.
* 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 All @@ -83,12 +85,12 @@ class decode_info_base_t {
* 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 \p decode_cache_t, which will invoke
* This is meant for use with #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 \p decode_info_base_t object, unless
* \p decode_cache_t was constructed with \p persist_decoded_instrs_
* lies with this #decode_info_base_t object, unless
* #decode_cache_t was constructed with \p persist_decoded_instrs_
* set to false, in which case no heap allocation takes place.
*/
virtual void
Expand All @@ -100,8 +102,8 @@ class decode_info_base_t {
};

/**
* Decode info including the full decoded instr_t. This should be used with an
* \p decode_cache_t constructed with \p persist_decoded_instrs_ set to
* 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.
*/
class instr_decode_info_t : public decode_info_base_t {
Expand All @@ -116,21 +118,23 @@ class instr_decode_info_t : public decode_info_base_t {
const dynamorio::drmemtrace::_memref_instr_t &memref_instr,
instr_t *instr) override;

// This is owned by the enclosing instr_decode_info_t object and will be
// instr_destroy()-ed in the destructor.
instr_t *instr_ = nullptr;
void *dcontext_ = nullptr;
};

/**
* Base class for \p decode_cache_t.
* Base class for #decode_cache_t.
*
* This is used to allow sharing the static data members among all template instances
* of \p decode_cache_t.
* of #decode_cache_t.
*/
class decode_cache_base_t {
protected:
/**
* Constructor for the base class, intentionally declared as protected so
* \p decode_cache_base_t cannot be instantiated directly but only via
* #decode_cache_base_t cannot be instantiated directly but only via
* a derived class.
*/
decode_cache_base_t() = default;
Expand All @@ -157,19 +161,23 @@ class decode_cache_base_t {
* In such a case, other concurrently running analysis tools should still
* be able to see encodings for JIT code.
*/
bool use_module_mapper_ = 0;
bool use_module_mapper_ = false;

/**
* Initializes the module_mapper_ object using make_module_mapper() and performs
* other bookkeeping and prerequisites.
*
* Returns the empty string on success, or an error message.
*/
std::string
init_module_mapper(const std::string &module_file_path,
const std::string &alt_module_dir);

/**
* Returns the address where the encoding for the instruction at trace_pc can
* be found.
* Returns in the \p decode_pc reference parameter the address where the encoding
* for the instruction at trace_pc can be found.
*
* Returns the empty string on success, or an error message.
*/
static std::string
find_mapped_trace_address(app_pc trace_pc, app_pc &decode_pc);
Expand All @@ -179,6 +187,8 @@ class decode_cache_base_t {
* Creates a module_mapper_t. This does not need to worry about races as the
* module_mapper_mutex_ will be acquired before calling.
* Non-static to allow test sub-classes to override.
*
* Returns the empty string on success, or an error message.
*/
virtual std::string
make_module_mapper(const std::string &module_file_path,
Expand All @@ -187,12 +197,12 @@ class decode_cache_base_t {

/**
* A cache to store decode info for instructions per observed app pc. The template arg
* DecodeInfo is a class derived from \p decode_info_base_t which implements the
* DecodeInfo is a class derived from #decode_info_base_t which implements the
* set_decode_info_derived() function that derives the required decode info from an
* \p instr_t object when invoked by \p decode_cache_t. This class handles the
* heavylifting of actually producing the decoded \p instr_t. The decoded \p instr_t
* #instr_t object when invoked by #decode_cache_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
* \p decode_cache_t object with \p persist_decoded_instrs_ set to true.
* #decode_cache_t object with \p persist_decoded_instrs_ set to true.
*
* Usage note: after constructing an object, init() must be called.
*/
Expand Down Expand Up @@ -231,7 +241,7 @@ template <class DecodeInfo> class decode_cache_t : public decode_cache_base_t {
*
* Uses the embedded encodings in the trace or, if init() was invoked with
* a module file path, the encodings from the instantiated
* \p module_mapper_t.
* #module_mapper_t.
*
* If there is a decoding failure, the default-constructed DecodeInfo that
* returns is_valid() = false will be added to the cache.
Expand Down Expand Up @@ -289,42 +299,47 @@ template <class DecodeInfo> class decode_cache_t : public decode_cache_base_t {
/**
* Performs initialization tasks such as verifying whether the given trace
* indeed has embedded encodings or not, and initializing the
* \p module_mapper_t if the module path is provided.
* #module_mapper_t if the module path is provided.
*
* It is important to note that the trace filetype may be obtained using the
* get_filetype() API on a \p memtrace_stream_t. However, not all instances of
* \p memtrace_stream_t have the filetype available at init time (before the
* \p TRACE_MARKER_TYPE_FILETYPE is actually returned by the stream).
* Particularly, the \p stream_t provided by the \p scheduler_t to analysis
* tools provides the file type at init time when #dynamorio::drmemtrace::
* scheduler_tmpl_t::scheduler_options_t.read_inputs_in_init is set. In other
* cases, the user may call this API after init time when the
* \p TRACE_MARKER_TYPE_FILETYPE marker is seen (still before any instr
* record).
*
* If the provided \p module_file_path is empty, the cache object uses
* the encodings embedded in the trace records.
* get_filetype() API on a #memtrace_stream_t. However, instances of
* #memtrace_stream_t have the filetype available at init time (before the
* #TRACE_MARKER_TYPE_FILETYPE is actually returned by the stream) only
* for offline analysis. In the online analysis case, the user would need to
* call this API after init time when the #TRACE_MARKER_TYPE_FILETYPE marker
* is seen (which is fine as it occurs before any instr record).
*
* If the \p module_file_path parameter is not empty, it instructs the
* \p decode_cache_t object that it should look for the instr encodings in the
* application binaries using a \p module_mapper_t. A \p module_mapper_t is
* #decode_cache_t object that it should look for the instr encodings in the
* application binaries using a #module_mapper_t. A #module_mapper_t is
* instantiated only one time and reused for all objects
* of \p decode_cache_t (of any template type). The user must provide a valid
* of #decode_cache_t (of any template type). The user must provide a valid
* \p module_file_path if decoding instructions from a trace that does not
* have embedded instruction encodings in it, as indicated by absence of
* the \p OFFLINE_FILE_TYPE_ENCODINGS bit in the \p TRACE_MARKER_TYPE_FILETYPE
* the #OFFLINE_FILE_TYPE_ENCODINGS bit in the #TRACE_MARKER_TYPE_FILETYPE
* marker. The user may provide a \p module_file_path also if they
* deliberately need to use the module mapper instead of the embedded
* encodings.
*
* If the provided \p module_file_path is empty, the cache object uses
* the encodings embedded in the trace records.
*/
std::string
init(offline_file_type_t filetype, const std::string &module_file_path = "",
const std::string &alt_module_dir = "")
{
// TODO i#7113: Should we perhaps also do dr_set_isa_mode for DR_ISA_REGDEPS
// that cannot be figured out automatically using the build but needs the
// trace filetype? Or does that belong in the analyzer framework which is more
// central than this decode_cache_t.
// If we are dealing with a regdeps trace, we need to set the dcontext
// ISA mode to the correct synthetic ISA (i.e., DR_ISA_REGDEPS).
if (TESTANY(OFFLINE_FILE_TYPE_ARCH_REGDEPS, filetype)) {
// Because isa_mode in dcontext is a global resource, we guard its access to
// avoid data races (even though this is a benign data race, as all threads
// are writing the same isa_mode value).
std::lock_guard<std::mutex> guard(dcontext_mutex_);
dr_isa_mode_t isa_mode = dr_get_isa_mode(dcontext_);
if (isa_mode != DR_ISA_REGDEPS)
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 "
Expand All @@ -338,6 +353,7 @@ template <class DecodeInfo> class decode_cache_t : public decode_cache_base_t {
private:
std::unordered_map<app_pc, DecodeInfo> decode_cache_;
void *dcontext_ = nullptr;
std::mutex dcontext_mutex_;
bool persist_decoded_instrs_ = false;

// Describes whether init() was invoked.
Expand All @@ -347,13 +363,11 @@ template <class DecodeInfo> class decode_cache_t : public decode_cache_base_t {
};

/**
* An \p decode_cache_t for testing which uses a \p test_module_mapper_t.
* A #decode_cache_t for testing which uses a #test_module_mapper_t.
*/
template <class DecodeInfo>
class test_decode_cache_t : public decode_cache_t<DecodeInfo> {
public:
using decode_cache_base_t::module_mapper_;

// The ilist arg is required only for testing the module_mapper_t
// decoding strategy.
test_decode_cache_t(void *dcontext, bool persist_decoded_instrs,
Expand All @@ -365,6 +379,8 @@ class test_decode_cache_t : public decode_cache_t<DecodeInfo> {
}

private:
using decode_cache_base_t::module_mapper_;

void *dcontext_;
instrlist_t *ilist_for_test_module_mapper_;

Expand Down
25 changes: 10 additions & 15 deletions clients/drcachesim/tools/invariant_checker.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,15 +159,6 @@ invariant_checker_t::parallel_shard_init_stream(int shard_index, void *worker_da
std::lock_guard<std::mutex> guard(init_mutex_);
per_shard->tid_ = shard_stream->get_tid();
shard_map_[shard_index] = std::move(per_shard);
// If we are dealing with an OFFLINE_FILE_TYPE_ARCH_REGDEPS trace, we need to set the
// dcontext ISA mode to the correct synthetic ISA (i.e., DR_ISA_REGDEPS).
uint64_t filetype = shard_stream->get_filetype();
if (TESTANY(OFFLINE_FILE_TYPE_ARCH_REGDEPS, filetype)) {
dr_isa_mode_t isa_mode = dr_get_isa_mode(drcontext_);
if (isa_mode != DR_ISA_REGDEPS) {
dr_set_isa_mode(drcontext_, DR_ISA_REGDEPS, nullptr);
}
}
return res;
}

Expand Down Expand Up @@ -789,15 +780,19 @@ invariant_checker_t::parallel_shard_memref(void *shard_data, const memref_t &mem
if (shard->error_ != "")
return false;
}
shard->error_ = shard->decode_cache_->add_decode_info(memref.instr);
if (shard->error_ != "") {
return false;
}
per_shard_t::decoding_info_t *decode_info =
shard->decode_cache_->get_decode_info(
reinterpret_cast<app_pc>(memref.instr.addr));
// The decode_info returned from get_decode_info will never be nullptr
// since we return if the prior add_decode_info returned an error.
if (decode_info == nullptr) {
shard->error_ = shard->decode_cache_->add_decode_info(memref.instr);
if (shard->error_ != "") {
return false;
}
// The decode_info returned here will never be nullptr since we
// return early if the prior add_decode_info returned an error.
decode_info = shard->decode_cache_->get_decode_info(
reinterpret_cast<app_pc>(memref.instr.addr));
}
cur_instr_info.decoding = *decode_info;

#ifdef X86
Expand Down
Loading

0 comments on commit 4157f23

Please sign in to comment.