Just coming back to the world of LLDB. Looks like lots of healthy discussion on this one! On Aug 10, 2014 10:30 PM, "Matthew Gardiner" <[email protected]> wrote:
> Thanks Shawn, > > If Greg agrees that we need the SyncIOHandler() invocation in the Command > object(s) (not the HandlePrivateEvent code) then I'll submit the patch. > > Greg? > > > Shawn Best wrote: > >> Hi Matt, >> >> Yeah that would probably be a good idea, particularly with a longer >> timeout. I have attached a revised patch: >> >> - changed units of timeout passed to SyncIOHandler() from us to ms. I >> think ms are a more appropriate unit for this kind of timeout. This will >> have the effect of increasing the timeout from 2ms to 2sec. >> >> - moved the sync point to be inside "if (error.Success())" to prevent >> case of an error causing the main thread to block for 2s. >> >> Shawn. >> >> >> On Fri, Aug 8, 2014 at 1:35 AM, Matthew Gardiner <[email protected] <mailto: >> [email protected]>> wrote: >> >> Hi Shawn, >> >> In the patch should not the call to SyncIOHandler in Target.cpp >> and CommandObjectProcess.cpp be inside the >> >> if (error.Success()) >> { >> >> ? >> >> My thinking that is, if the resume operation reports a failure >> then, presumably the running event won't be delivered and we won't >> PushIOHandler, and flag the condition to unblock the waiting >> thread. So in such a situation the main thread would be >> unnecessarily blocked. >> >> What are your thoughts? >> >> Matt >> >> >> >> Shawn Best wrote: >> >> Thanks for the feedback Greg. I have attached a revised patch >> based on your suggestions. >> >> - renamed m_pushed_IOHandler to m_iohandler_sync >> >> - got rid of method ClearPushedIOHandlerSync() and make calls >> directly to m_iohandler_sync.SetValue(false..) >> >> - renamed WaitForPushedIOHandlerSync() to SyncIOHandler() >> >> - only wait in case where m_process_input_reader != NULL >> >> I put the calls to reset the sync flag in both >> PrivateEventThread (after it has seen a public stop), and >> after the call to SyncIOHandler() is completed. As far as I >> can tell it should work fine, but my preference is normally to >> reset the flag immediately before using it instead of relying >> on it set to false. Having it internal does clean up the code >> though. >> >> I think the last suggestion of moving the sync point to >> Debugger::HandleProcessEvent() would defeat the purpose of the >> patch since it is called from the EventHandler thread. The >> race condition is the main thread returning up the call stack >> to start another round of commandIO handling before >> PushProcessIOHandler() gets called. >> >> >> On Fri, Aug 1, 2014 at 10:39 AM, Greg Clayton >> <[email protected] <mailto:[email protected]> >> <mailto:[email protected] <mailto:[email protected]>>> wrote: >> >> Comments: >> >> Why should anyone outside of lldb_private::Process have to >> call >> ClearPushedIOHandlerSync() manually? Can we get rid of this >> function and just have Process.cpp do it at the right times by >> directly calling m_pushed_IOHandler.SetValue(false, >> eBroadcastNever);? >> >> If so then the following fixes apply: >> 1 - remove Process::ClearPushedIOHandlerSync() since it >> will be >> done internally within process. >> 2 - rename m_pushed_IOHandler to m_iohandler_sync >> 3 - rename WaitForPushedIOHandlerSync() to SyncIOHandler >> >> Other general fixes: >> 1 - the WaitForPushedIOHandlerSync should do nothing if >> there is >> no process IOHandler (no stdio or we attached to a >> process. This >> can be done by testing m_process_input_reader with "if >> (m_process_input_reader) { ... }" >> >> I would also like the fix this sync issue by not having to >> have >> every command add a call to >> process->WaitForPushedIOHandlerSync(...). Can't we sync >> this in >> the Debugger::HandleProcessEvent()? >> >> > On Jul 31, 2014, at 2:57 PM, Shawn Best >> <[email protected] <mailto:[email protected]> >> <mailto:[email protected] >> <mailto:[email protected]>>> wrote: >> > >> > oops, with the attachment this time. >> > >> > >> > On Thu, Jul 31, 2014 at 2:56 PM, Shawn Best >> <[email protected] <mailto:[email protected]> >> <mailto:[email protected] >> <mailto:[email protected]>>> 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 >> <[email protected] <mailto:[email protected]> >> <mailto:[email protected] <mailto:[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] <mailto:[email protected]> >> <mailto:[email protected] >> <mailto:[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] <mailto:[email protected]> <mailto:[email protected] >> <mailto:[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]> <mailto:[email protected] >> <mailto:[email protected]>> <mailto:[email protected] <mailto:[email protected]> >> >> <mailto:[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]> >> <mailto:[email protected] >> <mailto:[email protected]>> >> <mailto:[email protected] >> <mailto:[email protected]> >> <mailto:[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> >> <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> >> <http://www.csr.com/blog>, CSR people >> > > blog, www.csr.com/people >> <http://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> >> <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> >> > > <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> >> > > <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> >> <http://www.aptx.com>. >> > > _______________________________________________ >> > > lldb-commits mailing list >> > > [email protected] >> <mailto:[email protected]> >> <mailto:[email protected] >> <mailto:[email protected]>> >> <mailto:[email protected] >> <mailto:[email protected]> >> <mailto:[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] >> <mailto:[email protected]> >> <mailto:[email protected] >> <mailto:[email protected]>> >> > > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits >> > >> > >> > >> > <sbest_iohandler_race_rev_05.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
