Close terminal after LaunchInTerminalTestCase test

[ki.stfu]

Summary

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

[vharron]

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

[ki.stfu]

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]

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.

[vharron]

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]

+1 to expressing flags in positive terms.

[jingham]

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

[zturner]

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.

[jingham]

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]

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

[ki.stfu]

Fix test in Diff 2

[ki.stfu]

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]

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

include/lldb/Core/Debugger.h

public:
     SetPropertyValue (const ExecutionContext *exe_ctx,
                       VarSetOperationType op,
                       const char *property_path,
                       const char *value);
 
     bool
     GetAutoConfirm () const;
 
+    bool
+    GetCloseAfterExit () const;
+
     const FormatEntity::Entry *
     GetDisassemblyFormat() const;
 
     const FormatEntity::Entry *
     GetFrameFormat() const;
 
     const FormatEntity::Entry *
     GetThreadFormat() const;

include/lldb/lldb-enumerations.h

typedef enum LaunchFlags
         eLaunchFlagLaunchInTTY  = (1u << 5),       ///< Launch the process in a new TTY if supported by the host
         eLaunchFlagLaunchInShell= (1u << 6),       ///< Launch the process inside a shell to get shell expansion
         eLaunchFlagLaunchInSeparateProcessGroup = (1u << 7), ///< Launch the process in a separate process group
         eLaunchFlagDontSetExitStatus = (1u << 8), ///< If you are going to hand the process off (e.g. to debugserver)
                                                    ///< set this flag so lldb & the handee don't race to set its exit status.
         eLaunchFlagDetachOnError = (1u << 9),     ///< If set, then the client stub should detach rather than killing the debugee
                                                    ///< if it loses connection with lldb.
         eLaunchFlagGlobArguments  = (1u << 10),       ///< Glob arguments without going through a shell
+        eLaunchFlagCloseTTYOnExit = (1u << 11),    ///< Close the opened TTY on exit
     } LaunchFlags;
         
     //----------------------------------------------------------------------
     // Thread Run Modes
     //----------------------------------------------------------------------
     typedef enum RunMode {
         eOnlyThisThread,
         eAllThreads,

source/Core/Debugger.cpp

#define DEFAULT_FRAME_FORMAT "frame #${frame.index}: ${frame.pc}"\
     "\\n"
 
 #define DEFAULT_DISASSEMBLY_FORMAT "${current-pc-arrow}${addr-file-or-load}{ <${function.name-without-args}${function.concrete-only-addr-offset-no-padding}>}: "
 
 static PropertyDefinition
 g_properties[] =
 {
 {   "auto-confirm",             OptionValue::eTypeBoolean     , true, false, NULL, NULL, "If true all confirmation prompts will receive their default reply." },
+{   "close-after-exit",         OptionValue::eTypeBoolean     , true, false, NULL, NULL, "If true, LLDB will automatically close a terminal after exit (default: false)." },
 {   "disassembly-format",       OptionValue::eTypeFormatEntity, true, 0    , DEFAULT_DISASSEMBLY_FORMAT, NULL, "The default disassembly format string to use when disassembling instruction sequences." },
 {   "frame-format",             OptionValue::eTypeFormatEntity, true, 0    , DEFAULT_FRAME_FORMAT, NULL, "The default frame format string to use when displaying stack frame information for threads." },
 {   "notify-void",              OptionValue::eTypeBoolean     , true, false, NULL, NULL, "Notify the user explicitly if an expression returns void (default: false)." },
 {   "prompt",                   OptionValue::eTypeString      , true, OptionValueString::eOptionEncodeCharacterEscapeSequences, "(lldb) ", NULL, "The debugger command line prompt displayed for the user." },
 {   "script-lang",              OptionValue::eTypeEnum        , true, eScriptLanguagePython, NULL, g_language_enumerators, "The script language to be used for evaluating user-written scripts." },
 {   "stop-disassembly-count",   OptionValue::eTypeSInt64      , true, 4    , NULL, NULL, "The number of disassembly lines to show when displaying a stopped context." },
 {   "stop-disassembly-display", OptionValue::eTypeEnum        , true, Debugger::eStopDisassemblyTypeNoSource, NULL, g_show_disassembly_enum_values, "Control when to display disassembly when displaying a stopped context." },
 {   "stop-line-count-after",    OptionValue::eTypeSInt64      , true, 3    , NULL, NULL, "The number of sources lines to display that come after the current source line when displaying a stopped context." },
 {   "stop-line-count-before",   OptionValue::eTypeSInt64      , true, 3    , NULL, NULL, "The number of sources lines to display that come before the current source line when displaying a stopped context." },
 {   "term-width",               OptionValue::eTypeSInt64      , true, 80   , NULL, NULL, "The maximum number of columns to use for displaying text." },
 {   "thread-format",            OptionValue::eTypeFormatEntity, true, 0    , DEFAULT_THREAD_FORMAT, NULL, "The default thread format string to use when displaying thread information." },
 {   "use-external-editor",      OptionValue::eTypeBoolean     , true, false, NULL, NULL, "Whether to use an external editor or not." },
 {   "use-color",                OptionValue::eTypeBoolean     , true, true , NULL, NULL, "Whether to use Ansi color codes or not." },
 {   "auto-one-line-summaries",  OptionValue::eTypeBoolean     , true, true, NULL, NULL, "If true, LLDB will automatically display small structs in one-liner format (default: true)." },
 {   "escape-non-printables",    OptionValue::eTypeBoolean     , true, true, NULL, NULL, "If true, LLDB will automatically escape non-printable and escape characters when formatting strings." },
 {   NULL,                       OptionValue::eTypeInvalid     , true, 0    , NULL, NULL, NULL }
 };
 
 enum
 {
     ePropertyAutoConfirm = 0,
+    ePropertyCloseAfterExit,
     ePropertyDisassemblyFormat,
     ePropertyFrameFormat,
     ePropertyNotiftVoid,
     ePropertyPrompt,
     ePropertyScriptLanguage,
     ePropertyStopDisassemblyCount,
     ePropertyStopDisassemblyDisplay,
     ePropertyStopLineCountAfter,

 
 bool
 Debugger::GetAutoConfirm () const
 {
     const uint32_t idx = ePropertyAutoConfirm;
     return m_collection_sp->GetPropertyAtIndexAsBoolean (NULL, idx, g_properties[idx].default_uint_value != 0);
 }
 
+bool
+Debugger::GetCloseAfterExit () const
+{
+    const uint32_t idx = ePropertyCloseAfterExit;
+    return m_collection_sp->GetPropertyAtIndexAsBoolean (NULL, idx, g_properties[idx].default_uint_value != 0);
+}
+
 const FormatEntity::Entry *
 Debugger::GetDisassemblyFormat() const
 {
     const uint32_t idx = ePropertyDisassemblyFormat;
     return m_collection_sp->GetPropertyAtIndexAsFormatEntity(NULL, idx);
 }
 
 const FormatEntity::Entry *

source/Host/macosx/Host.mm

if (argv)
                 command.Printf(" '%s'", argv[i]);
         }
     }
     else
     {
         command.Printf(" '%s'", exe_path);
     }
     command.PutCString (" ; echo Process exited with status $?");
+    if (launch_info.GetFlags().Test(lldb::eLaunchFlagCloseTTYOnExit))
+    {
+        command.PutCString (" ; exit");
+    }
     
     StreamString applescript_source;
 
     const char *tty_command = command.GetString().c_str();
 //    if (tty_name && tty_name[0])
 //    {
 //        applescript_source.Printf (applscript_in_existing_tty, 
 //                                   tty_command,

source/Target/Target.cpp

// launching.
         else
         {
             if (log)
                 log->Printf ("Target::%s the process instance doesn't currently exist.", __FUNCTION__);
         }
     }
 
     launch_info.GetFlags().Set (eLaunchFlagDebug);
-    
+    Debugger &debugger = GetDebugger();
+    if (launch_info.GetFlags().Test(eLaunchFlagLaunchInTTY) && debugger.GetCloseAfterExit())
+        launch_info.GetFlags().Set(eLaunchFlagCloseTTYOnExit);
+
     // Get the value of synchronous execution here.  If you wait till after you have started to
     // run, then you could have hit a breakpoint, whose command might switch the value, and
     // then you'll pick up that incorrect value.
-    Debugger &debugger = GetDebugger();
     const bool synchronous_execution = debugger.GetCommandInterpreter().GetSynchronous ();
     
     PlatformSP platform_sp (GetPlatform());
     
     // Finalize the file actions, and if none were given, default to opening
     // up a pseudo terminal
     const bool default_to_use_pty = platform_sp ? platform_sp->IsHost() : false;
     if (log)

test/functionalities/tty/TestTerminal.py

class LaunchInTerminalTestCase(TestBase):
 
     # Do we need to disable this test if the testsuite is being run on a rmeote system?
     # This env var is only defined when the shell is running in a local mac terminal window
     # @unittest2.skipUnless(os.environ.has_key('TERM_PROGRAM'), "test must be run on local system")
 
     def test_launch_in_terminal (self):
         exe = "/bin/ls"
         target = self.dbg.CreateTarget(exe)
+        self.dbg.HandleCommand("settings set close-after-exit true")
         launch_info = lldb.SBLaunchInfo(["-lAF", "/tmp/"])
         launch_info.SetLaunchFlags(lldb.eLaunchFlagLaunchInTTY)
         error = lldb.SBError()
         process = target.Launch (launch_info, error)
         self.assertTrue(error.Success(), "Make sure launch happened successfully in a terminal window")
         # Running in synchronous mode our process should have run and already exited by the time target.Launch() returns
         self.assertTrue(process.GetState() == lldb.eStateExited)
 
 if __name__ == '__main__':
     import atexit
     lldb.SBDebugger.Initialize()
     atexit.register(lambda: lldb.SBDebugger.Terminate())
     unittest2.main()