Page MenuHomePhabricator

Close terminal after LaunchInTerminalTestCase test
ClosedPublic

Authored by ki.stfu on Feb 6 2015, 7:57 AM.

Details

Summary

The test_launch_in_terminal test leaves a running terminal.
This patch adds "exit" after debugging with eLaunchFlagLaunchInTTY flag.

Diff Detail

Event Timeline

ki.stfu updated this revision to Diff 19484.Feb 6 2015, 7:57 AM
ki.stfu retitled this revision from to Close terminal after LaunchInTerminalTestCase test.
ki.stfu updated this object.
ki.stfu edited the test plan for this revision. (Show Details)
ki.stfu added reviewers: zturner, jingham, clayborg.
ki.stfu added subscribers: zturner, jingham, clayborg, Unknown Object (MLST).
vharron added a subscriber: vharron.Feb 6 2015, 9:31 AM

I'm not sure that this is desirable. I don't like having a bunch of stale
terminals left around but its worse when you need to see the output in the
terminal and the terminal disappears

emaste added a subscriber: emaste.Feb 6 2015, 9:35 AM

Ok, but:

  1. You can set "don't close after exit" flag in settings
  2. When we run this test on buildbot we get open terminal each time when it's executed.

Maybe we can fix this test but I don't know how to close terminal without its pid.

clayborg edited edge metadata.Feb 6 2015, 9:51 AM

We could add a new "settings set" setting that allows us to control if terminals close down after executing and we could set this from the test suite permanently. Search for "thread-format" and add a new bool setting.

ki.stfu planned changes to this revision.Feb 6 2015, 9:58 AM

You can set "don't close after exit" flag in settings

Awesome

FWIW, I prefer flags being expressed in positive terms. I think it
significantly improves understanding. (e.g. "close after exit" is better
than "don't close after exit") I would also default to "close after exit"

false

UI presentation:

  • Launch in new TTY
    • Close TTY after process exit
zturner edited edge metadata.Feb 6 2015, 11:21 AM

+1 to expressing flags in positive terms.

jingham edited edge metadata.Feb 6 2015, 11:40 AM

A long-standing bug in the lldb command help system is that it doesn't print what the default value of boolean flags is. So for instance you get:

(lldb) help process launch

Launch the executable in the debugger.

Syntax: process launch <cmd-options> [<run-args>]

Command Options Usage:

process launch [-s] [-A <boolean>] [-p <plugin>] [-w <directory>] [-a <arch>] [-v <none>] [-c[<filename>]] [-i <filename>] [-o <filename>] [-e <filename>] [<run-args>]
process launch [-st] [-A <boolean>] [-p <plugin>] [-w <directory>] [-a <arch>] [-v <none>] [-c[<filename>]] [<run-args>]
process launch [-ns] [-A <boolean>] [-p <plugin>] [-w <directory>] [-a <arch>] [-v <none>] [-c[<filename>]] [<run-args>]

     -A <boolean> ( --disable-aslr <boolean> )
          Set whether to disable address space layout randomization when launching a process.

You can't tell what you are going to get if don't specify -A. So I try to write the option so that it indicates what the default value is. In this case, if you say nothing we'll disable ASLR. This is obviously just a hack to work around an omission in the help system. And in a couple of places (like --one-shot in the "breakpoint set" command where I don't do this because it would have made the name really non-obvious.)

That's a pretty weak reason for the naming, and we really should include the default value - at least for booleans - in the command table and then use it in the help system.

Jim

This sounds like an easy patch. Is it subtly more complicated than it
sounds? If it's actually easy as it sounds, I'll just do it.

It's actually a bit of work.

Right now, all the default values are set in the OptionParsingStarting method of the Options subclass in the CommandObject subclass that implements the command. So the only entity that knows the default value of an option is the state of the associated ivar in the Option object right after OptionParsingStarting. And the relation between option flag and the variable that stores the flag value in the Options (and thus has the default value) is some arbitrary choice made by the Option class. There isn't even a general way to ask: "what ivar stores the option value for command option 'a'?" since it's up to Options::SetOptionValue to stuff values in ivars, and only the parse function knows which ivar went with which option flag.

You'd have to come up with some way to encode default values in the options tables, and then change over OptionParsingStarting to read the default values from the tables. You'd also have to deal with the OptionGroups which do things differently. I think once you started into a task like this, you'd end up rewriting a substantial portion of the option parsing machinery. That in itself would not be a bad thing, there are some crusty aspects of the command parser in general that we should revise at some point. But it ain't a Friday afternoon hack.

Jim

ki.stfu updated this revision to Diff 19652.Feb 10 2015, 2:08 AM
ki.stfu edited edge metadata.

Add close-after-exit debugger's option; Add eLaunchFlagCloseTTYOnExit; Fix test

ki.stfu updated this revision to Diff 19653.Feb 10 2015, 2:09 AM

Fix test in Diff 2

I had faced with problem how to get debugger's property in Host.mm file. I decided to pass ePropertyCloseAfterExit through launch_info using the eLaunchFlagCloseTTYOnExit flag. It looks good for me so I wanna remove close-after-exit debugger's property and use only launch flags. I'll make patch later.

ki.stfu updated this revision to Diff 19656.Feb 10 2015, 2:57 AM

Remove close-after-exit debugger's option (use eLaunchFlagCLoseTTYOnExit instead); Update test

clayborg accepted this revision.Feb 10 2015, 9:37 AM
clayborg edited edge metadata.
This revision is now accepted and ready to land.Feb 10 2015, 9:37 AM
ki.stfu closed this revision.Feb 10 2015, 8:53 PM