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

[Doug Evans <xdje42 at gmail dot com>]

Pierre-Marie de Rodat <derodat@adacore.com> writes:
> Doug,
>
> Once again, thanks for the review!
>
> On 08/15/2015 07:08 AM, Doug Evans wrote:
>>> +/* Implement the struct symbol_block_ops::get_frame_base method.  */
>>> +
>>> +static CORE_ADDR
>>> +block_op_get_frame_base (struct symbol *framefunc, struct frame_info *frame)
>>> +{
>>> +  if (SYMBOL_BLOCK_OPS (framefunc)->find_frame_base_location != NULL)
>>
>> ====
>> It's hard for this reader to figure out why this test is here.
>> If the code gets here, when would find_frame_base_location be NULL?
>> Seems like we could just assert find_frame_base_location is non-NULL.
>
> Good catch! This is fixed, now, with a comment explaining this.
>
>>> +  /* Old compilers may not provide a static link, or they may provide an
>>> +     invalid one.  For such cases, fallback on the old way to evaluate
>>> +     non-local references: just climb up the call stack and pick the first
>>> +     frame that contains the variable we are looking for.  */
>>> +  if (frame == NULL)
>>> +    {
>>> +      frame = block_innermost_frame (var_block);
>>> +      if (!frame)
>>
>> ====
>> frame == NULL
>
> Done.
>
>> Ok with them fixed (not sure what the right fix for the first one is
>> though, I could be missing something).
>
> As said in my previous mail, I'm not sure I want to commit it: I tried
> to investigate this 4% unexpected performance drop, at least to see
> what happens with a profiler but I could not reproduce.

I'd say let's go ahead.
It's certainly possible I'm seeing the difference due to local
artifacts. I've tried to rule them out, but TBH I haven't put a whole
lot of time into it (beyond trying twice in two different trees).
If someone else isn't seeing them then that's good data.
I'll play with this a bit more, if it's a local issue
it'd be good to find out what it is. :-)

> Pierre-Marie de Rodat
>
> From f8cb12e93bc4b317bf03dd31fc158cc05fc60367 Mon Sep 17 00:00:00 2001
> From: Pierre-Marie de Rodat <derodat@adacore.com>
> Date: Thu, 5 Feb 2015 17:00:06 +0100
> Subject: [PATCH] DWARF: handle non-local references in nested functions
>
> GDB's current behavior when dealing with non-local references in the
> context of nested fuctions is approximative:
>
>   - code using valops.c:value_of_variable read the first available stack
>     frame that holds the corresponding variable (whereas there can be
>     multiple candidates for this);
>
>   - code directly relying on read_var_value will instead read non-local
>     variables in frames where they are not even defined.
>
> This change adds the necessary context to symbol reads (to get the block
> they belong to) and to blocks (the static link property, if any) so that
> GDB can make the proper decisions when dealing with non-local varibale
> references.
>
> gdb/ChangeLog:
>
> 	* ada-lang.c (ada_read_var_value): Add a var_block argument
> 	and pass it to default_read_var_value.
> 	* block.c (block_static_link): New accessor.
> 	* block.h (block_static_link): Declare it.
> 	* buildsym.c (finish_block_internal): Add a static_link
> 	argument.  If there is a static link, associate it to the new
> 	block.
> 	(finish_block): Add a static link argument and pass it to
> 	finish_block_internal.
> 	(end_symtab_get_static_block): Update calls to finish_block and
> 	to finish_block_internal.
> 	(end_symtab_with_blockvector): Update call to
> 	finish_block_internal.
> 	* buildsym.h: Forward-declare struct dynamic_prop.
> 	(struct context_stack): Add a static_link field.
> 	(finish_block): Add a static link argument.
> 	* c-exp.y: Remove an obsolete comment (evaluation of variables
> 	already start from the selected frame, and now they climb *up*
> 	the call stack) and propagate the block information to the
> 	produced expression.
> 	* d-exp.y: Likewise.
> 	* f-exp.y: Likewise.
> 	* go-exp.y: Likewise.
> 	* jv-exp.y: Likewise.
> 	* m2-exp.y: Likewise.
> 	* p-exp.y: Likewise.
> 	* coffread.c (coff_symtab_read): Update calls to finish_block.
> 	* dbxread.c (process_one_symbol): Likewise.
> 	* xcoffread.c (read_xcoff_symtab): Likewise.
> 	* compile/compile-c-symbols.c (convert_one_symbol): Promote the
> 	"sym" parameter to struct block_symbol, update its uses and pass
> 	its block to calls to read_var_value.
> 	(convert_symbol_sym): Update the calls to convert_one_symbol.
> 	* compile/compile-loc2c.c (do_compile_dwarf_expr_to_c): Update
> 	call to read_var_value.
> 	* dwarf2loc.c (block_op_get_frame_base): New.
> 	(dwarf2_block_frame_base_locexpr_funcs): Implement the
> 	get_frame_base method.
> 	(dwarf2_block_frame_base_loclist_funcs): Likewise.
> 	(dwarf2locexpr_baton_eval): Add a frame argument and use it
> 	instead of the selected frame in order to evaluate the
> 	expression.
> 	(dwarf2_evaluate_property): Add a frame argument.  Update call
> 	to dwarf2_locexpr_baton_eval to provide a frame in available and
> 	to handle the absence of address stack.
> 	* dwarf2loc.h (dwarf2_evaluate_property): Add a frame argument.
> 	* dwarf2read.c (attr_to_dynamic_prop): Add a forward
> 	declaration.
> 	(read_func_scope): Record any available static link description.
> 	Update call to finish_block.
> 	(read_lexical_block_scope): Update call to finish_block.
> 	* findvar.c (follow_static_link): New.
> 	(get_hosting_frame): New.
> 	(default_read_var_value): Add a var_block argument.  Use
> 	get_hosting_frame to handle non-local references.
> 	(read_var_value): Add a var_block argument and pass it to the
> 	LA_READ_VAR_VALUE method.
> 	* gdbtypes.c (resolve_dynamic_range): Update calls to
> 	dwarf2_evaluate_property.
> 	(resolve_dynamic_type_internal): Likewise.
> 	* guile/scm-frame.c (gdbscm_frame_read_var): Update call to
> 	read_var_value, passing it the block coming from symbol lookup.
> 	* guile/scm-symbol.c (gdbscm_symbol_value): Update call to
> 	read_var_value (TODO).
> 	* infcmd.c (finish_command_continuation): Update call to
> 	read_var_value, passing it the block coming from symbol lookup.
> 	* infrun.c (insert_exception_resume_breakpoint): Likewise.
> 	* language.h (struct language_defn): Add a var_block argument to
> 	the LA_READ_VAR_VALUE method.
> 	* objfiles.c (struct static_link_htab_entry): New.
> 	(static_link_htab_entry_hash): New.
> 	(static_link_htab_entry_eq): New.
> 	(objfile_register_static_link): New.
> 	(objfile_lookup_static_link): New.
> 	(free_objfile): Free the STATIC_LINKS hashed map if needed.
> 	* objfiles.h: Include hashtab.h.
> 	(struct objfile): Add a static_links field.
> 	(objfile_register_static_link): New.
> 	(objfile_lookup_static_link): New.
> 	* printcmd.c (print_variable_and_value): Update call to
> 	read_var_value.
> 	* python/py-finishbreakpoint.c (bpfinishpy_init): Likewise.
> 	* python/py-frame.c (frapy_read_var): Update call to
> 	read_var_value, passing it the block coming from symbol lookup.
> 	* python/py-framefilter.c (extract_sym): Add a sym_block
> 	parameter and set the pointed value to NULL (TODO).
> 	(enumerate_args): Update call to extract_sym.
> 	(enumerate_locals): Update calls to extract_sym and to
> 	read_var_value.
> 	* python/py-symbol.c (sympy_value): Update call to
> 	read_var_value (TODO).
> 	* stack.c (read_frame_local): Update call to read_var_value.
> 	(read_frame_arg): Likewise.
> 	(return_command): Likewise.
> 	* symtab.h (struct symbol_block_ops): Add a get_frame_base
> 	method.
> 	(struct symbol): Add a block field.
> 	(SYMBOL_BLOCK): New accessor.
> 	* valops.c (value_of_variable): Remove frame/block handling and
> 	pass the block argument to read_var_value, which does this job
> 	now.
> 	(value_struct_elt_for_reference): Update calls to
> 	read_var_value.
> 	(value_of_this): Pass the block found to read_var_value.
> 	* value.h (read_var_value): Add a var_block argument.
> 	(default_read_var_value): Likewise.
>
> gdb/testsuite/ChangeLog:
>
> 	* gdb.base/nested-subp1.exp: New file.
> 	* gdb.base/nested-subp1.c: New file.
> 	* gdb.base/nested-subp2.exp: New file.
> 	* gdb.base/nested-subp2.c: New file.
> 	* gdb.base/nested-subp3.exp: New file.
> 	* gdb.base/nested-subp3.c: New file.

LGTM.
Thanks for the patch!