i#6662 encodings2regdeps: data race bug fix (#6809)

Fixes a data race due to multiple dr_standalone_init() done in parallel
(per shard) by encodings2regdeps_filter_t.
dcontext is now initialized one time by record_filter_t and passed to
its filters through the record_filter_info_t interface.

Avoids a data race in opcode_mix where all threads set the dcontext
isa_mode to DR_ISA_REGDEPS for regdeps input traces.

Issue #6662 #6812

clients/drcachesim/tests/record_filter-offline.templatex

@@ -5,6 +5,9 @@ Exiting process after .* references.
 Hello, world!
 #endif
 Trace invariant checks passed
+#ifndef WINDOWS
+<unable to determine lib path for cross-arch execve>
+#endif
 Output .* entries from .* entries.
 Done!
 Trace invariant checks passed

clients/drcachesim/tests/record_filter_unit_tests.cpp

@@ -210,6 +210,7 @@ bool
 process_entries_and_check_result(test_record_filter_t *record_filter,
                                  const std::vector<test_case_t> &entries, int index)
 {
+    record_filter->initialize_stream(nullptr);
     auto stream = std::unique_ptr<local_stream_t>(new local_stream_t());
     void *shard_data =
         record_filter->parallel_shard_init_stream(0, nullptr, stream.get());

clients/drcachesim/tools/filter/encodings2regdeps_filter.h

@@ -67,7 +67,6 @@ class encodings2regdeps_filter_t : public record_filter_t::record_filter_func_t
     parallel_shard_init(memtrace_stream_t *shard_stream,
                         bool partial_trace_filter) override
     {
-        dcontext_.dcontext = dr_standalone_init();
         return nullptr;
     }
 
@@ -77,6 +76,7 @@ class encodings2regdeps_filter_t : public record_filter_t::record_filter_func_t
         record_filter_t::record_filter_info_t &record_filter_info) override
     {
         std::vector<trace_entry_t> *last_encoding = record_filter_info.last_encoding;
+        void *dcontext = record_filter_info.dcontext;
 
         /* Modify file_type to regdeps ISA, removing the real ISA of the input trace.
          */
@@ -117,10 +117,10 @@ class encodings2regdeps_filter_t : public record_filter_t::record_filter_func_t
             /* Genenerate the real ISA instr_t by decoding the encoding bytes.
              */
             instr_t instr;
-            instr_init(dcontext_.dcontext, &instr);
-            app_pc next_pc = decode_from_copy(dcontext_.dcontext, encoding, pc, &instr);
+            instr_init(dcontext, &instr);
+            app_pc next_pc = decode_from_copy(dcontext, encoding, pc, &instr);
             if (next_pc == NULL || !instr_valid(&instr)) {
-                instr_free(dcontext_.dcontext, &instr);
+                instr_free(dcontext, &instr);
                 error_string_ =
                     "Failed to decode instruction " + to_hex_string(entry.addr);
                 return false;
@@ -129,18 +129,18 @@ class encodings2regdeps_filter_t : public record_filter_t::record_filter_func_t
             /* Convert the real ISA instr_t into a regdeps ISA instr_t.
              */
             instr_t instr_regdeps;
-            instr_init(dcontext_.dcontext, &instr_regdeps);
-            instr_convert_to_isa_regdeps(dcontext_.dcontext, &instr, &instr_regdeps);
-            instr_free(dcontext_.dcontext, &instr);
+            instr_init(dcontext, &instr_regdeps);
+            instr_convert_to_isa_regdeps(dcontext, &instr, &instr_regdeps);
+            instr_free(dcontext, &instr);
 
             /* Obtain regdeps ISA instr_t encoding bytes.
              */
             byte ALIGN_VAR(REGDEPS_ALIGN_BYTES)
                 encoding_regdeps[REGDEPS_MAX_ENCODING_LENGTH];
             memset(encoding_regdeps, 0, sizeof(encoding_regdeps));
             app_pc next_pc_regdeps =
-                instr_encode(dcontext_.dcontext, &instr_regdeps, encoding_regdeps);
-            instr_free(dcontext_.dcontext, &instr_regdeps);
+                instr_encode(dcontext, &instr_regdeps, encoding_regdeps);
+            instr_free(dcontext, &instr_regdeps);
             if (next_pc_regdeps == NULL) {
                 error_string_ =
                     "Failed to encode regdeps instruction " + to_hex_string(entry.addr);
@@ -190,19 +190,6 @@ class encodings2regdeps_filter_t : public record_filter_t::record_filter_func_t
         filetype |= OFFLINE_FILE_TYPE_ARCH_REGDEPS;
         return filetype;
     }
-
-private:
-    struct dcontext_cleanup_last_t {
-    public:
-        ~dcontext_cleanup_last_t()
-        {
-            if (dcontext != nullptr)
-                dr_standalone_exit();
-        }
-        void *dcontext = nullptr;
-    };
-
-    dcontext_cleanup_last_t dcontext_;
 };
 
 } // namespace drmemtrace

clients/drcachesim/tools/filter/record_filter.cpp

@@ -347,6 +347,13 @@ record_filter_t::open_new_chunk(per_shard_t *shard)
     return "";
 }
 
+std::string
+record_filter_t::initialize_stream(memtrace_stream_t *serial_stream)
+{
+    dcontext_.dcontext = dr_standalone_init();
+    return "";
+}
+
 void *
 record_filter_t::parallel_shard_init_stream(int shard_index, void *worker_data,
                                             memtrace_stream_t *shard_stream)
@@ -387,6 +394,7 @@ record_filter_t::parallel_shard_init_stream(int shard_index, void *worker_data,
         }
     }
     per_shard->record_filter_info.last_encoding = &per_shard->last_encoding;
+    per_shard->record_filter_info.dcontext = dcontext_.dcontext;
     std::lock_guard<std::mutex> guard(shard_map_mutex_);
     shard_map_[shard_index] = per_shard;
     return reinterpret_cast<void *>(per_shard);

clients/drcachesim/tools/filter/record_filter.h

@@ -70,6 +70,17 @@ class record_filter_t : public record_analysis_tool_t {
          * #trace_entry_t, hence the vector.
          */
         std::vector<trace_entry_t> *last_encoding;
+
+        /**
+         * Gives filters access to dcontext_t.
+         * Note that dcontext_t is not entirely thread-safe. AArch32 encoding and
+         * decoding is problematic as the global encode_state_t and decode_state_t are
+         * used for GLOBAL_DCONTEXT. Furthermore, modifying the ISA mode can lead to data
+         * races.
+         */
+        /* xref i#6690 i#1595: multi-dcontext_t solution.
+         */
+        void *dcontext;
     };
 
     /**
@@ -148,6 +159,8 @@ class record_filter_t : public record_analysis_tool_t {
                     std::vector<std::unique_ptr<record_filter_func_t>> filters,
                     uint64_t stop_timestamp, unsigned int verbose);
     ~record_filter_t() override;
+    std::string
+    initialize_stream(memtrace_stream_t *serial_stream) override;
     bool
     process_memref(const trace_entry_t &entry) override;
     bool
@@ -167,6 +180,16 @@ class record_filter_t : public record_analysis_tool_t {
     parallel_shard_error(void *shard_data) override;
 
 protected:
+    struct dcontext_cleanup_last_t {
+    public:
+        ~dcontext_cleanup_last_t()
+        {
+            if (dcontext != nullptr)
+                dr_standalone_exit();
+        }
+        void *dcontext = nullptr;
+    };
+
     // For core-sharded we need to remember encodings for an input that were
     // seen on a different core, as there is no reader_t remembering them for us.
     // XXX i#6635: Is this something the scheduler should help us with?
@@ -239,6 +262,8 @@ class record_filter_t : public record_analysis_tool_t {
     virtual std::string
     get_output_basename(memtrace_stream_t *shard_stream);
 
+    dcontext_cleanup_last_t dcontext_;
+
     std::unordered_map<int, per_shard_t *> shard_map_;
     // This mutex is only needed in parallel_shard_init. In all other accesses
     // to shard_map (print_results) we are single-threaded.

clients/drcachesim/tools/opcode_mix.cpp

@@ -172,8 +172,16 @@ opcode_mix_t::parallel_shard_memref(void *shard_data, const memref_t &memref)
         /* 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, memref.marker.marker_value))
-            dr_set_isa_mode(dcontext_.dcontext, DR_ISA_REGDEPS, nullptr);
+        if (TESTANY(OFFLINE_FILE_TYPE_ARCH_REGDEPS, memref.marker.marker_value)) {
+            /* 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_.dcontext);
+            if (isa_mode != DR_ISA_REGDEPS)
+                dr_set_isa_mode(dcontext_.dcontext, DR_ISA_REGDEPS, nullptr);
+        }
     } else if (memref.marker.type == TRACE_TYPE_MARKER &&
                memref.marker.marker_type == TRACE_MARKER_TYPE_VECTOR_LENGTH) {
 #ifdef AARCH64
@@ -190,6 +198,26 @@ opcode_mix_t::parallel_shard_memref(void *shard_data, const memref_t &memref)
         memref.data.type != TRACE_TYPE_INSTR_NO_FETCH) {
         return true;
     }
+
+    /* At this point we start processing memref instructions, so we're past the header of
+     * the trace, which contains the trace filetype. If we didn't encounter a
+     * TRACE_MARKER_TYPE_FILETYPE we return an error and stop processing the trace.
+     * We expected the value of TRACE_MARKER_TYPE_FILETYPE to contain at least one of the
+     * enum values of offline_file_type_t (e.g., OFFLINE_FILE_TYPE_ARCH_), so
+     * OFFLINE_FILE_TYPE_DEFAULT (== 0) should never be present alone and can be used as
+     * uninitialized value of filetype for an error check.
+     * XXX i#6812: we could allow traces that have some shards with no filetype, as long
+     * as there is at least one shard with it, by caching the filetype from shards that
+     * have it and using that one. We can do this using memtrace_stream_t::get_filetype(),
+     * which requires updating opcode_mix to use parallel_shard_init_stream() rather than
+     * the current (and deprecated) parallel_shard_init(). However, we should first decide
+     * whether we want to allow traces that have some shards without a filetype.
+     */
+    if (shard->filetype == OFFLINE_FILE_TYPE_DEFAULT) {
+        shard->error = "No file type found in this shard";
+        return false;
+    }
+
     ++shard->instr_count;
 
     app_pc decode_pc;

clients/drcachesim/tools/opcode_mix.h

@@ -199,6 +199,8 @@ class opcode_mix_t : public analysis_tool_t {
     // For serial operation.
     worker_data_t serial_worker_;
     shard_data_t serial_shard_;
+    // To guard the setting of isa_mode in dcontext.
+    std::mutex dcontext_mutex_;
 };
 
 } // namespace drmemtrace