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.
Details
Details
- Reviewers
labath clayborg jingham teemperor - Group Reviewers
Restricted Project - Commits
- rG61d5b0e66394: [lldb/Driver] Exit with a non-zero exit code in case of error in batch mode.
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
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.
| 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 ? |
Comment Actions
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...
| 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. |
Comment Actions
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. |
Comment Actions
(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). |
Comment Actions
Greg, you requested changes to this revision, I guess because of the dependencies. Anything here you'd like to see changed?
Comment Actions
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.
Comment Actions
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.