Fix handling of CommandInterpreter's events in lldb-mi

[ki.stfu]

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.

[ki.stfu]

Friendly ping

[abidh]

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?

[ki.stfu]

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]

@clayborg, take a look please.

[clayborg]

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?

[ki.stfu]

@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]

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);

[clayborg]

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]

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]

Remove SBBroadcaster::CheckInWithManager

[ki.stfu]

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.

[ki.stfu]

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.

lldb/trunk/include/lldb/API/SBCommandInterpreter.h

public:
 
     ~SBCommandInterpreter ();
 
     static const char * 
     GetArgumentTypeAsCString (const lldb::CommandArgumentType arg_type);
     
     static const char *
     GetArgumentDescriptionAsCString (const lldb::CommandArgumentType arg_type);
-    
+    static bool
+    EventIsCommandInterpreterEvent (const lldb::SBEvent &event);
+    
     bool
     IsValid() const;
 
     bool
     CommandExists (const char *cmd);
 
     bool
     AliasExists (const char *cmd);

lldb/trunk/source/API/SBCommandInterpreter.cpp

 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandObjectMultiword.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
 #include "lldb/Target/Target.h"
 
 #include "lldb/API/SBBroadcaster.h"
 #include "lldb/API/SBCommandReturnObject.h"
 #include "lldb/API/SBCommandInterpreter.h"
+#include "lldb/API/SBEvent.h"
 #include "lldb/API/SBExecutionContext.h"
 #include "lldb/API/SBProcess.h"
 #include "lldb/API/SBTarget.h"
 #include "lldb/API/SBListener.h"
 #include "lldb/API/SBStream.h"
 #include "lldb/API/SBStringList.h"
 
 using namespace lldb;

 
 const char * 
 SBCommandInterpreter::GetArgumentDescriptionAsCString (const lldb::CommandArgumentType arg_type)
 {
     return CommandObject::GetArgumentDescriptionAsCString (arg_type);
 }
 
 bool
+SBCommandInterpreter::EventIsCommandInterpreterEvent (const lldb::SBEvent &event)
+{
+    return strcmp (event.GetBroadcasterClass(), SBCommandInterpreter::GetBroadcasterClass()) == 0;
+}
+
+bool
 SBCommandInterpreter::SetCommandOverrideCallback (const char *command_name,
                                                   lldb::CommandOverrideCallback callback,
                                                   void *baton)
 {
     if (command_name && command_name[0] && m_opaque_ptr)
     {
         std::string command_name_str (command_name);
         CommandObject *cmd_obj = m_opaque_ptr->GetCommandObjectForCommand(command_name_str);

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

CMICmnLLDBDebugger::InitSBListener(void)
 
     eventMask = lldb::SBProcess::eBroadcastBitStateChanged | lldb::SBProcess::eBroadcastBitInterrupt |
                 lldb::SBProcess::eBroadcastBitSTDOUT | lldb::SBProcess::eBroadcastBitSTDERR | lldb::SBProcess::eBroadcastBitProfileData;
     bOk = bOk && RegisterForEvent(strDbgId, CMIUtilString(lldb::SBProcess::GetBroadcasterClassName()), eventMask);
 
     eventMask = lldb::SBCommandInterpreter::eBroadcastBitQuitCommandReceived | lldb::SBCommandInterpreter::eBroadcastBitThreadShouldExit |
                 lldb::SBCommandInterpreter::eBroadcastBitAsynchronousOutputData |
                 lldb::SBCommandInterpreter::eBroadcastBitAsynchronousErrorData;
-    bOk = bOk && RegisterForEvent(strDbgId, CMIUtilString(lldb::SBCommandInterpreter::GetBroadcasterClass()), eventMask);
+    bOk = bOk && RegisterForEvent(strDbgId, m_lldbDebugger.GetCommandInterpreter().GetBroadcaster(), eventMask);
 
     return bOk;
 }
 
 //++ ------------------------------------------------------------------------------------
 // Details: Register with the debugger, the SBListener, the type of events you are interested
 //          in. Others, like commands, may have already set the mask.
 // Type:    Method.

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

 #else
 #include <unistd.h> // For the ::access()
 #endif              // _WIN32
 
 // In-house headers:
 #include "MICmnLLDBDebuggerHandleEvents.h"
 #include "MICmnResources.h"
 #include "MICmnLog.h"
+#include "MICmnLLDBDebugger.h"
 #include "MICmnLLDBDebugSessionInfo.h"
 #include "MICmnMIResultRecord.h"
 #include "MICmnMIValueConst.h"
 #include "MICmnMIValueList.h"
 #include "MICmnMIOutOfBandRecord.h"
 #include "MICmnStreamStdout.h"
 #include "MICmnStreamStderr.h"
 #include "MIUtilDebug.h"
else if (lldb::SBThread::EventIsThreadEvent(vEvent))
         vrbHandledEvent = true;
         bOk = HandleEventSBThread(vEvent);
     }
     else if (lldb::SBTarget::EventIsTargetEvent(vEvent))
     {
         vrbHandledEvent = true;
         bOk = HandleEventSBTarget(vEvent);
     }
+    else if (lldb::SBCommandInterpreter::EventIsCommandInterpreterEvent(vEvent))
+    {
+        vrbHandledEvent = true;
+        bOk = HandleEventSBCommandInterpreter(vEvent);
+    }
 
     return bOk;
 }
 
 //++ ------------------------------------------------------------------------------------
 // Details: Handle a LLDB SBProcess event.
 // Type:    Method.
 // Args:    vEvent          - (R) An LLDB broadcast event.
switch (nEventType)
         //  m_pClientDriver->SetExitApplicationFlag();
         //  vrbYesExit = true;
         //  return MIstatus::success;
         //}     break;
         case lldb::SBCommandInterpreter::eBroadcastBitResetPrompt:
             pEventType = "eBroadcastBitResetPrompt";
             break;
         case lldb::SBCommandInterpreter::eBroadcastBitQuitCommandReceived:
+        {
             pEventType = "eBroadcastBitQuitCommandReceived";
+            const bool bForceExit = true;
+            CMICmnLLDBDebugger::Instance().GetDriver().SetExitApplicationFlag(bForceExit);
             break;
+        }
         case lldb::SBCommandInterpreter::eBroadcastBitAsynchronousOutputData:
             pEventType = "eBroadcastBitAsynchronousOutputData";
             break;
         case lldb::SBCommandInterpreter::eBroadcastBitAsynchronousErrorData:
             pEventType = "eBroadcastBitAsynchronousErrorData";
             break;
         default:
         {