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 ↗

(On Diff #35109)

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 ↗

(On Diff #35109)

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 ↗

(On Diff #35109)

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

[enlight]

tools/lldb-mi/MICmdCmdExec.cpp

92 ↗

(On Diff #35109)

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 ...

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

 """
 Test lldb-mi -exec-xxx commands.
 """
 
 import lldbmi_testcase
 from lldbtest import *
 import unittest2
 
 class MiExecTestCase(lldbmi_testcase.MiTestCaseBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
     @lldbmi_test
     @skipIfWindows #llvm.org/pr24452: Get lldb-mi tests working on Windows
     @skipIfFreeBSD # llvm.org/pr22411: Failure presumably due to known thread races
+    @expectedFailureLinux # llvm.org/pr25000: lldb-mi does not receive broadcasted notification from Core/Process about process stopped
+    def test_lldbmi_exec_run(self):
+        """Test that 'lldb-mi --interpreter' can stop at entry."""
+
+        self.spawnLldbMi(args = None)
+
+        # 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 --start")
+        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
+    @skipIfWindows #llvm.org/pr24452: Get lldb-mi tests working on Windows
+    @skipIfFreeBSD # llvm.org/pr22411: Failure presumably due to known thread races
     @expectedFailureAll("llvm.org/pr23139", oslist=["linux"], compiler="gcc", compiler_version=[">=","4.9"], archs=["i386"])
     def test_lldbmi_exec_abort(self):
         """Test that 'lldb-mi --interpreter' works for -exec-abort."""
 
         self.spawnLldbMi(args = None)
 
         # Test that -exec-abort fails on invalid process
         self.runCmd("-exec-abort")

lldb/trunk/tools/lldb-mi/MICmdCmdExec.h

class CMICmdCmdExecRun : public CMICmdBase
   public:
     /* ctor */ CMICmdCmdExecRun();
 
     // Overridden:
   public:
     // From CMICmdInvoker::ICmd
     bool Execute() override;
     bool Acknowledge() override;
+    bool ParseArgs() override;
     // From CMICmnBase
     /* dtor */ ~CMICmdCmdExecRun() override;
 
     // Attributes:
   private:
+    const CMIUtilString m_constStrArgStart; // StopAtEntry - run to first instruction or main() if specified
     lldb::SBCommandReturnObject m_lldbResult;
 };
 
 //++ ============================================================================
 // Details: MI command class. MI commands derived from the command base class.
 //          *this class implements MI command "exec-continue".
 //--
 class CMICmdCmdExecContinue : public CMICmdBase

lldb/trunk/tools/lldb-mi/MICmdCmdExec.cpp

 //++ ------------------------------------------------------------------------------------
 // Details: CMICmdCmdExecRun constructor.
 // Type:    Method.
 // Args:    None.
 // Return:  None.
 // Throws:  None.
 //--
 CMICmdCmdExecRun::CMICmdCmdExecRun()
+    : m_constStrArgStart("start")
 {
     // Command factory matches this name with that received from the stdin stream
     m_strMiCmd = "exec-run";
 
     // Required by the CMICmdFactory when registering *this command
     m_pSelfCreatorFn = &CMICmdCmdExecRun::CreateSelf;
 }
 
 //++ ------------------------------------------------------------------------------------
 // Details: CMICmdCmdExecRun destructor.
 // Type:    Overrideable.
 // Args:    None.
 // Return:  None.
 // Throws:  None.
 //--
 CMICmdCmdExecRun::~CMICmdCmdExecRun()
 {
 }
 
 //++ ------------------------------------------------------------------------------------
+// Details: The invoker requires this function. It parses the command line options'
+//          arguments to extract values for each of those arguments.
+// Type:    Overridden.
+// Args:    None.
+// Return:  MIstatus::success - Functional succeeded.
+//          MIstatus::failure - Functional failed.
+// Throws:  None.
+//--
+bool
+CMICmdCmdExecRun::ParseArgs()
+{
+    m_setCmdArgs.Add(
+        new CMICmdArgValOptionLong(m_constStrArgStart, false, true, CMICmdArgValListBase::eArgValType_OptionLong, 0));
+    return ParseValidateCmdOptions();
+}
+
+//++ ------------------------------------------------------------------------------------
 // Details: The invoker requires this function. The command does work in this function.
 //          The command is likely to communicate with the LLDB SBDebugger in here.
 // Type:    Overridden.
 // Args:    None.
 // Return:  MIstatus::success - Functional succeeded.
 //          MIstatus::failure - Functional failed.
 // Throws:  None.
 //--
 bool
 CMICmdCmdExecRun::Execute()
 {
     CMICmnLLDBDebugSessionInfo &rSessionInfo(CMICmnLLDBDebugSessionInfo::Instance());
     lldb::SBError error;
     lldb::SBStream errMsg;
     lldb::SBLaunchInfo launchInfo = rSessionInfo.GetTarget().GetLaunchInfo();
     launchInfo.SetListener(rSessionInfo.GetListener());
+
+    // Run to first instruction or main() requested?
+    CMICMDBASE_GETOPTION(pArgStart, OptionLong, m_constStrArgStart);
+    if (pArgStart->GetFound())
+    {
+        launchInfo.SetLaunchFlags(launchInfo.GetLaunchFlags() | lldb::eLaunchFlagStopAtEntry);
+    }
+
     lldb::SBProcess process = rSessionInfo.GetTarget().Launch(launchInfo, error);
     if ((!process.IsValid()) || (error.Fail()))
     {
         SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_INVALID_PROCESS), m_cmdData.strMiCmd.c_str(), errMsg.GetData()));
         return MIstatus::failure;
     }
 
     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;
     }
     return MIstatus::success;
 }
 
 //++ ------------------------------------------------------------------------------------
 // Details: The invoker requires this function. The command prepares a MI Record Result
 //          for the work carried out in the Execute().
+//          Called only if Execute() set status as successful on completion.
 // Type:    Overridden.
 // Args:    None.
 // Return:  MIstatus::success - Functional succeeded.
 //          MIstatus::failure - Functional failed.
 // Throws:  None.
 //--
 bool
 CMICmdCmdExecRun::Acknowledge()
 {
-    if (m_lldbResult.GetErrorSize() > 0)
-    {
-        const CMICmnMIValueConst miValueConst(m_lldbResult.GetError());
-        const CMICmnMIValueResult miValueResult("message", miValueConst);
-        const CMICmnMIResultRecord miRecordResult(m_cmdData.strMiCmdToken, CMICmnMIResultRecord::eResultClass_Error, miValueResult);
-        m_miResultRecord = miRecordResult;
-    }
-    else
-    {
-        const CMICmnMIResultRecord miRecordResult(m_cmdData.strMiCmdToken, CMICmnMIResultRecord::eResultClass_Running);
-        m_miResultRecord = miRecordResult;
 
-        CMICmnLLDBDebugSessionInfo &rSessionInfo(CMICmnLLDBDebugSessionInfo::Instance());
-        lldb::pid_t pid = rSessionInfo.GetProcess().GetProcessID();
-        // Give the client '=thread-group-started,id="i1" pid="xyz"'
-        m_bHasResultRecordExtra = true;
-        const CMICmnMIValueConst miValueConst2("i1");
-        const CMICmnMIValueResult miValueResult2("id", miValueConst2);
-        const CMIUtilString strPid(CMIUtilString::Format("%lld", pid));
-        const CMICmnMIValueConst miValueConst(strPid);
-        const CMICmnMIValueResult miValueResult("pid", miValueConst);
-        CMICmnMIOutOfBandRecord miOutOfBand(CMICmnMIOutOfBandRecord::eOutOfBand_ThreadGroupStarted, miValueResult2);
-        miOutOfBand.Add(miValueResult);
-        m_miResultRecordExtra = miOutOfBand.GetString();
-    }
 
     return MIstatus::success;
 }
 
 //++ ------------------------------------------------------------------------------------
 // Details: Required by the CMICmdFactory when registering *this command. The factory
 //          calls this function to create an instance of *this command.
 // Type:    Static method.

lldb/trunk/tools/lldb-mi/MICmdCmdSupportList.cpp

 // Args:    None.
 // Return:  MIstatus::success - Functional succeeded.
 //          MIstatus::failure - Functional failed.
 // Throws:  None.
 //--
 bool
 CMICmdCmdSupportListFeatures::Acknowledge()
 {
-    const CMICmnMIValueConst miValueConst("data-read-memory-bytes");
-    const CMICmnMIValueList miValueList(miValueConst);
+    // Declare supported features here
+    const CMICmnMIValueConst miValueConst1("data-read-memory-bytes");
+    const CMICmnMIValueConst miValueConst2("exec-run-start-option");
+    // Some features may depend on host and/or target, decide what to add below
+    CMICmnMIValueList miValueList(true);
+    miValueList.Add(miValueConst1);
+    miValueList.Add(miValueConst2);
     const CMICmnMIValueResult miValueResult("features", miValueList);
     const CMICmnMIResultRecord miRecordResult(m_cmdData.strMiCmdToken, CMICmnMIResultRecord::eResultClass_Done, miValueResult);
     m_miResultRecord = miRecordResult;
 
     return MIstatus::success;
 }
 
 //++ ------------------------------------------------------------------------------------