Page MenuHomePhabricator

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

Authored by KLapshin on Sep 18 2015, 10:48 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

KLapshin updated this revision to Diff 35107.Sep 18 2015, 10:48 AM
KLapshin retitled this revision from to LLDB MI addition for getting process stopped at first instruction right after launch via -exec-run.
KLapshin updated this object.
KLapshin added reviewers: dawn, ki.stfu, abidh.
KLapshin set the repository for this revision to rL LLVM.
KLapshin added a subscriber: lldb-commits.
KLapshin updated this object.Sep 18 2015, 10:50 AM
KLapshin updated this revision to Diff 35109.Sep 18 2015, 10:59 AM
KLapshin updated this object.
KLapshin removed rL LLVM as the repository for this revision.

Minor initial diff cleanup.

KLapshin set the repository for this revision to rL LLVM.Sep 18 2015, 11:00 AM
dawn edited edge metadata.Sep 18 2015, 12:42 PM

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

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 requested changes to this revision.Sep 19 2015, 11:13 PM
brucem added a reviewer: brucem.
brucem added a subscriber: brucem.

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

This revision now requires changes to proceed.Sep 19 2015, 11:13 PM
KLapshin added inline comments.Sep 21 2015, 10:55 AM
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.

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 added inline comments.Sep 21 2015, 11:04 AM
tools/lldb-mi/MICmdCmdExec.cpp
92 ↗(On Diff #35109)

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

enlight added inline comments.Sep 21 2015, 7:35 PM
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 added a comment.EditedSep 23 2015, 10:52 AM

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 updated this revision to Diff 35740.Sep 25 2015, 11:05 AM
KLapshin updated this object.
KLapshin edited edge metadata.

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

KLapshin marked an inline comment as done.Sep 25 2015, 11:07 AM

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

KLapshin edited edge metadata.Sep 25 2015, 11:15 AM
KLapshin edited subscribers, added: evgeny777; removed: brucem.
brucem added a subscriber: brucem.Sep 25 2015, 11:23 AM
dawn accepted this revision.Sep 25 2015, 2:14 PM
dawn edited edge metadata.

lgtm. Much improved.

tools/lldb-mi/MICmdCmdSupportList.cpp
78 ↗(On Diff #35740)

Fix comment: Some of features => Some features

@brucem, @enlight

Is this patch is fine for you now ?

ki.stfu requested changes to this revision.Sep 26 2015, 3:32 AM
ki.stfu edited edge metadata.

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;
[...]
This revision now requires changes to proceed.Sep 26 2015, 3:32 AM
KLapshin updated this revision to Diff 35865.Sep 28 2015, 7:06 AM
KLapshin updated this object.
KLapshin edited edge metadata.
KLapshin marked an inline comment as done.Sep 28 2015, 7:13 AM

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 accepted this revision.Sep 29 2015, 12:57 AM
ki.stfu edited edge metadata.

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 requested changes to this revision.Sep 29 2015, 1:15 AM
ki.stfu edited edge metadata.
ki.stfu added inline comments.
tools/lldb-mi/MICmdCmdExec.h
64 ↗(On Diff #35865)

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

This revision now requires changes to proceed.Sep 29 2015, 1:15 AM

Could you make one additional cleanup here please?

KLapshin updated this revision to Diff 35969.Sep 29 2015, 7:26 AM
KLapshin updated this object.
KLapshin edited edge metadata.
KLapshin marked an inline comment as done.Sep 29 2015, 7:34 AM

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 marked an inline comment as done.Sep 29 2015, 7:36 AM

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

ki.stfu requested changes to this revision.Sep 29 2015, 8:34 AM
ki.stfu edited edge metadata.
ki.stfu added inline comments.
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).

This revision now requires changes to proceed.Sep 29 2015, 8:34 AM
KLapshin added inline comments.Sep 29 2015, 11:29 AM
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 added inline comments.Sep 30 2015, 10:38 AM
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 updated this revision to Diff 36128.Sep 30 2015, 10:44 AM
KLapshin edited edge metadata.

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

KLapshin marked an inline comment as done.Sep 30 2015, 10:45 AM
KLapshin added inline comments.Sep 30 2015, 10:50 AM
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 accepted this revision.Sep 30 2015, 10:02 PM
ki.stfu edited edge metadata.

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 ...
This revision was automatically updated to reflect the committed changes.