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

Jan Kratochvil
2013-10-06T19:49:50Z

On Fri, 20 Sep 2013 13:30:36 +0200, Markus Metzger wrote:
[...]
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
[...]
> @@ -886,7 +886,7 @@ frame_pop (struct frame_info *this_frame)
>
> /* Ignore TAILCALL_FRAME type frames, they were executed already before
> entering THISFRAME. */
> - while (get_frame_type (prev_frame) == TAILCALL_FRAME)
> + while (frame_is_tailcall (prev_frame))

Such kind of refactorization should be in a standalone patch.

> prev_frame = get_prev_frame (prev_frame);
>
> /* Make a copy of all the register values unwound from this frame.
> @@ -2126,9 +2126,9 @@ get_frame_address_in_block (struct frame_info *this_frame)
> next_frame = next_frame->next;
>
> if ((get_frame_type (next_frame) == NORMAL_FRAME
> - || get_frame_type (next_frame) == TAILCALL_FRAME)
> + || frame_is_tailcall (next_frame))
> && (get_frame_type (this_frame) == NORMAL_FRAME
> - || get_frame_type (this_frame) == TAILCALL_FRAME
> + || frame_is_tailcall (this_frame)
> || get_frame_type (this_frame) == INLINE_FRAME))
> return pc - 1;
>
> diff --git a/gdb/frame.h b/gdb/frame.h
> index a5e1629..f0da19e 100644
> --- a/gdb/frame.h
> +++ b/gdb/frame.h
> @@ -216,7 +216,11 @@ enum frame_type
> ARCH_FRAME,
> /* Sentinel or registers frame. This frame obtains register values
> direct from the inferior's registers. */
> - SENTINEL_FRAME
> + SENTINEL_FRAME,
> + /* A branch tracing frame. */
> + BTRACE_FRAME,
> + /* A branch tracing tail call frame. */
> + BTRACE_TAILCALL_FRAME

One should update also fprint_frame_type, otherwise:
(gdb) set debug frame 1
{ get_prev_frame_1 (this_frame=2) -> {level=3,type=<unknown type>,unwind=0x104aaa0,pc=0x40051f,id=<unknown>,func=<unknown>} // cached
^^^^^^^^^^^^^^

(I have fixed it now for TAILCALL_FRAME which I also forgot to add to
fprint_frame_type.)

> };
>
> /* For every stopped thread, GDB tracks two frames: current and
> @@ -773,4 +777,15 @@ extern struct frame_info *create_new_frame (CORE_ADDR base, CORE_ADDR pc);
> extern int frame_unwinder_is (struct frame_info *fi,
> const struct frame_unwind *unwinder);
>
> +/* Return non-zero if FRAME is a tailcall frame, return zero otherwise. */
> +
> +static inline int
> +frame_is_tailcall (struct frame_info *frame)
> +{
> + enum frame_type type;
> +
> + type = get_frame_type (frame);
> + return (type == TAILCALL_FRAME || type == BTRACE_TAILCALL_FRAME);

Excessive parentheses.

This function does not need to be / should not be inlined in .h file, this
code is not a hot performance code path I think and moreover such
optimizations should be left for gcc -flto.

And I think when there are already two such unwinders there could be reather
a new field in struct frame_unwind:

struct frame_unwind
{
/* The frame's type. Should this instead be a collection of
predicates that test the frame for various attributes? */
enum frame_type type;
+ /* Is this unwinder for tail calls? Call + return replaced by jump
+ at the end of a function is a tail call. */
+ unsigned tail_call : 1;

This unfortunately needs a simple change for all existing unwinders...

> +}
> +
> #endif /* !defined (FRAME_H) */
[...]
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
[...]
> @@ -923,7 +1033,28 @@ static void
> record_btrace_frame_this_id (struct frame_info *this_frame, void **this_cache,
> struct frame_id *this_id)
> {
> - /* Leave there the outer_frame_id value. */
> + const struct btrace_frame_cache *cache;
> + const struct btrace_function *bfun;
> + CORE_ADDR stack, code, special;
> +
> + cache = *this_cache;
> +
> + bfun = cache->bfun;
> + gdb_assert (bfun != NULL);
> +

/* Get unique identifier of the function - frame independent on specific PC in
the function. */

> + while (bfun->segment.prev != NULL)
> + bfun = bfun->segment.prev;
> +
> + stack = 0;

As discussed elsewhere I do not find correct to set stack_p = 1 && stack = 0.
I think frame_id_p() can return true even if special_p == 1 but stack_p == 0.
You sure need some new function similar to frame_id_build_special().

> + 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.

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. */

I was trying to find countercase but I haven't found any so hopefully it will
work. Clearly btrace_function *

> +
> + *this_id = frame_id_build_special (stack, code, special);
> +
> + DEBUG ("[frame] %s id: (!stack, pc=%s, special=%s)",
> + btrace_get_bfun_name (cache->bfun),
> + core_addr_to_string_nz (this_id->code_addr),
> + core_addr_to_string_nz (this_id->special_addr));
> }
>
> /* Implement prev_register method for record_btrace_frame_unwind. */

Thanks,
Jan