Page MenuHomePhabricator

Allow specifying an exit code for the 'quit' command
ClosedPublic

Authored by teemperor on Jun 27 2018, 10:11 AM.

Details

Summary

This patch adds the possibility to specify an exit code when calling quit.
We accept any int, even though it depends on the user what happens if the int is
out of the range of what the operating system supports as exit codes.

Fixes rdar://problem/38452312

Diff Detail

Repository
rL LLVM

Event Timeline

teemperor created this revision.Jun 27 2018, 10:11 AM
jingham requested changes to this revision.Jun 27 2018, 10:25 AM
jingham added a subscriber: jingham.

This won't at all do what users expect in cases where the driver program doesn't cooperate. For instance, when run under Xcode, the lldb process can have many debuggers running concurrently, so the exit value of a single Debugger doesn't make much sense there.

I think we should require the client to say that it will obey the exit value, and have the command error out if the client has not told us it plans to respect our exit values.

This revision now requires changes to proceed.Jun 27 2018, 10:25 AM
clayborg requested changes to this revision.Jun 27 2018, 1:49 PM
clayborg added a subscriber: clayborg.

This change seems fine to me. It allows each debugger to have a different exit status and each debugger can grab one. Seems like this should belong in lldb_private::CommandInterpreter and lldb::SBCommandInterpreter instead of in lldb_private::Debugger and lldb::SBDebugger

include/lldb/API/SBDebugger.h
225–226 ↗(On Diff #153115)

How do you know if the quit command has been called? I mean, we can return zero when "quit" hasn't been called yet, but it might be nice to know by changing the signature a bit:

int GetExitCode(bool &exited);

exited would be set to true if the quit command has been called, false otherwise if the command interpreter is still running?

Also "GetExitCode()" might be better and more clear as being named "GetQuitStatus()" or "GetQuitCommandStatus()" to be more clear this the result of the "quit" command?

This seems like this belongs more on the SBCommandInterpreter since it is the result of the "quit" command. Maybe something like:

class SBCommandInterpreter {
  bool QuitHasBeenCalled();
  int GetQuitStatus();
};

Then the user can call "SBCommandInterpreter::QuitHasBeenCalled()" first and follow it with SBCommandInterpreter::GetQuitStatus() to get the status? So I this should be moved to SBCommandInterpreter.h

include/lldb/Core/Debugger.h
331–334 ↗(On Diff #153115)

move to command interpreter.

411–412 ↗(On Diff #153115)

move to command interpreter

source/API/SBDebugger.cpp
1084–1087 ↗(On Diff #153115)

move to SBCommandInterpreter

source/Commands/CommandObjectQuit.cpp
101 ↗(On Diff #153115)
m_interpreter.SetExitCode(exit_code);
tools/driver/Driver.cpp
1162 ↗(On Diff #153115)

Get exit from command interpreter

teemperor updated this revision to Diff 154318.Jul 5 2018, 2:34 PM
  • Client now has to explicitly allow the acceptance of exit codes.
  • Moved logic to the CommandInterpreter.
  • Added more tests.

Thanks for the feedback Jim and Greg, very much appreciated!

teemperor marked 2 inline comments as done.Jul 5 2018, 2:34 PM
teemperor marked 4 inline comments as done.
jingham accepted this revision.Jul 5 2018, 4:30 PM

That seems reasonable to me.

@clayborg was quite specific about the requested changes (which are all applied now), so I assume this is good to go.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 11 2018, 10:23 AM
This revision was automatically updated to reflect the committed changes.