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

Jan Kratochvil
2013-11-25T17:26:11Z

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