This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: Crash of GDB with gdbserver btrace enabled [Re: [patch v9 00/23] branch tracing support for Atom]
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