From 449647b693044423dabe07337a176048815ece87 Mon Sep 17 00:00:00 2001
From: Abhinav Anil Sharma <sharmaabhinav@google.com>
Date: Thu, 30 Nov 2023 19:13:05 -0500
Subject: [PATCH] Address reviewer comments.

---
 clients/drcachesim/drpt2trace/drir.h       | 41 +++++++++++++---------
 clients/drcachesim/drpt2trace/ir2trace.cpp |  2 +-
 clients/drcachesim/drpt2trace/pt2ir.cpp    |  7 ++--
 3 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/clients/drcachesim/drpt2trace/drir.h b/clients/drcachesim/drpt2trace/drir.h
index 6b6f0b0974a..a8f177a423c 100644
--- a/clients/drcachesim/drpt2trace/drir.h
+++ b/clients/drcachesim/drpt2trace/drir.h
@@ -121,35 +121,44 @@ class drir_t {
     void *drcontext_;
     instrlist_t *ilist_;
 #define SYSCALL_PT_ENCODING_BUF_SIZE (1024 * 1024)
+    // For each original app pc key, this stores a pair value: the first
+    // element is the address where the encoding is stored for the instruction
+    // at that app pc, the second element is the length of the encoding.
     std::unordered_map<app_pc, std::pair<app_pc, int>> decode_pc_;
+    // A vector of buffers of size SYSCALL_PT_ENCODING_BUF_SIZE. Each buffer
+    // stores some encoded instructions back-to-back. Note that each element
+    // in the buffer is a single byte, so one instr's encoding occupies possibly
+    // multiple consecutive elements.
+    // We allocate new memory to store kernel instruction encodings in
+    // increments of SYSCALL_PT_ENCODING_BUF_SIZE. We do not treat this like a
+    // cache and clear previously stored encodings because we want to ensure
+    // decode_pc uniqueness to callers of get_decode_pc.
     std::vector<std::unique_ptr<uint8_t[]>> instr_encodings_;
+    // Next available offset into instr_encodings_.back().
     size_t next_encoding_offset_ = 0;
 
     void
     record_encoding(app_pc orig_pc, int instr_len, uint8_t *encoding)
     {
-        if (instr_encodings_.empty() ||
-            next_encoding_offset_ + instr_len >= SYSCALL_PT_ENCODING_BUF_SIZE) {
-            // We allocate new memory to store kernel instruction encodings in
-            // increments of SYSCALL_PT_ENCODING_BUF_SIZE. We do not treat this
-            // like a cache and clear previously stored encodings because we want to
-            // ensure decode_pc uniqueness to callers.
-            instr_encodings_.emplace_back(new uint8_t[SYSCALL_PT_ENCODING_BUF_SIZE]);
-            next_encoding_offset_ = 0;
-        }
         auto it = decode_pc_.find(orig_pc);
         // We record the encoding only if we don't already have the same encoding for
         // the given orig_pc.
-        if (it == decode_pc_.end() ||
+        if (it != decode_pc_.end() &&
             // We confirm that the instruction encoding has not changed. Just in case
             // the kernel is doing JIT.
-            it->second.second != instr_len ||
-            memcmp(it->second.first, encoding, it->second.second) != 0) {
-            app_pc encode_pc = &instr_encodings_.back()[next_encoding_offset_];
-            memcpy(encode_pc, encoding, instr_len);
-            decode_pc_[orig_pc] = std::make_pair(encode_pc, instr_len);
-            next_encoding_offset_ += instr_len;
+            it->second.second == instr_len &&
+            memcmp(it->second.first, encoding, it->second.second) == 0) {
+            return;
+        }
+        if (instr_encodings_.empty() ||
+            next_encoding_offset_ + instr_len >= SYSCALL_PT_ENCODING_BUF_SIZE) {
+            instr_encodings_.emplace_back(new uint8_t[SYSCALL_PT_ENCODING_BUF_SIZE]);
+            next_encoding_offset_ = 0;
         }
+        app_pc encode_pc = &instr_encodings_.back()[next_encoding_offset_];
+        memcpy(encode_pc, encoding, instr_len);
+        decode_pc_[orig_pc] = std::make_pair(encode_pc, instr_len);
+        next_encoding_offset_ += instr_len;
     }
 };
 
diff --git a/clients/drcachesim/drpt2trace/ir2trace.cpp b/clients/drcachesim/drpt2trace/ir2trace.cpp
index 3006748f8fc..57e27e931d1 100644
--- a/clients/drcachesim/drpt2trace/ir2trace.cpp
+++ b/clients/drcachesim/drpt2trace/ir2trace.cpp
@@ -67,7 +67,7 @@ ir2trace_t::convert(DR_PARAM_IN drir_t *drir,
     while (instr != NULL) {
         trace_entry_t entry = {};
         entry.size = instr_length(GLOBAL_DCONTEXT, instr);
-        entry.addr = (uintptr_t)instr_get_app_pc(instr);
+        entry.addr = reinterpret_cast<uintptr_t>(instr_get_app_pc(instr));
 
         if (!trace.empty() && trace.back().type == TRACE_TYPE_INSTR_CONDITIONAL_JUMP) {
             if (instr_get_prev(instr) == nullptr ||
diff --git a/clients/drcachesim/drpt2trace/pt2ir.cpp b/clients/drcachesim/drpt2trace/pt2ir.cpp
index 40da57b8530..13ea7221daf 100644
--- a/clients/drcachesim/drpt2trace/pt2ir.cpp
+++ b/clients/drcachesim/drpt2trace/pt2ir.cpp
@@ -383,8 +383,9 @@ pt2ir_t::convert(DR_PARAM_IN const uint8_t *pt_data, DR_PARAM_IN size_t pt_data_
             instr_init(drir->get_drcontext(), instr);
             instr_set_isa_mode(instr,
                                insn.mode == ptem_32bit ? DR_ISA_IA32 : DR_ISA_AMD64);
-            if (decode_from_copy(drir->get_drcontext(), insn.raw, (app_pc)insn.ip,
-                                 instr) == nullptr) {
+            app_pc instr_ip = reinterpret_cast<app_pc>(insn.ip);
+            if (decode_from_copy(drir->get_drcontext(), insn.raw, instr_ip, instr) ==
+                nullptr) {
 #ifdef DEBUG
                 /* Print the invalid instruction‘s PC and raw bytes in DEBUG builds. */
                 if (verbosity_ >= 1) {
@@ -398,7 +399,7 @@ pt2ir_t::convert(DR_PARAM_IN const uint8_t *pt_data, DR_PARAM_IN size_t pt_data_
                 }
 #endif
             }
-            drir->append(instr, (app_pc)insn.ip, insn.size, insn.raw);
+            drir->append(instr, instr_ip, insn.size, insn.raw);
         }
     }
     return PT2IR_CONV_SUCCESS;