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]
<mailto:[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]> <mailto:[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]>>
<mailto:[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]>>
<mailto:[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]>>
<mailto:[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]>>
<mailto:[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]>>
<mailto:[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]>>
<mailto:[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]>>>
<mailto:[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]>>>
<mailto:[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>
> > <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>
<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>
<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>
<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>
> >
<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>
> > <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>
<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]>>>
<mailto:[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]>>
<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_05.diff>
_______________________________________________
lldb-commits mailing list
[email protected] <mailto:[email protected]>
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
