LLDB MI addition for getting process stopped at first instruction right after launch via -exec-run

[KLapshin]

Summary

In some cases debugger user may want to get debugged process stopped at very early point - at first instruction.

With lldb command line interpreter this may be done easily with exisiting option to "process launch" command:

process launch -s

Currently, if lldb-mi user want to use pure MI only - important for certain IDEs - there is no possibility to run debugged process and getting it stopped right on first instruction.

This patch is implementation of such option with --start option for -exec-run command, if feature supported on chosen host and/or target. For checking if --start option supported corresponding feature "exec-run-start-option" added to the list of options reported by -list-features command.

Example:

-list-features
^done,features=["data-read-memory-bytes","exec-run-start-option"]
(gdb)
-exec-run --start
...

This is optional addition, but may be useful for some developers.

[KLapshin]

Minor initial diff cleanup.

[dawn]

lgtm, but Ilia and Abid are more familiar with lldb-mi - please wait for their review.

[enlight]

According to the GDB-MI spec the exec-run command already has a start option. Support for the start option can be detected by checking for exec-run-start-option in the list of features returned by the list-features command. So, what's the rationale for diverging from the spec in this case?

In GDB your example would be written as:
-exec-run --start

tools/lldb-mi/MICmdCmdExec.cpp

92

The process should be launched through the SB API, not the command interpreter. I'm not very familiar with the SB API but I'd try using the lldb::eLaunchFlagStopAtEntry flag with SBLaunchInfo::SetLaunchFlags().

[brucem]

I think that enlight is correct in his comments and that this patch should be revised.

[KLapshin]

tools/lldb-mi/MICmdCmdExec.cpp

92

You are right, no doubt - this is clear what Target and Process API direct usage is faster and straightforward manner.

Just couple words regarding why interpreter used instead here - patch was prepared at moment when lldb-MI had lack synchronization (via Listener) with lldb Core, so setting corresponding flag gave random results - app may stop or not. Via interpreter it worked fine always - that's why I stated "more reliable" in review header.

I will check if flag pass is enough currently - this will make patch shorter and clearer.

[KLapshin]

In D12977#249437, @enlight wrote:

According to the GDB-MI spec the exec-run command already has a start option. Support for the start option can be detected by checking for exec-run-start-option in the list of features returned by the list-features command. So, what's the rationale for diverging from the spec in this case?

In GDB your example would be written as:
-exec-run --start

Fully agreed.

At same time - this option is relatively new, was not present in older GDBs. For example - GDB used in Xcode before switch to lldb just do stop at _dyld_start unconditionally always, no option.

Thanks for review, patch will be changed.

[KLapshin]

tools/lldb-mi/MICmdCmdExec.cpp

92

Also - in many other places lldb-MI still use interpreter for cmds.

[enlight]

tools/lldb-mi/MICmdCmdExec.cpp

92

Yes, it's unfortunate that the interpreter is used to implement some commands, this will be cleaned up in due time I hope. I just don't want new code using the interpreter unless absolutely necessary, it should be possible to (eventually) do everything through the SB API. If the SB API doesn't work as expected please file a bug,

[KLapshin]

As little summary what should be added to existing lldb-mi in context of this review:

• add "exec-run-start-option" feature to features currently reported by list-features MI command, behavior depends on platform - supported on iOS and OSX as stop at _dyld_start, on other platforms (Windows) - should work as run to main()
• add --start option to -exec-run, set StopAtEntry flag in LaunchInfo if --start option specified on input
• rework process launch in way SB target API only allowed,
• add tests

[KLapshin]

Patch reworked for suggested "-exec-run --start" manner, no "CLI" interpreter use and check if --start option supported via -list-features.

[KLapshin]

"CLI" intepreter not used in ExecRun handler in reworked patch.

[dawn]

lgtm. Much improved.

tools/lldb-mi/MICmdCmdSupportList.cpp

78 ↗

(On Diff #35740)

Fix comment: Some of features => Some features

[KLapshin]

@brucem, @enlight

Is this patch is fine for you now ?

[ki.stfu]

Update the summary + a few inline comments below.

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

13–31 ↗

(On Diff #35740)

And move this test case to test/tools/lldb-mi/control/TestMiExec.py

16 ↗

(On Diff #35740)

Rename it to

def test_lldbmi_exec_run(self):

28 ↗

(On Diff #35740)

Use lazy regex please:

self.expect("\*stopped,reason=\"signal-received\",signal-name=\"SIGSTOP\",signal-meaning=\"Stop\",.*?thread-id=\"1\",stopped-threads=\"all\"")

tools/lldb-mi/MICmdCmdExec.h

58 ↗

(On Diff #35740)

Please move it on few lines above for consistency with others:

bool ParseArgs() override;
bool Execute() override;
[...]

[KLapshin]

Requested changes applied, updated patch uploaded.

tools/lldb-mi/MICmdCmdExec.h

58 ↗

(On Diff #35865)

Ilia, I checked string positions for ParseArgs() method in MICmdCmdExec.h and other command headers - ParseArgs() placed always third, so this change done in accordance with current, at least public, lldb-mi headers and minimal patching as possible.

What inconsistency you mentioned ?

Please take a look on ExecRun command class modified with ParseArgs() method added and non-modified classes in lldb-mi - ExecFinish or ExecNext, for example:

class CMICmdCmdExecRun : public CMICmdBase
{
    // Statics:
  public:
    // Required by the CMICmdFactory when registering *this command
    static CMICmdBase *CreateSelf();

    // Methods:
  public:
    /* ctor */ CMICmdCmdExecRun();

    // Overridden:
  public:
    // From CMICmdInvoker::ICmd
    bool Execute() override;
    bool Acknowledge() override;
    bool ParseArgs() override;
    // From CMICmnBase
    /* dtor */ ~CMICmdCmdExecRun() override;

    // Attributes:
  private:
    lldb::SBCommandReturnObject m_lldbResult;
    const CMIUtilString m_constStrArgStart; // StopAtEntry - run to first instruction or main(), just run process if not specified
};

class CMICmdCmdExecFinish : public CMICmdBase
{
    // Statics:
  public:
    // Required by the CMICmdFactory when registering *this command
    static CMICmdBase *CreateSelf();

    // Methods:
  public:
    /* ctor */ CMICmdCmdExecFinish();

    // Overridden:
  public:
    // From CMICmdInvoker::ICmd
    bool Execute() override;
    bool Acknowledge() override;
    bool ParseArgs() override;  <---
    // From CMICmnBase
    /* dtor */ ~CMICmdCmdExecFinish() override;

    // Attributes:
  private:
    lldb::SBCommandReturnObject m_lldbResult;
    const CMIUtilString m_constStrArgThread; // Not specified in MI spec but Eclipse gives this option
    const CMIUtilString m_constStrArgFrame;  // Not specified in MI spec but Eclipse gives this option
};

[ki.stfu]

lgtm apart a lack of description about the changes in tools/lldb-mi/MICmdCmdSupportList.cpp

tools/lldb-mi/MICmdCmdExec.h

58 ↗

(On Diff #35865)

Ok, I see that it's already in consistency with others. I was baffled because ::ParseArgs() is followed by ::Execute() in source files.

[ki.stfu]

tools/lldb-mi/MICmdCmdExec.h

64 ↗

(On Diff #35865)

I checked that it's unused starting from r215656. Remove this please.

[ki.stfu]

Could you make one additional cleanup here please?

[KLapshin]

In D12977#255390, @ki.stfu wrote:

lgtm apart a lack of description about the changes in tools/lldb-mi/MICmdCmdSupportList.cpp

I added description about changes in list-features cmd handler in review summary. In source two comments present already.

tools/lldb-mi/MICmdCmdExec.h

64 ↗

(On Diff #35969)

Yes, @abidh added process start via Target Launch() method instead of "CLI" process command on 14 Aug 2014. But he forgot to remove m_lldbResult member usage (checking for error string has non-zero size) in Acknowledge() method. I removed this member usage finally.

BTW - as ExecRun is sync only command by its meaning (process started or not), Acknowledge() method called only if Execute() method reported successful status on completion, so we can skip any Process creation checks in Acknowledge(), thus m_lldbResult really not needed.

[KLapshin]

m_lldbResult usage has beed removed in ExecRun::Acknowledge() method, corresponding member removed from cmd class.

[ki.stfu]

test/tools/lldb-mi/control/TestMiExec.py

16 ↗

(On Diff #35969)

This test doesn't pass on Linux due to missing *stopped notification:

$ bin/lldb-mi echo
(gdb)
-file-exec-and-symbols "echo"
^done
(gdb)
=library-loaded,id="/bin/echo",target-name="/bin/echo",host-name="/bin/echo",symbols-loaded="0",loaded_addr="-",size="0"
-exec-run --start
^running
=thread-group-started,id="i1",pid="28031"
(gdb)
=thread-created,id="1",group-id="i1"
=thread-selected,id="1"
(gdb)
=library-loaded,id="/bin/echo",target-name="/bin/echo",host-name="/bin/echo",symbols-loaded="0",loaded_addr="-",size="0"
process status
Process 28031 stopped
* thread #1: tid = 28031, 0x00007ffff7dd9cd0, name = 'echo', stop reason = signal SIGSTOP
    frame #0: 0x00007ffff7dd9cd0
->  0x7ffff7dd9cd0: movq   %rsp, %rdi
    0x7ffff7dd9cd3: callq  0x7ffff7dddc30
    0x7ffff7dd9cd8: movq   %rax, %r12
    0x7ffff7dd9cdb: movl   0x22310f(%rip), %eax
^done
(gdb)

You can XFAIL this test case with the link to the corresponding issue on bug tracker (create a new one if needed).

[KLapshin]

test/tools/lldb-mi/control/TestMiExec.py

16 ↗

(On Diff #35969)

Ilia, thank you for checking on Linux - I just don't have lldb Linux build on hands yet, will setup it and do checks on Linux as well.

Will mark test as expected to fail on Linux yet with reference to issue on bug tracker.

[KLapshin]

test/tools/lldb-mi/control/TestMiExec.py

16 ↗

(On Diff #35969)

Bug about missing *stopped notification on Linux platform has been entered: https://llvm.org/bugs/show_bug.cgi?id=25000

[KLapshin]

Test has been marked as XFAILed for Linux (x86_64) case.

[KLapshin]

test/tools/lldb-mi/control/TestMiExec.py

16 ↗

(On Diff #36128)

It looks like lldb-mi didn't received SIGSTOP evbent on Linux platform because event was not rebroadcasted because "Process::WaitForProcessToStop (timeout = (nil))
Process::WaitForProcessToStop returning without waiting for events; process private and public states are already 'stopped'.
" - see full log (process, event channel) attached to llvm.org/pr25000 bug.

[ki.stfu]

lgtm

test/tools/lldb-mi/control/TestMiExec.py

16 ↗

(On Diff #36128)

just use

@expectedFailureLinux # llvm.org/pr25000: lldb-mi does not receive broadcasted notification from Core/Process about process stopped ...

test/tools/lldb-mi/TestMiGdbSetShow.py

         self.runCmd("-gdb-show unknown")
         self.expect("\^error")
 
+    @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_gdb_set_process_stopatentry_on(self):
+        """Test that 'lldb-mi --interpreter' can stop at entry."""
 
+        self.spawnLldbMi(args = None)
+
+        # Test that -gdb-set can set process-stopatentry flag
+        self.runCmd("-gdb-set process-stopatentry on")
+        self.expect("\^done")
+        self.runCmd("-gdb-show process-stopatentry")
+        self.expect("\^done,value=\"on\"")
+
+        # Load executable
+        self.runCmd("-file-exec-and-symbols %s" % self.myexe)
+        self.expect("\^done")
+
+        # Test that program is stopped at entry
+        self.runCmd("-exec-run")
+        self.expect("\^running")
+        self.expect("\*stopped,reason=\"signal-received\",signal-name=\"SIGSTOP\",signal-meaning=\"Stop\",.*thread-id=\"1\",stopped-threads=\"all\"")
+
+        # Test that lldb-mi is ready to execute next commands
+        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_gdb_show_process_stopatentry(self):
+        """Test that 'lldb-mi --interpreter' starts without process-stopatentry by default."""
+
+        self.spawnLldbMi(args = None)
+
+        # Test that default process-stopatentry value is "off"
+        self.runCmd("-gdb-show process-stopatentry")
+        self.expect("\^done,value=\"off\"")
+
+    @lldbmi_test
+    @expectedFailureWindows("llvm.org/pr22274: need a pexpect replacement for windows")
+    @skipIfFreeBSD # llvm.org/pr22411: Failure presumably due to known thread races
     @skipIfLinux # llvm.org/pr22841: lldb-mi tests fail on all Linux buildbots
     def test_lldbmi_gdb_set_ouptut_radix(self):
         """Test that 'lldb-mi --interpreter' works for -gdb-set output-radix."""

tools/lldb-mi/MICmdCmdExec.cpp

     CMICmnLLDBDebugSessionInfo &rSessionInfo(CMICmnLLDBDebugSessionInfo::Instance());
     lldb::SBError error;
     lldb::SBStream errMsg;
-    lldb::SBLaunchInfo launchInfo = rSessionInfo.GetTarget().GetLaunchInfo();
-    launchInfo.SetListener(rSessionInfo.GetListener());
-    lldb::SBProcess process = rSessionInfo.GetTarget().Launch(launchInfo, error);
-    if ((!process.IsValid()) || (error.Fail()))
+    lldb::SBProcess process;
+    bool bProcessMustStopAtEntry = false;
+
+    bProcessMustStopAtEntry = (rSessionInfo.SharedDataRetrieve<bool>(rSessionInfo.m_constStrProcessStopAtEntry, bProcessMustStopAtEntry)
+                               && bProcessMustStopAtEntry);
     {
-        SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_INVALID_PROCESS), m_cmdData.strMiCmd.c_str(), errMsg.GetData()));
-        return MIstatus::failure;
+        const char *pCmd = bProcessMustStopAtEntry ? "process launch -s" : "process launch";
+        const lldb::ReturnStatus rtn = rSessionInfo.GetDebugger().GetCommandInterpreter().HandleCommand(pCmd, m_lldbResult);
+        MIunused(rtn);
+
+        if (!rSessionInfo.GetProcess().IsValid())
+        {
+            SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_INVALID_PROCESS),
+                                           m_cmdData.strMiCmd.c_str(), errMsg.GetData()));
+            return MIstatus::failure;
+        }
+
+        if (m_lldbResult.GetErrorSize() == 0)
+        {
+            if (!CMIDriver::Instance().SetDriverStateRunningDebugging())
+            {
+                const CMIUtilString &rErrMsg(CMIDriver::Instance().GetErrorDescription());
+                SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_SET_NEW_DRIVER_STATE),
+                                               m_cmdData.strMiCmd.c_str(), rErrMsg.c_str()));
+                return MIstatus::failure;
+            }
+        }
+        else
+        {
+            // FIXME: Re-evaluate if this is required when the application is near finished
+            // as this parses the LLDB error message which is a hack and is brittle
+            const char *pLldbErr = m_lldbResult.GetError();
+            const CMIUtilString strLldbMsg(CMIUtilString(pLldbErr).StripCREndOfLine());
+            if (strLldbMsg == "error: Process must be launched.")
+            {
+                CMIDriver::Instance().SetExitApplicationFlag(true);
+            }
+        }
     }
 
     if (!CMIDriver::Instance().SetDriverStateRunningDebugging())

tools/lldb-mi/MICmdCmdGdbSet.h

     bool OptionFnSolibSearchPath(const CMIUtilString::VecString_t &vrWords);
     bool OptionFnOutputRadix(const CMIUtilString::VecString_t &vrWords);
     bool OptionFnFallback(const CMIUtilString::VecString_t &vrWords);
+    bool OptionFnProcessStopAtEntry(const CMIUtilString::VecString_t &vrWords);
 
     // Attributes:
   private:

tools/lldb-mi/MICmdCmdGdbSet.cpp

 // Instantiations:
 const CMICmdCmdGdbSet::MapGdbOptionNameToFnGdbOptionPtr_t CMICmdCmdGdbSet::ms_mapGdbOptionNameToFnGdbOptionPtr = {
     {"target-async", &CMICmdCmdGdbSet::OptionFnTargetAsync},
+    {"process-stopatentry", &CMICmdCmdGdbSet::OptionFnProcessStopAtEntry},
     {"print", &CMICmdCmdGdbSet::OptionFnPrint},
     // { "auto-solib-add", &CMICmdCmdGdbSet::OptionFnAutoSolibAdd },    // Example code if need to implement GDB set other options
     {"output-radix", &CMICmdCmdGdbSet::OptionFnOutputRadix},

 }
 
 //++ ------------------------------------------------------------------------------------
+// Details: Carry out work to complete the GDB set option 'process-stopatentry' to prepare
+//          and send back information asked for.
+// Type:    Method.
+// Args:    vrWords - (R) List of additional parameters used by this option.
+// Return:  MIstatus::success - Function succeeded.
+//          MIstatus::failure - Function failed.
+// Throws:  None.
+//--
+bool
+CMICmdCmdGdbSet::OptionFnProcessStopAtEntry(const CMIUtilString::VecString_t &vrWords)
+{
+    const bool bEmptyArg(vrWords.empty());
+    const bool bArgOn(bEmptyArg || CMIUtilString::Compare(vrWords[0], "on"));
+    const bool bArgOff(!bEmptyArg && CMIUtilString::Compare(vrWords[0], "off"));
+    if (!bArgOn && !bArgOff)
+    {
+        m_bGbbOptionFnHasError = true;
+        m_strGdbOptionFnError = MIRSRC(IDS_CMD_ERR_GDBSET_OPT_PROCESS_STOPATENTRY);
+        return MIstatus::failure;
+    }
+
+    // Save process stopatentry launch flag 
+    const bool bProcessStopAtEntry = bArgOn;
+
+    // Add process stopatentry flag to the shared data list
+    const CMIUtilString &rStrKeyProcessStopAtEntry(m_rLLDBDebugSessionInfo.m_constStrProcessStopAtEntry);
+    if (!m_rLLDBDebugSessionInfo.SharedDataAdd<bool>(rStrKeyProcessStopAtEntry, bProcessStopAtEntry))
+    {
+        m_bGbbOptionFnHasError = false;
+        SetError(CMIUtilString::Format(MIRSRC(IDS_DBGSESSION_ERR_SHARED_DATA_ADD), m_cmdData.strMiCmd.c_str(), rStrKeyProcessStopAtEntry.c_str()));
+        return MIstatus::failure;
+    }
+
+    return MIstatus::success;
+}
+
+//++ ------------------------------------------------------------------------------------
 // Details: Carry out work to complete the GDB set option 'output-radix' to prepare
 //          and send back information asked for.
 // Type:    Method.

tools/lldb-mi/MICmdCmdGdbShow.h

     bool OptionFnPrint(const CMIUtilString::VecString_t &vrWords);
     bool OptionFnLanguage(const CMIUtilString::VecString_t &vrWords);
     bool OptionFnFallback(const CMIUtilString::VecString_t &vrWords);
+    bool OptionFnProcessStopAtEntry(const CMIUtilString::VecString_t &vrWords);
 
     // Attributes:
   private:

tools/lldb-mi/MICmdCmdGdbShow.cpp

 // Instantiations:
 const CMICmdCmdGdbShow::MapGdbOptionNameToFnGdbOptionPtr_t CMICmdCmdGdbShow::ms_mapGdbOptionNameToFnGdbOptionPtr = {
     {"target-async", &CMICmdCmdGdbShow::OptionFnTargetAsync},
+    {"process-stopatentry", &CMICmdCmdGdbShow::OptionFnProcessStopAtEntry},
     {"print", &CMICmdCmdGdbShow::OptionFnPrint},
     {"language", &CMICmdCmdGdbShow::OptionFnLanguage},
     {"fallback", &CMICmdCmdGdbShow::OptionFnFallback}};

 }
 
 //++ ------------------------------------------------------------------------------------
+// Details: Carry out work to complete the GDB show option 'process-stopatentry' to prepare
+//          and send back the requested information.
+// Type:    Method.
+// Args:    vrWords - (R) List of additional parameters used by this option.
+// Return:  MIstatus::success - Function succeeded.
+//          MIstatus::failure - Function failed.
+// Throws:  None.
+//--
+bool
+CMICmdCmdGdbShow::OptionFnProcessStopAtEntry(const CMIUtilString::VecString_t &vrWords)
+{
+    MIunused(vrWords);
+
+    CMICmnLLDBDebugSessionInfo &rSessionInfo(CMICmnLLDBDebugSessionInfo::Instance());
+
+    bool bProcessMustStopAtEntry = false;
+
+    // Get current process stopatentry flag value and set result string
+    m_strValue = (rSessionInfo.SharedDataRetrieve<bool>(rSessionInfo.m_constStrProcessStopAtEntry, bProcessMustStopAtEntry)
+             && bProcessMustStopAtEntry) ? "on" : "off";
+
+    return MIstatus::success;
+}
+
+//++ ------------------------------------------------------------------------------------
 // Details: Carry out work to complete the GDB show option 'print' to prepare and send
 //          back the requested information.
 // Type:    Method.

tools/lldb-mi/MICmnLLDBDebugSessionInfo.h

     const CMIUtilString m_constStrPrintCharArrayAsString;
     const CMIUtilString m_constStrPrintExpandAggregates;
     const CMIUtilString m_constStrPrintAggregateFieldNames;
+    const CMIUtilString m_constStrProcessStopAtEntry;
 
     // Typedefs:
   private:

tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp

     , m_constStrSharedDataSolibPath("Solib Path")
     , m_constStrPrintCharArrayAsString("Print CharArrayAsString")
     , m_constStrPrintExpandAggregates("Print ExpandAggregates")
+    , m_constStrProcessStopAtEntry("Process StopAtEntry")
     , m_constStrPrintAggregateFieldNames("Print AggregateFieldNames")
 {
 }

tools/lldb-mi/MICmnResources.h

     IDS_CMD_ERR_GDBSET_OPT_SOLIBSEARCHPATH,
     IDS_CMD_ERR_GDBSET_OPT_PRINT_BAD_ARGS,
     IDS_CMD_ERR_GDBSET_OPT_PRINT_UNKNOWN_OPTION,
+    IDS_CMD_ERR_GDBSET_OPT_PROCESS_STOPATENTRY,
     IDS_CMD_ERR_GDBSHOW_OPT_PRINT_BAD_ARGS,
     IDS_CMD_ERR_GDBSHOW_OPT_PRINT_UNKNOWN_OPTION,
     IDS_CMD_ERR_EXPR_INVALID,

tools/lldb-mi/MICmnResources.cpp

     {IDS_CMD_ERR_GDBSET_OPT_SOLIBSEARCHPATH, "'solib-search-path' requires at least one argument"},
     {IDS_CMD_ERR_GDBSET_OPT_PRINT_BAD_ARGS, "'print' expects option-name and \"on\" or \"off\""},
     {IDS_CMD_ERR_GDBSET_OPT_PRINT_UNKNOWN_OPTION, "'print' error. The option '%s' not found"},
+    {IDS_CMD_ERR_GDBSET_OPT_PROCESS_STOPATENTRY, "'process-stopatentry' expects \"on\" or \"off\""},
     {IDS_CMD_ERR_GDBSHOW_OPT_PRINT_BAD_ARGS, "'print' expects option-name and \"on\" or \"off\""},
     {IDS_CMD_ERR_GDBSHOW_OPT_PRINT_UNKNOWN_OPTION, "'print' error. The option '%s' not found"},
     {IDS_CMD_ERR_EXPR_INVALID, "Failed to evaluate expression: %s"},