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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected] > <mailto:[email protected]>> 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 > > [email protected] <mailto:[email protected]> > > > > 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>. Keep up to date with CSR on our technical > > blog, www.csr.com/blog <http://www.csr.com/blog>, CSR people > > blog, www.csr.com/people <http://www.csr.com/people>, YouTube, > > 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>, or follow us > > on Twitter at 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>. > > _______________________________________________ > > lldb-commits mailing list > > [email protected] <mailto:[email protected]> > > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits > > > > > > > > > > > > > <sbest_iohandler_race_rev_04.diff>_______________________________________________ > > lldb-commits mailing list > > [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
