Re: [patch v4 18/24] record-btrace: extend unwinder

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

On Wed, 03 Jul 2013 11:14:28 +0200, Markus Metzger wrote:
> Extend the always failing unwinder to provide the PC based on the call structure
> detected in the branch trace.
> 
> There are several open points:
> 
> An assertion in get_frame_id at frame.c:340 requires that a frame provides a
> stack address.  The record-btrace unwinder can't provide this since the trace
> does not contain data.  I incorrectly set stack_addr_p to 1 to avoid the
> assertion.

Primarily record-btrace can provide the stack address.  You know $sp at the
end of the recoding and you can query .eh_frame/.debug_frame at any PC address
what is the difference between $sp and caller's $sp at that exact PC.
This assumes either all the involved binaries were built with
-fasynchronous-unwind-tables (for .eh_frame) or that debug info
(for .debug_frame) is present.  The former is true in Fedora / Red Hat
distros, unaware how others.

execute_cfa_program() will produce struct dwarf2_frame_state where you are
interested in regs.cfa_* fields.

It could be even interesting to the users for putting hardware watchpoint on
some stack variable location and re-running the code (so that one workarounds
the missing recorded data).  Sure there may be many false positives in such
cases.  Not sure how much useful that would be in practice, I use similar
debugging method for heap data locations (also with various false positives).

The current method of constant STACK_ADDR may have some problems with
frame_id_inner() but I did not investigate it more.


> When evaluating arguments for printing the stack back trace, there's an ugly
> error displayed: "error reading variable: can't compute CFA for this frame".
> The error is correct, we can't compute the CFA since we don't have the stack at
> that time, but it is rather annoying at this place and makes the back trace
> difficult to read.

This would also change, at least the error would be different or so.


> 
> Now that we set the PC to a different value and provide a fake unwinder, we have
> the potential to affect almost every other command.  How can this be tested
> sufficiently?  I added a few tests for the intended functionality, but nothing
> so far to ensure that it does not break some other command when used in this
> context.
> 
> Reviewed-by: Eli Zaretskii  <eliz@gnu.org>
> 2013-04-24  Markus Metzger  <markus.t.metzger@intel.com>
> 
> 	* frame.h (enum frame_type) <BTRACE_FRAME>: New.
> 	* record-btrace.c: Include hashtab.h.
> 	(btrace_get_bfun_name): New.
> 	(btrace_call_history): Call btrace_get_bfun_name.
> 	(struct btrace_frame_cache): New.
> 	(bfcache): New.
> 	(bfcache_hash, bfcache_eq, bfcache_new): New.
> 	(btrace_get_frame_function): New.
> 	(record_btrace_frame_unwind_stop_reason): Allow unwinding.
> 	(record_btrace_frame_this_id): Compute own id.
> 	(record_btrace_frame_prev_register): Provide PC, throw_error
> 	for all other registers.
> 	(record_btrace_frame_sniffer): Detect btrace frames.
> 	(record_btrace_frame_dealloc_cache): New.
> 	(record_btrace_frame_unwind): Add new functions.
> 	(_initialize_record_btrace): Allocate cache.
> 	* btrace.c (btrace_clear): Call reinit_frame_cache.
> 	* NEWS: Announce it.
> 
> testsuite/
> 	* gdb.btrace/record_goto.exp: Add backtrace test.
> 	* gdb.btrace/tailcall.exp: Add backtrace test.
> 
> 
> ---
>  gdb/NEWS                                 |    2 +
>  gdb/btrace.c                             |    4 +
>  gdb/frame.h                              |    4 +-
>  gdb/record-btrace.c                      |  259 +++++++++++++++++++++++++++---
>  gdb/testsuite/gdb.btrace/record_goto.exp |   13 ++
>  gdb/testsuite/gdb.btrace/tailcall.exp    |   17 ++
>  6 files changed, 279 insertions(+), 20 deletions(-)
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index bfe4dd4..9b9de71 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -14,6 +14,8 @@ Nios II GNU/Linux		nios2*-*-linux
>  Texas Instruments MSP430	msp430*-*-elf
>  
>  * The btrace record target supports the 'record goto' command.
> +  For locations inside the execution trace, the back trace is computed
> +  based on the information stored in the execution trace.
>  
>  * The command 'record function-call-history' supports a new modifier '/c' to
>    indent the function names based on their call stack depth.
> diff --git a/gdb/btrace.c b/gdb/btrace.c
> index 0bec2cf..822926c 100644
> --- a/gdb/btrace.c
> +++ b/gdb/btrace.c
> @@ -755,6 +755,10 @@ btrace_clear (struct thread_info *tp)
>  
>    DEBUG ("clear thread %d (%s)", tp->num, target_pid_to_str (tp->ptid));
>  
> +  /* Make sure btrace frames that may hold a pointer into the branch
> +     trace data are destroyed.  */
> +  reinit_frame_cache ();
> +
>    btinfo = &tp->btrace;
>  
>    it = btinfo->begin;
> diff --git a/gdb/frame.h b/gdb/frame.h
> index 31b9cb7..db4cc52 100644
> --- a/gdb/frame.h
> +++ b/gdb/frame.h
> @@ -216,7 +216,9 @@ 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
>  };
>  
>  /* For every stopped thread, GDB tracks two frames: current and
> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> index d6508bd..a528f8b 100644
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
> @@ -34,6 +34,7 @@
>  #include "filenames.h"
>  #include "regcache.h"
>  #include "frame-unwind.h"
> +#include "hashtab.h"
>  
>  /* The target_ops of record-btrace.  */
>  static struct target_ops record_btrace_ops;
> @@ -507,6 +508,28 @@ btrace_call_history_src_line (struct ui_out *uiout,
>    ui_out_field_int (uiout, "max line", end);
>  }
>  
> +/* Get the name of a branch trace function.  */
> +
> +static const char *
> +btrace_get_bfun_name (const struct btrace_function *bfun)
> +{
> +  struct minimal_symbol *msym;
> +  struct symbol *sym;
> +
> +  if (bfun == NULL)
> +    return "<none>";

_("<none>")


> +
> +  msym = bfun->msym;
> +  sym = bfun->sym;
> +
> +  if (sym != NULL)
> +    return SYMBOL_PRINT_NAME (sym);
> +  else if (msym != NULL)
> +    return SYMBOL_PRINT_NAME (msym);
> +  else
> +    return "<unknown>";

_("<unknown>")


> +}
> +
>  /* Disassemble a section of the recorded function trace.  */
>  
>  static void
> @@ -524,12 +547,8 @@ btrace_call_history (struct ui_out *uiout,
>    for (it = *begin; btrace_call_cmp (&it, end) != 0; btrace_call_next (&it, 1))
>      {
>        const struct btrace_function *bfun;
> -      struct minimal_symbol *msym;
> -      struct symbol *sym;
>  
>        bfun = btrace_call_get (&it);
> -      msym = bfun->msym;
> -      sym = bfun->sym;
>  
>        /* Print the function index.  */
>        ui_out_field_uint (uiout, "index", bfun->number);
> @@ -543,12 +562,7 @@ btrace_call_history (struct ui_out *uiout,
>  	    ui_out_text (uiout, "  ");
>  	}
>  
> -      if (sym != NULL)
> -	ui_out_field_string (uiout, "function", SYMBOL_PRINT_NAME (sym));
> -      else if (msym != NULL)
> -	ui_out_field_string (uiout, "function", SYMBOL_PRINT_NAME (msym));
> -      else
> -	ui_out_field_string (uiout, "function", "<unknown>");
> +      ui_out_field_string (uiout, "function", btrace_get_bfun_name (bfun));
>  
>        if ((flags & RECORD_PRINT_INSN_RANGE) != 0)
>  	{
> @@ -902,13 +916,100 @@ record_btrace_prepare_to_store (struct target_ops *ops,
>        }
>  }
>  
> +/* The branch trace frame cache.  */
> +
> +struct btrace_frame_cache
> +{
> +  /* The thread.  */
> +  struct thread_info *tp;
> +
> +  /* The frame info.  */
> +  struct frame_info *frame;
> +
> +  /* The branch trace function segment.  */
> +  const struct btrace_function *bfun;
> +
> +  /* The return PC into this frame.  */
> +  CORE_ADDR pc;
> +};
> +
> +/* A struct btrace_frame_cache hash table indexed by NEXT.  */
> +
> +static htab_t bfcache;
> +
> +/* hash_f for htab_create_alloc of bfcache.  */
> +
> +static hashval_t
> +bfcache_hash (const void *arg)
> +{
> +  const struct btrace_frame_cache *cache = arg;
> +
> +  return htab_hash_pointer (cache->frame);
> +}
> +
> +/* eq_f for htab_create_alloc of bfcache.  */
> +
> +static int
> +bfcache_eq (const void *arg1, const void *arg2)
> +{
> +  const struct btrace_frame_cache *cache1 = arg1;
> +  const struct btrace_frame_cache *cache2 = arg2;
> +
> +  return cache1->frame == cache2->frame;
> +}
> +
> +/* Create a new btrace frame cache.  */
> +
> +static struct btrace_frame_cache *
> +bfcache_new (struct frame_info *frame)
> +{
> +  struct btrace_frame_cache *cache;
> +  void **slot;
> +
> +  cache = FRAME_OBSTACK_ZALLOC (struct btrace_frame_cache);
> +  cache->frame = frame;
> +
> +  slot = htab_find_slot (bfcache, cache, INSERT);
> +  gdb_assert (*slot == NULL);
> +  *slot = cache;
> +
> +  return cache;
> +}
> +
> +/* Extract the branch trace function from a branch trace frame.  */
> +
> +static const struct btrace_function *
> +btrace_get_frame_function (struct frame_info *frame)
> +{
> +  const struct btrace_frame_cache *cache;
> +  const struct btrace_function *bfun;
> +  struct btrace_frame_cache pattern;
> +  void **slot;
> +
> +  pattern.frame = frame;
> +
> +  slot = htab_find_slot (bfcache, &pattern, NO_INSERT);
> +  if (slot == NULL)
> +    return NULL;
> +
> +  cache = *slot;
> +  return cache->bfun;
> +}
> +
>  /* Implement stop_reason method for record_btrace_frame_unwind.  */
>  
>  static enum unwind_stop_reason
>  record_btrace_frame_unwind_stop_reason (struct frame_info *this_frame,
>  					void **this_cache)
>  {
> -  return UNWIND_UNAVAILABLE;
> +  const struct btrace_frame_cache *cache;
> +
> +  cache = *this_cache;
> +
> +  if (cache->bfun == NULL)
> +    return UNWIND_UNAVAILABLE;
> +
> +  return UNWIND_NO_REASON;
>  }
>  
>  /* Implement this_id method for record_btrace_frame_unwind.  */
> @@ -917,7 +1018,21 @@ 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;
> +  CORE_ADDR stack, code, special;
> +
> +  cache = *this_cache;
> +
> +  stack = 0;
> +  code = get_frame_func (this_frame);
> +  special = (CORE_ADDR) cache->bfun;
> +
> +  *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.  */
> @@ -927,8 +1042,31 @@ record_btrace_frame_prev_register (struct frame_info *this_frame,
>  				   void **this_cache,
>  				   int regnum)
>  {
> -  throw_error (NOT_AVAILABLE_ERROR,
> -              _("Registers are not available in btrace record history"));
> +  const struct btrace_frame_cache *cache;
> +  const struct btrace_function *bfun;
> +  struct gdbarch *gdbarch;
> +  CORE_ADDR pc;
> +  int pcreg;
> +
> +  gdbarch = get_frame_arch (this_frame);
> +  pcreg = gdbarch_pc_regnum (gdbarch);
> +  if (pcreg < 0 || regnum != pcreg)
> +    throw_error (NOT_AVAILABLE_ERROR,
> +		 _("Registers are not available in btrace record history"));
> +
> +  cache = *this_cache;
> +  bfun = cache->bfun;
> +  if (bfun == NULL)
> +    throw_error (NOT_AVAILABLE_ERROR,
> +		 _("Registers are not available in btrace record history"));
> +
> +  pc = cache->pc;
> +
> +  DEBUG ("[frame] unwound PC for %s on level %d: %s",
> +	 btrace_get_bfun_name (bfun), bfun->level,
> +	 core_addr_to_string_nz (pc));
> +
> +  return frame_unwind_got_address (this_frame, regnum, pc);
>  }
>  
>  /* Implement sniffer method for record_btrace_frame_unwind.  */
> @@ -938,9 +1076,14 @@ record_btrace_frame_sniffer (const struct frame_unwind *self,
>  			     struct frame_info *this_frame,
>  			     void **this_cache)
>  {
> +  const struct btrace_thread_info *btinfo;
> +  const struct btrace_insn_iterator *replay;
> +  const struct btrace_insn *insn;
> +  const struct btrace_function *bfun, *caller;
> +  struct btrace_frame_cache *cache;
>    struct thread_info *tp;
> -  struct btrace_thread_info *btinfo;
> -  struct btrace_insn_iterator *replay;
> +  struct frame_info *next;
> +  CORE_ADDR pc;
>  
>    /* This doesn't seem right.  Yet, I don't see how I could get from a frame
>       to its thread.  */
> @@ -948,7 +1091,81 @@ record_btrace_frame_sniffer (const struct frame_unwind *self,
>    if (tp == NULL)
>      return 0;
>  
> -  return btrace_is_replaying (tp);
> +  replay = tp->btrace.replay;
> +  if (replay == NULL)
> +    return 0;
> +
> +  /* Find the next frame's branch trace function.  */
> +  next = get_next_frame (this_frame);
> +  if (next == NULL)
> +    {
> +      /* The sentinel frame below corresponds to our replay position.  */
> +      bfun = replay->function;
> +    }
> +  else
> +    {
> +      /* This is an outer frame.  It must be the predecessor of another
> +	 branch trace frame.  Let's get this frame's branch trace function
> +	 so we can compute our own.  */
> +      bfun = btrace_get_frame_function (next);
> +    }
> +
> +  /* If we did not find a branch trace function, this is not our frame.  */
> +  if (bfun == NULL)
> +    return 0;
> +
> +  /* Go up to the calling function segment.  */
> +  caller = bfun->up;
> +  pc = 0;
> +
> +  /* Determine where to find the PC in the upper function segment.  */
> +  if (caller != NULL)
> +    {
> +      if ((bfun->flags & BFUN_UP_LINKS_TO_RET) != 0)
> +	{
> +	  insn = VEC_index (btrace_insn_s, caller->insn, 0);
> +	  pc = insn->pc;
> +	}
> +      else
> +	{
> +	  insn = VEC_last (btrace_insn_s, caller->insn);
> +	  pc = insn->pc;
> +
> +	  /* We link directly to the jump instruction in the case of a tail
> +	     call, since the next instruction will likely be outside of the
> +	     caller function.  */
> +	  if ((bfun->flags & BFUN_UP_LINKS_TO_TAILCALL) == 0)
> +	    pc += gdb_insn_length (get_frame_arch (this_frame), pc);
> +	}
> +
> +      DEBUG ("[frame] sniffed frame for %s on level %d",
> +	     btrace_get_bfun_name (caller), caller->level);
> +    }
> +  else
> +    DEBUG ("[frame] sniffed top btrace frame");
> +
> +  /* This is our frame.  Initialize the frame cache.  */
> +  cache = bfcache_new (this_frame);
> +  cache->tp = tp;
> +  cache->bfun = caller;
> +  cache->pc = pc;
> +
> +  *this_cache = cache;
> +  return 1;
> +}
> +
> +static void
> +record_btrace_frame_dealloc_cache (struct frame_info *self, void *this_cache)
> +{
> +  struct btrace_frame_cache *cache;
> +  void **slot;
> +
> +  cache = this_cache;
> +
> +  slot = htab_find_slot (bfcache, cache, NO_INSERT);
> +  gdb_assert (slot != NULL);
> +
> +  htab_remove_elt (bfcache, cache);
>  }
>  
>  /* btrace recording does not store previous memory content, neither the stack
> @@ -959,12 +1176,13 @@ record_btrace_frame_sniffer (const struct frame_unwind *self,
>  
>  static const struct frame_unwind record_btrace_frame_unwind =
>  {
> -  NORMAL_FRAME,
> +  BTRACE_FRAME,
>    record_btrace_frame_unwind_stop_reason,
>    record_btrace_frame_this_id,
>    record_btrace_frame_prev_register,
>    NULL,
> -  record_btrace_frame_sniffer
> +  record_btrace_frame_sniffer,
> +  record_btrace_frame_dealloc_cache
>  };
>  
>  /* The to_resume method of target record-btrace.  */
> @@ -1178,4 +1396,7 @@ _initialize_record_btrace (void)
>  
>    init_record_btrace_ops ();
>    add_target (&record_btrace_ops);
> +
> +  bfcache = htab_create_alloc (50, bfcache_hash, bfcache_eq, NULL,
> +			       xcalloc, xfree);
>  }
> diff --git a/gdb/testsuite/gdb.btrace/record_goto.exp b/gdb/testsuite/gdb.btrace/record_goto.exp
> index a9f9a64..8477a03 100644
> --- a/gdb/testsuite/gdb.btrace/record_goto.exp
> +++ b/gdb/testsuite/gdb.btrace/record_goto.exp
> @@ -75,6 +75,19 @@ gdb_test "record instruction-history" "
>  gdb_test "record goto 26" "
>  .*fun3 \\(\\) at record_goto.c:35.*" "record_goto - goto 26"
>  
> +# check the back trace at that location
> +gdb_test "backtrace" "
> +#0.*fun3.*at record_goto.c:35.*\r
> +#1.*fun4.*at record_goto.c:44.*\r
> +#2.*main.*at record_goto.c:51.*\r
> +Backtrace stopped: not enough registers or memory available to unwind further" "backtrace at 25"
> +
> +# walk the backtrace
> +gdb_test "up" "
> +.*fun4.*at record_goto.c:44.*" "up to fun4"
> +gdb_test "up" "
> +.*main.*at record_goto.c:51.*" "up to main"
> +
>  # the function call history should start at the new location
>  gdb_test "record function-call-history /ci -" "
>  8\t    fun3\tinst 19,21\r
> diff --git a/gdb/testsuite/gdb.btrace/tailcall.exp b/gdb/testsuite/gdb.btrace/tailcall.exp
> index cf9fdf3..ada4b14 100644
> --- a/gdb/testsuite/gdb.btrace/tailcall.exp
> +++ b/gdb/testsuite/gdb.btrace/tailcall.exp
> @@ -47,3 +47,20 @@ gdb_test "record function-call-history /c 1" "
>  1\t  foo\r
>  2\t    bar\r
>  3\tmain" "tailcall - calls indented"
> +
> +# go into bar
> +gdb_test "record goto 3" "
> +.*bar \\(\\) at .*x86-tailcall.c:24.*" "go to bar"
> +
> +# check the backtrace
> +gdb_test "backtrace" "
> +#0.*bar.*at .*x86-tailcall.c:24.*\r
> +#1.*foo.*at .*x86-tailcall.c:29.*\r
> +#2.*main.*at .*x86-tailcall.c:37.*\r
> +Backtrace stopped: not enough registers or memory available to unwind further" "backtrace in bar"
> +
> +# walk the backtrace
> +gdb_test "up" "
> +.*foo \\(\\) at .*x86-tailcall.c:29.*" "up to foo"
> +gdb_test "up" "
> +.*main \\(\\) at .*x86-tailcall.c:37.*" "up to main"
> -- 
> 1.7.1