Re: Crash of GDB with gdbserver btrace enabled [Re: [patch v9 00/23] branch tracing support for Atom]

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Thu, 07 Mar 2013 13:32:16 +0100, Metzger, Markus T wrote:
> > > > OK, I agree target_close seems excessive here, it also does not correspond to
> > > > the current description of target_close:
> > > > 	This routine is automatically always called after popping the target
> > > > 	off the target stack
> > > >
> > > > While it is nice cleanup I find it a separate patch, not required for btrace.
> > >
> > > With this patch, the crash is gone with only minimal changes to btrace.
> > 
> > It is only a coincidence and workaround of it.
> 
> Hmmm, if we must no longer call methods in a certain target, why is
> removing that target a workaround?

OK, wrong term, it is snot a "workaround" but it hides the incorrect
implementation approach.


> Pedro already moved the target_close call after removing the target in
> http://sourceware.org/ml/gdb-patches/2012-01/msg00701.html.

Yes, such fix of pop_target seems to be OK.  The problem is that it may IMO
have unexpected implications not good to check-in right before a release.
And I do not see a real reason to push that fix for gdb-7.6 right now.


> If we removed disabling btrace from to_close, we would need to add a
> separate path for "record stop".

Yes.


> It would further break the symmetry with to_open.  Tracing would be
> enabled in to_open but it would not be disabled in to_close, any more.

It is more questionable whether tracing of inferiors should really be done at
to_open time.  If you want symmetry then record_btrace_open could be split and
btrace_enable calls could be done from a new method (after to_open finishes).

But GDB already commonly does 'start' of the target from its to_open, this is
also why to_open has args.  This brings in the asymmetry.  It can be safely
done because to_open is called only under controlled conditions.  Contrary to
it to_close can be called in arbitrary crash/teardown state so it should not
rely on much.


> We also must not clear btrace in to_close since this would prevent btrace
> from actually being disabled when the threads are discarded sometime
> after the record target has been unpushed.  We would thus leave threads
> traced after the record target is gone and rely on thread cleanup to do
> the actual disabling.  This does not feel right.

to_close can be called in controlled (such as "record stop") or uncontrolled
(inferior dies / gets killed).  In the first case "record stop" should
explicitly disable the tracing before calling to_close.  In the second case it
does not matter when the tracing disable happens, it just needs to happen.

For the second case there must be always someone who does the munmap + close.
In the linux-nat case it can be easily done from btrace's to_close.  In the
gdbserver case it is gdbserver's responsibility (where it actually already
works).


By the plan above one avoids the situation of half-closed target, this is
still (AFAIK) the Pedro's design from the original mail.

pop_target fix is nice, thanks for catching it, but it is not a prerequisite.


Thanks,
Jan