Re: RFC: DW_OP_call_frame_cfa, again

[Doug Evans <dje at google dot 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) ?*/
>