Cleanup and assert fix.

clients/drcachesim/tests/decode_cache_test.cpp

@@ -42,6 +42,14 @@
 namespace dynamorio {
 namespace drmemtrace {
 
+#define CHECK(cond, msg, ...)             \
+    do {                                  \
+        if (!(cond)) {                    \
+            fprintf(stderr, "%s\n", msg); \
+            exit(1);                      \
+        }                                 \
+    } while (0)
+
 static constexpr addr_t TID_A = 1;
 static constexpr offline_file_type_t ENCODING_FILE_TYPE =
     static_cast<offline_file_type_t>(OFFLINE_FILE_TYPE_ENCODINGS);
@@ -61,32 +69,22 @@ class test_decode_info_t : public decode_info_base_t {
                             const dynamorio::drmemtrace::_memref_instr_t &memref_instr,
                             instr_t *instr, app_pc decode_pc) override
     {
-        if (decode_info_set_) {
-            std::cerr << "decode_cache_t should call set_decode_info only one time per "
-                         "object\n";
-            assert(false);
-        }
+        CHECK(!decode_info_set_,
+              "decode_cache_t should call set_decode_info only one time per object");
         instr_t my_decoded_instr;
         instr_init(dcontext, &my_decoded_instr);
         app_pc next_pc = decode_from_copy(dcontext, decode_pc,
                                           reinterpret_cast<app_pc>(memref_instr.addr),
                                           &my_decoded_instr);
-        assert(next_pc != nullptr && instr_valid(&my_decoded_instr));
+        CHECK(next_pc != nullptr && instr_valid(&my_decoded_instr),
+              "Expected to see a valid instr decoded from provided decode_pc");
         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);
-            }
+            CHECK(instr != nullptr, "Expected to see a decoded instr_t");
+            CHECK(instr_same(instr, &my_decoded_instr),
+                  "Expected provided decoded instr_t and self decoded instr_t to be the "
+                  "same");
         } else {
-            if (instr != nullptr) {
-                std::cerr << "Expected to see a null decoded instr\n";
-                assert(false);
-            }
+            CHECK(instr == nullptr, "Expected to see a null decoded instr");
         }
 
         is_nop_ = instr_is_nop(&my_decoded_instr);
@@ -138,8 +136,7 @@ check_decode_caching(void *drcontext, bool use_module_mapper, bool include_decod
     }
 
     if (persist_decoded_instr) {
-        // This test mode needs the decoded instr_t.
-        assert(include_decoded_instr);
+        CHECK(include_decoded_instr, "persist_decoded_instr needs the decoded instr_t");
         // 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_decoded_instr = false (see

clients/drcachesim/tests/opcode_mix_test.cpp

@@ -65,7 +65,10 @@ class test_opcode_mix_t : public opcode_mix_t {
     make_decode_cache(shard_data_t *shard, void *dcontext) override
     {
         shard->decode_cache = std::unique_ptr<decode_cache_t<opcode_data_t>>(
-            new test_decode_cache_t<opcode_data_t>(dcontext, false, instrs_));
+            new test_decode_cache_t<opcode_data_t>(dcontext,
+                                                   /*include_decoded_instr=*/true,
+                                                   /*persist_decoded_instrs=*/false,
+                                                   instrs_));
     }
 
 private:

clients/drcachesim/tools/common/decode_cache.h

@@ -58,7 +58,8 @@ 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, decoded from raw bytes
-     * at the provided address in \p decode_pc which are valid only for this call.
+     * at the provided address in \p decode_pc (raw bytes address is valid only for
+     * this call).
      *
      * This invokes the set_decode_info_derived() provided by the derived class, and
      * additionally does other required bookkeeping.
@@ -93,10 +94,10 @@ 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, 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.
+     * address in \p decode_pc (raw bytes address is 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.
@@ -122,7 +123,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_instr_ set to true.
+ * \p include_decoded_instr_ and \p persist_decoded_instr_ set to true.
  */
 class instr_decode_info_t : public decode_info_base_t {
 public:
@@ -234,12 +235,23 @@ 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 #dynamorio::drmemtrace::decode_info_base_t which
  * implements the set_decode_info_derived() function that derives the required decode
- * info from an #instr_t object when invoked by #dynamorio::drmemtrace::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
+ * info from an #instr_t object and raw encoding bytes when invoked by
+ * #dynamorio::drmemtrace::decode_cache_t. This class handles the heavy lifting of
+ * actually producing the decoded #instr_t and managing the DecodeInfo cache (which
+ * includes invalidating stale DecodeInfo based on the encoding_is_new field in
+ * traces with embedded encodings).
+ *
+ * In general use, \p include_decoded_instr_ should be set to true, but may be set to
+ * false if the user wants to perform decoding themselves. In this case, the #instr_t
+ * provided to set_decode_info_derived() will be nullptr, and the #decode_cache_t
+ * merely acts as a cache and provider of the raw bytes.
+ *
+ * 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_instr_ set to true.
  *
+ * \p include_decoded_instr_ cannot be false if \p persist_decoded_instr_ is true.
+ *
  * Usage note: after constructing an object, init() must be called.
  */
 template <class DecodeInfo> class decode_cache_t : public decode_cache_base_t {
@@ -342,7 +354,7 @@ template <class DecodeInfo> class decode_cache_t : public decode_cache_base_t {
         }
         cached_decode_info = &info->second;
 
-        // Get address for the instr encoding.
+        // Get address for the instr encoding raw bytes.
         app_pc decode_pc;
         if (!use_module_mapper_) {
             decode_pc = const_cast<app_pc>(memref_instr.encoding);