Remove quit hook in CMIDriver::DoMainLoop (MI)

[ki.stfu]

Summary

This patch removes quit hook and fixes 1 bug:

1. Fix "quit" hook in CMIDriver::DoMainLoop (MI)
2. Fix bug when the handler thread exits without any notification (MI)
3. Fix a race condition in CMICmnLLDBDebugger::MonitorSBListenerEvents (MI)

[abidh]

tools/lldb-mi/MIDriver.cpp

554

How is this supposed to work. Will this thread block until the other thread gets an event. what happens when a command does not generate any event?

[ki.stfu]

tools/lldb-mi/MIDriver.cpp

554

It will block only if there are events that should be handled, otherwise it will return immediately.

[abidh]

tools/lldb-mi/MIDriver.cpp

554

I may be misunderstanding but is it not a race condition. You are expecting that you would get the exit event when you reach this line.
CMICmnLLDBDebugger::Instance().WaitForHandleEvent();

What if lldb takes more time and you find the event queue empty?

[ki.stfu]

tools/lldb-mi/MIDriver.cpp

554

The quit command puts the exit event into queue, therefore there is no race condition. It will work always.

[abidh]

tools/lldb-mi/MIDriver.cpp

554

I am not saying that quit will not create the event. My point is that "will event always be there" before this thread goes to check the queue. Because if it is late, this thread will block in fgets and then you will need an extra enter key for lldb-mi to quit.

Because that event is generated by a separate thread in lldb, it does not seem to me that we can guarantee that event will always be there when we go and check the queue.

[ki.stfu]

tools/lldb-mi/MIDriver.cpp

554

I am not saying that quit will not create the event. My point is that "will event always be there" before this thread goes to check the queue. Because if it is late, this thread will block in fgets and then you will need an extra enter key for lldb-mi to quit.

As I said above, the quit command (i.e. CommandObjectQuit::DoExecute) queues the exit event, and therefore if we look into this queue later from another thread then this event will be there.

[ki.stfu]

Fix a race condition in CMICmnLLDBDebugger::MonitorSBListenerEvents (MI)

[ki.stfu]

Friendly ping.

[abidh]

In D9275#165810, @ki.stfu wrote:

Friendly ping.

I will try to test it later today to make sure that no race condition exist.

[abidh]

OK I can see how it is working. The command processing thread would have put the event in the queue before returning. So there is no chance of us not finding it. Apart from a few inline comments, looks good.

tools/lldb-mi/MICmnLLDBDebugger.cpp

225

It would be better to add a few lines of comment to explain how it waits for empty queue which means that all events have been processed by the MonitorSBListenerEvents.

681

I am not sure what our policy is about using assert.

[ki.stfu]

Add explanation how does the WaitForHandleEvent work

tools/lldb-mi/MICmnLLDBDebugger.h

 //===-- MICmnLLDBDebugger.h -------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #pragma once
 
 // Third party headers
-#include <queue>
+#include <condition_variable>
 #include <map>
+#include <mutex>
 #include "lldb/API/SBDebugger.h"
 #include "lldb/API/SBListener.h"
 #include "lldb/API/SBEvent.h"
 
 // In-house headers:
 #include "MICmnBase.h"
 #include "MIUtilThreadBaseStd.h"
 #include "MIUtilSingletonBase.h"
class CMICmnLLDBDebugger : public CMICmnBase, public CMIUtilThreadActiveObjBase, public MI::ISingleton<CMICmnLLDBDebugger>
   public:
     bool Initialize(void);
     bool Shutdown(void);
 
     bool SetDriver(const CMIDriverBase &vClientDriver);
     CMIDriverBase &GetDriver(void) const;
     lldb::SBDebugger &GetTheDebugger(void);
     lldb::SBListener &GetTheListener(void);
+    void WaitForHandleEvent(void);
 
     // MI Commands can use these functions to listen for events they require
     bool RegisterForEvent(const CMIUtilString &vClientName, const CMIUtilString &vBroadcasterClass, const MIuint vEventMask);
     bool UnregisterForEvent(const CMIUtilString &vClientName, const CMIUtilString &vBroadcasterClass);
     bool RegisterForEvent(const CMIUtilString &vClientName, const lldb::SBBroadcaster &vBroadcaster, const MIuint vEventMask);
     bool UnregisterForEvent(const CMIUtilString &vClientName, const lldb::SBBroadcaster &vBroadcaster);
 
     // Overridden:
private:
     // Attributes:
   private:
     CMIDriverBase *m_pClientDriver;  // The driver that wants to use *this LLDB debugger
     lldb::SBDebugger m_lldbDebugger; // SBDebugger is the primordial object that creates SBTargets and provides access to them
     lldb::SBListener m_lldbListener; // API clients can register its own listener to debugger events
     const CMIUtilString m_constStrThisThreadId;
     MapBroadcastClassNameToEventMask_t m_mapBroadcastClassNameToEventMask;
     MapIdToEventMask_t m_mapIdToEventMask;
+    std::mutex m_mutexEventQueue;
+    std::condition_variable m_conditionEventQueueEmpty;
 };

tools/lldb-mi/MICmnLLDBDebugger.cpp

 //--
 CMIDriverBase &
 CMICmnLLDBDebugger::GetDriver(void) const
 {
     return *m_pClientDriver;
 }
 
 //++ ------------------------------------------------------------------------------------
+// Details: Wait until all events have been handled.
+// Type:    Method.
+// Args:    None.
+// Return:  None.
+// Throws:  None.
+//--
+void
+CMICmnLLDBDebugger::WaitForHandleEvent(void)
+{
+    std::unique_lock<std::mutex> lock(m_mutexEventQueue);
+
+    lldb::SBEvent event;
+    if (ThreadIsActive() && m_lldbListener.PeekAtNextEvent(event))
+        m_conditionEventQueueEmpty.wait(lock);
+}
+
+//++ ------------------------------------------------------------------------------------
 // Details: Initialize the LLDB Debugger object.
 // Type:    Method.
 // Args:    None.
 // Return:  MIstatus::success - Functionality succeeded.
 //          MIstatus::failure - Functionality failed.
 // Throws:  None.
 //--
 bool

 //          MIstatus::failure - Functional failed.
 // Throws:  None.
 //--
 bool
 CMICmnLLDBDebugger::MonitorSBListenerEvents(bool &vrbIsAlive)
 {
     vrbIsAlive = true;
 
+    // Lock the mutex of event queue
+    // Note that it should be locked while we are in CMICmnLLDBDebugger::MonitorSBListenerEvents to
+    // avoid a race condition with CMICmnLLDBDebugger::WaitForHandleEvent
+    std::unique_lock<std::mutex> lock(m_mutexEventQueue);
+
     lldb::SBEvent event;
     const bool bGotEvent = m_lldbListener.GetNextEvent(event);
-    if (!bGotEvent || !event.IsValid())
+    if (!bGotEvent)
     {
+        // Notify that we are finished and unlock the mutex of event queue before sleeping
+        m_conditionEventQueueEmpty.notify_one();
+        lock.unlock();
+
+        // Wait a bit to reduce CPU load
         const std::chrono::milliseconds time(1);
         std::this_thread::sleep_for(time);
         return MIstatus::success;
     }
-    if (!event.GetBroadcaster().IsValid())
-        return MIstatus::success;
+    assert(event.IsValid());
+    assert(event.GetBroadcaster().IsValid());
 
     // Debugging
     m_pLog->WriteLog(CMIUtilString::Format("##### An event occurred: %s", event.GetBroadcasterClass()));
 
     bool bHandledEvent = false;
-
     bool bOk = false;
     {
         // Lock Mutex before handling events so that we don't disturb a running cmd
         CMIUtilThreadLock lock(CMICmnLLDBDebugSessionInfo::Instance().GetSessionMutex());
         bOk = CMICmnLLDBDebuggerHandleEvents::Instance().HandleEvent(event, bHandledEvent);
     }
+
     if (!bHandledEvent)
     {
         const CMIUtilString msg(CMIUtilString::Format(MIRSRC(IDS_LLDBDEBUGGER_WRN_UNKNOWN_EVENT), event.GetBroadcasterClass()));
         m_pLog->WriteLog(msg);
     }
+
     if (!bOk)
-    {
         m_pLog->WriteLog(CMICmnLLDBDebuggerHandleEvents::Instance().GetErrorDescription());
-    }
 
-    return bOk;
+    return MIstatus::success;
 }
 
 //++ ------------------------------------------------------------------------------------
 // Details: The main worker method for this thread.
 // Type:    Method.
 // Args:    vrbIsAlive  - (W) True = *this thread is working, false = thread has exited.
 // Return:  MIstatus::success - Functional succeeded.
 //          MIstatus::failure - Functional failed.

tools/lldb-mi/MIDriver.cpp

CMIDriver::DoMainLoop(void)
     {
         CMIUtilString errorText;
         const MIchar *pCmd = m_rStdin.ReadLine (errorText);
         if (pCmd != nullptr)
         {
             CMIUtilString lineText(pCmd);
             if (!lineText.empty ())
             {
-                if (lineText == "quit")
-                {
-                    // We want to be exiting when receiving a quit command
-                    m_bExitApp = true;
-                    break;
-                }
+                // Check that the handler thread is alive (otherwise we stuck here)
+                assert(CMICmnLLDBDebugger::Instance().ThreadIsActive());
 
                 {
                     // Lock Mutex before processing commands so that we don't disturb an event
                     // being processed
                     CMIUtilThreadLock lock(CMICmnLLDBDebugSessionInfo::Instance().GetSessionMutex());
                     bOk = InterpretCommand(lineText);
                 }
+
                 // Draw prompt if desired
                 if (bOk && m_rStdin.GetEnablePrompt())
                     bOk = m_rStdOut.WriteMIResponse(m_rStdin.GetPrompt());
+
+                // Wait while the handler thread handles incoming events
+                CMICmnLLDBDebugger::Instance().WaitForHandleEvent();
             }
         }
     }
 
     // Signal that the application is shutting down
     DoAppQuit();
 
     // Close and wait for the workers to stop