[PING][PATCH][gdb/symtab] Prefer var def over decl

[Tom de Vries <tdevries@suse.de>]

On 10-09-19 17:43, Tom de Vries wrote:
> Hi,
> 
> Consider the DWARF as generated by gcc with the tentative patch to fix gcc
> PR91507 - "wrong debug for completed array with previous incomplete
> declaration":
> ...
>  <1><f4>: Abbrev Number: 2 (DW_TAG_array_type)
>     <f5>   DW_AT_type        : <0xff>
>     <f9>   DW_AT_sibling     : <0xff>
>  <2><fd>: Abbrev Number: 3 (DW_TAG_subrange_type)
>  <2><fe>: Abbrev Number: 0
>  <1><ff>: Abbrev Number: 4 (DW_TAG_pointer_type)
>     <100>   DW_AT_byte_size   : 8
>     <101>   DW_AT_type        : <0x105>
>  <1><105>: Abbrev Number: 5 (DW_TAG_base_type)
>     <106>   DW_AT_byte_size   : 1
>     <107>   DW_AT_encoding    : 6       (signed char)
>     <108>   DW_AT_name        : (indirect string, offset: 0x19f): char
>  <1><10c>: Abbrev Number: 6 (DW_TAG_variable)
>     <10d>   DW_AT_name        : zzz
>     <111>   DW_AT_decl_file   : 1
>     <112>   DW_AT_decl_line   : 1
>     <113>   DW_AT_decl_column : 14
>     <114>   DW_AT_type        : <0xf4>
>     <118>   DW_AT_external    : 1
>     <118>   DW_AT_declaration : 1
>  <1><118>: Abbrev Number: 2 (DW_TAG_array_type)
>     <119>   DW_AT_type        : <0xff>
>     <11d>   DW_AT_sibling     : <0x128>
>  <1><12f>: Abbrev Number: 8 (DW_TAG_variable)
>     <130>   DW_AT_specification: <0x10c>
>     <134>   DW_AT_decl_line   : 2
>     <135>   DW_AT_decl_column : 7
>     <136>   DW_AT_type        : <0x118>
>     <13a>   DW_AT_location    : 9 byte block: 3 30 10 60 0 0 0 0 0      (DW_OP_addr: 601030)
> ...
> 
> The DWARF will result in two entries in the symbol table, a decl with type
> char *[] and a def with type char*[2].
> 
> When trying to print the value of zzz:
> ...
> $ gdb a.spec.out -batch -ex "p zzz"
> ...
> the decl (rather than the def) will be found in the symbol table, which is
> missing the location information, and consequently we get:
> ...
> $1 = 0x601030 <zzz>
> ...
> 
> [ There is a fallback mechanism that finds the address of the variable in the
> minimal symbol table, but that's not used here, because the type of the decl
> does not specify a size.  We could use the symbol size here to get the size
> of the type, but that's currently not done: PR exp/24989.  Still, fixing that
> PR would not fix the generic case, where minimal symbol info is not
> available. ]
> 
> Fix this by preferring defs over decls when searching in the symbol table.
> 
> Build and reg-tested on x86_64-linux.
> 
> [ The test-case is a bit simpler than the DWARF example listed above, because
> the new variable varval3 that is used is not listed in the minimal symbols, so
> there's no need to work around the fallback mechanism to trigger the problem. ]
> 
> OK for trunk?

Ping.

Thanks,
- Tom

> [gdb/symtab] Prefer var def over decl
> 
> gdb/ChangeLog:
> 
> 2019-09-10  Tom de Vries  <tdevries@suse.de>
> 
> 	PR symtab/24971
> 	* block.c (best_symbol, better_symbol): New function.
> 	(block_lookup_symbol_primary): Prefer def over decl.
> 
> gdb/testsuite/ChangeLog:
> 
> 2019-09-10  Tom de Vries  <tdevries@suse.de>
> 
> 	* gdb.dwarf2/varval.exp: Add decl before def test.
> 
> ---
>  gdb/block.c                         | 36 ++++++++++++++++++++++++++++++++++--
>  gdb/testsuite/gdb.dwarf2/varval.exp | 18 +++++++++++++++++-
>  2 files changed, 51 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/block.c b/gdb/block.c
> index ca4dc22cf3..1c0d206a7a 100644
> --- a/gdb/block.c
> +++ b/gdb/block.c
> @@ -725,6 +725,38 @@ block_lookup_symbol (const struct block *block, const char *name,
>      }
>  }
>  
> +static bool
> +best_symbol (struct symbol *a, const domain_enum domain)
> +{
> +  return (SYMBOL_DOMAIN (a) == domain
> +	  && SYMBOL_CLASS (a) != LOC_UNRESOLVED);
> +}
> +
> +static struct symbol *
> +better_symbol (struct symbol *a, struct symbol *b, const domain_enum domain)
> +{
> +  if (a == NULL)
> +    return b;
> +  if (b == NULL)
> +    return a;
> +
> +  if (SYMBOL_DOMAIN (a) == domain
> +      && SYMBOL_DOMAIN (b) != domain)
> +    return a;
> +  if (SYMBOL_DOMAIN (b) == domain
> +      && SYMBOL_DOMAIN (a) != domain)
> +    return b;
> +
> +  if (SYMBOL_CLASS (a) != LOC_UNRESOLVED
> +      && SYMBOL_CLASS (b) == LOC_UNRESOLVED)
> +    return a;
> +  if (SYMBOL_CLASS (b) != LOC_UNRESOLVED
> +      && SYMBOL_CLASS (a) == LOC_UNRESOLVED)
> +    return b;
> +
> +  return a;
> +}
> +
>  /* See block.h.  */
>  
>  struct symbol *
> @@ -746,7 +778,7 @@ block_lookup_symbol_primary (const struct block *block, const char *name,
>         sym != NULL;
>         sym = mdict_iter_match_next (lookup_name, &mdict_iter))
>      {
> -      if (SYMBOL_DOMAIN (sym) == domain)
> +      if (best_symbol (sym, domain))
>  	return sym;
>  
>        /* This is a bit of a hack, but symbol_matches_domain might ignore
> @@ -755,7 +787,7 @@ block_lookup_symbol_primary (const struct block *block, const char *name,
>  	 exactly the same domain.  PR 16253.  */
>        if (symbol_matches_domain (SYMBOL_LANGUAGE (sym),
>  				 SYMBOL_DOMAIN (sym), domain))
> -	other = sym;
> +	other = better_symbol (other, sym, domain);
>      }
>  
>    return other;
> diff --git a/gdb/testsuite/gdb.dwarf2/varval.exp b/gdb/testsuite/gdb.dwarf2/varval.exp
> index 594591025f..0f3707bd5a 100644
> --- a/gdb/testsuite/gdb.dwarf2/varval.exp
> +++ b/gdb/testsuite/gdb.dwarf2/varval.exp
> @@ -55,7 +55,7 @@ proc setup_exec { arg_bad } {
>  		    var_b_label var_c_label var_p_label var_bad_label \
>  		    varval_label var_s_label var_untyped_label \
>  		    var_a_abstract_label var_a_concrete_label \
> -		    varval2_label
> +		    varval2_label varval3_def_label varval3_decl_label
>  
>  		set int_size [get_sizeof "int" -1]
>  
> @@ -171,6 +171,20 @@ proc setup_exec { arg_bad } {
>  		    {DW_AT_location {DW_OP_addr [gdb_target_symbol "var_b"]} SPECIAL_expr}
>  		}
>  
> +		    varval3_decl_label: DW_TAG_variable {
> +			{DW_AT_name "varval3"}
> +			{DW_AT_type :${int_label}}
> +			{DW_AT_external 1 DW_FORM_flag}
> +			{DW_AT_declaration 1 DW_FORM_flag}
> +		    }
> +		    varval3_def_label: DW_TAG_variable {
> +			{DW_AT_name "varval3"}
> +			{DW_AT_external 1 DW_FORM_flag}
> +			{DW_AT_type :${int_label}}
> +			{DW_AT_location {DW_OP_addr [gdb_target_symbol "var_a"]} SPECIAL_expr}
> +		    }
> +
> +
>  		DW_TAG_subprogram {
>  		    {MACRO_AT_func { "main" "${srcdir}/${subdir}/${srcfile}" }}
>  		    {DW_AT_type :${int_label}}
> @@ -192,6 +206,7 @@ proc setup_exec { arg_bad } {
>  			    DW_OP_stack_value
>  			} SPECIAL_expr}
>  		    }
> +
>  		    var_a_concrete_label: DW_TAG_variable {
>  			{DW_AT_abstract_origin :${var_a_abstract_label}}
>  			{DW_AT_location {DW_OP_addr [gdb_target_symbol "var_a"]} SPECIAL_expr}
> @@ -290,6 +305,7 @@ if { [setup_exec 0] == -1 } {
>  
>  gdb_test "print varval" "= 8"
>  gdb_test "print varval2" "= 8"
> +gdb_test "print varval3" "= 8"
>  gdb_test "print constval" "= 53"
>  gdb_test "print mixedval" "= 42"
>  gdb_test "print pointerval" "= \\(int \\*\\) $hex <var_b>"
>