[Lldb-commits] [PATCH]race condition calling PushProcessIOHandler

Matthew Gardiner mg11 at csr.com
Thu Jul 31 23:13:45 PDT 2014


Hi Shawn,

Thanks for sending in this new patch. I planned to look at this morning, 
but my SVN sync of the TOT no longer builds on linux.

See:
http://lists.cs.uiuc.edu/pipermail/lldb-dev/2014-August/004726.html

My only other working lldb copy is on my company's perforce repo, and 
it's going to hard to apply your patch over there. I promise to look at 
this on monday! If you come up with with anything newer in the meantime, 
feel free to send that over.

Matt


Shawn Best wrote:
> oops, with the attachment this time.
>
>
> On Thu, Jul 31, 2014 at 2:56 PM, Shawn Best <sbest at blueshiftinc.com 
> <mailto:sbest at blueshiftinc.com>> wrote:
>
>     Thanks everyone for the feedback.  I have reworked the patch to
>     use Predicate <bool>, it reads much cleaner now.
>
>     Shawn.
>
>     On Wed, Jul 30, 2014 at 6:34 PM, Greg Clayton <gclayton at apple.com
>     <mailto:gclayton at apple.com>> wrote:
>
>         You will want to use a Predicate<bool> here in stead of what
>         you have since it is exactly what we use a predicate for. The
>         following:
>
>         +    bool  m_process_running_sync;         // used with
>         WaitForProcessRunning() synchronization
>         +    std::condition_variable m_condition_process_running;  
>          // used with WaitForProcessRunning() synchronization
>         +    std::mutex  m_mutex_process_running;        // used with
>         WaitForProcessRunning() synchronization
>
>         Is exactly what the Predicate class does: protect a value with
>         a mutex and condition.
>
>         The above code should be replaced with:
>
>              Predicate<bool> m_process_running_sync;
>
>         The API on Predicate should do what you want. See the header
>         file at "lldb/Host/Predicate.h" and also look for other places
>         that use this class to wait for a value to be equal to another
>         value, or wait for a value to not be equal to something.
>
>         Let me know when you have a patch that uses Predicate and we
>         will look at that.
>
>         Greg
>
>         > On Jul 30, 2014, at 4:03 PM, Shawn Best
>         <sbest at blueshiftinc.com <mailto:sbest at blueshiftinc.com>> wrote:
>         >
>         > I have reworked the patch to use std::condition_variable.
>          This particular sync mechanism was new to me, I hope I used
>         it correctly.  Is it portable across all target
>         platforms/compilers?  I tested on linux and OSX.
>         >
>         > The timeout is pretty small (1ms) but seems ample based on
>         the measurements I made.
>         >
>         >
>         > On Tue, Jul 29, 2014 at 9:58 PM, Matthew Gardiner
>         <mg11 at csr.com <mailto:mg11 at csr.com>> wrote:
>         > Cool, let us know how you get on!
>         > Matt
>         >
>         > Shawn Best wrote:
>         > Thanks for the feedback guys.
>         >
>         > Studying the code, I had figured going with a straight int
>         would in practice be most efficient and not run into
>         multi-threaded problems, even if initially appearing a bit
>         risky.  I will rework it to use a std::condition_variable.
>          That will be more robust and readable.
>         >
>         > Shawn.
>         >
>         > On 7/29/2014 10:53 AM, Zachary Turner wrote:
>         > Even better would be an std::condition_variable
>         >
>         >
>         > On Mon, Jul 28, 2014 at 10:30 PM, Matthew Gardiner
>         <mg11 at csr.com <mailto:mg11 at csr.com> <mailto:mg11 at csr.com
>         <mailto:mg11 at csr.com>>> wrote:
>         >
>         >     Hi Shawn,
>         >
>         >     I use 64-bit linux and I see this issue a lot. It usually
>         >     manifests itself as the prompt just not being printed
>         (or perhaps
>         >     it just gets overwritten) - regardless - I invoke a
>         command, and
>         >     I don't see an (lldb) prompt when I should. So I'm well
>         pleased
>         >     that you are looking at this!
>         >
>         >     Would it not be more robust to use a semaphore than
>         usleep to
>         >     synchronise the problematic threads?
>         >
>         >     Although I've not looked too deeply into this particular
>         issue,
>         >     whenever I've seen similar races, I found that it's almost
>         >     impossible to pick the right value when using a sleep
>         command. A
>         >     semaphore, though, should always ensure the waiting
>         thread will
>         >     wake precisely.
>         >
>         >     I'd be happy to help to test such a fix.
>         >
>         >     Matt
>         >
>         >
>         >     Shawn Best wrote:
>         >
>         >         Hi,
>         >
>         >         I have attached a patch which addresses 3 related race
>         >         conditions that cause the command line (lldb) prompt
>         to get
>         >         displayed inappropriately and make it appear it is not
>         >         working correctly.  This issue can be seen on linux and
>         >         FreeBSD.  I can also artificailly induce the problem
>         on OSX.
>         >
>         >         The issue happens when the command handler (in the main
>         >         thread) issues a command such as run, step or continue.
>         >          After the command finishes initiating its action,
>         it returns
>         >         up the call stack and goes back into the main
>         command loop
>         >         waiting for user input.  Simultaneously, as the inferior
>         >         process starts up, the MonitorChildProcess thread
>         picks up
>         >         the change and posts to the PrivateEvent thread.
>         >          HandePrivateEvent() then calls
>         PushProcessIOHandler() which
>         >         will disable the command IO handler and give the
>         inferior
>         >         control of the TTY.  To observe this on OSX, put a
>         >
>         >         usleep(100);
>         >
>         >         immediately prior the PushProcessIOHandler() in
>         >         HandlePrivateEvent.
>         >
>         >
>         >         My proposed solution is that after a 'run', 'step', or
>         >         'continue' command, insert a synchronization point
>         and wait
>         >         until HandlePrivateEvent knows the inferior process is
>         >         running and has pushed the IO handler.  One context
>         switch
>         >         (<100us) is usually all the time it takes on my
>         machine.  As
>         >         an additional safety, I have a timeout (currently
>         1ms) so it
>         >         will never hang the main thread.
>         >
>         >         Any thoughts, or suggestions would be appreciated.
>         >
>         >         Regards,
>         >         Shawn.
>         >
>         >
>         >         To report this email as spam click here
>         >        
>         <https://www.mailcontrol.com/sr/MZbqvYs5QwJvpeaetUwhCQ==>.
>         >
>         >
>         >
>         > _______________________________________________
>         >         lldb-commits mailing list
>         > lldb-commits at cs.uiuc.edu <mailto:lldb-commits at cs.uiuc.edu>
>         <mailto:lldb-commits at cs.uiuc.edu
>         <mailto:lldb-commits at cs.uiuc.edu>>
>         >
>         > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>         >
>         >
>         >
>         >
>         >     Member of the CSR plc group of companies. CSR plc
>         registered in
>         >     England and Wales, registered number 4187346, registered
>         office
>         >     Churchill House, Cambridge Business Park, Cowley Road,
>         Cambridge,
>         >     CB4 0WZ, United Kingdom
>         >     More information can be found at www.csr.com
>         <http://www.csr.com>
>         >     <http://www.csr.com>. Keep up to date with CSR on our
>         technical
>         >     blog, www.csr.com/blog <http://www.csr.com/blog>
>         <http://www.csr.com/blog>, CSR people
>         >     blog, www.csr.com/people <http://www.csr.com/people>
>         <http://www.csr.com/people>, YouTube,
>         > www.youtube.com/user/CSRplc
>         <http://www.youtube.com/user/CSRplc>
>         <http://www.youtube.com/user/CSRplc>,
>         >     Facebook, www.facebook.com/pages/CSR/191038434253534
>         <http://www.facebook.com/pages/CSR/191038434253534>
>         >     <http://www.facebook.com/pages/CSR/191038434253534>, or
>         follow us
>         >     on Twitter at www.twitter.com/CSR_plc
>         <http://www.twitter.com/CSR_plc>
>         >     <http://www.twitter.com/CSR_plc>.
>         >
>         >     New for 2014, you can now access the wide range of products
>         >     powered by aptX at www.aptx.com <http://www.aptx.com>
>         <http://www.aptx.com>.
>         > _______________________________________________
>         >     lldb-commits mailing list
>         > lldb-commits at cs.uiuc.edu <mailto:lldb-commits at cs.uiuc.edu>
>         <mailto:lldb-commits at cs.uiuc.edu
>         <mailto:lldb-commits at cs.uiuc.edu>>
>         > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>         >
>         >
>         >
>         >
>         >
>         >
>         <sbest_iohandler_race_rev_04.diff>_______________________________________________
>         > lldb-commits mailing list
>         > lldb-commits at cs.uiuc.edu <mailto:lldb-commits at cs.uiuc.edu>
>         > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>
>
>




More information about the lldb-commits mailing list