Add -s/--source option support (MI)

[ki.stfu]

Summary

This patch adds -s/--source option to execute source file with prepared command.
For example:

$ cat start_script 
target create ~/p/hello
process launch -s
continue
$ bin/lldb-mi -s start_script 
(gdb)
target create ~/p/hello
Current executable set to '~/p/hello' (x86_64).
^done
(gdb)
process launch -s
=shlibs-added,shlib-info=[num="1",name="hello",dyld-addr="-",reason="dyld",path="/Users/IliaK/p/hello",loaded_addr="-",dsym-objpath="/Users/IliaK/p/hello.dSYM/Contents/Resources/DWARF/hello"]
Process 33289 launched: '/Users/IliaK/p/hello' (x86_64)
^done
(gdb)
continue
=thread-created,id="1",group-id="i1"
=thread-selected,id="1"
(gdb)
Process 33289 resuming
Process 33289 exited with status = 0 (0x00000000) 
^done

[abidh]

First of all, can you explain the use cases where this functionality will be useful. I can see that it may help with testing. But I dont see much use otherwise.

Patch looks ok apart from some inline comments. Also this patch is dependent on other patch that implements WaitForHandleEvent, so wait for that patch to be reviewed/committed.

tools/lldb-mi/MIDriver.cpp

554

As I informed you in another email, there are problem with sync mode. So should this be set to true.

1297

We dont want comments to be printed on sonsole so this check should be done before TextToStdout call.

1310

Please complete the comment.

[ki.stfu]

In D9278#161869, @abidh wrote:

First of all, can you explain the use cases where this functionality will be useful. I can see that it may help with testing. But I dont see much use otherwise.

This option is very useful in case when IDE needs an extra efforts to setup debug session. For example, to debug remote platform we should select ios-remote first, then create a target for this platform and connect to remote debugserver. All these commands may be written to init.cmds file that should be passed to lldb-mi via --source option.

tools/lldb-mi/MIDriver.cpp

554

Yep. I remember about this problem but we really should execute prepared commands in sync mode, otherwise we can't do the following:

-file-exec-and-symbols ~/p/hello
^done
-break-insert -f main
^done
-exec-run
^running
-stack-list-arguments 1
^error,msg="invalid thread"
*stopped,reason="breakpoint-hit"

As you see, in async mode the *stopped notification comes after we have tried to execute -stack-list-arguments (what's wrong).

I'll look at *stopped in sync mode a bit later.

1297

It's a little workaround for lldb because it doesn't support the echo command (unlike gdb). Therefore to emulate this behavior I can add a comment in init.cmds that will be printed during execution.

1310

ops.

[ki.stfu]

Fix a comment and skip empty lines

[abidh]

This option is very useful in case when IDE needs an extra efforts to setup debug session. For example, to debug remote platform we should select ios-remote first, then create a target for this platform and connect to remote debugserver. All these commands may be written to init.cmds file that should be passed to lldb-mi via --source option.

What is the problem if those commands are sent normally instead of init.cmds. Would that not work? Eclipse sends a lot of other command before starting debugging. So it would seems a little unnecessary to me.

I am not opposed to the patch but I don't think that it solves any problem.

[ki.stfu]

In D9278#162701, @abidh wrote:

This option is very useful in case when IDE needs an extra efforts to setup debug session. For example, to debug remote platform we should select ios-remote first, then create a target for this platform and connect to remote debugserver. All these commands may be written to init.cmds file that should be passed to lldb-mi via --source option.

What is the problem if those commands are sent normally instead of init.cmds. Would that not work? Eclipse sends a lot of other command before starting debugging. So it would seems a little unnecessary to me.

I am not opposed to the patch but I don't think that it solves any problem.

It just a new functionality that serves for execution of prepared commands from source file. Of course we can pass them by hand, but I'd prefer to pass prepared file as a --source arg instead of typing all of them by hand (or copy-pasting :)).

[abidh]

It just a new functionality that serves for execution of prepared commands from source file. Of course we can pass them by hand, but I'd prefer to pass prepared file as a --source arg instead of typing all of them by hand (or copy-pasting :)).

I understand as I do a lot of testing by giving manual commands. But in the end, MI commands are not supposed to be given by hand. It is assumed that some front end IDE will use it where it should not be a problem to generate those commands.

As such I am not opposed to the patch but I think it will be adding a thing we don't really need. Once this code is in then we will have to maintain it and make sure that it keeps working as people can depend on that behaviour. If you still want it in then I will not stand in the way. Please go ahead and commit.

[ki.stfu]

In D9278#163041, @abidh wrote:

It just a new functionality that serves for execution of prepared commands from source file. Of course we can pass them by hand, but I'd prefer to pass prepared file as a --source arg instead of typing all of them by hand (or copy-pasting :)).

I understand as I do a lot of testing by giving manual commands. But in the end, MI commands are not supposed to be given by hand. It is assumed that some front end IDE will use it where it should not be a problem to generate those commands.

As such I am not opposed to the patch but I think it will be adding a thing we don't really need. Once this code is in then we will have to maintain it and make sure that it keeps working as people can depend on that behaviour. If you still want it in then I will not stand in the way. Please go ahead and commit.

You are suggesting that there is a front-end that can pass these commands to lldb-mi, but it's not always true. For example, to test lldb-mi functionality on device I run lldb-mi with --source option and then grep the output to check my test cases. Of course I can write a simple front-end to read these commands from file and pass them to lldb-mi, but another way is just add --source support in lldb-mi like in lldb/gdb. So yes, I'm sure that it's needed.

[labath]

These new tests have been failing on the linux build bot:

======================================================================
ERROR: test_lldbmi_source_option_start_script (TestMiStartupOptions.MiStartupOptionsTestCase)
    Test that 'lldb-mi --interpreter' can execute user's commands after initial commands were executed.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/lldb-buildbot/lldbSlave/buildWorkingDir/llvm/tools/lldb/test/lldbtest.py", line 447, in wrapper
    return func(self, *args, **kwargs)
  File "/lldb-buildbot/lldbSlave/buildWorkingDir/llvm/tools/lldb/test/lldbtest.py", line 562, in wrapper
    func(*args, **kwargs)
  File "/lldb-buildbot/lldbSlave/buildWorkingDir/llvm/tools/lldb/test/tools/lldb-mi/startup_options/TestMiStartupOptions.py", line 167, in test_lldbmi_source_option_start_script
    self.expect("\^done,value=\"10\"")
  File "/lldb-buildbot/lldbSlave/buildWorkingDir/llvm/tools/lldb/test/tools/lldb-mi/lldbmi_testcase.py", line 46, in expect
    return self.child.expect(pattern, *args, **kwargs)
  File "/lldb-buildbot/lldbSlave/buildWorkingDir/llvm/tools/lldb/test/pexpect-2.4/pexpect.py", line 1316, in expect
    return self.expect_list(compiled_pattern_list, timeout, searchwindowsize)
  File "/lldb-buildbot/lldbSlave/buildWorkingDir/llvm/tools/lldb/test/pexpect-2.4/pexpect.py", line 1330, in expect_list
    return self.expect_loop(searcher_re(pattern_list), timeout, searchwindowsize)
  File "/lldb-buildbot/lldbSlave/buildWorkingDir/llvm/tools/lldb/test/pexpect-2.4/pexpect.py", line 1414, in expect_loop
    raise TIMEOUT (str(e) + '\n' + str(self))
TIMEOUT: Timeout exceeded in read_nonblocking().
<pexpect.spawn object at 0x7f440f6f0a10>
version: 2.4 ($Revision: 516 $)
command: /lldb-buildbot/lldbSlave/buildWorkingDir/build/bin/lldb-mi
args: ['/lldb-buildbot/lldbSlave/buildWorkingDir/build/bin/lldb-mi', '--interpreter', '--source', 'start_script']
searcher: searcher_re:
    0: re.compile("\^done,value="10"")
buffer (last 100 chars): 
-data-evaluate-expression a
^error,msg="Could not evaluate expression"

[ki.stfu]

In D9278#167649, @labath wrote:

These new tests have been failing on the linux build bot:

Fixed by r236708.

test/tools/lldb-mi/startup_options/TestMiStartupOptions.py

def test_lldbmi_executable_option_unknown_path(self):
         self.expect(self.child_prompt, exactly = True)
 
         # Test that the executable isn't loaded when file was specified using unknown path
         self.expect("-file-exec-and-symbols \"%s\"" % path)
         self.expect("\^error,msg=\"Command 'file-exec-and-symbols'\. Target binary '%s' is invalid\. error: unable to find executable for '%s'\"" % (path, path))
 
         # Test that lldb-mi is ready when executable was loaded
         self.expect(self.child_prompt, exactly = True)
-    
     @lldbmi_test
     @expectedFailureWindows("llvm.org/pr22274: need a pexpect replacement for windows")
+    def test_lldbmi_source_option_start_script(self):
+        """Test that 'lldb-mi --interpreter' can execute user's commands after initial commands were executed."""
+
+        # Prepared source file
+        sourceFile = "start_script"
+
+        self.spawnLldbMi(args = "--source %s" % sourceFile)
+
+        # After '-file-exec-and-symbols a.out'
+        self.expect("-file-exec-and-symbols %s" % self.myexe)
+        self.expect("\^done")
+
+        # After '-break-insert -f main'
+        self.expect("-break-insert -f main")
+        self.expect("\^done,bkpt={number=\"1\"")
+
+        # After '-exec-run'
+        self.expect("-exec-run")
+        self.expect("\^running")
+
+        # After '-break-insert main.cpp:BP_return'
+        line = line_number('main.cpp', '// BP_return')
+        self.expect("-break-insert main.cpp:%d" % line)
+        self.expect("\^done,bkpt={number=\"2\"")
+
+        # After '-exec-continue'
+        self.expect("-exec-continue")
+        self.expect("\^running")
+
+        # Test that lldb-mi is ready after execution of --source start_script
+        self.expect(self.child_prompt, exactly = True)
+
+        # Try to evaluate 'a' expression
+        self.runCmd("-data-evaluate-expression a")
+        self.expect("\^done,value=\"10\"")
+        self.expect(self.child_prompt, exactly = True)
+
+    @lldbmi_test
+    @expectedFailureWindows("llvm.org/pr22274: need a pexpect replacement for windows")
+    def test_lldbmi_source_option_start_script_exit(self):
+        """Test that 'lldb-mi --interpreter' can execute a prepared file which passed via --source option."""
+
+        # Prepared source file
+        sourceFile = "start_script_exit"
+
+        self.spawnLldbMi(args = "--source %s" % sourceFile)
+
+        # After '-file-exec-and-symbols a.out'
+        self.expect("-file-exec-and-symbols %s" % self.myexe)
+        self.expect("\^done")
+
+        # After '-break-insert -f main'
+        self.expect("-break-insert -f main")
+        self.expect("\^done,bkpt={number=\"1\"")
+
+        # After '-exec-run'
+        self.expect("-exec-run")
+        self.expect("\^running")
+
+        # After '-break-insert main.cpp:BP_return'
+        line = line_number('main.cpp', '// BP_return')
+        self.expect("-break-insert main.cpp:%d" % line)
+        self.expect("\^done,bkpt={number=\"2\"")
+
+        # After '-exec-continue'
+        self.expect("-exec-continue")
+        self.expect("\^running")
+
+        # After '-data-evaluate-expression a'
+        self.expect("-data-evaluate-expression a")
+        self.expect("\^done,value=\"10\"")
+
+        # After '-gdb-exit'
+        self.expect("-gdb-exit")
+        self.expect("\^exit")
+
+    @lldbmi_test
+    @expectedFailureWindows("llvm.org/pr22274: need a pexpect replacement for windows")
+    def test_lldbmi_source_option_start_script_error(self):
+        """Test that 'lldb-mi --interpreter' stops execution of initial commands in case of error."""
+
+        # Prepared source file
+        sourceFile = "start_script_error"
+
+        self.spawnLldbMi(args = "--source %s" % sourceFile)
+
+        # After '-file-exec-and-symbols a.out'
+        self.expect("-file-exec-and-symbols %s" % self.myexe)
+        self.expect("\^done")
+
+        # After '-break-ins -f main'
+        self.expect("-break-ins -f main")
+        self.expect("\^error")
+
+        # Test that lldb-mi is ready after execution of --source start_script
+        self.expect(self.child_prompt, exactly = True)
+    
+    @lldbmi_test
+    @expectedFailureWindows("llvm.org/pr22274: need a pexpect replacement for windows")
     @skipIfFreeBSD # llvm.org/pr22411: Failure presumably due to known thread races
     def test_lldbmi_log_option(self):
         """Test that 'lldb-mi --log' creates a log file in the current directory."""
     
         logDirectory = "."
         self.spawnLldbMi(args = "%s --log" % self.myexe)
 
         # Test that lldb-mi is ready after startup

test/tools/lldb-mi/startup_options/main.cpp

 //===-- main.cpp ------------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 int
 main(int argc, char const *argv[])
 {
-    return 0;
+    int a = 10;
+    return 0; // BP_return
 }

test/tools/lldb-mi/startup_options/start_script

+-file-exec-and-symbols a.out
+-break-insert -f main
+-exec-run
+-break-insert main.cpp:14
+-exec-continue

test/tools/lldb-mi/startup_options/start_script_error

+-file-exec-and-symbols a.out
+-break-ins -f main

test/tools/lldb-mi/startup_options/start_script_exit

+-file-exec-and-symbols a.out
+-break-insert -f main
+-exec-run
+-break-insert main.cpp:14
+-exec-continue
+-data-evaluate-expression a
+-gdb-exit

tools/lldb-mi/MIDriver.h

private:
     CMIUtilString WrapCLICommandIntoMICommand(const CMIUtilString &vTextLine) const;
     bool InterpretCommandFallThruDriver(const CMIUtilString &vTextLine, bool &vwbCmdYesValid);
     bool ExecuteCommand(const SMICmdData &vCmdData);
     bool StartWorkerThreads(void);
     bool StopWorkerThreads(void);
     bool InitClientIDEToMIDriver(void) const;
     bool InitClientIDEEclipse(void) const;
     bool LocalDebugSessionStartupExecuteCommands(void);
+    bool ExecuteCommandFile(const bool vbAsyncMode);
 
     // Overridden:
   private:
     // From CMICmnBase
     /* dtor */ virtual ~CMIDriver(void);
 
     // Attributes:
   private:
     static const CMIUtilString ms_constAppNameShort;
     static const CMIUtilString ms_constAppNameLong;
     static const CMIUtilString ms_constMIVersion;
     //
     bool m_bFallThruToOtherDriverEnabled; // True = yes fall through, false = do not pass on command
     CMIUtilThreadMutex m_threadMutex;
     bool m_bDriverIsExiting;           // True = yes, driver told to quit, false = continue working
     void *m_handleMainThread;          // *this driver is run by the main thread
     CMICmnStreamStdin &m_rStdin;
     CMICmnLLDBDebugger &m_rLldbDebugger;
     CMICmnStreamStdout &m_rStdOut;
     DriverState_e m_eCurrentDriverState;
     bool m_bHaveExecutableFileNamePathOnCmdLine; // True = yes, executable given as one of the parameters to the MI Driver, false = not found
     CMIUtilString m_strCmdLineArgExecuteableFileNamePath;
     bool m_bDriverDebuggingArgExecutable; // True = the MI Driver (MI mode) is debugging executable passed as argument,
                                           // false = running via a client (e.g. Eclipse)
+    bool m_bHaveCommandFileNamePathOnCmdLine; // True = file with initial commands given as one of the parameters to the MI Driver, false = not found
+    CMIUtilString m_strCmdLineArgCommandFileNamePath;
 };

tools/lldb-mi/MIDriver.cpp

 //===-- MIDriver.cpp --------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 // Third party headers:
+#include <fstream>
 #include "lldb/API/SBError.h"
 
 // In-house headers:
 #include "MIDriver.h"
 #include "MICmnResources.h"
 #include "MICmnLog.h"
 #include "MICmdMgr.h"
 #include "MICmnLLDBDebugger.h"
CMIDriver::CMIDriver(void)
     , m_bDriverIsExiting(false)
     , m_handleMainThread(0)
     , m_rStdin(CMICmnStreamStdin::Instance())
     , m_rLldbDebugger(CMICmnLLDBDebugger::Instance())
     , m_rStdOut(CMICmnStreamStdout::Instance())
     , m_eCurrentDriverState(eDriverState_NotRunning)
     , m_bHaveExecutableFileNamePathOnCmdLine(false)
     , m_bDriverDebuggingArgExecutable(false)
+    , m_bHaveCommandFileNamePathOnCmdLine(false)
 {
 }
 
 //++ ------------------------------------------------------------------------------------
 // Details: CMIDriver destructor.
 // Type:    Overridden.
 // Args:    None.
 // Return:  None.
CMIDriver::ParseArgs(const int argc, const char *argv[], FILE *vpStdOut, bool &vwbExiting)
 
     // CODETAG_MIDRIVE_CMD_LINE_ARG_HANDLING
     // Look for the command line options
     bool bHaveExecutableFileNamePath = false;
     bool bHaveExecutableLongOption = false;
 
     if (bHaveArgs)
     {
-        // Search right to left to look for the executable
+        // Search right to left to look for filenames
         for (MIint i = argc - 1; i > 0; i--)
         {
             const CMIUtilString strArg(argv[i]);
             const CMICmdArgValFile argFile;
+
+            // Check for a filename
             if (argFile.IsFilePath(strArg) || CMICmdArgValString(true, false, true).IsStringArg(strArg))
             {
+                // Is this the command file for the '-s' or '--source' options?
+                const CMIUtilString strPrevArg(argv[i - 1]);
+                if (strPrevArg.compare("-s") == 0 || strPrevArg.compare("--source") == 0)
+                {
+                    m_strCmdLineArgCommandFileNamePath = strArg;
+                    m_bHaveCommandFileNamePathOnCmdLine = true;
+                    i--; // skip '-s' on the next loop
+                    continue;
+                }
+                // Else, must be the executable
                 bHaveExecutableFileNamePath = true;
                 m_strCmdLineArgExecuteableFileNamePath = strArg;
                 m_bHaveExecutableFileNamePathOnCmdLine = true;
             }
-            // This argument is also check for in CMIDriverMgr::ParseArgs()
-            if (0 == strArg.compare("--executable")) // Used to specify that there is executable argument also on the command line
-            {                                        // See fn description.
+            // Report error if no command file was specified for the '-s' or '--source' options
+            else if (strArg.compare("-s") == 0 || strArg.compare("--source") == 0)
+            {
+                vwbExiting = true;
+                const CMIUtilString errMsg = CMIUtilString::Format(MIRSRC(IDS_CMD_ARGS_ERR_VALIDATION_MISSING_INF), strArg.c_str());
+                errStatus.SetErrorString(errMsg.c_str());
+                break;
+            }
+            // This argument is also checked for in CMIDriverMgr::ParseArgs()
+            else if (strArg.compare("--executable") == 0) // Used to specify that there is executable argument also on the command line
+            {                                             // See fn description.
                 bHaveExecutableLongOption = true;
             }
         }
     }
 
     if (bHaveExecutableFileNamePath && bHaveExecutableLongOption)
     {
         SetDriverDebuggingArgExecutable();
if (HaveExecutableFileNamePathOnCmdLine())
             SetErrorDescription(MIRSRC(IDS_MI_INIT_ERR_LOCAL_DEBUG_SESSION));
             bOk = MIstatus::failure;
         }
     }
 
     // App is not quitting currently
     m_bExitApp = false;
 
+    // Handle source file
+    if (m_bHaveCommandFileNamePathOnCmdLine)
+    {
+        const bool bAsyncMode = false;
+        ExecuteCommandFile(bAsyncMode);
+    }
+
     // While the app is active
     while (bOk && !m_bExitApp)
     {
         CMIUtilString errorText;
         const MIchar *pCmd = m_rStdin.ReadLine (errorText);
         if (pCmd != nullptr)
         {
             CMIUtilString lineText(pCmd);

 //--
 bool
 CMIDriver::IsDriverDebuggingArgExecutable(void) const
 {
     return m_bDriverDebuggingArgExecutable;
 }
 
 //++ ------------------------------------------------------------------------------------
+// Details: Execute commands from prepared source file
+// Type:    Method.
+// Args:    vbAsyncMode       - (R) True = execute commands in asynchronous mode, false = otherwise.
+// Return:  MIstatus::success - Function succeeded.
+//          MIstatus::failure - Function failed.
+// Throws:  None.
+//--
+bool
+CMIDriver::ExecuteCommandFile(const bool vbAsyncMode)
+{
+    std::ifstream ifsStartScript(m_strCmdLineArgCommandFileNamePath.c_str());
+    if (!ifsStartScript.is_open())
+    {
+        const CMIUtilString errMsg(
+            CMIUtilString::Format(MIRSRC(IDS_UTIL_FILE_ERR_OPENING_FILE_UNKNOWN), m_strCmdLineArgCommandFileNamePath.c_str()));
+        SetErrorDescription(errMsg.c_str());
+        const bool bForceExit = true;
+        SetExitApplicationFlag(bForceExit);
+        return MIstatus::failure;
+    }
+
+    // Switch lldb to synchronous mode
+    CMICmnLLDBDebugSessionInfo &rSessionInfo(CMICmnLLDBDebugSessionInfo::Instance());
+    const bool bAsyncSetting = rSessionInfo.GetDebugger().GetAsync();
+    rSessionInfo.GetDebugger().SetAsync(vbAsyncMode);
+
+    // Execute commands from file
+    bool bOk = MIstatus::success;
+    CMIUtilString strCommand;
+    while (!m_bExitApp && std::getline(ifsStartScript, strCommand))
+    {
+        // Print command
+        bOk = CMICmnStreamStdout::TextToStdout(strCommand);
+
+        // Skip if it's a comment or empty line
+        if (strCommand.empty() || strCommand[0] == '#')
+            continue;
+
+        // Execute if no error
+        if (bOk)
+        {
+            CMIUtilThreadLock lock(rSessionInfo.GetSessionMutex());
+            bOk = InterpretCommand(strCommand);
+        }
+
+        // Draw the prompt after command will be executed (if enabled)
+        if (bOk && m_rStdin.GetEnablePrompt())
+            bOk = m_rStdOut.WriteMIResponse(m_rStdin.GetPrompt());
+
+        // Exit if there is an error
+        if (!bOk)
+        {
+            const bool bForceExit = true;
+            SetExitApplicationFlag(bForceExit);
+            break;
+        }
+
+        // Wait while the handler thread handles incoming events
+        CMICmnLLDBDebugger::Instance().WaitForHandleEvent();
+    }
+
+    // Switch lldb back to initial mode
+    rSessionInfo.GetDebugger().SetAsync(bAsyncSetting);
+
+    return bOk;
+}
+
+//++ ------------------------------------------------------------------------------------
 // Details: Gets called when lldb-mi gets a signal. Stops the process if it was SIGINT.
 //
 // Type:    Method.
 // Args:    signal that was delivered
 // Return:  None.
 // Throws:  None.
 //--
 void
 CMIDriver::DeliverSignal(int signal)
 {
     if (signal == SIGINT && (m_eCurrentDriverState == eDriverState_RunningDebugging))
         InterpretCommand("-exec-interrupt");
 }