From f0204b6f921a401a5db06bc1d0721b7275d87a69 Mon Sep 17 00:00:00 2001 From: Simon Farre Date: Tue, 15 Aug 2023 16:36:40 +0200 Subject: [PATCH 1/2] 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 --- CMakeLists.txt | 1 + src/RecordCommand.cc | 34 ++++++++++++++++++++++++------- src/RecordSession.cc | 8 ++++---- src/RecordSession.h | 7 +++---- src/TraceStream.cc | 48 +++++++++++++++++++++++++------------------- src/TraceStream.h | 4 ++-- src/test/save_as.run | 27 +++++++++++++++++++++++++ src/util.h | 6 ++++++ 8 files changed, 97 insertions(+), 38 deletions(-) create mode 100644 src/test/save_as.run diff --git a/CMakeLists.txt b/CMakeLists.txt index 1c3a4780745..3fd57588028 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1751,6 +1751,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 index adcda9d8ca6..aa0fd423cf3 100644 --- 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 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= print trace directory followed by a newline\n" " to given file descriptor\n" " --syscall-buffer-sig= the signal used for communication with the\n" @@ -134,7 +137,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; @@ -200,7 +203,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), @@ -282,6 +285,7 @@ static bool parse_record_arg(vector& args, RecordFlags& flags) { { 18, "tsan", NO_PARAMETER }, { 19, "intel-pt", NO_PARAMETER }, { 20, "check-outside-mmaps", NO_PARAMETER }, + { 21, "save-as", HAS_PARAMETER }, { 'c', "num-cpu-ticks", HAS_PARAMETER }, { 'h', "chaos", NO_PARAMETER }, { 'i', "ignore-signal", HAS_PARAMETER }, @@ -293,6 +297,7 @@ static bool parse_record_arg(vector& 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)) { @@ -327,7 +332,8 @@ static bool parse_record_arg(vector& 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; @@ -501,6 +507,10 @@ static bool parse_record_arg(vector& args, RecordFlags& flags) { case 20: flags.check_outside_mmaps = true; break; + case 21: + flags.path.name = opt.value; + flags.path.usr_provided_name = true; + break; case 's': flags.always_switch = true; break; @@ -684,11 +694,11 @@ static void* repeat_SIGTERM(__attribute__((unused)) void* p) { static WaitStatus record(const vector& 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, flags.check_outside_mmaps); @@ -720,7 +730,7 @@ static WaitStatus record(const vector& 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) { @@ -881,6 +891,16 @@ int RecordCommand::run(vector& 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 index 4990247913d..5bb234cbdfc 100644 --- a/src/RecordSession.cc +++ b/src/RecordSession.cc @@ -2383,10 +2383,10 @@ static string lookup_by_path(const string& name) { /*static*/ RecordSession::shr_ptr RecordSession::create( const vector& argv, const vector& 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, @@ -2500,7 +2500,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, check_outside_mmaps)); session->excluded_ranges_ = std::move(exe_info.sanitizer_exclude_memory_ranges); session->fixed_global_exclusion_range_ = std::move(exe_info.fixed_global_exclusion_range); @@ -2514,13 +2514,13 @@ 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, bool check_outside_mmaps) - : 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 index de150d88c25..23c7fb7c536 100644 --- 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& argv, const std::vector& 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, @@ -219,7 +220,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, @@ -285,8 +286,6 @@ class RecordSession final : public Session { */ std::map detached_task_map; - std::string output_trace_dir; - bool use_audit_; bool unmap_vdso_; bool check_outside_mmaps_; diff --git a/src/TraceStream.cc b/src/TraceStream.cc index c9983c99ca9..cee2ba4fa81 100644 --- a/src/TraceStream.cc +++ b/src/TraceStream.cc @@ -1314,23 +1314,8 @@ 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; @@ -1338,7 +1323,7 @@ static string make_trace_dir(const string& exe_path, const string& output_trace_ 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); @@ -1349,6 +1334,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 @@ -1357,10 +1364,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 index 7dfde3f12ed..2bf5c2c60f8 100644 --- 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 { @@ -262,8 +263,7 @@ class TraceWriter : public TraceStream { * The trace name is determined by |file_name| and _RR_TRACE_DIR (if set) * or by setting -o=. */ - 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 new file mode 100644 index 00000000000..fcb76f35011 --- /dev/null +++ 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 +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" \ No newline at end of file diff --git a/src/util.h b/src/util.h index 4a4c1638c56..f03b77c0c0d 100644 --- a/src/util.h +++ b/src/util.h @@ -696,6 +696,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; +}; std::optional read_perf_event_paranoid(); From 535762be549123419376ca9c73852b459bbd43de Mon Sep 17 00:00:00 2001 From: Simon Farre Date: Tue, 19 Nov 2024 15:58:35 +0100 Subject: [PATCH 2/2] Documented fields --- src/util.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/util.h b/src/util.h index f03b77c0c0d..8125bddc910 100644 --- a/src/util.h +++ b/src/util.h @@ -697,8 +697,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 { + // The root trace directory, where recorded traces will be saved std::string output_trace_dir; + // The name of the recording that's currently being created std::string name; + // Flags that are set, to signal if user requested different + // trace dirs or different names of the recordings bool usr_provided_outdir; bool usr_provided_name; };