Page MenuHomePhabricator

Fix a missing "*stopped" notification in LLDB-MI after "process launch -s" in case of remote-macosx
ClosedPublic

Authored by ki.stfu on Jan 29 2015, 3:12 PM.

Details

Summary

This patch fixes *stopped notification for remote target when started with eLaunchFlagStopAtEntry (for example, using "process launch -s").

See explanation below:

Target::Launch (ProcessLaunchInfo &launch_info, Stream *stream)
{
...
if (state != eStateConnected && platform_sp && platform_sp->CanDebugProcess ())
{
   ...
}
else
{
   ...
   if (m_process_sp)
      error = m_process_sp->Launch (launch_info);
}

if (error.Success())
{
    if (launch_info.GetFlags().Test(eLaunchFlagStopAtEntry) == false)
    {
        ....
    }
    -- missing event if eLaunchFlagStopAtEntry is set --
    m_process_sp->RestoreProcessEvents ();
}
...
return error

Also this patch contains tests and you can check how it works.

Diff Detail

Event Timeline

ki.stfu updated this revision to Diff 18995.Jan 29 2015, 3:12 PM
ki.stfu retitled this revision from to Fix a missing "*stopped" notification in LLDB-MI after "process launch -s" in case of remote-ios .
ki.stfu updated this object.
ki.stfu edited the test plan for this revision. (Show Details)
ki.stfu set the repository for this revision to rL LLVM.
ki.stfu added subscribers: Unknown Object (MLST), zturner.

Today I'll try to add a test case for it.

ki.stfu updated this revision to Diff 19216.Feb 3 2015, 6:04 AM
ki.stfu retitled this revision from Fix a missing "*stopped" notification in LLDB-MI after "process launch -s" in case of remote-ios to Fix a missing "*stopped" notification in LLDB-MI after "process launch -s" in case of remote-macosx.
ki.stfu updated this object.
ki.stfu added reviewers: zturner, abidh, clayborg.
ki.stfu added subscribers: abidh, clayborg.

add tests

ki.stfu added inline comments.Feb 3 2015, 7:07 AM
test/tools/lldb-mi/TestMiNotification.py
36 ↗(On Diff #19216)

I got an ImportError on another machine.

ki.stfu updated this revision to Diff 19218.Feb 3 2015, 7:18 AM

Fixed ImportError when run tests by using "./dotest.py -v --executable $BUILDDIR/bin/lldb tools/lldb-mi/".
Previously these tests were run by using "./dotest.py -v --executable $BUILDDIR/bin/lldb -f MiNotificationTestCase" command and tests were passed.

Can anyone take a look?

ki.stfu added inline comments.Feb 4 2015, 3:23 AM
source/Target/Process.cpp
3128

I looked again at event subsystem and I think here should be "HandlePrivateEvent (event_sp)".

clayborg accepted this revision.Feb 4 2015, 10:21 AM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Feb 4 2015, 10:21 AM
ki.stfu updated this revision to Diff 19356.EditedFeb 4 2015, 3:07 PM
ki.stfu edited edge metadata.
ki.stfu removed rL LLVM as the repository for this revision.

Update patch:

  • "HandlePrivateEvent(event_sp)" is used instead of "m_private_state_broadcaster.BroadcastEvent(event_sp)".
  • test/tools/lldb-mi/TestMiNotification.py was removed from this review because it will be added in http://reviews.llvm.org/D7410 soon.

Can anyone commit it for me?

UPDATE:
I can commit it by myself.

ki.stfu set the repository for this revision to rL LLVM.Feb 4 2015, 3:08 PM

Looks good.

ki.stfu updated this revision to Diff 19419.Feb 5 2015, 10:48 AM

Update patch:

  • Enable test_lldbmi_stopped_when_stopatentry_local test
  • Fix and enable test_lldbmi_stopped_when_stopatentry_remote test

Patch ready for submitting. All tests pass on OS X.

@abidh, Can you check these tests on Linux, please?

abidh edited edge metadata.Feb 6 2015, 7:01 AM

The test time outs for me on Linux (Ubuntu 14.04). There is some issue on Linux that we need to figure out. There are 2 stepping tests too which fails. I xfailed them yesterday. I will try to debug them next week.

Ok. But can I commit this patch?

abidh added a comment.Feb 6 2015, 7:47 AM

I dont have any objection if you xfail the tests on Linux. I can have a look at those fails later. But lets wait a few hours and see if Greg has any further comments.

Still looks good. Feel free to xfail the linux tests for now until you can figure out what is going on.

ki.stfu closed this revision.Feb 6 2015, 10:16 AM

@clayborg, @abidh,

Extra fixes were made in r228824