[Lldb-commits] [lldb] r223222 - Fix a hang on OSX while executing -exec-run.

Ilia K ki.stfu at gmail.com
Sat Dec 13 15:28:46 PST 2014


Hello Abid,

I have not seen any problems on OSX and all tests work (no regressions).
My patch file in attachments. I have used #ifdef (__APPLE__) - #endif block
for OSX dependent code to avoid problems you faced before. If it is ugly I
can rename MICmnStreamStdinLinux to MICmnStreamStdinUnix or just create
separate file for OSX platform (MICmnStreamStdinOsx).

Thanks,
Ilia


On Thu, Dec 11, 2014 at 12:11 PM, Abid, Hafiz <Hafiz_Abid at mentor.com> wrote:
>
>  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:ki.stfu at gmail.com]
> *Sent:* 10 December 2014 09:29
>
> *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 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/20141214/eb2b52e9/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lldbmi_hang_fix_select_insteadof_ioctl.patch
Type: text/x-patch
Size: 4491 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20141214/eb2b52e9/attachment.bin>


More information about the lldb-commits mailing list