Re: RFC: DW_OP_call_frame_cfa, again

[Doug Evans <dje@google.com>]

On Tue, Sep 1, 2009 at 11:08 AM, Tom Tromey<tromey@redhat.com> wrote:
>>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:
>
> Tom> Here is a new version of my patch to implement DW_OP_call_frame_cfa.
> Tom> Previous thread here:
> Tom>     http://sourceware.org/ml/gdb-patches/2009-06/msg00191.html
> Tom> With followup the next month (I wish our mailing list archive handled
> Tom> this more nicely):
> Tom>     http://sourceware.org/ml/gdb-patches/2009-07/msg00570.html
>
> Here is an updated version.  This fixes a couple of bugs found by
> testing it in Fedora rawhide.
>
> I'm happy with this version, so I plan to check it in next week or so.
> Comment soon if you think it needs some change.
>
> Tom


I hate to nitpick on style issues (much :-)) but since this one
bothers me, and I'm not clear on what the rules are, and there are
clear rules for other things, I thought I'd bring it up.

IWBN IMHO to require a blank line between a function's comment and its
definition.  It helps readability (to me anyway).
I notice this patch has a mixture of both having the blank line and
not having it.
[I don't mind the absence of a blank line in declarations as much,
I've left that for a separate discussion.]


>
> 2009-09-01  Tom Tromey  <tromey@redhat.com>
>
>        * frame.h (frame_unwinder_is): Declare.
>        * frame.c (frame_unwinder_is): New function.
>        * dwarf2loc.c: Include dwarf2-frame.h.
>        (dwarf_expr_frame_cfa): New function.
>        (dwarf2_evaluate_loc_desc): Use it.
>        (needs_frame_frame_cfa): New function.
>        (dwarf2_loc_desc_needs_frame): Use it.
>        * dwarf2expr.h (struct dwarf_expr_context) <get_frame_cfa>: New
>        field.
>        * dwarf2expr.c (execute_stack_op) <DW_OP_call_frame_cfa>: New
>        case.
>        * dwarf2-frame.h (dwarf2_frame_cfa): Declare.
>        * dwarf2-frame.c (no_get_frame_cfa): New function.
>        (execute_stack_op): Use it.
>        (dwarf2_frame_cfa): New function.
>
> 2009-09-01  Tom Tromey  <tromey@redhat.com>
>
>        * gdb.dwarf2/callframecfa.exp: New file.
>        * gdb.dwarf2/callframecfa.S: New file.
>
> diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
> index 427f58f..f32a44f 100644
> --- a/gdb/dwarf2-frame.c
> +++ b/gdb/dwarf2-frame.c
> @@ -309,6 +309,14 @@ no_get_frame_base (void *baton, gdb_byte **start, size_t *length)
>                  _("Support for DW_OP_fbreg is unimplemented"));
>  }
>
> +/* Helper function for execute_stack_op.  */
> +static CORE_ADDR
> +no_get_frame_cfa (void *baton)
> +{
> +  internal_error (__FILE__, __LINE__,
> +                 _("Support for DW_OP_call_frame_cfa is unimplemented"));
> +}
> +
>  static CORE_ADDR
>  no_get_tls_address (void *baton, CORE_ADDR offset)
>  {
> @@ -363,6 +371,7 @@ execute_stack_op (gdb_byte *exp, ULONGEST len, int addr_size,
>   ctx->read_reg = read_reg;
>   ctx->read_mem = read_mem;
>   ctx->get_frame_base = no_get_frame_base;
> +  ctx->get_frame_cfa = no_get_frame_cfa;
>   ctx->get_tls_address = no_get_tls_address;
>
>   dwarf_expr_push (ctx, initial);
> @@ -1250,6 +1259,20 @@ dwarf2_frame_base_sniffer (struct frame_info *this_frame)
>
>   return NULL;
>  }
> +
> +/* Compute the CFA for THIS_FRAME, but only if THIS_FRAME came from
> +   the DWARF unwinder.  This is used to implement
> +   DW_OP_call_frame_cfa.  */
> +
> +CORE_ADDR
> +dwarf2_frame_cfa (struct frame_info *this_frame)
> +{
> +  while (get_frame_type (this_frame) == INLINE_FRAME)
> +    this_frame = get_prev_frame (this_frame);
> +  if (! frame_unwinder_is (this_frame, &dwarf2_frame_unwind))
> +    error (_("can't compute CFA for this frame"));
> +  return get_frame_base (this_frame);
> +}
>
>  const struct objfile_data *dwarf2_frame_objfile_data;
>
> diff --git a/gdb/dwarf2-frame.h b/gdb/dwarf2-frame.h
> index b203661..dd03d59 100644
> --- a/gdb/dwarf2-frame.h
> +++ b/gdb/dwarf2-frame.h
> @@ -118,4 +118,8 @@ extern const struct frame_base *
>
>  void dwarf2_frame_build_info (struct objfile *objfile);
>
> +/* Compute the DWARF CFA for a frame.  */
> +
> +CORE_ADDR dwarf2_frame_cfa (struct frame_info *this_frame);
> +
>  #endif /* dwarf2-frame.h */
> diff --git a/gdb/dwarf2expr.c b/gdb/dwarf2expr.c
> index 2721065..6401e72 100644
> --- a/gdb/dwarf2expr.c
> +++ b/gdb/dwarf2expr.c
> @@ -716,6 +716,10 @@ execute_stack_op (struct dwarf_expr_context *ctx,
>          }
>          break;
>
> +       case DW_OP_call_frame_cfa:
> +         result = (ctx->get_frame_cfa) (ctx->baton);
> +         break;
> +
>        case DW_OP_GNU_push_tls_address:
>          /* Variable is at a constant offset in the thread-local
>          storage block into the objfile for the current thread and
> diff --git a/gdb/dwarf2expr.h b/gdb/dwarf2expr.h
> index 2306e49..6b3a068 100644
> --- a/gdb/dwarf2expr.h
> +++ b/gdb/dwarf2expr.h
> @@ -55,6 +55,9 @@ struct dwarf_expr_context
>      expression evaluation is complete.  */
>   void (*get_frame_base) (void *baton, gdb_byte **start, size_t *length);
>
> +  /* Return the CFA for the frame.  */
> +  CORE_ADDR (*get_frame_cfa) (void *baton);
> +
>   /* Return the thread-local storage address for
>      DW_OP_GNU_push_tls_address.  */
>   CORE_ADDR (*get_tls_address) (void *baton, CORE_ADDR offset);
> diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
> index b6c9b11..95ee2f5 100644
> --- a/gdb/dwarf2loc.c
> +++ b/gdb/dwarf2loc.c
> @@ -36,6 +36,7 @@
>  #include "dwarf2.h"
>  #include "dwarf2expr.h"
>  #include "dwarf2loc.h"
> +#include "dwarf2-frame.h"
>
>  #include "gdb_string.h"
>  #include "gdb_assert.h"
> @@ -194,6 +195,15 @@ dwarf_expr_frame_base (void *baton, gdb_byte **start, size_t * length)
>           SYMBOL_NATURAL_NAME (framefunc));
>  }
>
> +/* Helper function for dwarf2_evaluate_loc_desc.  Computes the CFA for
> +   the frame in BATON.  */
> +static CORE_ADDR
> +dwarf_expr_frame_cfa (void *baton)
> +{
> +  struct dwarf_expr_baton *debaton = (struct dwarf_expr_baton *) baton;
> +  return dwarf2_frame_cfa (debaton->frame);
> +}
> +
>  /* Using the objfile specified in BATON, find the address for the
>    current thread's thread-local storage with offset OFFSET.  */
>  static CORE_ADDR
> @@ -237,6 +247,7 @@ dwarf2_evaluate_loc_desc (struct symbol *var, struct frame_info *frame,
>   ctx->read_reg = dwarf_expr_read_reg;
>   ctx->read_mem = dwarf_expr_read_mem;
>   ctx->get_frame_base = dwarf_expr_frame_base;
> +  ctx->get_frame_cfa = dwarf_expr_frame_cfa;
>   ctx->get_tls_address = dwarf_expr_tls_address;
>
>   dwarf_expr_eval (ctx, data, size);
> @@ -331,6 +342,15 @@ needs_frame_frame_base (void *baton, gdb_byte **start, size_t * length)
>   nf_baton->needs_frame = 1;
>  }
>
> +/* CFA accesses require a frame.  */
> +static CORE_ADDR
> +needs_frame_frame_cfa (void *baton)
> +{
> +  struct needs_frame_baton *nf_baton = baton;
> +  nf_baton->needs_frame = 1;
> +  return 1;
> +}
> +
>  /* Thread-local accesses do require a frame.  */
>  static CORE_ADDR
>  needs_frame_tls_address (void *baton, CORE_ADDR offset)
> @@ -363,6 +383,7 @@ dwarf2_loc_desc_needs_frame (gdb_byte *data, unsigned short size,
>   ctx->read_reg = needs_frame_read_reg;
>   ctx->read_mem = needs_frame_read_mem;
>   ctx->get_frame_base = needs_frame_frame_base;
> +  ctx->get_frame_cfa = needs_frame_frame_cfa;
>   ctx->get_tls_address = needs_frame_tls_address;
>
>   dwarf_expr_eval (ctx, data, size);
> diff --git a/gdb/frame.c b/gdb/frame.c
> index 67e0607..3f32471 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -1843,6 +1843,17 @@ get_frame_args_address (struct frame_info *fi)
>   return fi->base->this_args (fi, &fi->base_cache);
>  }
>
> +/* Return true if the frame base for frame FI is BASE; false
> +   otherwise.  */
> +
> +int
> +frame_unwinder_is (struct frame_info *fi, const struct frame_unwind *unwinder)
> +{
> +  if (fi->unwind == NULL)
> +    fi->unwind = frame_unwind_find_by_frame (fi, &fi->prologue_cache);
> +  return fi->unwind == unwinder;
> +}
> +
>  /* Level of the selected frame: 0 for innermost, 1 for its caller, ...
>    or -1 for a NULL frame.  */
>
> diff --git a/gdb/frame.h b/gdb/frame.h
> index febef5c..611c6d3 100644
> --- a/gdb/frame.h
> +++ b/gdb/frame.h
> @@ -696,4 +696,10 @@ extern struct frame_info *deprecated_safe_get_selected_frame (void);
>
>  extern struct frame_info *create_new_frame (CORE_ADDR base, CORE_ADDR pc);
>
> +/* Return true if the frame unwinder for frame FI is UNWINDER; false
> +   otherwise.  */
> +
> +extern int frame_unwinder_is (struct frame_info *fi,
> +                             const struct frame_unwind *unwinder);
> +
>  #endif /* !defined (FRAME_H)  */
>