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

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

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