I will more thoroughly test all different combination on Linux to see how they affect performance. I will let you know what I find although it may be a few days. Please also check your patch on OSX to see it does not create any other issue before sending.
Thanks, Abid From: Ilia K [mailto:[email protected]] Sent: 10 December 2014 09:29 To: Abid, Hafiz Cc: [email protected] Subject: Re: [Lldb-commits] [lldb] r223222 - Fix a hang on OSX while executing -exec-run. Hello Abid, Thanks again for the fix :) As far as I know this needed only on OSX and it isn't required on other platforms. But after it was committed 'lldb-mi --interpreter' began to load cpu over 100%!! It's very bad on my side. And as I understood it happened on Linux too. Of course it helped with hangs on OSX, but it made worse for Linux (and OSX). You said that 'select' patch increases time to run test many times, but I didn't face with it on OSX (although I think 'select' shouldn't affect it). I believe that it is necessary to revert 'ioctl' patch (to avoid overheads of CPU) and apply 'select' patch to OSX only (by using #ifdef __APPLE__ or by creating separate file). I can create patch for it if you agree. Thanks, Ilia On Thu, Dec 4, 2014 at 2:04 PM, Abid, Hafiz <[email protected]<mailto:[email protected]>> wrote: Please send them. I will try. Thanks, Abid From: Ilia K [mailto:[email protected]<mailto:[email protected]>] Sent: 04 December 2014 11:02 To: Abid, Hafiz Cc: [email protected]<mailto:[email protected]> Subject: Re: [Lldb-commits] [lldb] r223222 - Fix a hang on OSX while executing -exec-run. Abid, Thank you. I have a few more patches and possible I'll send some of them today. Could you review them too? Thanks, Ilia On Thu, Dec 4, 2014 at 1:53 PM, Abid, Hafiz <[email protected]<mailto:[email protected]>> wrote: I tried to remove setbuf() yesterday and it didn't affect, so I think it is not needed. Could you remove code related to setbuf() from my patch yourself? Ok I will remove and then commit the patch. If you intend to contribute more patches. I would be good to get commit access. Thanks, Abid From: Ilia K [mailto:[email protected]<mailto:[email protected]>] Sent: 04 December 2014 10:41 To: Abid, Hafiz Cc: [email protected]<mailto:[email protected]> Subject: Re: [Lldb-commits] [lldb] r223222 - Fix a hang on OSX while executing -exec-run. Hi Abid, > It is now returning false when ioctl fails I think that it's right, because status is unavailable. > Regarding your question about setbuf, I thought that setting it to null makes > ioctl type function to return when some input is available and not wait for > the new line character I realized that setbuf is not needed. As you said, it makes ioctl type function to return when some input is available and not wait for the new line character, but in canonical mode, you don't get control back until the new line character had typed. > Can you try on OSX after removing the setbuf call I tried to remove setbuf() yesterday and it didn't affect, so I think it is not needed. Could you remove code related to setbuf() from my patch yourself? Thanks, Ilia On Thu, Dec 4, 2014 at 1:17 PM, Abid, Hafiz <[email protected]<mailto:[email protected]>> wrote: Hi Ilia, Thanks for the patch. It looks much cleaner. I was also not sure how tcgetattr type functions were helping with the original fix. Regarding your patch, I only have one comment. It is now returning false when ioctl fails. It can result in lldb-mi quitting. Previously this function was always returning true. I don’t think this is a big deal though. Regarding your question about setbuf, I thought that setting it to null makes ioctl type function to return when some input is available and not wait for the new line character. I may be wrong here too. Can you try on OSX after removing the setbuf call. Does that change the behaviour in any way. Because I cannot see any noticeable difference on Linux. Regards, Abid From: Ilia K [mailto:[email protected]<mailto:[email protected]>] Sent: 03 December 2014 16:17 To: Abid, Hafiz Cc: [email protected]<mailto:[email protected]> Subject: Re: [Lldb-commits] [lldb] r223222 - Fix a hang on OSX while executing -exec-run. Hi Abid, Sorry for remarks, but there is a one problem with "ioctl" patch: it turns off canonical mode so we can't modify commands on input line correctly :(. Also I added error checks and moved "setbuf(stdin, NULL)" into Initialize function to avoid ugly in-place initialization using static variable (but, if be honest I don't know why we need to disable bufferization before "ioctl" call). I prepared new patch. Review it please. Thanks, Ilia On Wed, Dec 3, 2014 at 3:52 PM, Ilia K <[email protected]<mailto:[email protected]>> wrote: Thank you On 3 Dec 2014 15:50, "Abid, Hafiz" <[email protected]<mailto:[email protected]>> wrote: Thanks for catching this. I have removed the extra changes in r223227. Thanks, Abid From: Ilia K [mailto:[email protected]<mailto:[email protected]>] Sent: 03 December 2014 12:40 To: Abid, Hafiz Cc: [email protected]<mailto:[email protected]> Subject: RE: [Lldb-commits] [lldb] r223222 - Fix a hang on OSX while executing -exec-run. Yes. "select" patch requires extra handling on exit. Thanks, Ilia On 3 Dec 2014 15:37, "Abid, Hafiz" <[email protected]<mailto:[email protected]>> wrote: You mean that just the change in ‘InputAvailable’ was needed? Rest of the changes were required with ‘select’. Regards, Abid From: Ilia K [mailto:[email protected]<mailto:[email protected]>] Sent: 03 December 2014 10:59 To: Abid, Hafiz Cc: [email protected]<mailto:[email protected]> Subject: Re: [Lldb-commits] [lldb] r223222 - Fix a hang on OSX while executing -exec-run. Hello Abid, Thanks for commit :) But "ioctl" patch requires less changes than "select" version of this patch. Could you commit another patch to revert all unnecessary changes? Thanks, Ilia On Wed, Dec 3, 2014 at 1:23 PM, Hafiz Abid Qadeer <[email protected]<mailto:[email protected]>> wrote: Author: abidh Date: Wed Dec 3 04:23:06 2014 New Revision: 223222 URL: http://llvm.org/viewvc/llvm-project?rev=223222&view=rev Log: Fix a hang on OSX while executing -exec-run. Now we wait for input to become available before blocking in fgets. More details on problem can be found in http://lists.cs.uiuc.edu/pipermail/lldb-commits/Week-of-Mon-20141201/014290.html Patch from [email protected]<mailto:[email protected]>. Modified: lldb/trunk/tools/lldb-mi/MICmnStreamStdin.cpp lldb/trunk/tools/lldb-mi/MICmnStreamStdin.h lldb/trunk/tools/lldb-mi/MICmnStreamStdinLinux.cpp lldb/trunk/tools/lldb-mi/MICmnStreamStdinLinux.h lldb/trunk/tools/lldb-mi/MIDriver.cpp Modified: lldb/trunk/tools/lldb-mi/MICmnStreamStdin.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/tools/lldb-mi/MICmnStreamStdin.cpp?rev=223222&r1=223221&r2=223222&view=diff ============================================================================== --- lldb/trunk/tools/lldb-mi/MICmnStreamStdin.cpp (original) +++ lldb/trunk/tools/lldb-mi/MICmnStreamStdin.cpp Wed Dec 3 04:23:06 2014 @@ -434,3 +434,16 @@ CMICmnStreamStdin::SetOSStdinHandler(IOS return MIstatus::success; } + +//++ ------------------------------------------------------------------------------------ +// Details: Do some actions before exiting. +// Type: Method. +// Args: None. +// Return: None. +// Throws: None. +//-- +void +CMICmnStreamStdin::OnExitHandler(void) +{ + m_pStdinReadHandler->InterruptReadLine(); +} Modified: lldb/trunk/tools/lldb-mi/MICmnStreamStdin.h URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/tools/lldb-mi/MICmnStreamStdin.h?rev=223222&r1=223221&r2=223222&view=diff ============================================================================== --- lldb/trunk/tools/lldb-mi/MICmnStreamStdin.h (original) +++ lldb/trunk/tools/lldb-mi/MICmnStreamStdin.h Wed Dec 3 04:23:06 2014 @@ -66,6 +66,7 @@ class CMICmnStreamStdin : public CMICmnB public: virtual bool InputAvailable(bool &vwbAvail) = 0; virtual const MIchar *ReadLine(CMIUtilString &vwErrMsg) = 0; + virtual void InterruptReadLine(void){}; /* dtor */ virtual ~IOSStdinHandler(void){}; }; @@ -82,6 +83,7 @@ class CMICmnStreamStdin : public CMICmnB void SetCtrlCHit(void); bool SetVisitor(IStreamStdin &vrVisitor); bool SetOSStdinHandler(IOSStdinHandler &vrHandler); + void OnExitHandler(void); // Overridden: public: Modified: lldb/trunk/tools/lldb-mi/MICmnStreamStdinLinux.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/tools/lldb-mi/MICmnStreamStdinLinux.cpp?rev=223222&r1=223221&r2=223222&view=diff ============================================================================== --- lldb/trunk/tools/lldb-mi/MICmnStreamStdinLinux.cpp (original) +++ lldb/trunk/tools/lldb-mi/MICmnStreamStdinLinux.cpp Wed Dec 3 04:23:06 2014 @@ -22,7 +22,9 @@ // Third Party Headers: #if !defined(_MSC_VER) #include <sys/select.h> +#include <unistd.h> #include <termios.h> +#include <sys/ioctl.h> #endif // !defined( _MSC_VER ) #include <string.h> // For std::strerror() @@ -153,30 +155,29 @@ CMICmnStreamStdinLinux::Shutdown(void) bool CMICmnStreamStdinLinux::InputAvailable(bool &vwbAvail) { - /* AD: Not used ATM but could come in handy just in case we need to do - this, poll for input - - static const int STDIN = 0; - static bool bInitialized = false; - - if( !bInitialized ) - { - // Use termios to turn off line buffering - ::termios term; - ::tcgetattr( STDIN, &term ); - ::term.c_lflag &= ~ICANON; - ::tcsetattr( STDIN, TCSANOW, &term ); - ::setbuf( stdin, NULL ); - bInitialized = true; - } - - int nBytesWaiting; - ::ioctl( STDIN, FIONREAD, &nBytesWaiting ); - vwbAvail = (nBytesWaiting > 0); - - return MIstatus::success; - */ - +#if !defined(_WIN32) + // 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. + // A solution based on 'select' was also proposed but it seems to slow things down + // a lot. + static bool bInitialized = false; + + if (!bInitialized) + { + // Use termios to turn off line buffering + ::termios term; + ::tcgetattr(STDIN_FILENO, &term); + term.c_lflag &= ~ICANON; + ::tcsetattr(STDIN_FILENO, TCSANOW, &term); + ::setbuf(stdin, NULL); + bInitialized = true; + } + + int nBytesWaiting; + ::ioctl(STDIN_FILENO, FIONREAD, &nBytesWaiting); + vwbAvail = (nBytesWaiting > 0); +#endif return MIstatus::success; } @@ -213,3 +214,16 @@ CMICmnStreamStdinLinux::ReadLine(CMIUtil return pText; } + +//++ ------------------------------------------------------------------------------------ +// Details: Interrupt current and prevent new ReadLine operations. +// Type: Method. +// Args: None. +// Return: None. +// Throws: None. +//-- +void +CMICmnStreamStdinLinux::InterruptReadLine(void) +{ + fclose(stdin); +} Modified: lldb/trunk/tools/lldb-mi/MICmnStreamStdinLinux.h URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/tools/lldb-mi/MICmnStreamStdinLinux.h?rev=223222&r1=223221&r2=223222&view=diff ============================================================================== --- lldb/trunk/tools/lldb-mi/MICmnStreamStdinLinux.h (original) +++ lldb/trunk/tools/lldb-mi/MICmnStreamStdinLinux.h Wed Dec 3 04:23:06 2014 @@ -51,6 +51,7 @@ class CMICmnStreamStdinLinux : public CM // From CMICmnStreamStdin::IOSpecificReadStreamStdin virtual bool InputAvailable(bool &vwbAvail); virtual const MIchar *ReadLine(CMIUtilString &vwErrMsg); + virtual void InterruptReadLine(void); // Methods: private: Modified: lldb/trunk/tools/lldb-mi/MIDriver.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/tools/lldb-mi/MIDriver.cpp?rev=223222&r1=223221&r2=223222&view=diff ============================================================================== --- lldb/trunk/tools/lldb-mi/MIDriver.cpp (original) +++ lldb/trunk/tools/lldb-mi/MIDriver.cpp Wed Dec 3 04:23:06 2014 @@ -1075,6 +1075,7 @@ CMIDriver::SetExitApplicationFlag(const { CMIUtilThreadLock lock(m_threadMutex); m_bExitApp = true; + m_rStdin.OnExitHandler(); return; } @@ -1089,6 +1090,7 @@ CMIDriver::SetExitApplicationFlag(const } m_bExitApp = true; + m_rStdin.OnExitHandler(); } //++ ------------------------------------------------------------------------------------ _______________________________________________ lldb-commits mailing list [email protected]<mailto:[email protected]> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
_______________________________________________ lldb-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
