Fix multiple problems of lldb-mi blocking on input monitoring and needing a return.

[abidh]

Summary

One of the problem is reported here.
http://llvm.org/bugs/show_bug.cgi?id=22411

A fix was committed for this problem that works only for OSX. This revision
extends that fix to other system. The select system call has some limitation with
multi-threaded application which have been addresses here.

LLDB-mi exits if quit command is given but needs an extra retur if -gdb-exit is
given. That issue has also been addressed.

[ki.stfu]

test/tools/lldb-mi/TestMiExit.py

33–34

It checks that lldb-mi really exited. If it doesn't work on Linux we can skip test or it should be commented out and we should mark it as "FIXME" for future fixes.
I think "skip test" is preferable.

tools/lldb-mi/MICmnStreamStdinLinux.cpp

162–164

Have you tried to monitor errors using 3rd arg of select?

165

timeout is 4th argument

tools/lldb-mi/MIDriver.cpp

524

I do not like this "if", but you have extended its functionality. Maybe it would be better to call CMICmnStreamStdinLinux::InterruptReadLine() from -gdb-exit command? or use CMIDriver::GetExitApplicationFlag() instead of m_waitForInput in CMICmnStreamStdinLinux::InputAvailable()?

[ki.stfu]

test/tools/lldb-mi/TestMiExit.py

31

please remove it also from test_lldbmi_stopped_when_stopatentry_remote (test/tools/lldb-mi/TestMiNotification.py).

[abidh]

test/tools/lldb-mi/TestMiExit.py

33–34

I will restore the checks. It was failing when I have not done the select fix. Now it passes on Linux.

tools/lldb-mi/MICmnStreamStdinLinux.cpp

162–164

The documentation in man pages is quite explicit.
"On Linux (and some other systems), closing the file descriptor in another thread has no

effect on select()."

165

Thanks. I will fix this. It is actually 5th :-)

tools/lldb-mi/MIDriver.cpp

524

Yes. I dont like it either. It was like a quick fix. I will look at the alternate options.

[abidh]

@emaste, @ki.stfu
There is a problem with this revision. Dont review/test it for now. I will post an updated one later. Sorry for inconvenience.

[ki.stfu]

In D7529#121171, @abidh wrote:

Yes. I dont like it either. It was like a quick fix. I will look at the alternate options.

lgtm except the line #524.

[abidh]

Following changes are done in this revision.

1. Initialize values to the arguments of select inside loop as they can be changed by select call.
2. Assign values of m_waitForInput correctly.
3. Remove sepcial check for -gdb-exit. Above 2 are enough to make sure lldb-mi exits without needing an extra return.
4. Remove some changes from test file as suggested in review.

[ki.stfu]

Nice. Lgtm.

I will check it on OS X later.

[ki.stfu]

tools/lldb-mi/MICmnStreamStdinLinux.cpp

191

Set vwbAwail to false before this return, ant return success here.

[ki.stfu]

tools/lldb-mi/MICmnStreamStdinLinux.cpp

191

no. it is right.

[ki.stfu]

All tests work on OS X.

[emaste]

I think the comment at the beginning of CMICmnStreamStdinLinux::InputAvailable is more verbose than necessary now. It was important to explain why things were done in a certain way when the ifdef APPLE guards were in place, but now I think having of all of the history described there may just be confusing.

I think this change is generally an improvement, although the tests still hang on FreeBSD with this change in place.

[emaste]

In D7529#121356, @emaste wrote:

I think this change is generally an improvement, although the tests still hang on FreeBSD with this change in place.

Actually, I think this is incorrect: there are still four lldb-mi tests that hang on FreeBSD, but prior to this change I think the vast majority of them failed. So thank you, this seems like a big improvement. I will investigate further and either update the details in the PR or submit a new PR as appropriate.

[abidh]

The real problem is that lldb-mi has a very poor design regarding threading. It is using 3 threads.

1. To wait on input
2. To process commands
3. To handle event.

These problems mostly occur due to interaction of 1 & 2. I also found out that running 2 & 3 at the same time produces race condition so I have put a mutex on them so that they dont run at the same time. The signal handling in totally messed up too. I think lldb-mi should be changed to be a single threaded app (or first 2 threads should be merged at least). It will get rid of a lot of complexity and these problems.

[ki.stfu]

In D7529#122418, @abidh wrote:

The real problem is that lldb-mi has a very poor design regarding threading. It is using 3 threads.

1. To wait on input
2. To process commands
3. To handle event.

These problems mostly occur due to interaction of 1 & 2. I also found out that running 2 & 3 at the same time produces race condition so I have put a mutex on them so that they dont run at the same time. The signal handling in totally messed up too. I think lldb-mi should be changed to be a single threaded app (or first 2 threads should be merged at least). It will get rid of a lot of complexity and these problems.

I agree that we should merge 1st and 2nd threads.
Is there any chance that you will be working on it?

[abidh]

I am working on cleanup of GetVariableInfo () family of functions and making sure that struct/array are printed correctly by the -stack-list* commands. But I will get to these threading issues sometime next week.

test/tools/lldb-mi/TestMiExit.py

def test_lldbmi_gdbexit(self):
         self.runCmd("-break-insert -f main")
         self.expect("\^done,bkpt={number=\"1\"")
         self.runCmd("-exec-run")
         self.expect("\^running")
         self.expect("\*stopped,reason=\"breakpoint-hit\"")
 
         # Test -gdb-exit: try to exit and check that program is finished
         self.runCmd("-gdb-exit")
-        self.runCmd("") #FIXME hangs here on Linux; extra return is needed
         self.expect("\^exit")
-        import pexpect
-        self.expect(pexpect.EOF)
 
     @lldbmi_test
     @expectedFailureWindows("llvm.org/pr22274: need a pexpect replacement for windows")
     def test_lldbmi_quit(self):
         """Test that 'quit' exits immediately."""
 
         self.spawnLldbMi(args = None)
 

tools/lldb-mi/MICmnStreamStdinLinux.h

private:
     // From CMICmnBase
     /* dtor */ virtual ~CMICmnStreamStdinLinux(void);
 
     // Attributes:
   private:
     const MIuint m_constBufferSize;
     FILE *m_pStdin;
     MIchar *m_pCmdBuffer;
+    bool m_waitForInput;
 };

tools/lldb-mi/MICmnStreamStdinLinux.cpp

 // Environment: Compilers:  Visual C++ 12.
 //                          gcc (Ubuntu/Linaro 4.8.1-10ubuntu9) 4.8.1
 //              Libraries:  See MIReadmetxt.
 //
 // Copyright:   None.
 //--
 
 // Third Party Headers:
-#if defined(__APPLE__)
 #include <sys/select.h>
 #include <unistd.h> // For STDIN_FILENO
-#endif              // defined( __APPLE__ )
 #include <string.h> // For std::strerror()
 
 // In-house headers:
 #include "MICmnStreamStdinLinux.h"
 #include "MICmnLog.h"
 #include "MICmnResources.h"
 #include "MIUtilSingletonHelper.h"
 
 //++ ------------------------------------------------------------------------------------
 // Details: CMICmnStreamStdinLinux constructor.
 // Type:    Method.
 // Args:    None.
 // Return:  None.
 // Throws:  None.
 //--
 CMICmnStreamStdinLinux::CMICmnStreamStdinLinux(void)
     : m_constBufferSize(1024)
     , m_pStdin(nullptr)
     , m_pCmdBuffer(nullptr)
+    , m_waitForInput(false)
 {
 }
 
 //++ ------------------------------------------------------------------------------------
 // Details: CMICmnStreamStdinLinux destructor.
 // Type:    Overridable.
 // Args:    None.
 // Return:  None.

 // Args:    vwbAvail    - (W) True = There is chars available, false = nothing there.
 // Return:  MIstatus::success - Functional succeeded.
 //          MIstatus::failure - Functional failed.
 // Throws:  None.
 //--
 bool
 CMICmnStreamStdinLinux::InputAvailable(bool &vwbAvail)
 {
-#if defined(__APPLE__)
     // The code below is needed on OSX where lldb-mi hangs when doing -exec-run.
-    // The hang seems to come from calling fgets and fileno from different thread.
-    // Although this problem was not observed on Linux.
+    // Similar behavior has been observed on other systems too. See
+    // http://llvm.org/bugs/show_bug.cgi?id=22411
     // A solution based on 'ioctl' was initially committed but it seems to make
     // lldb-mi takes much more processor time. The solution based on 'select' works
     // well but it seems to slow the execution of lldb-mi tests a lot on Linux.
-    // As a result, this code is #defined to run only on OSX.
+    // The reason for delay was behavior of select in multithreaded app.
+    // The man pages for select suggest that closing a fd which is monitored
+    // by select from another thread has no effect. It effectively caused the
+    // application to block in select forever and test cases timed out.
+    // To fix this issue, I have introduced the a timeout using 3rd arg of select. It
+    // gives us chance to see if other thread has asked us to stop waiting for
+    // input by setting m_waitForInput to false.
     fd_set setOfStdin;
     FD_ZERO(&setOfStdin);
     FD_SET(STDIN_FILENO, &setOfStdin);
-
-    // Wait while input would be available
-    if (::select(STDIN_FILENO + 1, &setOfStdin, nullptr, nullptr, nullptr) == -1)
+    struct timeval tv;
+    tv.tv_sec = 1;
+    tv.tv_usec = 0;
+
+    while (m_waitForInput)
+    {
+        int ret = ::select(STDIN_FILENO + 1, &setOfStdin, nullptr, nullptr, &tv);
+        if (ret == 0) // Timeout. Loop back if m_waitForInput is true
+            continue;
+        else if (ret == -1) // Error condition. Return
         {
             vwbAvail = false;
             return MIstatus::failure;
         }
-
-#endif // defined( __APPLE__ )
+        else // Have some valid input
+        {
             vwbAvail = true;
             return MIstatus::success;
         }
+    }
+    return MIstatus::success;
+}
 
 //++ ------------------------------------------------------------------------------------
 // Details: Wait on new line of data from stdin stream (completed by '\n' or '\r').
 // Type:    Method.
 // Args:    vwErrMsg    - (W) Empty string ok or error description.
 // Return:  MIchar * - text buffer pointer or NULL on failure.
 // Throws:  None.
 //--

 // Type:    Method.
 // Args:    None.
 // Return:  None.
 // Throws:  None.
 //--
 void
 CMICmnStreamStdinLinux::InterruptReadLine(void)
 {
-    fclose(stdin);
+    m_waitForInput = true;
 }

tools/lldb-mi/MIDriver.cpp

 //--
 bool
 CMIDriver::ReadLine(const CMIUtilString &vStdInBuffer, bool &vrwbYesExit)
 {
     // For debugging. Update prompt show stdin is working
     // printf( "%s\n", vStdInBuffer.c_str() );
     // fflush( stdout );
 
-    // Special case look for the quit command here so stop monitoring stdin stream
-    // So we do not go back to fgetc() and wait and hang thread on exit
-    if (vStdInBuffer == "quit")
+    // Special case look for the quit or -gdb-exit command here to stop monitoring stdin stream.
+    // Otherwise the stdin monitoring thread will go and starts waiting for
+    // input and needs an extra return before exiting.
+    if ((vStdInBuffer == "quit") || (vStdInBuffer.find("-gdb-exit") != std::string::npos))
         vrwbYesExit = true;
 
     // 1. Put new line in the queue container by stdin monitor thread
     // 2. Then *this driver calls ReadStdinLineQueue() when ready to read the queue in its
     // own thread
     const bool bOk = QueueMICommand(vStdInBuffer);
 
     // Check to see if the *this driver is shutting down (exit application)