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

2014-08-11 Thread Todd Fiala
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 m...@csr.com wrote: Thanks Shawn, If Greg agrees that we need the SyncIOHandler() invocation in the Command object(s) (not the HandlePrivateEvent code) then

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

2014-08-08 Thread Matthew Gardiner
Shawn, To make this kind of solution robust in all scheduling scenarios, for example on a PC with a lot of additional load/other processes etc. I think we should make this timeout a lot bigger, if we know that the ProcessIOHandler will *always* be pushed, I'm thinking at least in terms of

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

2014-08-08 Thread Matthew Gardiner
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,

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

2014-08-08 Thread Shawn Best
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

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

2014-08-05 Thread Shawn Best
The disappearing (lldb) prompt and a few similar bugs are theoretically present on OSX. I can easily reproduce them with inserting a small delay to provoke a context switch, delaying the call to push the io handler. Shawn. On 8/4/2014 10:01 PM, Matthew Gardiner wrote: Thanks for this

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

2014-08-04 Thread Matthew Gardiner
Thanks for this update, Jim! Hopefully, we'll have it all fixed for when he returns ;-) Matt jing...@apple.com wrote: Note, Greg is on vacation this week. Dunno if he'll chime in from the beach, but he's sure to have opinions. Just wanted to make sure folks didn't think his silence

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

2014-08-01 Thread Greg Clayton
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

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

2014-07-31 Thread Shawn Best
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 gclay...@apple.com wrote: You will want to use a Predicatebool here in stead of what you have since it is exactly what we use a

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

2014-07-31 Thread Shawn Best
oops, with the attachment this time. On Thu, Jul 31, 2014 at 2:56 PM, Shawn Best sb...@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

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

2014-07-30 Thread Shawn Best
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

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

2014-07-30 Thread Greg Clayton
You will want to use a Predicatebool here in stead of what you have since it is exactly what we use a predicate for. The following: +boolm_process_running_sync; // used with WaitForProcessRunning() synchronization +std::condition_variable

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

2014-07-30 Thread Matthew Gardiner
Hi Shawn, Yes, the condition variable paradigm can initially be a little tricky to grasp. Have a look at something like http://pages.cs.wisc.edu/~remzi/OSTEP/threads-cv.pdf, first. I think the text on page on page 14 sums up the concept best. You should probably study it's operation in a

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

2014-07-29 Thread Zachary Turner
Even better would be an std::condition_variable On Mon, Jul 28, 2014 at 10:30 PM, Matthew Gardiner m...@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) -

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

2014-07-29 Thread Matthew Gardiner
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