Skip to content

Conversation

theIDinside
Copy link
Contributor

This is a resend of PR ##3576 because I messed that one up @GitMensch || @rocallahan

This PR is rebased onto master and also added test cases for the feature.

--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

// Strip any directory part from the filename `s`
void base_name(std::string& s);
struct TraceOutputPath {
std::string output_trace_dir;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please carefully document these fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See new comments added to the type - are they enough?

@GitMensch
Copy link
Contributor

@theIDinside can you please debug the new elements in util.h as requested and rebase, so that we may see that be integrated?

`--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
@theIDinside
Copy link
Contributor Author

@theIDinside can you please debug the new elements in util.h as requested and rebase, so that we may see that be integrated?

Rebased it and added documentation to the fields

@rocallahan
Copy link
Collaborator

Under what circumstances would someone want to use this feature?

@GitMensch
Copy link
Contributor

Under what circumstances would someone want to use this feature?

currently the default name is derived from the binary name; with --save-as we can make this specific, in my scenario there's a single binary which loads different programs - and those are the relevant ones which can now be named easily.

@rocallahan
Copy link
Collaborator

I think the behaviour implemented in this PR is confusing. Currently the setup is relatively simple: you can override the default trace base directory (where new traces are added with their default names) with _RR_TRACE_DIR, or you can specify a full trace directory including its trace name with -o, which means _RR_TRACE_DIR is ignored. As I understand it, this PR intends to add an option to override just the trace name, using the normal trace base directory (possibly overridden by _RR_TRACE_DIR). But I think that is fundamentally incompatible with using -o. I don't understand why you want to make this work with -o.

Note that you can get the effect of this PR by creating a symbolic link to the binary to record with the desired name.

@GitMensch
Copy link
Contributor

I think the following solution would work, but think something is missing:

  • -o as a (relative or absolute) path (= ending with a slash or pointing to an ISDIR) -> specifies the path (instead of _RR_TRACE_DIR), name is derived from the binary
  • -o as a (relative or absolute) path (= not ending with a slash or pointing to an !ISDIR) -> specifies the path (instead of _RR_TRACE_DIR)
  • -o as a simple name (no slash included) uses the default place / _RR_TRACE_DIR

Ideally, in each of those cases the "number suffix" is only done as a replacement (like !! or !n) with possibly more replacements (like !p for the process id under trace, if this is possible).

@theIDinside
Copy link
Contributor Author

I haven't looked at this in a long time @rocallahan but, what what I take from your last comment, is that this feature would better be served by just having the --save-as function on the trace name and instead, if the user wants to override a root-directory, do so with the _RR_TRACE_DIR?

If that's what you're saying, I think I can agree, as it reduces the combinatorial effect and instead just have a --save-as and if the user wants it to be stored somewhere else, s/he will have to override it by doing _RR_TRACE_DIR=/foo/bar?

I have a vague memory of their being an issue with root dirs and whatnot, but I may be remembering wrong, if so, I'll let you know.

@rocallahan
Copy link
Collaborator

what I take from your last comment, is that this feature would better be served by just having the --save-as function on the trace name and instead, if the user wants to override a root-directory, do so with the _RR_TRACE_DIR?

Yes.

Also, if the user specifies -o then --save-as should be ignored.

@epasveer
Copy link

As a user, I find specifying _RR_TRACE_DIR in the environment each time I debug a different program inconvenient. I like to have the trace directory is the same folder as the program I'm debugging. Not at some global folder like my home directory or machine level location. (I realize others may a different workflow)

I just wish there was a command line option to tell RR where to put the trace directory. -o is not the same as it doesn't provide multiple recordings and a latest-trace softlink.

So maybe this?

-o, --output-trace-dir<DIR>        set the output trace directory. A single recording. No latest-trace softlink.
-r, --output-trace-root<DIR>       set the output root trace directory. Multiple recordings with the latest-trace softlink pointing to the most recent.

Only one can be provided. It both are, an error happens. Both will override _RR_TRACE_DIR if that is in the environment.

@GitMensch
Copy link
Contributor

GitMensch commented Sep 21, 2025

@epasveer Why not aliasing rr with _RR_TRACE_DIR=$PWD rr (or similar)?
"Where the program resides" doesn't make any sense because then there's no trace dir for the replay...)

@epasveer
Copy link

@epasveer Why not aliasing rr with _RR_TRACE_DIR=$PWD rr (or similar)? "Where the program resides" doesn't make any sense because then there's no trace dir for the replay...)

I believe there is, if I understand your reply correctly.

$ rr replay -h
 rr replay [OPTION]... [<trace-dir>] [-- <debugger-options>]
  -a, --autopilot            replay without debugger server

You optionally give it the name of a trace-dir, which can be the same path as --output-trace-root<DIR>

@GitMensch
Copy link
Contributor

You're correct. Feel free to provide a wrapper script that checks for the argument, and if it is "record" or a file, set --output-trace-root depending on the argument's realpath (possibly fall back to $HOME/basename in case the base is under /usr).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants