Page MenuHomePhabricator

Fix handling of CommandInterpreter's events in lldb-mi
ClosedPublic

Authored by ki.stfu on Mar 17 2015, 10:20 AM.

Details

Summary

Previously lldb-mi contains a stub for that but it didn't work and all CommanInterpreter's events were ignored.
This commit adds a handling of CommandInterpreter's events in lldb-mi.

Steps:

  1. Fix CMICmnLLDBDebugger::InitSBListener
  2. Add SBCommandInterpreter::EventIsCommandInterpreterEvent
  3. Exit on lldb::SBCommandInterpreter::eBroadcastBitQuitCommandReceived

All tests pass on OS X.

In further we can remove "quit" hack in lldb-mi.

Diff Detail

Repository
rL LLVM

Event Timeline

ki.stfu updated this revision to Diff 22101.Mar 17 2015, 10:20 AM
ki.stfu retitled this revision from to Fix handling of CommandInterpreter's events in lldb-mi.
ki.stfu updated this object.
ki.stfu edited the test plan for this revision. (Show Details)
ki.stfu added reviewers: abidh, clayborg.
ki.stfu added subscribers: abidh, clayborg, Unknown Object (MLST).

Friendly ping

abidh accepted this revision.Mar 19 2015, 2:52 AM
abidh edited edge metadata.

MI part look ok. Greg will have to approve the API part.

(gdb)
*stopped,reason="breakpoint-hit"
(gdb)<press Enter>

Even with this patch, we still need to press enter once to exit as other thread has been blocked in fgets. So we will keep handling the "quit" in MI as we do currently.

MI: Program exited OK

I think we should remove this message.

tools/lldb-mi/MICmnLLDBDebugger.cpp
300 ↗(On Diff #22101)

Why we have to do this for command interpreter?

This revision is now accepted and ready to land.Mar 19 2015, 2:52 AM
In D8382#143198, @abidh wrote:

(gdb)
*stopped,reason="breakpoint-hit"
(gdb)<press Enter>

Even with this patch, we still need to press enter once to exit as other thread has been blocked in fgets. So we will keep handling the "quit" in MI as we do currently.

I think it's leak of lldb-mi architecture. Even if we keep "quit" handling in DoMainLoop we still need an extra Enter in case of source file with prepared commands.

MI: Program exited OK

I think we should remove this message.

Ok. I'll do it.

tools/lldb-mi/MICmnLLDBDebugger.cpp
300 ↗(On Diff #22101)

Because CommandInterpreter object already has been created in contrast to Target/Thread/Process.

ki.stfu requested a review of this revision.Mar 19 2015, 9:32 AM
ki.stfu edited edge metadata.

@clayborg, take a look please.

clayborg requested changes to this revision.Mar 19 2015, 11:32 AM
clayborg edited edge metadata.

SBBroadcaster::CheckInWithManager() should not be exposed as no one should have to worry about this from a public API perspective. CommandInterpreter::CommandInterpreter() calls CheckInWithManager() already.

include/lldb/API/SBBroadcaster.h
58–60 ↗(On Diff #22101)

Remove this. See comments below.

source/API/SBBroadcaster.cpp
155–161 ↗(On Diff #22101)

Remove this. You shouldn't have to do this. CommandInterpreter::CommandInterpreter() already calls CheckInWithManager ();

source/API/SBCommandInterpreter.cpp
554 ↗(On Diff #22101)

The strings are returns as the "const char *" from a lldb_private::ConstString object, so just do a pointer compare instead of a strcmp.

tools/lldb-mi/MICmnLLDBDebugger.cpp
300 ↗(On Diff #22101)

You shouldn't have to do this, it is being done in the lldb_private::CommandInterpreter::CommandInterpreter(). How did you manage to not contract a CommandInterpreter and yet you are able to listen to events from it?

This revision now requires changes to proceed.Mar 19 2015, 11:32 AM
ki.stfu requested a review of this revision.Mar 20 2015, 7:47 AM
ki.stfu edited edge metadata.

@clayborg, see below:

source/API/SBCommandInterpreter.cpp
554 ↗(On Diff #22101)

I think it's wrong.

file1.cpp:

const char s1 [] = "abc";
file2.cpp:

extern const char s1 [];
const char *s2 = "abc";
assert (s1==s2); // error
assert (!strcmp (s1,s2)); // ok

I'd prefer to keep it as is.

tools/lldb-mi/MICmnLLDBDebugger.cpp
300 ↗(On Diff #22101)

RE: You shouldn't have to do this, it is being done in the lldb_private::CommandInterpreter::CommandInterpreter().

But I have a problem:

  1. CMICmnLLDBDebugger::InitSBListener() registers events (which are needed for lldb-mi) using the SBListener::StartListeningForEventClass().
  2. SBListener::StartListeningForEventClass() is used for classes that weren't created (otherwise the SBBroadcast::CheckInWithManager() should be called).
  3. In this situation, we register events (using SBListener::StartListeningForEventClass()) for already created class SBCommandInterpreter. I can't register events before this class is created because it is done simultaneously with SBDebugger (which is used in SBListener::StartListeningForEventClass()).

I assume we can use SBListener::StartListeningForEventClass() for SBTarget/SBProcess/SBThread and SBListener::StartListeningForEvents() for SBCommandInterpreter but I'm not sure that it's right solution.

Do we have any restrictions to use SBListener::StartListeningForEventClass()?

RE: How did you manage to not contract a CommandInterpreter and yet you are able to listen to events from it?

Previously lldb-mi ignored all CommandInterpreter's events.

clayborg requested changes to this revision.Mar 20 2015, 10:15 AM
clayborg edited edge metadata.

No need to strcmp, see my inlined comments.

source/API/SBCommandInterpreter.cpp
554 ↗(On Diff #22101)

What I am saying is the "const char *" that comes back from both "event.GetBroadcasterClass()" is a "const char *" that comes from a ConstString object. Same thing with the "const char *" that is returned from "SBCommandInterpreter::GetBroadcasterClass()". So of course "file1.cpp" and "file2.cpp" will fail, that is obvious. So your example will work with ConstString objects:

file1.cpp:
ConstString s1("abc");
file2.cpp:
extern ConstString s1;
ConstString s2("abc");
assert(s1.GetCString() == s2.GetCString());
assert(strcmp(s1.GetCString(), s2.GetCString()) == 0);
This revision now requires changes to proceed.Mar 20 2015, 10:15 AM

We still need to remove the CheckInWithManager (); from the public API. We need to make this work without adding these calls to the API. Not sure how we will fix this, but no one should have to worry about this. Please include Jim Ingham on the review list as he did the CheckInWithManager (); stuff.

jingham added inline comments.
tools/lldb-mi/MICmnLLDBDebugger.cpp
300 ↗(On Diff #22101)

There are two restrictions for SBListener::StartListeningForEventClass. The first is that the debugger you pass into the call must be a valid debugger. Otherwise the StartListeningForEventClass will return 0, meaning you got signed up for no event bits. Although lldb supports multiple debuggers, it does not support coordination among debuggers, you can't have a listener that listens to all classes of events for all debuggers. So this restriction is necessary.

The other restriction in the BroadcasterManager is that we only let one listener sign up for events of a given class/bit pair. So if somebody else had grabbed that bit for that event already, then you wouldn't get it. Again, you should be able to tell from the return value to StartListeningForEventClass whether you got the bits you requested or not (the call will return 0 if there are no available bits.)

If the problem is some other listener already acquired the event spec you are asking for, we can relax the requirement that event class listeners be unique. We went back & forth about whether we wanted to support multiple listeners for some events or not. Looks like I did the BroadcasterManager stuff at the point where we wanted one Listener per Broadcaster/EventBit. For the most part, however, Events can really be "broadcast" rather than go to a single listener without causing problems. So I have no problem relaxing this restriction for listening by event class.

The exception to this is that the Process broadcaster really needs to have only one StateChanged listener. When you create a process you have to give it the Listener that is going to drive it. So we don't want people to sign up for Process events by class. So if the problem is not allowing multiple listeners per class, we can change BroadcasterManager::RegisterListenerForEvents to not filter the request against the extant listeners. But then we should add a call to the BroadcasterManager to say which BroadcasterEventSpec's can't be listened for by class - and make the Debugger outlaw listening to Process events by class.

ki.stfu updated this revision to Diff 22409.Mar 21 2015, 3:41 AM
ki.stfu edited edge metadata.

Remove SBBroadcaster::CheckInWithManager

ki.stfu updated this object.Mar 21 2015, 3:41 AM
ki.stfu edited edge metadata.
ki.stfu added inline comments.Mar 21 2015, 3:51 AM
source/API/SBCommandInterpreter.cpp
554 ↗(On Diff #22101)

Ok. Thanks for explanation. I'll do it here and for SBTarget/SBThread/SBProcess in another commit.

This revision was automatically updated to reflect the committed changes.
ki.stfu added inline comments.Mar 21 2015, 4:14 AM
source/API/SBCommandInterpreter.cpp
554 ↗(On Diff #22101)

$ svn commit
Sending source/API/SBCommandInterpreter.cpp
Sending source/API/SBProcess.cpp
Transmitting file data ..
Committed revision 232892.