Re: [patch v6 18/21] record-btrace: extend unwinder

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

On Wed, 06 Nov 2013 14:03:04 +0100, Metzger, Markus T wrote:
> Besides frame_id_p I also had to change frame_id_eq.  Here's how it look
> like.  Are you OK with such changes?  Should I put the frame.[hc] changes into
> a separate patch, as well?
> 
> I had to make the first test in frame_id_eq stricter than it was since
> otherwise all artificial frames would have been equal to each other.
> I ran the full test suite without regressions (native on 64bit IA FC19), but I'm
> Not exactly sure what has been tested.  The report lists 34 untested and 133
> unsupported for both runs.

That is the patch part below.  I find the patch correct, thanks.

From what I checked the condition '!l.stack_addr_p && l.special_addr_p' in
current GDB code base can happen only for outer_frame_id.  Also the comment
there assumes it is outer_frame_id.  So I find your change correct.

There is only a bit fragile memcmp for frame_id, as there may be uninitialized
inter-field alignment gaps.  But specifically frame_id is always initialized
from a copy of null_frame_id so it should be safe.  Also memcmp is already
used in frame_id_p().



> diff --git a/gdb/frame.c b/gdb/frame.c
> index 3157167..e8dd029 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
[...]
> @@ -524,13 +540,11 @@ frame_id_eq (struct frame_id l, struct frame_id r)
>  {
>    int eq;
>  
> -  if (!l.stack_addr_p && l.special_addr_p
> -      && !r.stack_addr_p && r.special_addr_p)
> -    /* The outermost frame marker is equal to itself.  This is the
> -       dodgy thing about outer_frame_id, since between execution steps
> -       we might step into another function - from which we can't
> -       unwind either.  More thought required to get rid of
> -       outer_frame_id.  */
> +  if (memcmp (&l, &r, sizeof (l)) == 0)
> +    /* Every frame is equal to itself.
> +       This is the dodgy thing about outer_frame_id, since between execution
> +       steps we might step into another function - from which we can't unwind
> +       either.  More thought required to get rid of outer_frame_id.  */
>      eq = 1;
>    else if (!l.stack_addr_p || !r.stack_addr_p)
>      /* Like a NaN, if either ID is invalid, the result is false.



> > > +  code = get_frame_func (this_frame);
> > > +  special = (CORE_ADDR) bfun;
> > 
> > This is not safe, GDB host may be 64-bit and target 32-bit and in such case
> > without --enable-64-bit-bfd there will be sizeof (CORE_ADDR) == 4,
> > therefore different BFUNs may get the same SPECIAL.
> > bfun->insn_offset or bfun->insn_offset should serve better I think.
> 
> I changed this to use bfun->number, instead.

OK.


> > Also there could be a comment like (it still applies if one uses any of bfun,
> > bfun->insn_offset or bfun->insn_offset):
> >   /* The btrace_function structures can be rebuilt but only after inferior has
> >      run.  In such case all btrace frame have been deleted and there remain no
> >      stale uses of BFUN addresses.  */
> 
> Doesn't gdb keep frame id's alive during stepping?  I thought they were used
> to detect steps into subroutines.

Yes.


> We could end up with the same frame id but a different frame if we
> recomputed the entire trace (i.e. the trace buffer overflows and we can't
> stitch traces) and we happened to have the same function at the same
> position in the trace.

OK, good catch.  But I do not think it can happen for a user, there may be
some way but I do not have an idea how to reproduce it, that is to find
a caller with set_momentary_breakpoint (), such as from "finish" between two
btrace frames.  If there is breakpoint->frame_id from btrace the real
execution cannot suddenly resume.  Inferior calls or gnu-ifunc resolution all
cannot happen in btrace.  "finish" into btrace parent cannot resume inferior.
etc.

There could be a sanity cleanup when btrace resumes real execution that no
stale btrace frame_id remains in breakpoints, something like in
check_longjmp_breakpoint_for_call_dummy.  I would call error() although it may
be also rather even internal_error().


> But this might also happen for all other frame id's.

It does not, or at least it should not.  As you said GDB uses frame_ids only
for small moves like step.  In other cases - like "finish" or return address
from inferior call (see pop_dummy_frame_bpt) GDB takes care to remove any
breakpoints having stale frame_id.

breakpoint->frame_id is never present for too long.


Thanks,
Jan