From 4de50c02553ffa19225e2f5c0d36beb6c605a44a Mon Sep 17 00:00:00 2001 From: Derek Bruening Date: Mon, 22 Aug 2022 18:15:34 -0400 Subject: [PATCH 1/8] i#5520: Add instruction encodings to offline drmemtraces Adds a new trace_entry_t type TRACE_TYPE_ENCODING. Encodings are stored in one or more consecutive such entries by raw2trace for the first occurrence of each instruction or modified instruction, with encodings repeated in new chunks. The trace version is bumped for this new type. Adds a new encoding field to the end of memref_instr_t. The reader_t class caches the trace_entry_t encodings and uses them to fill in this field. A new file type OFFLINE_FILE_TYPE_ENCODINGS indicates whether encodings are present (to support stripping them out). Augments the opcode_mix and view tools to use the new encoding records and only need the module_mapper for legacy traces. Updates the documentation and unit tests. Still remaining is adding markers when code changes, joint with #2062. Fixes #5520 --- api/docs/debug_memtrace.dox | 3 +- api/docs/release.dox | 2 +- clients/drcachesim/common/memref.h | 9 ++- clients/drcachesim/common/trace_entry.h | 26 +++++++- clients/drcachesim/drcachesim.dox.in | 8 ++- clients/drcachesim/reader/reader.cpp | 34 ++++++++++ clients/drcachesim/reader/reader.h | 10 +++ .../drcachesim/tests/raw2trace_unit_tests.cpp | 7 ++ clients/drcachesim/tools/opcode_mix.cpp | 64 ++++++++++++------- clients/drcachesim/tools/opcode_mix.h | 9 ++- clients/drcachesim/tools/view.cpp | 36 +++++++---- clients/drcachesim/tools/view.h | 5 ++ clients/drcachesim/tracer/raw2trace.cpp | 17 ++++- clients/drcachesim/tracer/raw2trace.h | 37 +++++++++++ 14 files changed, 221 insertions(+), 46 deletions(-) diff --git a/api/docs/debug_memtrace.dox b/api/docs/debug_memtrace.dox index 5d3026d56bb..f841d9e188d 100644 --- a/api/docs/debug_memtrace.dox +++ b/api/docs/debug_memtrace.dox @@ -103,8 +103,9 @@ Type cheat sheet (from trace_type_t enum): - 0x0e direct call - 0x00 load - 0x01 store -- 0x1d: non-fetched instr +- 0x1d non-fetched instr - 0x1a footer +- 0x2f instr encoding type_is_instr: 0xa-0x10 + 0x1e diff --git a/api/docs/release.dox b/api/docs/release.dox index 4ad4a01d3ad..fb4e253eabc 100644 --- a/api/docs/release.dox +++ b/api/docs/release.dox @@ -171,7 +171,7 @@ Further non-compatibility-affecting changes include: the .zip, which sets the granularity of a fast seek. - Added dr_register_post_attach_event(), dr_unregister_post_attach_event(), dr_register_pre_detach_event(), and dr_unregister_pre_detach_event(). - + - Added insruction encodings to drmemtrace offline traces. The changes between version 9.0.1 and 9.0.0 include the following compatibility changes: diff --git a/clients/drcachesim/common/memref.h b/clients/drcachesim/common/memref.h index 5350312b9b2..0a96b6fc824 100644 --- a/clients/drcachesim/common/memref.h +++ b/clients/drcachesim/common/memref.h @@ -1,5 +1,5 @@ /* ********************************************************** - * Copyright (c) 2015-2021 Google, Inc. All rights reserved. + * Copyright (c) 2015-2022 Google, Inc. All rights reserved. * **********************************************************/ /* @@ -66,6 +66,13 @@ struct _memref_instr_t { memref_tid_t tid; /**< Thread id. */ addr_t addr; /**< The address of the instruction (i.e., program counter). */ size_t size; /**< The length of the instruction. */ + /** + * The instruction's raw encoding. This field is only valid when the the file type + * (see #TRACE_MARKER_TYPE_FILETYPE) has #OFFLINE_FILE_TYPE_ENCODINGS set. + * DynamoRIO's decode_from_copy() (or any other decoding library) can be used to + * decode into a higher-level instruction representation. + */ + unsigned char encoding[MAX_ENCODING_LENGTH]; }; /** A trace entry representing a software-requested explicit cache flush. */ diff --git a/clients/drcachesim/common/trace_entry.h b/clients/drcachesim/common/trace_entry.h index 34e944e6995..b3122531544 100644 --- a/clients/drcachesim/common/trace_entry.h +++ b/clients/drcachesim/common/trace_entry.h @@ -65,8 +65,18 @@ typedef enum { * PC of the interruption point provided today. */ TRACE_ENTRY_VERSION_NO_KERNEL_PC = 2, + /** + * #TRACE_MARKER_TYPE_KERNEL_EVENT records provide the absolute + * PC of the interruption point. + */ + TRACE_ENTRY_VERSION_KERNEL_PC = 3, + /** + * The trace supports embedded instruction encodings, but they are only present + * if #OFFLINE_FILE_TYPE_ENCODINGS is set. + */ + TRACE_ENTRY_VERSION_ENCODINGS = 4, /** The latest version of the trace format. */ - TRACE_ENTRY_VERSION, + TRACE_ENTRY_VERSION = TRACE_ENTRY_VERSION_ENCODINGS, } trace_version_t; /** The type of a trace entry in a #memref_t structure. */ @@ -206,6 +216,12 @@ typedef enum { TRACE_TYPE_PREFETCH_WRITE_L3, /**< Store prefetch to L3 cache. */ TRACE_TYPE_PREFETCH_WRITE_L3_NT, /**< Non-temporal store prefetch to L3 cache. */ + // Internal value for encoding bytes. + // Currently this is only used for offline traces with OFFLINE_FILE_TYPE_ENCODINGS. + // XXX i#5520: Add to online traces, but under an option since extra + // encoding entries add runtime overhead. + TRACE_TYPE_ENCODING, + // Update trace_type_names[] when adding here. } trace_type_t; @@ -446,6 +462,8 @@ marker_type_is_function_marker(const trace_marker_type_t mark) return mark >= TRACE_MARKER_TYPE_FUNC_ID && mark <= TRACE_MARKER_TYPE_FUNC_RETVAL; } +#define MAX_ENCODING_LENGTH 17 + // This is the data format generated by the online tracer and produced after // post-processing of raw offline traces. // The reader_t class transforms this into memref_t before handing to analysis tools. @@ -460,12 +478,15 @@ START_PACKED_STRUCTURE struct _trace_entry_t { unsigned short type; // 2 bytes: trace_type_t // 2 bytes: mem ref size, instr length, or num of instrs for instr bundle, - // or marker sub-type. + // or marker sub-type, or num of bytes in encoding[] array. unsigned short size; union { addr_t addr; // 4/8 bytes: mem ref addr, instr pc, tid, pid, marker val // The length of each instr in the instr bundle unsigned char length[sizeof(addr_t)]; + // The raw encoding bytes for the subsequent instruction fetch entry. + // There may be multiple consecutive records to hold long instructions. + unsigned char encoding[sizeof(addr_t)]; }; } END_PACKED_STRUCTURE; typedef struct _trace_entry_t trace_entry_t; @@ -565,6 +586,7 @@ typedef enum { OFFLINE_FILE_TYPE_ARCH_X86_64, /**< All possible architecture types. */ OFFLINE_FILE_TYPE_IFILTERED = 0x80, /**< Instruction addresses filtered online. */ OFFLINE_FILE_TYPE_DFILTERED = 0x100, /**< Data addresses filtered online. */ + OFFLINE_FILE_TYPE_ENCODINGS = 0x200, /**< Instruction encodings are included. */ } offline_file_type_t; static inline const char * diff --git a/clients/drcachesim/drcachesim.dox.in b/clients/drcachesim/drcachesim.dox.in index d0e617494bc..362fb990947 100644 --- a/clients/drcachesim/drcachesim.dox.in +++ b/clients/drcachesim/drcachesim.dox.in @@ -111,8 +111,12 @@ Executed instructions are stored in #_memref_instr_t. The program counter and length of the encoded instruction are provided. The length can be used to compute the address of the subsequent instruction. -The instruction records do not contain instruction encodings. Such -information can be obtained by disassembling the application and +The raw encoding of the instruction is provided. This can be decoded +using the drdecode decoder or any other decoder. + +Older legacy traces may not contain instruction encodings. For those +traces, encodings for static code can be obtained by +disassembling the application and library binaries. The provided interfaces module_mapper_t::get_loaded_modules() and module_mapper_t::find_mapped_trace_address() facilitate loading in diff --git a/clients/drcachesim/reader/reader.cpp b/clients/drcachesim/reader/reader.cpp index 06d73fe8ac9..9f09cf94c3e 100644 --- a/clients/drcachesim/reader/reader.cpp +++ b/clients/drcachesim/reader/reader.cpp @@ -31,6 +31,7 @@ */ #include +#include #include "reader.h" #include "../common/memref.h" #include "../common/utils.h" @@ -104,6 +105,18 @@ reader_t::operator++() // use to obtain the PC for subsequent data references. cur_ref_.data.pc = cur_pc_; break; + case TRACE_TYPE_ENCODING: + if (last_encoding_.size + input_entry_->size > MAX_ENCODING_LENGTH) { + ERRMSG("Invalid too-large encoding size %zu + %d\n", last_encoding_.size, + input_entry_->size); + assert(false); + at_eof_ = true; + break; + } + memcpy(last_encoding_.bits + last_encoding_.size, input_entry_->encoding, + input_entry_->size); + last_encoding_.size += input_entry_->size; + break; case TRACE_TYPE_INSTR_MAYBE_FETCH: // While offline traces can convert rep string per-iter instrs into // no-fetch entries, online can't w/o extra work, so we do the work @@ -140,6 +153,24 @@ reader_t::operator++() prev_instr_addr_ = input_entry_->addr; if (cur_ref_.instr.type != TRACE_TYPE_INSTR_NO_FETCH) ++cur_instr_count_; + // Look for encoding bits. + const auto &it = encodings_.find(cur_ref_.instr.addr); + if (it != encodings_.end()) { + memcpy(cur_ref_.instr.encoding, it->second.bits, it->second.size); + } else if (last_encoding_.size > 0) { + if (last_encoding_.size != cur_ref_.instr.size) { + ERRMSG("Encoding size %zu != instr size %zu\n", + last_encoding_.size, cur_ref_.instr.size); + assert(false); + } + memcpy(cur_ref_.instr.encoding, last_encoding_.bits, + last_encoding_.size); + encodings_[cur_ref_.instr.addr] = last_encoding_; + } else if (!expect_no_encodings_) { + ERRMSG("Missing encoding for 0x%zx\n", cur_ref_.instr.addr); + assert(false); + } + last_encoding_.size = 0; } break; case TRACE_TYPE_INSTR_BUNDLE: @@ -242,6 +273,9 @@ reader_t::operator++() last_timestamp_instr_count_ = cur_instr_count_; else if (cur_ref_.marker.marker_type == TRACE_MARKER_TYPE_CHUNK_INSTR_COUNT) chunk_instr_count_ = cur_ref_.marker.marker_value; + else if (cur_ref_.marker.marker_type == TRACE_MARKER_TYPE_FILETYPE && + TESTANY(OFFLINE_FILE_TYPE_ENCODINGS, cur_ref_.marker.marker_value)) + expect_no_encodings_ = false; break; default: ERRMSG("Unknown trace entry type %d\n", input_entry_->type); diff --git a/clients/drcachesim/reader/reader.h b/clients/drcachesim/reader/reader.h index d7ec06715ba..4696ad9ba27 100644 --- a/clients/drcachesim/reader/reader.h +++ b/clients/drcachesim/reader/reader.h @@ -62,11 +62,13 @@ class reader_t : public std::iterator { public: reader_t() { + cur_ref_ = {}; } reader_t(int verbosity, const char *prefix) : verbosity_(verbosity) , output_prefix_(prefix) { + cur_ref_ = {}; } virtual ~reader_t() { @@ -134,6 +136,11 @@ class reader_t : public std::iterator { const char *output_prefix_ = "[reader]"; private: + struct encoding_info_t { + size_t size = 0; + unsigned char bits[MAX_ENCODING_LENGTH]; + }; + trace_entry_t *input_entry_ = nullptr; memref_t cur_ref_; memref_tid_t cur_tid_ = 0; @@ -147,6 +154,9 @@ class reader_t : public std::iterator { uint64_t chunk_instr_count_ = 0; // Unchanging once set to non-zero. uint64_t last_timestamp_instr_count_ = 0; bool skip_next_cpu_ = false; + bool expect_no_encodings_ = true; + encoding_info_t last_encoding_; + std::unordered_map encodings_; }; #endif /* _READER_H_ */ diff --git a/clients/drcachesim/tests/raw2trace_unit_tests.cpp b/clients/drcachesim/tests/raw2trace_unit_tests.cpp index 9a97b856392..b1956eca026 100644 --- a/clients/drcachesim/tests/raw2trace_unit_tests.cpp +++ b/clients/drcachesim/tests/raw2trace_unit_tests.cpp @@ -307,8 +307,11 @@ test_branch_delays(void *drcontext) check_entry(entries, idx, TRACE_TYPE_MARKER, TRACE_MARKER_TYPE_TIMESTAMP) && check_entry(entries, idx, TRACE_TYPE_MARKER, TRACE_MARKER_TYPE_CPU_ID) && // Both branches should be delayed until after the timestamp+cpu markers: + check_entry(entries, idx, TRACE_TYPE_ENCODING, -1) && check_entry(entries, idx, TRACE_TYPE_INSTR_CONDITIONAL_JUMP, -1) && + check_entry(entries, idx, TRACE_TYPE_ENCODING, -1) && check_entry(entries, idx, TRACE_TYPE_INSTR_DIRECT_JUMP, -1) && + check_entry(entries, idx, TRACE_TYPE_ENCODING, -1) && check_entry(entries, idx, TRACE_TYPE_INSTR, -1) && check_entry(entries, idx, TRACE_TYPE_THREAD_EXIT, -1) && check_entry(entries, idx, TRACE_TYPE_FOOTER, -1)); @@ -413,13 +416,17 @@ test_marker_placement(void *drcontext) check_entry(entries, idx, TRACE_TYPE_MARKER, TRACE_MARKER_TYPE_CACHE_LINE_SIZE) && check_entry(entries, idx, TRACE_TYPE_MARKER, TRACE_MARKER_TYPE_CHUNK_INSTR_COUNT) && + check_entry(entries, idx, TRACE_TYPE_ENCODING, -1) && check_entry(entries, idx, TRACE_TYPE_INSTR, -1) && + check_entry(entries, idx, TRACE_TYPE_ENCODING, -1) && check_entry(entries, idx, TRACE_TYPE_INSTR, -1) && check_entry(entries, idx, TRACE_TYPE_MARKER, TRACE_MARKER_TYPE_FUNC_ID) && check_entry(entries, idx, TRACE_TYPE_MARKER, TRACE_MARKER_TYPE_FUNC_RETADDR) && check_entry(entries, idx, TRACE_TYPE_MARKER, TRACE_MARKER_TYPE_FUNC_ARG) && + check_entry(entries, idx, TRACE_TYPE_ENCODING, -1) && check_entry(entries, idx, TRACE_TYPE_INSTR, -1) && check_entry(entries, idx, TRACE_TYPE_READ, -1) && + check_entry(entries, idx, TRACE_TYPE_ENCODING, -1) && check_entry(entries, idx, TRACE_TYPE_INSTR, -1) && check_entry(entries, idx, TRACE_TYPE_MARKER, TRACE_MARKER_TYPE_FUNC_ID) && check_entry(entries, idx, TRACE_TYPE_MARKER, TRACE_MARKER_TYPE_FUNC_RETADDR) && diff --git a/clients/drcachesim/tools/opcode_mix.cpp b/clients/drcachesim/tools/opcode_mix.cpp index 7ec845b1120..dc9a982aa60 100644 --- a/clients/drcachesim/tools/opcode_mix.cpp +++ b/clients/drcachesim/tools/opcode_mix.cpp @@ -65,9 +65,13 @@ std::string opcode_mix_t::initialize() { serial_shard_.worker = &serial_worker_; - if (module_file_path_.empty()) - return "Module file path is missing"; dcontext_.dcontext = dr_standalone_init(); + // The module_file_path is optional and unused for traces with + // OFFLINE_FILE_TYPE_ENCODINGS. + if (module_file_path_.empty()) + return ""; + // Legacy trace support where binaries are needed. + // We do not support non-module code for such traces. std::string error = directory_.initialize_module_file(module_file_path_); if (!error.empty()) return "Failed to initialize directory: " + error; @@ -134,6 +138,7 @@ opcode_mix_t::parallel_shard_memref(void *shard_data, const memref_t &memref) shard_data_t *shard = reinterpret_cast(shard_data); if (memref.marker.type == TRACE_TYPE_MARKER && memref.marker.marker_type == TRACE_MARKER_TYPE_FILETYPE) { + shard->filetype = static_cast(memref.marker.marker_value); if (TESTANY(OFFLINE_FILE_TYPE_ARCH_ALL, memref.marker.marker_value) && !TESTANY(build_target_arch_type(), memref.marker.marker_value)) { shard->error = std::string("Architecture mismatch: trace recorded on ") + @@ -149,44 +154,59 @@ opcode_mix_t::parallel_shard_memref(void *shard_data, const memref_t &memref) } ++shard->instr_count; - app_pc mapped_pc; + app_pc decode_pc; const app_pc trace_pc = reinterpret_cast(memref.instr.addr); - if (trace_pc >= shard->last_trace_module_start && - static_cast(trace_pc - shard->last_trace_module_start) < - shard->last_trace_module_size) { - mapped_pc = - shard->last_mapped_module_start + (trace_pc - shard->last_trace_module_start); + if (TESTANY(OFFLINE_FILE_TYPE_ENCODINGS, shard->filetype)) { + // The trace has instruction encodings inside it. + decode_pc = const_cast(memref.instr.encoding); } else { - std::lock_guard guard(mapper_mutex_); - mapped_pc = module_mapper_->find_mapped_trace_bounds( - trace_pc, &shard->last_mapped_module_start, &shard->last_trace_module_size); - if (!module_mapper_->get_last_error().empty()) { - shard->last_trace_module_start = nullptr; - shard->last_trace_module_size = 0; - shard->error = "Failed to find mapped address for " + - to_hex_string(memref.instr.addr) + ": " + - module_mapper_->get_last_error(); + // Legacy trace support where we need the binaries. + if (!module_mapper_) { + shard->error = + "Module file path is missing and trace has no embedded encodings"; return false; } - shard->last_trace_module_start = - trace_pc - (mapped_pc - shard->last_mapped_module_start); + if (trace_pc >= shard->last_trace_module_start && + static_cast(trace_pc - shard->last_trace_module_start) < + shard->last_trace_module_size) { + decode_pc = shard->last_mapped_module_start + + (trace_pc - shard->last_trace_module_start); + } else { + std::lock_guard guard(mapper_mutex_); + decode_pc = module_mapper_->find_mapped_trace_bounds( + trace_pc, &shard->last_mapped_module_start, + &shard->last_trace_module_size); + if (!module_mapper_->get_last_error().empty()) { + shard->last_trace_module_start = nullptr; + shard->last_trace_module_size = 0; + shard->error = "Failed to find mapped address for " + + to_hex_string(memref.instr.addr) + ": " + + module_mapper_->get_last_error(); + return false; + } + shard->last_trace_module_start = + trace_pc - (decode_pc - shard->last_mapped_module_start); + } } int opcode; - auto cached_opcode = shard->worker->opcode_cache.find(mapped_pc); + // TODO i#2062,i#5520: We need to invalidate opcode_cache on changed app code. + // The reader may add markers to help us with that. + // For now we assume unchanging code. + auto cached_opcode = shard->worker->opcode_cache.find(trace_pc); if (cached_opcode != shard->worker->opcode_cache.end()) { opcode = cached_opcode->second; } else { instr_t instr; instr_init(dcontext_.dcontext, &instr); app_pc next_pc = - decode_from_copy(dcontext_.dcontext, mapped_pc, trace_pc, &instr); + decode_from_copy(dcontext_.dcontext, decode_pc, trace_pc, &instr); if (next_pc == NULL || !instr_valid(&instr)) { shard->error = "Failed to decode instruction " + to_hex_string(memref.instr.addr); return false; } opcode = instr_get_opcode(&instr); - shard->worker->opcode_cache[mapped_pc] = opcode; + shard->worker->opcode_cache[trace_pc] = opcode; instr_free(dcontext_.dcontext, &instr); } ++shard->opcode_counts[opcode]; diff --git a/clients/drcachesim/tools/opcode_mix.h b/clients/drcachesim/tools/opcode_mix.h index edf72c4f6b1..ba55f34f81b 100644 --- a/clients/drcachesim/tools/opcode_mix.h +++ b/clients/drcachesim/tools/opcode_mix.h @@ -1,5 +1,5 @@ /* ********************************************************** - * Copyright (c) 2018-2020 Google, Inc. All rights reserved. + * Copyright (c) 2018-2022 Google, Inc. All rights reserved. * **********************************************************/ /* @@ -43,6 +43,8 @@ class opcode_mix_t : public analysis_tool_t { public: + // The module_file_path is optional and unused for traces with + // OFFLINE_FILE_TYPE_ENCODINGS. opcode_mix_t(const std::string &module_file_path, unsigned int verbose, const std::string &alt_module_dir = ""); virtual ~opcode_mix_t(); @@ -93,6 +95,7 @@ class opcode_mix_t : public analysis_tool_t { app_pc last_trace_module_start; size_t last_trace_module_size; app_pc last_mapped_module_start; + offline_file_type_t filetype = OFFLINE_FILE_TYPE_DEFAULT; }; struct dcontext_cleanup_last_t { @@ -109,13 +112,15 @@ class opcode_mix_t : public analysis_tool_t { * destroying the other fields which may use DR heap. */ dcontext_cleanup_last_t dcontext_; + + // These are all optional and unused for OFFLINE_FILE_TYPE_ENCODINGS. std::string module_file_path_; std::unique_ptr module_mapper_; std::mutex mapper_mutex_; - // We reference directory.modfile_bytes throughout operation, so its lifetime // must match ours. raw2trace_directory_t directory_; + std::unordered_map shard_map_; // This mutex is only needed in parallel_shard_init. In all other accesses to // shard_map (process_memref, print_results) we are single-threaded. diff --git a/clients/drcachesim/tools/view.cpp b/clients/drcachesim/tools/view.cpp index d57231dc02d..27814de7cb6 100644 --- a/clients/drcachesim/tools/view.cpp +++ b/clients/drcachesim/tools/view.cpp @@ -90,11 +90,11 @@ view_t::initialize() } if (!has_modules_) { // Continue but omit disassembly to support cases where binaries are - // not available. + // not available and OFFLINE_FILE_TYPE_ENCODINGS is not present. return ""; } - // Non-module instruction entries will be in the final trace (i#2062,i#5520) so - // we do not need the encoding file and leave it as its default INVALID_FILE. + // Legacy trace support where binaries are needed. + // We do not support non-module code for such traces. module_mapper_ = module_mapper_t::create(directory_.modfile_bytes_, nullptr, nullptr, nullptr, nullptr, knob_verbose_, knob_alt_module_dir_); @@ -442,17 +442,27 @@ view_t::process_memref(const memref_t &memref) return true; } - app_pc mapped_pc; - app_pc orig_pc = (app_pc)memref.instr.addr; - mapped_pc = module_mapper_->find_mapped_trace_address(orig_pc); - if (!module_mapper_->get_last_error().empty()) { - error_string_ = "Failed to find mapped address for " + - to_hex_string(memref.instr.addr) + ": " + module_mapper_->get_last_error(); - return false; + app_pc decode_pc; + const app_pc orig_pc = (app_pc)memref.instr.addr; + if (TESTANY(OFFLINE_FILE_TYPE_ENCODINGS, filetype_)) { + // The trace has instruction encodings inside it. + decode_pc = const_cast(memref.instr.encoding); + } else { + // Legacy trace support where we need the binaries. + decode_pc = module_mapper_->find_mapped_trace_address(orig_pc); + if (!module_mapper_->get_last_error().empty()) { + error_string_ = "Failed to find mapped address for " + + to_hex_string(memref.instr.addr) + ": " + + module_mapper_->get_last_error(); + return false; + } } std::string disasm; - auto cached_disasm = disasm_cache_.find(mapped_pc); + // TODO i#2062,i#5520: We need to invalidate opcode_cache on changed app code. + // The reader may add markers to help us with that. + // For now we assume unchanging code. + auto cached_disasm = disasm_cache_.find(orig_pc); if (cached_disasm != disasm_cache_.end()) { disasm = cached_disasm->second; } else { @@ -460,7 +470,7 @@ view_t::process_memref(const memref_t &memref) // exported so we just use the same value here. char buf[196]; byte *next_pc = disassemble_to_buffer( - dcontext_.dcontext, mapped_pc, orig_pc, /*show_pc=*/false, + dcontext_.dcontext, decode_pc, orig_pc, /*show_pc=*/false, /*show_bytes=*/true, buf, BUFFER_SIZE_ELEMENTS(buf), /*printed=*/nullptr); if (next_pc == nullptr) { @@ -468,7 +478,7 @@ view_t::process_memref(const memref_t &memref) return false; } disasm = buf; - disasm_cache_.insert({ mapped_pc, disasm }); + disasm_cache_.insert({ orig_pc, disasm }); } // Put our prefix on raw byte spillover, and skip the other columns. auto newline = disasm.find('\n'); diff --git a/clients/drcachesim/tools/view.h b/clients/drcachesim/tools/view.h index 6f8e75f3793..435cdabfdb1 100644 --- a/clients/drcachesim/tools/view.h +++ b/clients/drcachesim/tools/view.h @@ -45,6 +45,8 @@ class view_t : public analysis_tool_t { public: + // The module_file_path is optional and unused for traces with + // OFFLINE_FILE_TYPE_ENCODINGS. view_t(const std::string &module_file_path, memref_tid_t thread, uint64_t skip_refs, uint64_t sim_refs, const std::string &syntax, unsigned int verbose, const std::string &alt_module_dir = ""); @@ -102,9 +104,12 @@ class view_t : public analysis_tool_t { * destroying the other fields which may use DR heap. */ dcontext_cleanup_last_t dcontext_; + + // These are all optional and unused for OFFLINE_FILE_TYPE_ENCODINGS. std::string module_file_path_; std::unique_ptr module_mapper_; raw2trace_directory_t directory_; + unsigned int knob_verbose_; int trace_version_; static const std::string TOOL_NAME; diff --git a/clients/drcachesim/tracer/raw2trace.cpp b/clients/drcachesim/tracer/raw2trace.cpp index d3e40a151c5..d13618a4117 100644 --- a/clients/drcachesim/tracer/raw2trace.cpp +++ b/clients/drcachesim/tracer/raw2trace.cpp @@ -781,6 +781,16 @@ raw2trace_t::do_conversion() return ""; } +bool +raw2trace_t::record_encoding_emitted(void *tls, app_pc pc) +{ + auto tdata = reinterpret_cast(tls); + if (tdata->encoding_emitted.find(pc) != tdata->encoding_emitted.end()) + return false; + tdata->encoding_emitted.insert(pc); + return true; +} + raw2trace_t::block_summary_t * raw2trace_t::lookup_block_summary(void *tls, uint64 modidx, uint64 modoffs, app_pc block_start) @@ -1148,8 +1158,9 @@ raw2trace_t::open_new_chunk(raw2trace_thread_data_t *tdata) if (!error.empty()) return error; - // TODO i#5520: Once we emit encodings, we need to clear the encoding cache - // here so that each chunk is self-contained. + // We need to clear the encoding cache so that each chunk is self-contained + // and repeats all encodings used inside it. + tdata->encoding_emitted.clear(); // TODO i#5538: Add a virtual-to-physical cache and clear it here. // We'll need to add a routine for trace_converter_t to call to query our cache -- @@ -1397,6 +1408,8 @@ trace_metadata_reader_t::is_thread_start(const offline_entry_t *entry, if (ver < OFFLINE_FILE_VERSION_HEADER_FIELDS_SWAP) return false; } + type = static_cast(static_cast(type) | + OFFLINE_FILE_TYPE_ENCODINGS); if (version != nullptr) *version = ver; if (file_type != nullptr) diff --git a/clients/drcachesim/tracer/raw2trace.h b/clients/drcachesim/tracer/raw2trace.h index 13939c62ada..bacf4e2358c 100644 --- a/clients/drcachesim/tracer/raw2trace.h +++ b/clients/drcachesim/tracer/raw2trace.h @@ -47,6 +47,7 @@ #include #include #include +#include #include #include "trace_entry.h" #include "instru.h" @@ -678,6 +679,12 @@ struct trace_header_t { * Increases the per-thread counter for the statistic identified by stat by value. * * + *
  • bool raw2trace_t::record_encoding_emitted(void *tls, app_pc pc) + * + * Returns false if an encoding was already emitted. + * Otherwise, remembers that an encoding is being emitted now, and returns true. + *
    • + * *
  • bool raw2trace_t::instr_summary_exists(void *tls, uint64 modidx, uint64 modoffs, * int index) const * @@ -1213,6 +1220,11 @@ template class trace_converter_t { if (!error.empty()) return error; } + if (!skip_icache && impl()->record_encoding_emitted(tls, decode_pc)) { + error = append_encoding(instr, buf, decode_pc); + if (!error.empty()) + return error; + } // TODO i#1729: make bundles via lazy accum until hit memref/end. buf->type = instr->type(); if (buf->type == TRACE_TYPE_INSTR_MAYBE_FETCH) { @@ -1334,6 +1346,27 @@ template class trace_converter_t { return ""; } + std::string + append_encoding(const instr_summary_t *instr, trace_entry_t *&buf, app_pc pc) + { + size_t size_left = instr->length(); + size_t offs = 0; + do { + buf->type = TRACE_TYPE_ENCODING; + buf->size = std::min(size_left, sizeof(buf->encoding)); + memcpy(buf->encoding, pc + offs, buf->size); + if (buf->size < sizeof(buf->encoding)) { + // We don't have to set the rest to 0 but it is nice. + memset(buf->encoding + buf->size, 0, sizeof(buf->encoding) - buf->size); + } + offs += buf->size; + size_left -= buf->size; + ++buf; + impl()->log(4, "Appended encoding entry for %p\n", pc); + } while (size_left > 0); + return ""; + } + // Returns true if a kernel interrupt happened at cur_pc. // Outputs a kernel interrupt if this is the right location. // Outputs any other markers observed if !instrs_are_separate, since they @@ -1818,6 +1851,8 @@ class raw2trace_t : public trace_converter_t { uint64 chunk_count_ = 0; uint64 last_timestamp_ = 0; uint last_cpu_ = 0; + + std::set encoding_emitted; }; virtual std::string @@ -1860,6 +1895,8 @@ class raw2trace_t : public trace_converter_t { write_delayed_branches(void *tls, const trace_entry_t *start, const trace_entry_t *end); bool + record_encoding_emitted(void *tls, app_pc pc); + bool instr_summary_exists(void *tls, uint64 modidx, uint64 modoffs, app_pc block_start, int index, app_pc pc); block_summary_t * From 7b80e920aa972dbe116700692688361a2054c610 Mon Sep 17 00:00:00 2001 From: Derek Bruening Date: Tue, 27 Sep 2022 23:21:06 -0400 Subject: [PATCH 2/8] Fix Windows std::min issue; fix 32-bit unit test output; fix rseq rollback --- .../drcachesim/tests/raw2trace_unit_tests.cpp | 4 ++++ clients/drcachesim/tracer/raw2trace.h | 19 +++++++++---------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/clients/drcachesim/tests/raw2trace_unit_tests.cpp b/clients/drcachesim/tests/raw2trace_unit_tests.cpp index b1956eca026..a3812c09a96 100644 --- a/clients/drcachesim/tests/raw2trace_unit_tests.cpp +++ b/clients/drcachesim/tests/raw2trace_unit_tests.cpp @@ -308,6 +308,10 @@ test_branch_delays(void *drcontext) check_entry(entries, idx, TRACE_TYPE_MARKER, TRACE_MARKER_TYPE_CPU_ID) && // Both branches should be delayed until after the timestamp+cpu markers: check_entry(entries, idx, TRACE_TYPE_ENCODING, -1) && +#ifdef X86_32 + // An extra encoding entry is needed for the cbr. + check_entry(entries, idx, TRACE_TYPE_ENCODING, -1) && +#endif check_entry(entries, idx, TRACE_TYPE_INSTR_CONDITIONAL_JUMP, -1) && check_entry(entries, idx, TRACE_TYPE_ENCODING, -1) && check_entry(entries, idx, TRACE_TYPE_INSTR_DIRECT_JUMP, -1) && diff --git a/clients/drcachesim/tracer/raw2trace.h b/clients/drcachesim/tracer/raw2trace.h index bacf4e2358c..9042dc1a9b5 100644 --- a/clients/drcachesim/tracer/raw2trace.h +++ b/clients/drcachesim/tracer/raw2trace.h @@ -41,6 +41,7 @@ * @brief DrMemtrace offline trace post-processing customization. */ +#define NOMINMAX // Avoid windows.h messing up std::min. #include "dr_api.h" #include "drmemtrace.h" #include "drcovlib.h" @@ -1225,7 +1226,8 @@ template class trace_converter_t { if (!error.empty()) return error; } - // TODO i#1729: make bundles via lazy accum until hit memref/end. + // XXX i#1729: make bundles via lazy accum until hit memref/end, if + // we don't need encodings. buf->type = instr->type(); if (buf->type == TRACE_TYPE_INSTR_MAYBE_FETCH) { // We want it to look like the original rep string, with just one instr @@ -1247,6 +1249,7 @@ template class trace_converter_t { buf->size = (ushort)(skip_icache ? 0 : instr->length()); buf->addr = (addr_t)orig_pc; ++buf; + impl()->log(4, "Appended instr fetch for original %p\n", orig_pc); decode_pc = pc; // Check for a signal *after* the instruction. The trace is recording // instruction *fetches*, not instruction retirement, and we want to @@ -1438,15 +1441,11 @@ template class trace_converter_t { // includes the rseq committing store before the native rseq // execution hits the native abort. Pretend the native abort // happened *before* the committing store by walking the store - // backward. - trace_type_t skipped_type; - do { - impl()->log(4, "Rolling back entry for rseq abort\n"); - --*buf_in; - skipped_type = static_cast((*buf_in)->type); - DR_ASSERT(*buf_in >= buf_start); - } while (!type_is_instr(skipped_type) && - skipped_type != TRACE_TYPE_INSTR_NO_FETCH); + // backward. Everything in the buffer is for the store; + // there should be no (other) intra-bb markers not for the store. + impl()->log(4, "Rolling back %d entries for rseq abort\n", + *buf_in - buf_start); + *buf_in = buf_start; } } else { // Put it back (below). We do not have a problem with other markers From 35ca79cc0dcab6fd4384222df70009b124a01a01 Mon Sep 17 00:00:00 2001 From: Derek Bruening Date: Wed, 28 Sep 2022 00:23:58 -0400 Subject: [PATCH 3/8] Roll back last encoding too when roll back instr for rseq abort --- clients/drcachesim/tracer/raw2trace.cpp | 8 ++++++++ clients/drcachesim/tracer/raw2trace.h | 14 ++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/clients/drcachesim/tracer/raw2trace.cpp b/clients/drcachesim/tracer/raw2trace.cpp index d13618a4117..4e68a341542 100644 --- a/clients/drcachesim/tracer/raw2trace.cpp +++ b/clients/drcachesim/tracer/raw2trace.cpp @@ -788,9 +788,17 @@ raw2trace_t::record_encoding_emitted(void *tls, app_pc pc) if (tdata->encoding_emitted.find(pc) != tdata->encoding_emitted.end()) return false; tdata->encoding_emitted.insert(pc); + tdata->last_encoding_emitted = pc; return true; } +void +raw2trace_t::rollback_last_encoding(void *tls) +{ + auto tdata = reinterpret_cast(tls); + tdata->encoding_emitted.erase(tdata->last_encoding_emitted); +} + raw2trace_t::block_summary_t * raw2trace_t::lookup_block_summary(void *tls, uint64 modidx, uint64 modoffs, app_pc block_start) diff --git a/clients/drcachesim/tracer/raw2trace.h b/clients/drcachesim/tracer/raw2trace.h index 9042dc1a9b5..4bee6f6a4d3 100644 --- a/clients/drcachesim/tracer/raw2trace.h +++ b/clients/drcachesim/tracer/raw2trace.h @@ -686,6 +686,11 @@ struct trace_header_t { * Otherwise, remembers that an encoding is being emitted now, and returns true. *
    • * + *
  • void raw2trace_t::rollback_last_encoding(void *tls) + * + * Removes the record of the last encoding remembered in record_encoding_emitted(). + *
    • + * *
  • bool raw2trace_t::instr_summary_exists(void *tls, uint64 modidx, uint64 modoffs, * int index) const * @@ -1445,6 +1450,12 @@ template class trace_converter_t { // there should be no (other) intra-bb markers not for the store. impl()->log(4, "Rolling back %d entries for rseq abort\n", *buf_in - buf_start); + for (trace_entry_t *entry = buf_start; entry < *buf_in; ++entry) { + if (entry->type == TRACE_TYPE_ENCODING) { + impl()->rollback_last_encoding(tls); + break; + } + } *buf_in = buf_start; } } else { @@ -1852,6 +1863,7 @@ class raw2trace_t : public trace_converter_t { uint last_cpu_ = 0; std::set encoding_emitted; + app_pc last_encoding_emitted = nullptr; }; virtual std::string @@ -1895,6 +1907,8 @@ class raw2trace_t : public trace_converter_t { const trace_entry_t *end); bool record_encoding_emitted(void *tls, app_pc pc); + void + rollback_last_encoding(void *tls); bool instr_summary_exists(void *tls, uint64 modidx, uint64 modoffs, app_pc block_start, int index, app_pc pc); From 34b476f229db6b6290821c27ea256956eee6d910 Mon Sep 17 00:00:00 2001 From: Derek Bruening Date: Wed, 28 Sep 2022 00:25:54 -0400 Subject: [PATCH 4/8] Put the Windows std::min fix in cpp file too --- clients/drcachesim/tracer/raw2trace.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/clients/drcachesim/tracer/raw2trace.cpp b/clients/drcachesim/tracer/raw2trace.cpp index 4e68a341542..e1bee161e01 100644 --- a/clients/drcachesim/tracer/raw2trace.cpp +++ b/clients/drcachesim/tracer/raw2trace.cpp @@ -34,6 +34,7 @@ * by the cache simulator and other analysis tools. */ +#define NOMINMAX // Avoid windows.h messing up std::min. #include "dr_api.h" #include "drcovlib.h" #include "raw2trace.h" From b4dca01826de1b6c6895616018489d7c1b064e44 Mon Sep 17 00:00:00 2001 From: Derek Bruening Date: Wed, 28 Sep 2022 00:59:41 -0400 Subject: [PATCH 5/8] Bump version and type in phys_SUDO test template; unit test 32-bit extra encoding; fix another Windows build warning --- clients/drcachesim/tests/offline-phys.templatex | 4 ++-- clients/drcachesim/tests/raw2trace_unit_tests.cpp | 6 +++++- clients/drcachesim/tracer/raw2trace.h | 3 ++- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/clients/drcachesim/tests/offline-phys.templatex b/clients/drcachesim/tests/offline-phys.templatex index e2943fbb676..0450a7d94d2 100644 --- a/clients/drcachesim/tests/offline-phys.templatex +++ b/clients/drcachesim/tests/offline-phys.templatex @@ -11,8 +11,8 @@ Adios world! Output format: : T ------------------------------------------------------------ - 1: T[0-9]+ - 2: T[0-9]+ + 1: T[0-9]+ + 2: T[0-9]+ 3: T[0-9]+ 4: T[0-9]+ 5: T[0-9]+ diff --git a/clients/drcachesim/tests/raw2trace_unit_tests.cpp b/clients/drcachesim/tests/raw2trace_unit_tests.cpp index a3812c09a96..6984dee1e59 100644 --- a/clients/drcachesim/tests/raw2trace_unit_tests.cpp +++ b/clients/drcachesim/tests/raw2trace_unit_tests.cpp @@ -309,11 +309,15 @@ test_branch_delays(void *drcontext) // Both branches should be delayed until after the timestamp+cpu markers: check_entry(entries, idx, TRACE_TYPE_ENCODING, -1) && #ifdef X86_32 - // An extra encoding entry is needed for the cbr. + // An extra encoding entry is needed. check_entry(entries, idx, TRACE_TYPE_ENCODING, -1) && #endif check_entry(entries, idx, TRACE_TYPE_INSTR_CONDITIONAL_JUMP, -1) && check_entry(entries, idx, TRACE_TYPE_ENCODING, -1) && +#ifdef X86_32 + // An extra encoding entry is needed. + check_entry(entries, idx, TRACE_TYPE_ENCODING, -1) && +#endif check_entry(entries, idx, TRACE_TYPE_INSTR_DIRECT_JUMP, -1) && check_entry(entries, idx, TRACE_TYPE_ENCODING, -1) && check_entry(entries, idx, TRACE_TYPE_INSTR, -1) && diff --git a/clients/drcachesim/tracer/raw2trace.h b/clients/drcachesim/tracer/raw2trace.h index 4bee6f6a4d3..20e85e2f4aa 100644 --- a/clients/drcachesim/tracer/raw2trace.h +++ b/clients/drcachesim/tracer/raw2trace.h @@ -1361,7 +1361,8 @@ template class trace_converter_t { size_t offs = 0; do { buf->type = TRACE_TYPE_ENCODING; - buf->size = std::min(size_left, sizeof(buf->encoding)); + buf->size = + static_cast(std::min(size_left, sizeof(buf->encoding))); memcpy(buf->encoding, pc + offs, buf->size); if (buf->size < sizeof(buf->encoding)) { // We don't have to set the rest to 0 but it is nice. From e568f7521d7472967b8bd2febef0de39cdd3d0cc Mon Sep 17 00:00:00 2001 From: Derek Bruening Date: Wed, 28 Sep 2022 01:18:46 -0400 Subject: [PATCH 6/8] One more double-entry for raw2trace_unit_tests 32-bit --- clients/drcachesim/tests/raw2trace_unit_tests.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/clients/drcachesim/tests/raw2trace_unit_tests.cpp b/clients/drcachesim/tests/raw2trace_unit_tests.cpp index 6984dee1e59..c8aca5f3bae 100644 --- a/clients/drcachesim/tests/raw2trace_unit_tests.cpp +++ b/clients/drcachesim/tests/raw2trace_unit_tests.cpp @@ -432,6 +432,10 @@ test_marker_placement(void *drcontext) check_entry(entries, idx, TRACE_TYPE_MARKER, TRACE_MARKER_TYPE_FUNC_RETADDR) && check_entry(entries, idx, TRACE_TYPE_MARKER, TRACE_MARKER_TYPE_FUNC_ARG) && check_entry(entries, idx, TRACE_TYPE_ENCODING, -1) && +#ifdef X86_32 + // An extra encoding entry is needed. + check_entry(entries, idx, TRACE_TYPE_ENCODING, -1) && +#endif check_entry(entries, idx, TRACE_TYPE_INSTR, -1) && check_entry(entries, idx, TRACE_TYPE_READ, -1) && check_entry(entries, idx, TRACE_TYPE_ENCODING, -1) && From 116f53856ac0b4a73022e051c750cdcf17b83d9c Mon Sep 17 00:00:00 2001 From: Derek Bruening Date: Wed, 28 Sep 2022 20:14:32 -0400 Subject: [PATCH 7/8] Review requests: Fix incorrect encoding entry vs cache lookup order; add asserts; add comments --- clients/drcachesim/common/trace_entry.h | 7 ++++++- clients/drcachesim/reader/reader.cpp | 18 ++++++++++-------- clients/drcachesim/tools/opcode_mix.h | 4 ++++ clients/drcachesim/tools/view.h | 4 ++++ clients/drcachesim/tracer/raw2trace.cpp | 4 ++++ clients/drcachesim/tracer/raw2trace.h | 17 +++++++++++++++-- 6 files changed, 43 insertions(+), 11 deletions(-) diff --git a/clients/drcachesim/common/trace_entry.h b/clients/drcachesim/common/trace_entry.h index b3122531544..4fff58daeac 100644 --- a/clients/drcachesim/common/trace_entry.h +++ b/clients/drcachesim/common/trace_entry.h @@ -462,6 +462,9 @@ marker_type_is_function_marker(const trace_marker_type_t mark) return mark >= TRACE_MARKER_TYPE_FUNC_ID && mark <= TRACE_MARKER_TYPE_FUNC_RETVAL; } +// The longest instruction on any architecture. +// This matches DR's MAX_INSTR_LENGTH for x86 but we want the same +// size for all architectures and DR's define is available ifdef X86 only. #define MAX_ENCODING_LENGTH 17 // This is the data format generated by the online tracer and produced after @@ -478,7 +481,7 @@ START_PACKED_STRUCTURE struct _trace_entry_t { unsigned short type; // 2 bytes: trace_type_t // 2 bytes: mem ref size, instr length, or num of instrs for instr bundle, - // or marker sub-type, or num of bytes in encoding[] array. + // or marker sub-type, or num of bytes (max sizeof(addr_t)) in encoding[] array. unsigned short size; union { addr_t addr; // 4/8 bytes: mem ref addr, instr pc, tid, pid, marker val @@ -486,6 +489,8 @@ struct _trace_entry_t { unsigned char length[sizeof(addr_t)]; // The raw encoding bytes for the subsequent instruction fetch entry. // There may be multiple consecutive records to hold long instructions. + // The reader should keep concatenating these bytes until the subsequent + // insruction fetch entry is found. unsigned char encoding[sizeof(addr_t)]; }; } END_PACKED_STRUCTURE; diff --git a/clients/drcachesim/reader/reader.cpp b/clients/drcachesim/reader/reader.cpp index 9f09cf94c3e..ac454c95663 100644 --- a/clients/drcachesim/reader/reader.cpp +++ b/clients/drcachesim/reader/reader.cpp @@ -153,11 +153,8 @@ reader_t::operator++() prev_instr_addr_ = input_entry_->addr; if (cur_ref_.instr.type != TRACE_TYPE_INSTR_NO_FETCH) ++cur_instr_count_; - // Look for encoding bits. - const auto &it = encodings_.find(cur_ref_.instr.addr); - if (it != encodings_.end()) { - memcpy(cur_ref_.instr.encoding, it->second.bits, it->second.size); - } else if (last_encoding_.size > 0) { + // Look for encoding bits that belong to this instr. + if (last_encoding_.size > 0) { if (last_encoding_.size != cur_ref_.instr.size) { ERRMSG("Encoding size %zu != instr size %zu\n", last_encoding_.size, cur_ref_.instr.size); @@ -166,9 +163,14 @@ reader_t::operator++() memcpy(cur_ref_.instr.encoding, last_encoding_.bits, last_encoding_.size); encodings_[cur_ref_.instr.addr] = last_encoding_; - } else if (!expect_no_encodings_) { - ERRMSG("Missing encoding for 0x%zx\n", cur_ref_.instr.addr); - assert(false); + } else { + const auto &it = encodings_.find(cur_ref_.instr.addr); + if (it != encodings_.end()) { + memcpy(cur_ref_.instr.encoding, it->second.bits, it->second.size); + } else if (!expect_no_encodings_) { + ERRMSG("Missing encoding for 0x%zx\n", cur_ref_.instr.addr); + assert(false); + } } last_encoding_.size = 0; } diff --git a/clients/drcachesim/tools/opcode_mix.h b/clients/drcachesim/tools/opcode_mix.h index ba55f34f81b..56594e86370 100644 --- a/clients/drcachesim/tools/opcode_mix.h +++ b/clients/drcachesim/tools/opcode_mix.h @@ -45,6 +45,8 @@ class opcode_mix_t : public analysis_tool_t { public: // The module_file_path is optional and unused for traces with // OFFLINE_FILE_TYPE_ENCODINGS. + // XXX: Once we update our toolchains to guarantee C++17 support we could use + // std::optional here. opcode_mix_t(const std::string &module_file_path, unsigned int verbose, const std::string &alt_module_dir = ""); virtual ~opcode_mix_t(); @@ -114,6 +116,8 @@ class opcode_mix_t : public analysis_tool_t { dcontext_cleanup_last_t dcontext_; // These are all optional and unused for OFFLINE_FILE_TYPE_ENCODINGS. + // XXX: Once we update our toolchains to guarantee C++17 support we could use + // std::optional here. std::string module_file_path_; std::unique_ptr module_mapper_; std::mutex mapper_mutex_; diff --git a/clients/drcachesim/tools/view.h b/clients/drcachesim/tools/view.h index 435cdabfdb1..ec60700d12e 100644 --- a/clients/drcachesim/tools/view.h +++ b/clients/drcachesim/tools/view.h @@ -47,6 +47,8 @@ class view_t : public analysis_tool_t { public: // The module_file_path is optional and unused for traces with // OFFLINE_FILE_TYPE_ENCODINGS. + // XXX: Once we update our toolchains to guarantee C++17 support we could use + // std::optional here. view_t(const std::string &module_file_path, memref_tid_t thread, uint64_t skip_refs, uint64_t sim_refs, const std::string &syntax, unsigned int verbose, const std::string &alt_module_dir = ""); @@ -106,6 +108,8 @@ class view_t : public analysis_tool_t { dcontext_cleanup_last_t dcontext_; // These are all optional and unused for OFFLINE_FILE_TYPE_ENCODINGS. + // XXX: Once we update our toolchains to guarantee C++17 support we could use + // std::optional here. std::string module_file_path_; std::unique_ptr module_mapper_; raw2trace_directory_t directory_; diff --git a/clients/drcachesim/tracer/raw2trace.cpp b/clients/drcachesim/tracer/raw2trace.cpp index e1bee161e01..4e0889bbbfb 100644 --- a/clients/drcachesim/tracer/raw2trace.cpp +++ b/clients/drcachesim/tracer/raw2trace.cpp @@ -793,11 +793,15 @@ raw2trace_t::record_encoding_emitted(void *tls, app_pc pc) return true; } +// This can only be called once between calls to record_encoding_emitted() +// and only after record_encoding_emitted() returns true. void raw2trace_t::rollback_last_encoding(void *tls) { auto tdata = reinterpret_cast(tls); + DEBUG_ASSERT(tdata->last_encoding_emitted != nullptr); tdata->encoding_emitted.erase(tdata->last_encoding_emitted); + tdata->last_encoding_emitted = nullptr; } raw2trace_t::block_summary_t * diff --git a/clients/drcachesim/tracer/raw2trace.h b/clients/drcachesim/tracer/raw2trace.h index 20e85e2f4aa..4a4eda54b84 100644 --- a/clients/drcachesim/tracer/raw2trace.h +++ b/clients/drcachesim/tracer/raw2trace.h @@ -689,6 +689,8 @@ struct trace_header_t { *
  • void raw2trace_t::rollback_last_encoding(void *tls) * * Removes the record of the last encoding remembered in record_encoding_emitted(). + * This can only be called once between calls to record_encoding_emitted() + * and only after record_encoding_emitted() returns true. *
    • * *
  • bool raw2trace_t::instr_summary_exists(void *tls, uint64 modidx, uint64 modoffs, @@ -1227,7 +1229,7 @@ template class trace_converter_t { return error; } if (!skip_icache && impl()->record_encoding_emitted(tls, decode_pc)) { - error = append_encoding(instr, buf, decode_pc); + error = append_encoding(instr, buf, buf_start, decode_pc); if (!error.empty()) return error; } @@ -1355,7 +1357,8 @@ template class trace_converter_t { } std::string - append_encoding(const instr_summary_t *instr, trace_entry_t *&buf, app_pc pc) + append_encoding(const instr_summary_t *instr, trace_entry_t *&buf, + trace_entry_t *buf_start, app_pc pc) { size_t size_left = instr->length(); size_t offs = 0; @@ -1371,6 +1374,8 @@ template class trace_converter_t { offs += buf->size; size_left -= buf->size; ++buf; + DR_CHECK(static_cast(buf - buf_start) < WRITE_BUFFER_SIZE, + "Too many entries for write buffer"); impl()->log(4, "Appended encoding entry for %p\n", pc); } while (size_left > 0); return ""; @@ -1451,6 +1456,12 @@ template class trace_converter_t { // there should be no (other) intra-bb markers not for the store. impl()->log(4, "Rolling back %d entries for rseq abort\n", *buf_in - buf_start); + // If we recorded and emitted an encoding we would not emit + // it next time and be missing the encoding so we must clear + // the cache for that entry. This will only happen once + // for any new encoding (one synchronous signal/rseq abort + // per instr) so we will satisfy the one-time limit of + // rollback_last_encoding() (it has an assert to verify). for (trace_entry_t *entry = buf_start; entry < *buf_in; ++entry) { if (entry->type == TRACE_TYPE_ENCODING) { impl()->rollback_last_encoding(tls); @@ -1908,6 +1919,8 @@ class raw2trace_t : public trace_converter_t { const trace_entry_t *end); bool record_encoding_emitted(void *tls, app_pc pc); + // This can only be called once between calls to record_encoding_emitted() + // and only after record_encoding_emitted() returns true. void rollback_last_encoding(void *tls); bool From f11f2eaac028084128463fd31c3be1206721dec8 Mon Sep 17 00:00:00 2001 From: Derek Bruening Date: Wed, 28 Sep 2022 21:30:26 -0400 Subject: [PATCH 8/8] FIx misspelling --- clients/drcachesim/common/trace_entry.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clients/drcachesim/common/trace_entry.h b/clients/drcachesim/common/trace_entry.h index 4fff58daeac..932e0b46dfe 100644 --- a/clients/drcachesim/common/trace_entry.h +++ b/clients/drcachesim/common/trace_entry.h @@ -490,7 +490,7 @@ struct _trace_entry_t { // The raw encoding bytes for the subsequent instruction fetch entry. // There may be multiple consecutive records to hold long instructions. // The reader should keep concatenating these bytes until the subsequent - // insruction fetch entry is found. + // instruction fetch entry is found. unsigned char encoding[sizeof(addr_t)]; }; } END_PACKED_STRUCTURE;