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

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

On 24-09-19 17:28, Tom de Vries wrote:
> 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^2.

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>"
>>