This is an archive of the discontinued LLVM Phabricator instance.

[lldb/Driver] Exit with a non-zero exit code in batch mode when stopping because of an error.
ClosedPublic

Authored by JDevlieghere on Apr 24 2020, 12:40 PM.

Details

Summary

We have the option to stop running commands in batch mode when an error occurs. Currently we still exit with a zero exit code when that happens. This is counter intuitive and not very POSIX-y.

Diff Detail

Event Timeline

JDevlieghere created this revision.Apr 24 2020, 12:40 PM
vsk added a subscriber: vsk.Apr 24 2020, 1:31 PM

Can we delete an override of SBDebugger::RunCommandInterpreter, or are they all part of the stable API? If we can, it'd be nice to get rid of the one with 6 args.

Otherwise this lgtm.

labath added inline comments.Apr 27 2020, 3:40 AM
lldb/source/API/SBDebugger.cpp
1205–1209 ↗(On Diff #259965)

What's the relationship between num_errors and stopped_for_error? Could we infer that we stopped because of an error if num_errors>0 ?

In D78825#2002598, @vsk wrote:

Can we delete an override of SBDebugger::RunCommandInterpreter, or are they all part of the stable API?

They are. The way that has been handled so far is that once a function starts to get too many overloads, we create a overload taking a class like SBFuncOptions, which contains all the arguments and is extensible. That seems to have kind of happened here already, as we have the SBCommandInterpreterRunOptions class. However, in this case, that struct only contains "input" arguments, which is why I am reluctant to recommend shove output arguments there.

That said, I am wondering about the semantics of the num_errors argument. If it is nonzero only in cases where we "stop because of an error" (or we can adjust it so that this is the case), then maybe we don't need a new api for this...

JDevlieghere marked an inline comment as done.Apr 27 2020, 9:06 AM
JDevlieghere added inline comments.
lldb/source/API/SBDebugger.cpp
1205–1209 ↗(On Diff #259965)

I considered that, m_num_errors is incremented unconditionally, while stopped_for_error is only set when eHandleCommandFlagStopOnError is set. If you know that "stop on error" is set you could infer it based on the value being non-zero, but to me that sounds a bit fragile, and given that we already have stopped_for_crash I preferred the symmetry. I'm not married to this approach so if you feel strongly about it I'm happy to change it.

In D78825#2002598, @vsk wrote:

Can we delete an override of SBDebugger::RunCommandInterpreter, or are they all part of the stable API? If we can, it'd be nice to get rid of the one with 6 args.

We can't remove any existing API because it is stable, yes.

See my inline comment and let me know your thoughts.

lldb/include/lldb/API/SBDebugger.h
300–303 ↗(On Diff #259965)

I'd vote to add a new function like:

SBCommandInterpreterRunResults RunCommandInterpreter(SBCommandInterpreterRunOptions &options);

We would need to add "auto_handle_events" and "spawn_thread" to SBCommandInterpreterRunOptions:

class LLDB_API SBCommandInterpreterRunOptions {
  friend class SBDebugger;
  friend class SBCommandInterpreter;

public:
  bool GetAutoHandleEvents() const;
  void SetAutoHandleEvents(bool);

  bool GetSpawnThread() const;
  void SetSpawnThread(bool);

and then we have an object: SBCommandInterpreterRunResults that we can query for number of errors, quit requested, stopped_for_crash, and stopped_for_error using accessors. This allows us to add new accessors without changing the API.

(I made this comment yesterday, but it seems I didn't click send)

lldb/source/API/SBDebugger.cpp
1205–1209 ↗(On Diff #259965)

Yea, needing to remember what you set as the value for "stop on error" does make this fairly finicky. OTOH, one could argue that errors shouldn't be counted as real errors (and reported here) unless stop-on-error is set. That would make checking for error-stops easier.

I don't feel strongly about any of that, but I can't say I am thrilled by the prospect of introducing another overload. So, if we do that, I'd like to ensure some thought goes into it.

Greg's idea of making a struct for the return values sounds like a good one -- having so many by-ref results is not very nice (I don't like having even one tbh). My question there would be should we have separate bool flags for the "stop reasons". It seems like these conditions should be mutually exclusive, so it may be better to have an enum to indicate a reason for stopping (+ an accessor for the error count, I guess -- though I am not exactly sure how useful it is to know the exact number of errors without being able to access them in any way).

Use SBCommandInterpreterRunResults

clayborg requested changes to this revision.Apr 29 2020, 5:30 PM
This revision now requires changes to proceed.Apr 29 2020, 5:30 PM

Greg, you requested changes to this revision, I guess because of the dependencies. Anything here you'd like to see changed?

labath added a comment.May 4 2020, 2:15 AM

Greg, you requested changes to this revision, I guess because of the dependencies. Anything here you'd like to see changed?

I'm guessing the answer is no, but it would certainly help of you rebased the patch to see how the patch looks like after the changes in the previous patches.

Greg, you requested changes to this revision, I guess because of the dependencies. Anything here you'd like to see changed?

I'm guessing the answer is no, but it would certainly help of you rebased the patch to see how the patch looks like after the changes in the previous patches.

Thanks, I thought I had.

clayborg accepted this revision.May 4 2020, 7:00 PM

LGTM now. Pavel?

This revision is now accepted and ready to land.May 4 2020, 7:00 PM
labath accepted this revision.May 5 2020, 4:04 AM

I note that the call to exit(1) bypasses some of the finalization code in (most notably, reset_stdin_termios). That does not seem ideal, but it looks like a pre-existing problem.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2020, 11:20 AM