[Lldb-commits] [lldb] r223222 - Fix a hang on OSX while executing -exec-run.
Ilia K
ki.stfu at gmail.com
Wed Dec 10 01:29:01 PST 2014
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 <Hafiz_Abid at mentor.com> wrote:
> Please send them. I will try.
>
>
>
> Thanks,
>
> Abid
>
> *From:* Ilia K [mailto:ki.stfu at gmail.com]
> *Sent:* 04 December 2014 11:02
>
> *To:* Abid, Hafiz
> *Cc:* lldb-commits at cs.uiuc.edu
> *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 <Hafiz_Abid at mentor.com> 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:ki.stfu at gmail.com]
> *Sent:* 04 December 2014 10:41
>
>
> *To:* Abid, Hafiz
> *Cc:* lldb-commits at cs.uiuc.edu
> *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 <Hafiz_Abid at mentor.com> 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:ki.stfu at gmail.com]
> *Sent:* 03 December 2014 16:17
>
>
> *To:* Abid, Hafiz
> *Cc:* lldb-commits at cs.uiuc.edu
> *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 <ki.stfu at gmail.com> wrote:
>
> Thank you
>
> On 3 Dec 2014 15:50, "Abid, Hafiz" <Hafiz_Abid at mentor.com> wrote:
>
> Thanks for catching this. I have removed the extra changes in r223227.
>
>
>
> Thanks,
>
> Abid
>
>
>
> *From:* Ilia K [mailto:ki.stfu at gmail.com]
> *Sent:* 03 December 2014 12:40
> *To:* Abid, Hafiz
> *Cc:* lldb-commits at cs.uiuc.edu
> *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" <Hafiz_Abid at mentor.com> 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:ki.stfu at gmail.com]
> *Sent:* 03 December 2014 10:59
> *To:* Abid, Hafiz
> *Cc:* lldb-commits at cs.uiuc.edu
> *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 <hafiz_abid at mentor.com>
> 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 dawn at burble.org.
>
>
> 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
> lldb-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>
>
>
>
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20141210/6e96582b/attachment.html>
More information about the lldb-commits
mailing list