Review and offline discussion changes: turn a mix of preferred into a…

…n error; plus additional comments and readability tweaks
DynamoRIO · Oct 17, 2024 · 1ea126a · 1ea126a
1 parent 204f2f7
commit 1ea126a
Show file tree

Hide file tree

Showing 5 changed files with 43 additions and 31 deletions.
diff --git a/clients/drcachesim/analysis_tool.h b/clients/drcachesim/analysis_tool.h
@@ -157,9 +157,12 @@ template <typename RecordType> class analysis_tool_tmpl_t {
         return "";
     }
     /**
-     * Identifies the preferred shard type for this analysis.  If every tool requests
-     * #SHARD_BY_CORE, the framework may decided to use that mode even if the user
-     * left the default thread-sharded mode in place.
+     * Identifies the preferred shard type for this analysis.  This only applies when
+     * the user does not specify a shard type for a run.  In that case, if every tool
+     * being run prefers #SHARD_BY_CORE, the framework uses that mode.  If tools
+     * disagree then an error is raised.  This is ignored if the user specifies a
+     * shard type via one of -core_sharded, -core_serial, -no_core_sharded,
+     * -no_core_serial, or -cpu_scheduling.
      */
     virtual shard_type_t
     preferred_shard_type()

diff --git a/clients/drcachesim/analyzer.cpp b/clients/drcachesim/analyzer.cpp
@@ -342,6 +342,8 @@ analyzer_tmpl_t<RecordType, ReaderType>::init_scheduler_common(
                 if (i == 0 && shard_type_ == SHARD_BY_CORE) {
                     // This is almost certainly user error.
                     // Better to exit than risk user confusion.
+                    // Ideally this could be detected in analyzer_multi but that is
+                    // not simple so today the user has to re-run.
                     error_string_ =
                         "Re-scheduling a core-sharded-on-disk trace is generally a "
                         "mistake; re-run with -no_core_sharded.\n";

diff --git a/clients/drcachesim/analyzer_multi.cpp b/clients/drcachesim/analyzer_multi.cpp
@@ -461,6 +461,7 @@ analyzer_multi_tmpl_t<RecordType, ReaderType>::analyzer_multi_tmpl_t()
             if (!error.empty()) {
                 this->success_ = false;
                 this->error_string_ = "raw2trace failed: " + error;
+                return;
             }
         }
     }
@@ -472,27 +473,29 @@ analyzer_multi_tmpl_t<RecordType, ReaderType>::analyzer_multi_tmpl_t()
         return;
     }
 
-    bool offline = !op_indir.get_value().empty() || !op_infile.get_value().empty();
+    bool sharding_specified = op_core_sharded.specified() || op_core_serial.specified() ||
+        // -cpu_scheduling implies thread-sharded.
+        op_cpu_scheduling.get_value();
     // TODO i#7040: Add core-sharded support for online tools.
-    if (offline && !op_core_sharded.specified() && !op_core_serial.specified() &&
-        !op_cpu_scheduling.get_value()) {
-        bool switch_to_core_sharded = true;
-        bool one_prefers_core_sharded = false;
+    bool offline = !op_indir.get_value().empty() || !op_infile.get_value().empty();
+    if (offline && !sharding_specified) {
+        bool all_prefer_thread_sharded = true;
+        bool all_prefer_core_sharded = true;
         for (int i = 0; i < this->num_tools_; ++i) {
-            if (this->tools_[i]->preferred_shard_type() == SHARD_BY_CORE) {
-                one_prefers_core_sharded = true;
-            } else {
-                switch_to_core_sharded = false;
-                break;
+            if (this->tools_[i]->preferred_shard_type() == SHARD_BY_THREAD) {
+                all_prefer_core_sharded = false;
+            } else if (this->tools_[i]->preferred_shard_type() == SHARD_BY_CORE) {
+                all_prefer_thread_sharded = false;
             }
             if (this->parallel_ && !this->tools_[i]->parallel_shard_supported()) {
                 this->parallel_ = false;
             }
         }
-        if (switch_to_core_sharded) {
+        if (all_prefer_core_sharded) {
             // XXX i#6949: Ideally we could detect a core-sharded-on-disk input
-            // here and avoid this but that's not simple so we rely on the user
-            // to pass -no_core_sharded for such inputs.
+            // here and avoid this but that's not simple so currently we have a
+            // fatal error from the analyzer and the user must re-run with
+            // -no_core_sharded for such inputs.
             if (this->parallel_) {
                 if (op_verbose.get_value() > 0)
                     fprintf(stderr, "Enabling -core_sharded as all tools prefer it\n");
@@ -502,12 +505,11 @@ analyzer_multi_tmpl_t<RecordType, ReaderType>::analyzer_multi_tmpl_t()
                     fprintf(stderr, "Enabling -core_serial as all tools prefer it\n");
                 op_core_serial.set_value(true);
             }
-        } else if (one_prefers_core_sharded) {
-            if (op_verbose.get_value() > 0) {
-                fprintf(stderr,
-                        "Some tool(s) prefer core-sharded: consider re-running with "
-                        "-core_sharded or -core_serial enabled for best results.\n");
-            }
+        } else if (!all_prefer_thread_sharded) {
+            this->success_ = false;
+            this->error_string_ = "Selected tools differ in preferred sharding: please "
+                                  "re-run with -[no_]core_sharded or -[no_]core_serial";
+            return;
         }
     }
 

diff --git a/clients/drcachesim/scheduler/scheduler.cpp b/clients/drcachesim/scheduler/scheduler.cpp
@@ -3244,9 +3244,14 @@ scheduler_tmpl_t<RecordType, ReaderType>::pick_next_input(output_ordinal_t outpu
                                                           uint64_t blocked_time)
 {
     VDO(this, 1, {
-        static int global_heartbeat;
+        static int64_t global_heartbeat;
+        // 10K is too frequent for simple analyzer runs: it is too noisy with
+        // the new core-sharded-by-default for new users using defaults.
+        // 50K is a reasonable compromise.
+        // XXX: Add a runtime option to tweak this.
+        static constexpr int64_t GLOBAL_HEARTBEAT_CADENCE = 50000;
         // We are ok with races as the cadence is approximate.
-        if (++global_heartbeat % 50000 == 0) {
+        if (++global_heartbeat % GLOBAL_HEARTBEAT_CADENCE == 0) {
             print_queue_stats();
         }
     });

diff --git a/suite/tests/CMakeLists.txt b/suite/tests/CMakeLists.txt
@@ -2132,7 +2132,7 @@ if (AARCH64 AND UNIX AND ZLIB_FOUND)
     torunonly_api(tool.drcacheoff.tlb_simulator_v2p "${drcachesim_path}"
       "offline-tlb_simulator_v2p" ""
       # Do not use core-sharded scheduling, so we'll have a deterministic result.
-      "-indir;${locdir};-tool;TLB;-alt_module_dir;${srcdir};-module_file;${locdir}/raw/modules.log;-v2p_file;${srcdir}/v2p.textproto;-use_physical;-no_core_serial"
+      "-indir;${locdir};-tool;TLB;-alt_module_dir;${srcdir};-module_file;${locdir}/raw/modules.log;-v2p_file;${srcdir}/v2p.textproto;-use_physical;-no_core_sharded"
       OFF OFF)
     set(tool.drcacheoff.tlb_simulator_v2p_basedir
       "${PROJECT_SOURCE_DIR}/clients/drcachesim/tests")
@@ -3973,7 +3973,6 @@ if (BUILD_CLIENTS)
         set(core_sharded_dir
           "${PROJECT_SOURCE_DIR}/clients/drcachesim/tests/drmemtrace.threadsig-core-sharded.x64.tracedir")
         torunonly_simtool(core_on_disk ${ci_shared_app}
-          # Avoid the default core-sharded from re-scheduling the trace.
           "-indir ${core_sharded_dir} -tool basic_counts" "")
         set(tool.core_on_disk_rawtemp ON) # no preprocessor
 
@@ -4107,7 +4106,7 @@ if (BUILD_CLIENTS)
     # Test reading a legacy pre-interleaved file in thread-sharded mode.
     if (ZLIB_FOUND)
       torunonly_api(tool.drcacheoff.legacy "${drcachesim_path}" "offline-legacy.c" ""
-        "-infile;${PROJECT_SOURCE_DIR}/clients/drcachesim/tests/offline-legacy-trace.gz;-no_core_serial"
+        "-infile;${PROJECT_SOURCE_DIR}/clients/drcachesim/tests/offline-legacy-trace.gz;-no_core_sharded"
         OFF OFF)
       set(tool.drcacheoff.legacy_basedir
         "${PROJECT_SOURCE_DIR}/clients/drcachesim/tests")
@@ -4152,7 +4151,7 @@ if (BUILD_CLIENTS)
 
       # With a legacy serial tool (full simulator) in thread-sharded mode.
       torunonly_api(tool.drcacheoff.snappy_serial "${drcachesim_path}" "offline-snappy-serial.c" ""
-        "-indir;${PROJECT_SOURCE_DIR}/clients/drcachesim/tests/drmemtrace.chase-snappy.x64.tracedir;-no_core_serial"
+        "-indir;${PROJECT_SOURCE_DIR}/clients/drcachesim/tests/drmemtrace.chase-snappy.x64.tracedir;-no_core_sharded"
         OFF OFF)
       set(tool.drcacheoff.snappy_serial_basedir
         "${PROJECT_SOURCE_DIR}/clients/drcachesim/tests")
@@ -4449,15 +4448,16 @@ if (BUILD_CLIENTS)
         # Run with -trace_after_instrs to ensure we test the
         # drbbdup + rseq combo (i#5658, i#5659).
         "-trace_after_instrs 5K"
-        "@${test_mode_flag}@-test_mode_name@rseq_app" "")
+        # Run thread-sharded for the invariant checker.
+        "@${test_mode_flag}@-test_mode_name@rseq_app@-no_core_sharded" "")
       # Test filtering.
       torunonly_drcacheoff(rseq-filter linux.rseq
         "-trace_after_instrs 5K -L0_filter"
-        "@${test_mode_flag}@-test_mode_name@rseq_app" "")
+        "@${test_mode_flag}@-test_mode_name@rseq_app@-no_core_sharded" "")
       set(tool.drcacheoff.rseq-filter_expectbase "offline-rseq")
       torunonly_drcacheoff(rseq-dfilter linux.rseq
         "-trace_after_instrs 5K -L0D_filter"
-        "@${test_mode_flag}@-test_mode_name@rseq_app" "")
+        "@${test_mode_flag}@-test_mode_name@rseq_app@-no_core_sharded" "")
       set(tool.drcacheoff.rseq-dfilter_expectbase "offline-rseq")
     endif ()