Add --save-as param to record

`--save-as` specifies the trace dir name (basename?), not the full path. Merged `output_trace_dir` in RecordFlags with `name` into new type `TraceOutputPath`. Changed all usage of `output_trace_dir` string to use this type instead. This PR is backwards compatible, in the sense that the old usage of the `-o` flag remains the same, if `--save-as` is not provided, i.e., will error out, if dir exists etc. If `--save-as` is provided together with `-o` the new behavior will happen instead, where the output dir will be the "root dir", thus, the user can save many traces there. If only `--save-as` is provided, record uses normal behavior, of setting the trace root dir to $RR_TRACE_DIR (or it's user provided env var). Naming of user provided dirs, follows old behavior, i.e. appending -0, -1, -2 etc to the trace dir. I think this is preferable - if some automated thing is recording something with a specific name provided, this makes it so the end user don't have to manage the file system (i.e. checking if that name is "taken" and having to do clean up before recording, etc.) Added test cases
rr-debugger · Jul 20, 2024 · c841941 · c841941
1 parent 556551e
commit c841941
Show file tree

Hide file tree

Showing 8 changed files with 97 additions and 38 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -1730,6 +1730,7 @@ set(TESTS_WITHOUT_PROGRAM
   run_end
   run_in_function
   sanity
+  save_as
   seekticks
   shm_checkpoint
   siginfo

diff --git a/src/RecordCommand.cc b/src/RecordCommand.cc
@@ -61,6 +61,9 @@ RecordCommand RecordCommand::singleton(
     "                             _RR_TRACE_DIR gets ignored.\n"
     "                             Directory name is given name, not the\n"
     "                             application name.\n"
+    "  --save-as=<NAME>           Name of the new recording's directory. If\n"
+    "                             a recording with that name already exists, normal\n"
+    "                             number appending is applied."
     "  -p --print-trace-dir=<NUM> print trace directory followed by a newline\n"
     "                             to given file descriptor\n"
     "  --syscall-buffer-sig=<NUM> the signal used for communication with the\n"
@@ -132,7 +135,7 @@ struct RecordFlags {
 
   int print_trace_dir;
 
-  string output_trace_dir;
+  TraceOutputPath path;
 
   /* Whether to use file-cloning optimization during recording. */
   bool use_file_cloning;
@@ -195,7 +198,7 @@ struct RecordFlags {
         use_syscall_buffer(RecordSession::ENABLE_SYSCALL_BUF),
         syscall_buffer_size(0),
         print_trace_dir(-1),
-        output_trace_dir(""),
+        path{"", "", false, false},
         use_file_cloning(true),
         use_read_cloning(true),
         bind_cpu(BIND_CPU),
@@ -275,6 +278,7 @@ static bool parse_record_arg(vector<string>& args, RecordFlags& flags) {
     { 17, "asan", NO_PARAMETER },
     { 18, "tsan", NO_PARAMETER },
     { 19, "intel-pt", NO_PARAMETER },
+    { 20, "save-as", HAS_PARAMETER },
     { 'c', "num-cpu-ticks", HAS_PARAMETER },
     { 'h', "chaos", NO_PARAMETER },
     { 'i', "ignore-signal", HAS_PARAMETER },
@@ -286,6 +290,7 @@ static bool parse_record_arg(vector<string>& args, RecordFlags& flags) {
     { 'u', "cpu-unbound", NO_PARAMETER },
     { 'v', "env", HAS_PARAMETER },
     { 'w', "wait", NO_PARAMETER }};
+
   ParsedOption opt;
   auto args_copy = args;
   if (!Command::parse_option(args_copy, options, &opt)) {
@@ -320,7 +325,8 @@ static bool parse_record_arg(vector<string>& args, RecordFlags& flags) {
       flags.print_trace_dir = opt.int_value;
       break;
     case 'o':
-      flags.output_trace_dir = opt.value;
+      flags.path.output_trace_dir = opt.value;
+      flags.path.usr_provided_outdir = true;
       break;
     case 0:
       flags.use_read_cloning = false;
@@ -491,6 +497,10 @@ static bool parse_record_arg(vector<string>& args, RecordFlags& flags) {
     case 19:
       flags.intel_pt = true;
       break;
+    case 20:
+      flags.path.name = opt.value;
+      flags.path.usr_provided_name = true;
+      break;
     case 's':
       flags.always_switch = true;
       break;
@@ -674,11 +684,11 @@ static void* repeat_SIGTERM(__attribute__((unused)) void* p) {
 
 static WaitStatus record(const vector<string>& args, const RecordFlags& flags) {
   LOG(info) << "Start recording...";
-
+  DEBUG_ASSERT(!flags.path.name.empty() && !flags.path.output_trace_dir.empty() && "No output dir or trace dir name set");
   auto session = RecordSession::create(
-      args, flags.extra_env, flags.disable_cpuid_features,
+      args, flags.extra_env, flags.disable_cpuid_features, flags.path,
       flags.use_syscall_buffer, flags.syscallbuf_desched_sig,
-      flags.bind_cpu, flags.output_trace_dir,
+      flags.bind_cpu,
       flags.trace_id.get(),
       flags.stap_sdt, flags.unmap_vdso, flags.asan, flags.tsan,
       flags.intel_pt);
@@ -710,7 +720,7 @@ static WaitStatus record(const vector<string>& args, const RecordFlags& flags) {
     bool done_initial_exec = session->done_initial_exec();
     step_result = session->record_step();
     // Only create latest-trace symlink if --output-trace-dir is not being used
-    if (!done_initial_exec && session->done_initial_exec() && flags.output_trace_dir.empty()) {
+    if (!done_initial_exec && session->done_initial_exec() && !flags.path.usr_provided_outdir) {
       session->trace_writer().make_latest_trace();
     }
     if (term_requested) {
@@ -871,6 +881,16 @@ int RecordCommand::run(vector<string>& args) {
     flags.extra_env.push_back(padding);
   }
 
+  if(flags.path.name.empty()) {
+    flags.path.name = args[0];
+    flags.path.usr_provided_name = false;
+  }
+
+  if(flags.path.output_trace_dir.empty()) {
+    flags.path.usr_provided_outdir = false;
+    flags.path.output_trace_dir = trace_save_dir();
+  }
+
   WaitStatus status = record(args, flags);
 
   // Everything should have been cleaned up by now.

diff --git a/src/RecordSession.cc b/src/RecordSession.cc
@@ -2375,10 +2375,10 @@ static string lookup_by_path(const string& name) {
 /*static*/ RecordSession::shr_ptr RecordSession::create(
     const vector<string>& argv, const vector<string>& extra_env,
     const DisableCPUIDFeatures& disable_cpuid_features,
+    const TraceOutputPath& path_info,
     SyscallBuffering syscallbuf,
     unsigned char syscallbuf_desched_sig,
     BindCPU bind_cpu,
-    const string& output_trace_dir,
     const TraceUuid* trace_id,
     bool use_audit,
     bool unmap_vdso,
@@ -2514,7 +2514,7 @@ static string lookup_by_path(const string& name) {
   shr_ptr session(
       new RecordSession(full_path, argv, env, disable_cpuid_features,
                         syscallbuf, syscallbuf_desched_sig, bind_cpu,
-                        output_trace_dir, trace_id, use_audit, unmap_vdso,
+                        path_info, trace_id, use_audit, unmap_vdso,
                         intel_pt));
   session->excluded_ranges_ = std::move(exe_info.sanitizer_exclude_memory_ranges);
   session->fixed_global_exclusion_range_ = std::move(exe_info.fixed_global_exclusion_range);
@@ -2528,12 +2528,12 @@ RecordSession::RecordSession(const std::string& exe_path,
                              SyscallBuffering syscallbuf,
                              int syscallbuf_desched_sig,
                              BindCPU bind_cpu,
-                             const string& output_trace_dir,
+                             const TraceOutputPath& path,
                              const TraceUuid* trace_id,
                              bool use_audit,
                              bool unmap_vdso,
                              bool intel_pt_enabled)
-    : trace_out(argv[0], output_trace_dir, ticks_semantics_),
+    : trace_out(path, ticks_semantics_),
       scheduler_(*this),
       trace_id(trace_id),
       disable_cpuid_features_(disable_cpuid_features),

diff --git a/src/RecordSession.h b/src/RecordSession.h
@@ -11,6 +11,7 @@
 #include "Session.h"
 #include "ThreadGroup.h"
 #include "TraceFrame.h"
+#include "TraceStream.h"
 #include "WaitStatus.h"
 
 namespace rr {
@@ -63,10 +64,10 @@ class RecordSession final : public Session {
       const std::vector<std::string>& argv,
       const std::vector<std::string>& extra_env,
       const DisableCPUIDFeatures& features,
+      const TraceOutputPath& path_info,
       SyscallBuffering syscallbuf = ENABLE_SYSCALL_BUF,
       unsigned char syscallbuf_desched_sig = SIGPWR,
       BindCPU bind_cpu = BIND_CPU,
-      const std::string& output_trace_dir = "",
       const TraceUuid* trace_id = nullptr,
       bool use_audit = false,
       bool unmap_vdso = false,
@@ -217,7 +218,7 @@ class RecordSession final : public Session {
                 SyscallBuffering syscallbuf,
                 int syscallbuf_desched_sig,
                 BindCPU bind_cpu,
-                const std::string& output_trace_dir,
+                const TraceOutputPath& path_info,
                 const TraceUuid* trace_id,
                 bool use_audit,
                 bool unmap_vdso,
@@ -282,8 +283,6 @@ class RecordSession final : public Session {
    */
   std::map<pid_t, RecordTask*> detached_task_map;
 
-  std::string output_trace_dir;
-
   bool use_audit_;
   bool unmap_vdso_;
 };

diff --git a/src/TraceStream.cc b/src/TraceStream.cc
@@ -1309,31 +1309,16 @@ bool TraceReader::read_raw_data_metadata_for_frame(RawDataMetadata& d) {
   return true;
 }
 
-static string make_trace_dir(const string& exe_path, const string& output_trace_dir) {
-  if (!output_trace_dir.empty()) {
-    // save trace dir in given output trace dir with option -o
-    int ret = mkdir(output_trace_dir.c_str(), S_IRWXU | S_IRWXG);
-    if (ret == 0) {
-      return output_trace_dir;
-    }
-    if (EEXIST == errno) {
-      CLEAN_FATAL() << "Trace directory `" << output_trace_dir << "' already exists.";
-    } else if (EACCES == errno) {
-      CLEAN_FATAL() << "Permission denied to create trace directory `" << output_trace_dir << "'";
-    } else {
-      FATAL() << "Unable to create trace directory `" << output_trace_dir << "'";
-    }
-  } else {
-    // save trace dir set in _RR_TRACE_DIR or in the default trace dir
-    ensure_dir(trace_save_dir(), "trace directory", S_IRWXU);
+static string create_unique_dir(const TraceOutputPath& path) {
+    ensure_dir(path.output_trace_dir, "trace directory", S_IRWXU);
 
     // Find a unique trace directory name.
     int nonce = 0;
     int ret;
     string dir;
     do {
       stringstream ss;
-      ss << trace_save_dir() << "/" << basename(exe_path.c_str()) << "-"
+      ss << path.output_trace_dir << "/" << basename(path.name.c_str()) << "-"
          << nonce++;
       dir = ss.str();
       ret = mkdir(dir.c_str(), S_IRWXU | S_IRWXG);
@@ -1344,6 +1329,28 @@ static string make_trace_dir(const string& exe_path, const string& output_trace_
     }
 
     return dir;
+}
+
+static string make_trace_dir(const TraceOutputPath& path) {
+  if (path.usr_provided_outdir) {
+    // save trace dir in given output trace dir with option -o
+    int ret = mkdir(path.output_trace_dir.c_str(), S_IRWXU | S_IRWXG);
+    if(path.usr_provided_name) {
+      return create_unique_dir(path);
+    }
+    if (ret == 0) {
+      return path.output_trace_dir;
+    }
+    if (EEXIST == errno) {
+      CLEAN_FATAL() << "Trace directory `" << path.output_trace_dir << "' already exists.";
+    } else if (EACCES == errno) {
+      CLEAN_FATAL() << "Permission denied to create trace directory `" << path.output_trace_dir << "'";
+    } else {
+      FATAL() << "Unable to create trace directory `" << path.output_trace_dir << "'";
+    }
+  } else {
+    // save trace dir set in _RR_TRACE_DIR or in the default trace dir
+    return create_unique_dir(path);
   }
 
   return ""; // not reached
@@ -1352,10 +1359,9 @@ static string make_trace_dir(const string& exe_path, const string& output_trace_
 #define STR_HELPER(x) #x
 #define STR(x) STR_HELPER(x)
 
-TraceWriter::TraceWriter(const std::string& file_name,
-                         const string& output_trace_dir,
+TraceWriter::TraceWriter(const TraceOutputPath& trace_output,
                          TicksSemantics ticks_semantics_)
-    : TraceStream(make_trace_dir(file_name, output_trace_dir),
+    : TraceStream(make_trace_dir(trace_output),
                   // Somewhat arbitrarily start the
                   // global time from 1.
                   1),

diff --git a/src/TraceStream.h b/src/TraceStream.h
@@ -19,6 +19,7 @@
 #include "TraceFrame.h"
 #include "TraceTaskEvent.h"
 #include "remote_ptr.h"
+#include "util.h"
 
 namespace rr {
 
@@ -249,8 +250,7 @@ class TraceWriter : public TraceStream {
    * The trace name is determined by |file_name| and _RR_TRACE_DIR (if set)
    * or by setting -o=<OUTPUT_TRACE_DIR>.
    */
-  TraceWriter(const std::string& file_name,
-              const string& output_trace_dir, TicksSemantics ticks_semantics);
+  TraceWriter(const TraceOutputPath& path, TicksSemantics ticks_semantics);
 
   /**
    * Called after the calling thread is actually bound to |bind_to_cpu|.

diff --git a/src/test/save_as.run b/src/test/save_as.run
@@ -0,0 +1,27 @@
+source `dirname $0`/util.sh
+
+function dir_exists { checkDir=$1;
+  if [ -d "$checkDir" ]; then
+    echo "$checkDir" exists
+  else
+    failed "$checkDir doesn't exist"
+    exit 1
+  fi
+}
+
+RECORD_ARGS="--save-as test-name"
+record hello
+dir_exists "$workdir/test-name-0"
+
+record hello
+dir_exists "$workdir/test-name-1"
+
+
+# Check that --save-as plays nice with -o <trace dir>
+RECORD_ARGS="-o $workdir/arbitrary-dir --save-as test-name"
+record hello
+dir_exists "$workdir/arbitrary-dir/test-name-0"
+record hello
+dir_exists "$workdir/arbitrary-dir/test-name-1"
+
+passed_msg "--save-as and --save-as -o combination succeeded"
diff --git a/src/util.h b/src/util.h
@@ -663,6 +663,12 @@ void replace_in_buffer(MemoryRange src, const uint8_t* src_data,
 
 // Strip any directory part from the filename `s`
 void base_name(std::string& s);
+struct TraceOutputPath {
+  std::string output_trace_dir;
+  std::string name;
+  bool usr_provided_outdir;
+  bool usr_provided_name;
+};
 
 } // namespace rr