change CLI around --force-backtrace and --max-backtrace-len

[japaric]

We are going to change the CLI around --force-backtrace and --max-backtrace-len. The accepted proposal is #198 (comment) . This is only waiting on a PR to implement that proposal.

---

Original ticket:

(this is technically a breaking change so it should wait until v0.3.0)

--max-backtrace-len=0 is currently allowed. While working on PR #197 it wasn't clear to me what the semantics of --force-backtrace --max-backtrace-len=0 should be but I made --max-backtrace-len=0 never print a backtrace regardless of other flags and the presence of stack overflows or exceptions.

IMO, it'd be better to reject --max-backtrace-len=0 and only accepts lengths equal or greater than 1; that avoids the above ambiguity.

cc @Lotterleben

[Lotterleben]

makes sense. I'm wondering, is there a case where one might not want any backtraces ever?

[japaric]

I'm wondering, is there a case where one might not want any backtraces ever?

we also print the backtrace to stdout, right? perhaps if you want to grep for something in the logs but not in the backtrace then it may be useful to completely disable the backtrace. this use case would be best served by the proposed JSON output (#178), IMO

[jonas-schievink]

IMO it makes sense to reject --force-backtrace --max-backtrace-len=0, but I don't think --max-backtrace-len=0 by itself should be rejected. I would expect it to just turn off the backtrace (either completely, with no output referring to the backtrace, or in a way that just prints a backtrace with 0 frames).

[japaric]

but I don't think --max-backtrace-len=0 by itself should be rejected. I would expect it to just turn off the backtrace

that sounds like 'some way to disable the backtrace' is a desirable thing.

perhaps we can harmonize this to work like Cargo's --color flag? --backtrace={auto,always,never}. auto is the current, default behavior (e.g. backtrace on panic, but not on exit). always is --force-backtrace (which is no longer needed with this scheme). never disables the backtrace completely.

THEN --max-backtrace-len can be limited to be greater than 1 and shall only be used with --backtrace={auto,always}. e.g. --max-backtrace-len=10 --backtrace=never would be an error.

OR we don't restrict mixing --max-backtrace-len and --backtrace=never (i.e. that's not an error) but simply ignore the --max-backtrace-len flag in that case.

---

Also, I'd like the default to be "no limit on backtrace length" (e.g. if the backtrace is the result of a hard to reproduce condition then you want all the info you can get your hands on; not just some arbitrary number of frames). Right now, the backtrace is limited to 50 frames and there's no good way to undo it -- I mean you can use --max-backtrace-len=999 or something like that but that looks a bit ridiculous.

We could make the default be 'the number of backtrace frames is not limited' OR we could make --max-backtrace-len=0 mean 'do not limit the backtrace size' and keep the max-backtrace-len default of 50 frames. I'd prefer the former option.

[jonas-schievink]

We could also rename it to --backtrace-limit, and have 0 mean "unlimited", I believe that's what some tools do

[japaric]

OK, let me more concretely propose this:

• --force-backtrace is removed
• --backtrace-len is renamed to --backtrace-limit
• --backtrace is added

The interaction between these flags is specified in the table below:

--backtrace=	--backtrace-limit=N	Backtrace is printed
never	is ignored	never
auto	has an effect	if program panics or stack overflows
always	has an effect	always

Additionally,

• --backtrace flag is optional and defaults to auto
• --backtrace-limit flag is optional and defaults to 50 (+)
• --backtrace-limit=0 is accepted and means "no limit"

(+) rationale on setting a default limit: bugs in debug info (DWARF) generation can result in infinite backtraces that even the mature GDB can't handle / detect #127 (comment) so err on the side of caution and set a limit to avoid spamming frames to the console in presence of these bugs.

@jonas-schievink , @Lotterleben can I get a 'sounds good to me' / 'I have a concern' reaction on the above proposal as a comment -- I don't get notified about emoji reactions to comments so that would be easy to miss.

Changelog

• 2021-08-02 changed default --backtrace-limit to 50.

[jonas-schievink]

an infinite backtrace is a bug in probe-run and it should be promptly fixed, instead of being papered over with an artificial --limit

I agree in principle, but we've seen programs that even resulted in an infinite backtrace in GDB, so I don't think this is realistic (#127 (comment)).

Everything else sounds good to me though!

[japaric]

Fair, I can compromise on keeping the 50 frames limit.

[Lotterleben]

+1 to what Jonas says. We also picked the 50 line limit quite arbitrarily, so I'd be OK with increasing it.

[japaric]

OK. That's two approvals. I'll amend my proposal; link it from the issue description as accepted and change this ticket status to 'needs PR'