Reduce number of threads in lldb-mi.

[abidh]

Summary

LLDB-mi have 3 threads.

1. Wait for input.
2. Process commands.
3. Process events.

This revision merges 1 & 2. Same thread waits on input and then process the
command. This way, no synchronization is needed between first and 2nd. Also it is
easy to check when to exit.

This revision just makes the minimal changes to use one thread for reading intput
and then processnig the commands. A lot of code has become redundant now. I will
clean it up gradually.

All lldb-mi tests pass with gcc and clang as test compiler. Also did minimal testing
on command line and works ok. The "quit" and "-gdb-exit" command close the application
without needing any further return.

After this revision goes in, it will not be possible to give the name of the
executable on command line and lldb-mi accepting it. This functionality is already
stubbed out in the code by he following #ifdef so we will not lose anything.
MICONFIG_ENABLE_MI_DRIVER_MI_MODE_CMDLINE_ARG_EXECUTABLE_DEBUG_SESSION

For posix system, it is even possible to make lldb-mi a single thread app. But on
windows, it is difficult to wait on stdin with timeout like what you can do with
select. So next best thing is to use 2 threads instead of 3.

[abidh]

Lock mutex before processing command.

[ki.stfu]

Extra cleaning is needed.

tools/lldb-mi/MIDriver.cpp

556

Why you don't remove CMICmnStreamStdin::ThreadRun() and CMICmnStreamStdin::MonitorStdin()?

588–609

I think you should use MICmnStreamStdin module. For example, m_rStdin.ReadLine() function.

Also you should check if we need IOSStdinHandler::InputAvailable and IOSStdinHandler::InterruptReadLine functions and use them (or remove).

631

We should remove ReadStdinLineQueue if it not used any more.

tools/lldb-mi/MIDriver.h

183

I think you should use existing code from MICmnStreamStdin.

[ki.stfu]

I have tested the Diff 2 on OS X, all tests passed.

[abidh]

tools/lldb-mi/MIDriver.cpp

588–609

As I said in my comments above. Not only these functions but a lot of other code will be redundant after this and I will remove them gradually.

631

I will surly do in a separate revision. There will be a lot of code cleanup.

tools/lldb-mi/MIDriver.h

183

A lot of those classes will be unnecessary after this revision and I plan to remove them. So only this Readline will be kept.

[ki.stfu]

After this revision goes in, it will not be possible to give the name of the executable on command line and lldb-mi accepting it. This functionality is already stubbed out in the code by he following #ifdef so we will not lose anything.

Why? We can use ExecuteCommand instead of InjectCommand in LocalDebugSessionStartupInjectCommands. I have tested it, and it works.

For posix system, it is even possible to make lldb-mi a single thread app. But on windows, it is difficult to wait on stdin with timeout like what you can do with select. So next best thing is to use 2 threads instead of 3.

Can you explain me, why? I don't mind, I'm just wondering.

[abidh]

After this revision goes in, it will not be possible to give the name of the executable on command line and lldb-mi accepting it. This functionality is already stubbed out in the code by he following #ifdef so we will not lose anything.

Why? We can use ExecuteCommand instead of InjectCommand in LocalDebugSessionStartupInjectCommands. I have tested it, and it works.

You are right. We can keep this functionality. But I dont like code being stubbed out with random #ifdef. If it is needed then it should be enabled all times and tested. Otherwise it should be removed.

For posix system, it is even possible to make lldb-mi a single thread app. But on windows, it is difficult to wait on stdin with timeout like what you can do with select. So next best thing is to use 2 threads instead of 3.

Can you explain me, why? I don't mind, I'm just wondering.

On linux, I have experimented with a single-threaded lldb-mi and it works. The sequence is

1. Wait for stdin with a timeout.
2. If a command, process it and go to 4.
3. If time out, go to 4.
4. Process events if any and go to 1.

This worked on Linux. But problem is that 'select' call works only with sockets on Windows. I tried other Windows APIs but nothing provides a simple interface as select. So I abandoned the idea of making it single threaded.

[ki.stfu]

tools/lldb-mi/MIDriver.cpp

631

If be honest I'd prefer to do it simultaneously. It gives a chance to review all your changes at once and it will help me (and possible us) adopt a more exact solution.

[abidh]

Remove ReadStdinLineQueue. Move ReadLine to the end of the file.

[ki.stfu]

In D7746#126426, @abidh wrote:

After this revision goes in, it will not be possible to give the name of the executable on command line and lldb-mi accepting it. This functionality is already stubbed out in the code by he following #ifdef so we will not lose anything.

Why? We can use ExecuteCommand instead of InjectCommand in LocalDebugSessionStartupInjectCommands. I have tested it, and it works.

You are right. We can keep this functionality. But I dont like code being stubbed out with random #ifdef. If it is needed then it should be enabled all times and tested. Otherwise it should be removed.

We can enable this ability by default. It will not require additional efforts on startup.

For posix system, it is even possible to make lldb-mi a single thread app. But on windows, it is difficult to wait on stdin with timeout like what you can do with select. So next best thing is to use 2 threads instead of 3.

Can you explain me, why? I don't mind, I'm just wondering.

On linux, I have experimented with a single-threaded lldb-mi and it works. The sequence is

1. Wait for stdin with a timeout.
2. If a command, process it and go to 4.
3. If time out, go to 4.
4. Process events if any and go to 1.

This worked on Linux. But problem is that 'select' call works only with sockets on Windows. I tried other Windows APIs but nothing provides a simple interface as select. So I abandoned the idea of making it single threaded.

A small interval will load CPU. 2 threads is a good solution.

[ki.stfu]

tools/lldb-mi/MIDriver.h

183

I think we should move this functionality to CMICmnStreamStdin class and keep it (if you want to delete entire class).

[ki.stfu]

Let's summarize:

• remove unnecessary code in this patch (don't do it in different revision)
• keep CMICmnStreamStdin/CMICmnStreamStdout/CMICmnStreamStderr class
• use CMICmnStreamStdin::ReadLine
• use ExecuteCommand instead of InjectCommand in LocalDebugSessionStartupInjectCommands (and rename it to LocalDebugSessionStartupExecuteCommands)

[abidh]

In D7746#126486, @ki.stfu wrote:

Let's summarize:

• remove unnecessary code in this patch (don't do it in different revision)

Ok. I have done more cleanup. But a lot of things are connected. I prefer to go in relatively small patches. In that cases, it is easier to figure out which commit caused a breakage.

• keep CMICmnStreamStdin/CMICmnStreamStdout/CMICmnStreamStderr class

I dont intend to remove them in this patch anyway. But longer term, we will see.

• use CMICmnStreamStdin::ReadLine

I dont think that is correct. CMICmnStreamStdin::ReadLine delegate to classes for Windows and Linux. Windows class especailly touches some members which are set in InputAvailable which we are not using anymore.

When we have removed the input monitoring thread, most of the function in this class are redundant. Longer term, I think this class will go.

• use ExecuteCommand instead of InjectCommand in LocalDebugSessionStartupInjectCommands (and rename it to LocalDebugSessionStartupExecuteCommands)

I have done that and removed Queue and InjectMICommands functions.

I will post updated patch shortly.

[abidh]

The above patch does not handle the signal correctly. I will fix that too.

[ki.stfu]

In D7746#126524, @abidh wrote:

• use CMICmnStreamStdin::ReadLine

I dont think that is correct. CMICmnStreamStdin::ReadLine delegate to classes for Windows and Linux. Windows class especailly touches some members which are set in InputAvailable which we are not using anymore.
When we have removed the input monitoring thread, most of the function in this class are redundant. Longer term, I think this class will go.

We can reduce its logic and remove CMICmnStreamStdinWindows and CMICmnStreamStdinLinux. I just want to keep the stdin operations in separate file, as it were done for stdout/stderr. It looks good for me.

[abidh]

Used CMICmnStreamStdin::ReadLine to read input.
Changed the scheme of how singal are handled.
Removed some un-used function and data from CMIDriver.

[ki.stfu]

Looks good. What about further cleaning? We can clean CMICmnStreamStdin and remove MICmnStreamStdinLinux MICmnStreamStdinWindows classes. You want to do it in separate review?

tools/lldb-mi/MICmnStreamStdin.h

125–126 ↗

(On Diff #20306)

Let's do it the same way as in MICmnStreamStdinLinux.cpp:

• Specify m_constBufferSize and m_pCmdBuffer in header file:

67     // Attributes:
68   private:
69     const MIuint m_constBufferSize;
70     FILE *m_pStdin;
71     MIchar *m_pCmdBuffer;

• Initialize in ctor:

42 CMICmnStreamStdinLinux::CMICmnStreamStdinLinux(void)
43     : m_constBufferSize(1024)
44     , m_pStdin(nullptr)
45     , m_pCmdBuffer(nullptr)
46     , m_waitForInput(true)

• Create buffer in CMICmnStreamStdinLinux::Initialize:

83     // Other resources required
84     if (bOk)
85     {
86         m_pCmdBuffer = new MIchar[m_constBufferSize];
87         m_pStdin = stdin;
88     }

• Use it in fgets:

199     // Read user input
200     const MIchar *pText = ::fgets(&m_pCmdBuffer[0], m_constBufferSize, stdin);

• Don't forget to free it in CMICmnStreamStdinLinux::Shutdown:

126     // Tidy up
127     if (m_pCmdBuffer != nullptr)
128     {
129         delete[] m_pCmdBuffer;
130         m_pCmdBuffer = nullptr;
131     }

tools/lldb-mi/MIDriver.cpp

1035

The InjectMICommand prints the passed command using m_rStdOut.WriteMIResponse. You should do the same.

1244–1246

It should be like:

const CMIUtilString strCommand(CMIUtilString::Format("-file-exec-and-symbols \"%s\"", m_strCmdLineArgExecuteableFileNamePath.c_str()));
const bool bOk = CMICmnStreamStdout::TextToStdout(strCommand);
return (bOk && InterpretCommand(strCommand));

tools/lldb-mi/MIDriverMain.cpp

137 ↗

(On Diff #20306)

Should we remove it too?

tools/lldb-mi/MIDriverMgr.cpp

792 ↗

(On Diff #20306)

void DeliverSignal(const int vnSignal)

tools/lldb-mi/MIDriverMgr.h

82 ↗

(On Diff #20306)

Please, specify an argument name and let's use lldb-mi style:

virtual void DeliverSignal(const int vnSignal) = 0;

110 ↗

(On Diff #20306)

void DeliverSignal(const int vnSignal);

[abidh]

We can reduce its logic and remove CMICmnStreamStdinWindows and CMICmnStreamStdinLinux. I just want to keep the stdin operations in separate file, as it were done for stdout/stderr. It looks good for me.

In D7746#126540, @ki.stfu wrote:

In D7746#126524, @abidh wrote:

• use CMICmnStreamStdin::ReadLine

I dont think that is correct. CMICmnStreamStdin::ReadLine delegate to classes for Windows and Linux. Windows class especailly touches some members which are set in InputAvailable which we are not using anymore.
When we have removed the input monitoring thread, most of the function in this class are redundant. Longer term, I think this class will go.

We can reduce its logic and remove CMICmnStreamStdinWindows and CMICmnStreamStdinLinux. I just want to keep the stdin operations in separate file, as it were done for stdout/stderr. It looks good for me.

OK I will move it CMICmnStreamStdin with a different name. There is already a function with similar name throughout the hierarchy with different signature.

In D7746#126634, @ki.stfu wrote:

Looks good. What about further cleaning? We can clean CMICmnStreamStdin and remove MICmnStreamStdinLinux MICmnStreamStdinWindows classes. You want to do it in separate review?

Yes. As I said earlier, there is a lot of code to cleanup. But we should do it in steps.

tools/lldb-mi/MICmnStreamStdin.h

125–126 ↗

(On Diff #20306)

What benefits that gives us to use dynamic buffer?

tools/lldb-mi/MIDriver.cpp

1035

I did it intentionally. Eclipse uses signal to stop the execution. It we print that command on the output, it makes eclipse switch view and show it in console which does not look nice. It is an internal details that we stopped the execution using -exec-interrupt command. We could have called the lldb API directly. So we should not be printing that information anyway.

1244–1246

Why is TextToStdout necessary? There are many small clean-up and fixes that can be done. But not all in one revision.

tools/lldb-mi/MIDriverMain.cpp

137 ↗

(On Diff #20306)

I did not understand. What should we remove?

tools/lldb-mi/MIDriverMgr.h

82 ↗

(On Diff #20306)

Ok I will add the name, not sure about the lldb-mi style. I think we should slowly move the codebase to look similar to lldb proper.

[abidh]

Add name of some function arguments.
Some smaller cleanup suggested in review.

[ki.stfu]

Fix lines 125-126 and 1,177-1,179 and feel free to commit.

tools/lldb-mi/MICmnStreamStdin.h

125–126 ↗

(On Diff #20306)

Very large buffers on stack may cause an segfault. But if dynamic memory fails it will throw bad_alloc exception (what is better :))

tools/lldb-mi/MIDriver.cpp

1035

ok

1244–1246

Agree, but a cleaning of CMICmnStreamStdin class not affect to lldb-mi functionality unlike this case. Please fix it in this commit.

tools/lldb-mi/MIDriverMain.cpp

137 ↗

(On Diff #20306)

I meant CMICmnStreamStdin::SetCtrlCHit

tools/lldb-mi/MIDriverMgr.h

82 ↗

(On Diff #20306)

lldb-mi is too big. I'm not sure about it.

[zturner]

tools/lldb-mi/MICmnStreamStdin.h

125–126 ↗

(On Diff #20306)

Use dynamic buffer IMO. If your program is out of heap space good luck doing anything else anyway. Maybe this call doesn't fail by using stack, but the next call will. We just had this discussion with Greg the other day on lldb-dev, and we agreed that we should reduce the usage of stack buffers in LLDB as a whole. Performance of 1 heap allocation is not a concern, but using stack buffer reduces the amount of stack LLDB uses, and we don't know if it decides to do a deep recursion or something.

(BTW, if dynamic memory fails it won't throw bad_alloc exception, it will just terminate the program, because we compile with exceptions disabled. So whether it's here, or 5 lines of code later, doesn't really make a difference :)

[abidh]

In D7746#126707, @ki.stfu wrote:

Fix lines 125-126 and 1,177-1,179 and feel free to commit.

OK I have made the changes requested and committed it in 230003.

tools/lldb-mi/MIDriver.h

private:
 
     // Methods:
   private:
     /* ctor */ CMIDriver(void);
     /* ctor */ CMIDriver(const CMIDriver &);
     void operator=(const CMIDriver &);
 
     lldb::SBError ParseArgs(const int argc, const char *argv[], FILE *vpStdOut, bool &vwbExiting);
-    bool ReadStdinLineQueue(void);
     bool DoAppQuit(void);
     bool InterpretCommand(const CMIUtilString &vTextLine);
     bool InterpretCommandThisDriver(const CMIUtilString &vTextLine, bool &vwbCmdYesValid);
     bool InterpretCommandFallThruDriver(const CMIUtilString &vTextLine, bool &vwbCmdYesValid);
     bool ExecuteCommand(const SMICmdData &vCmdData);
     bool StartWorkerThreads(void);
     bool StopWorkerThreads(void);
     bool InitClientIDEToMIDriver(void) const;
private:
     CMICmnLLDBDebugger &m_rLldbDebugger;
     CMICmnStreamStdout &m_rStdOut;
     DriverState_e m_eCurrentDriverState;
     bool m_bHaveExecutableFileNamePathOnCmdLine; // True = Yes executable given as one of the parameters to the MI Driver, false = not found
     CMIUtilString m_strCmdLineArgExecuteableFileNamePath;
     bool m_bDriverDebuggingArgExecutable; // True = The MI Driver (MI mode) is debugging executable passed as argument, false = running via
                                           // a client i.e Eclipse
 };
+
+bool ReadLine(CMIUtilString &cmd);

tools/lldb-mi/MIDriver.cpp

 bool
 CMIDriver::StartWorkerThreads(void)
 {
     bool bOk = MIstatus::success;
 
     // Grab the thread manager
     CMICmnThreadMgrStd &rThreadMgr = CMICmnThreadMgrStd::Instance();
 
-    // Start the stdin thread
-    bOk &= m_rStdin.SetVisitor(*this);
-    if (bOk && !rThreadMgr.ThreadStart<CMICmnStreamStdin>(m_rStdin))
-    {
-        const CMIUtilString errMsg = CMIUtilString::Format(MIRSRC(IDS_THREADMGR_ERR_THREAD_FAIL_CREATE),
-                                                           CMICmnThreadMgrStd::Instance().GetErrorDescription().c_str());
-        SetErrorDescriptionn(errMsg);
-        return MIstatus::failure;
-    }
-
     // Start the event polling thread
     if (bOk && !rThreadMgr.ThreadStart<CMICmnLLDBDebugger>(m_rLldbDebugger))
     {
         const CMIUtilString errMsg = CMIUtilString::Format(MIRSRC(IDS_THREADMGR_ERR_THREAD_FAIL_CREATE),
                                                            CMICmnThreadMgrStd::Instance().GetErrorDescription().c_str());
         SetErrorDescriptionn(errMsg);
         return MIstatus::failure;
     }

 
 //++ ------------------------------------------------------------------------------------
 // Details: Call this function puts *this driver to work.
 //          This function is used by the application's main thread.
 // Type:    Overridden.
 // Args:    None.
 // Return:  MIstatus::success - Functional succeeded.
 //          MIstatus::failure - Functional failed.
 // Throws:  None.
 //--
 bool
 CMIDriver::DoMainLoop(void)
 {
     if (!InitClientIDEToMIDriver()) // Init Eclipse IDE
     {
         SetErrorDescriptionn(MIRSRC(IDS_MI_INIT_ERR_CLIENT_USING_DRIVER));
         return MIstatus::failure;
     }
 
     if (!StartWorkerThreads())
         return MIstatus::failure;
 
     // App is not quitting currently
     m_bExitApp = false;
 
 // CODETAG_CMDLINE_ARG_EXECUTABLE_DEBUG_SESSION
 #if MICONFIG_ENABLE_MI_DRIVER_MI_MODE_CMDLINE_ARG_EXECUTABLE_DEBUG_SESSION
     if (HaveExecutableFileNamePathOnCmdLine())
     {
         if (!LocalDebugSessionStartupInjectCommands())
         {
             SetErrorDescription(MIRSRC(IDS_MI_INIT_ERR_LOCAL_DEBUG_SESSION));
             return MIstatus::failure;
         }
     }
 #endif // MICONFIG_ENABLE_MI_DRIVER_MI_MODE_CMDLINE_ARG_EXECUTABLE_DEBUG_SESSION
 
     // While the app is active
     while (!m_bExitApp)
     {
-        // Poll stdin queue and dispatch
-        if (!ReadStdinLineQueue())
-        {
-            // Something went wrong
-            break;
-        }
-    }
-
-    // Signal that the application is shutting down
-    DoAppQuit();
-
-    // Close and wait for the workers to stop
-    StopWorkerThreads();
-
-    // Ensure that a new line is sent as the last act of the dying driver
-    m_rStdOut.WriteMIResponse("\n", false);
-
-    return MIstatus::success;
-}
-
-//++ ------------------------------------------------------------------------------------
-// Details: *this driver sits and waits for input to the stdin line queue shared by *this
-//          driver and the stdin monitor thread, it queues, *this reads, interprets and
-//          reacts.
-//          This function is used by the application's main thread.
-// Type:    Method.
-// Args:    None.
-// Return:  MIstatus::success - Functional succeeded.
-//          MIstatus::failure - Functional failed.
-// Throws:  None.
-//--
-bool
-CMIDriver::ReadStdinLineQueue(void)
-{
-    // True when queue contains input
-    bool bHaveInput = false;
-
-    // Stores the current input line
         CMIUtilString lineText;
-    {
-        // Lock while we access the queue
-        CMIUtilThreadLock lock(m_threadMutex);
-        if (!m_queueStdinLine.empty())
-        {
-            lineText = m_queueStdinLine.front();
-            m_queueStdinLine.pop();
-            bHaveInput = !lineText.empty();
-        }
-    }
-
-    // Process while we have input
-    if (bHaveInput)
+        if (::ReadLine (lineText) && (!lineText.empty ()) )
         {
             if (lineText == "quit")
             {
                 // We want to be exiting when receiving a quit command
                 m_bExitApp = true;
-            return MIstatus::success;
+                break;
             }
 
-        // Process the command
             bool bOk = false;
             {
                 // Lock Mutex before processing commands so that we don't disturb an event
-            // that is being processed.
+                // being processed
                 CMIUtilThreadLock lock(CMICmnLLDBDebugSessionInfo::Instance().GetSessionMutex());
                 bOk = InterpretCommand(lineText);
             }
-
             // Draw prompt if desired
             if (bOk && m_rStdin.GetEnablePrompt())
                 m_rStdOut.WriteMIResponse(m_rStdin.GetPrompt());
-
-        // Input has been processed
-        bHaveInput = false;
         }
-    else
-    {
-        // Give resources back to the OS
-        const std::chrono::milliseconds time(1);
-        std::this_thread::sleep_for(time);
     }
 
+    // Signal that the application is shutting down
+    DoAppQuit();
+
+    // Close and wait for the workers to stop
+    StopWorkerThreads();
+
+    // Ensure that a new line is sent as the last act of the dying driver
+    m_rStdOut.WriteMIResponse("\n", false);
+
     return MIstatus::success;
 }
 
 //++ ------------------------------------------------------------------------------------
 // Details: Set things in motion, set state etc that brings *this driver (and the
 //          application) to a tidy shutdown.
 //          This function is used by the application's main thread.
 // Type:    Method.
CMIDriver::SetExitApplicationFlag(const bool vbForceExit)
     }
 
     // CODETAG_DEBUG_SESSION_RUNNING_PROG_RECEIVED_SIGINT_PAUSE_PROGRAM
     // Did we receive a SIGINT from the client during a running debug program, if
     // so then SIGINT is not to be taken as meaning kill the MI driver application
     // but halt the inferior program being debugged instead
     if (m_eCurrentDriverState == eDriverState_RunningDebugging)
     {
         InjectMICommand("-exec-interrupt");
         return;
     }
 
     m_bExitApp = true;
     m_rStdin.OnExitHandler();
 }
 
 //++ ------------------------------------------------------------------------------------

 // Args:    None.
 // Return:  MIstatus::success - Functionality succeeded.
 //          MIstatus::failure - Functionality failed.
 // Throws:  None.
 //--
 bool
 CMIDriver::LocalDebugSessionStartupInjectCommands(void)
 {
     const CMIUtilString strCmd(CMIUtilString::Format("-file-exec-and-symbols %s", m_strCmdLineArgExecuteableFileNamePath.c_str()));
 
     return InjectMICommand(strCmd);
 }
 
 //++ ------------------------------------------------------------------------------------
 // Details: Set the MI Driver into "its debugging an executable passed as an argument"
 //          mode as against running via a client like Eclipse.
 // Type:    Method.
 // Args:    None.
 // Return:  None.

 // Return:  None.
 // Throws:  None.
 //--
 bool
 CMIDriver::IsDriverDebuggingArgExecutable(void) const
 {
     return m_bDriverDebuggingArgExecutable;
 }
+
+//++ ------------------------------------------------------------------------------------
+// Details: Wait on new line of data from stdin stream (completed by '\n' or '\r').
+// Type:    Method.
+// Args:    cmd    - (W) Cmd buffer to fill.
+// Return:  bool   - false of error and true otherwise.
+// Throws:  None.
+//--
+bool
+ReadLine(CMIUtilString &cmd)
+{
+    const int SIZE = 2048;
+    char buf[SIZE];
+    // Read user input
+    const MIchar *pText = ::fgets(&buf[0], SIZE, stdin);
+    if (pText == nullptr)
+        return false;
+
+    // Strip off new line characters
+    for (MIchar *pI = buf; *pI != '\0'; pI++)
+    {
+        if ((*pI == '\n') || (*pI == '\r'))
+        {
+            *pI = '\0';
+            break;
+        }
+    }
+    cmd = CMIUtilString(buf);
+    return true;
+}