Re: [PATCH] Add proper handling for non-local references in nested functions

[Pedro Alves <palves at redhat dot com>]

On 03/20/2015 12:24 PM, Pierre-Marie de Rodat wrote:

>> > If someone is motivated to clean this up, it'd be better to make the
>> > symbol lookup functions return a structure that included both symbol
>> > and block (and maybe more), in the spirit of struct
>> > bound_minimal_symbol:
> This would look cleaner indeed. It's a big change itself though so if 
> most consider this as a good idea I don't mind doing it... although it 
> would be for another commit!

I would think it great if someone did that.  :-)

> 
>> > For the block->static_link case, maybe put the static link chain
>> > in a separate hash indexed by block?
> This is what I did in the updated patch: I don't think there's a 
> realistic use case for gdbarch-owned block having a static link so I 
> added a hashed table to all objfiles. It's indexed by blocks and 
> contains static links for these (if they have one). Then we perform 
> lookups when reading variables...
> 
> Thank you for your review, Pedro! :-)
> 

Thanks for the update, and sorry about the delay.

This overall looks very reasonable to me.  It's fine with me to
let the core changes in, and address Python API issues separately.

> diff --git a/gdb/block.c b/gdb/block.c
> index 00a7012..db2e868 100644
> --- a/gdb/block.c
> +++ b/gdb/block.c
> @@ -428,6 +428,19 @@ set_block_compunit_symtab (struct block *block, struct compunit_symtab *cu)
>    gb->compunit_symtab = cu;
>  }
>  

Add:

/* See block.h.  */

> +struct dynamic_prop *
> +block_static_link (const struct block *block)
> +{

...

> diff --git a/gdb/block.h b/gdb/block.h
> index bdc5888..5f0ccf3 100644
> --- a/gdb/block.h
> +++ b/gdb/block.h
> @@ -190,6 +190,8 @@ extern struct block *allocate_global_block (struct obstack *obstack);
>  extern void set_block_compunit_symtab (struct block *,
>  				       struct compunit_symtab *);
>  
> +extern struct dynamic_prop *block_static_link (const struct block *block);
> +

It'd be great if you could skim over the patch add any missing
function intro comments.  You've already done a good job at that,
I think only here and there missed it.

> diff --git a/gdb/language.h b/gdb/language.h
> index 436fd6e..1ab0699 100644
> --- a/gdb/language.h
> +++ b/gdb/language.h
> @@ -241,13 +241,19 @@ struct language_defn
>      void (*la_value_print) (struct value *, struct ui_file *,
>  			    const struct value_print_options *);
>  
> -    /* Given a symbol VAR, and a stack frame id FRAME, read the value
> -       of the variable an return (pointer to a) struct value containing
> -       the value.
> +    /* Given a symbol VAR, the corresponding block VAR_BLOCK (if any) and a
> +       stack frame id FRAME, read the value of the variable and return (pointer
> +       to a) struct value containing the value.
> +
> +       VAR_BLOCK is needed there's a possibility for VAR to be outside FRAME.

I think an "if" is missing after "needed".

> +       This is what happens if FRAME correspond to a nested function and VAR is
> +       defined in the outer function.  If callers know that VAR is located in
> +       FRAME, NULL can be passed as VAR_BLOCK.



> --- a/gdb/printcmd.c
> +++ b/gdb/printcmd.c
> @@ -1990,7 +1990,11 @@ print_variable_and_value (const char *name, struct symbol *var,
>        struct value *val;
>        struct value_print_options opts;
>  
> -      val = read_var_value (var, frame);
> +      /* READ_VAR_VALUE needs a block in order to deal with non-local
> +	 references (i.e. to handlee nested functions). In this context, we

typo "handle".  Double space after period.


> +# Please email any bugs, comments, and/or additions to this file to:
> +# bug-gdb@gnu.org

It no longer makes sense to add this email address to tests.  Please
drop it (here and elsewhere).

> +void iter_str (const char *str, void (*callback) (char c))

Could you make this follow GNU formatting?  That is:

void
iter_str (const char *str, void (*callback) (char c))

Here and elsewhere.

> +int
> +main ()

int
main (void)

(more cases).

> +
> +# Check we get correct values for both local and non-local variable references.
> +
> +# Note that in order to get the following test passing, one has to use a
> +# patched GCC: see <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53927>.
> +gdb_test "print first"        "1"
> +gdb_test "print parent_first" "1"

Please make this XFAIL instead of FAIL with unpatched GCC.

Otherwise looks good to me.

Thanks,
Pedro Alves