Re: [PATCH 08/10] Use the linkage name if it exists

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

On 25-03-2020 21:07, Tom Tromey wrote:
> The DWARF reader has had some odd code since the "physname" patches landed.
> 
> In particular, these patches caused PR symtab/12707; namely, they made
> it so "set print demangle off" no longer works.
> 
> This patch attempts to fix the problem.  It arranges to store the
> linkage name on the symbol if it exists, and it changes the DWARF
> reader so that the demangled name is no longer (usually) stored in the
> symbol's "linkage name" field.
> 
> c-linkage-name.exp needed a tweak, because it started working
> correctly.  This conforms to what I think ought to happen, so this
> seems like an improvement here.
> 
> compile-object-load.c needed a small change to use
> symbol_matches_search_name rather than directly examining the linkage
> name.  Looking directly at the name does the wrong thing for C++.
> 
> There is still some name-related confusion in the DWARF reader:
> 
> * "physname" often refers to the logical name and not what I would
>   consider to be the "physical" name;
> 
> * dwarf2_full_name, dwarf2_name, and dwarf2_physname all exist and
>   return different strings -- but this seems like at least one name
>   too many.  For example, Fortran requires dwarf2_full_name, but other
>   languages do not.
> 
> * To my surprise, dwarf2_physname prefers the form emitted by the
>   demangler over the one that it computes.  This seems backward to me,
>   given that the partial symbol reader prefers the opposite, and it
>   seems to me that this choice may perform better as well.
> 
> I didn't attempt to clean up these things.  It would be good to do,
> but whenever I contemplate it I get caught up in dreams of truly
> rewriting the DWARF reader instead.
> 

Hi,

As you mentioned on IRC, there's a regression with
gdb.dwarf2/dw2-bad-mips-linkage-name.exp and target board
cc-with-debug-names. I tracked that down to this patch in the series.

Using readelf -w, I can read the .debug_names section, and see without
this patch:
...
[  6] #0002b60b f: <3> DW_TAG_subprogram DW_IDX_compile_unit=3
DW_IDX_GNU_internal=1
[  8] #0002b60c g: <3> DW_TAG_subprogram DW_IDX_compile_unit=3
DW_IDX_GNU_internal=1
...
and with this patch:
...
[  6] #0ef9dc4b _Z1fv: <3> DW_TAG_subprogram DW_IDX_compile_unit=3
DW_IDX_GNU_internal=1
[  8] #0002b60c g: <3> DW_TAG_subprogram DW_IDX_compile_unit=3
DW_IDX_GNU_internal=1
...

So we end up with the mangled name for f in the index.  That looks
significant, but I'm not sure yet if that is the reason why we're not
able to print the type of f.

Thanks,
- Tom

> gdb/ChangeLog
> 2020-03-25  Tom Tromey  <tom@tromey.com>
> 
> 	PR symtab/12707:
> 	* dwarf2/read.c (add_partial_symbol): Use the linkage name if it
> 	exists.
> 	(new_symbol): Likewise.
> 	* compile/compile-object-load.c (get_out_value_type): Use
> 	symbol_matches_search_name.
> 
> gdb/testsuite/ChangeLog
> 2020-03-25  Tom Tromey  <tom@tromey.com>
> 
> 	PR symtab/12707:
> 	* gdb.python/py-symbol.exp: Update expected results for
> 	linkage_name test.
> 	* gdb.cp/print-demangle.exp: New file.
> 	* gdb.base/c-linkage-name.exp: Fix test.
> 	* gdb.guile/scm-symbol.exp: Update expected results for
> 	linkage_name test.
> ---
>  gdb/ChangeLog                             |  9 +++++++
>  gdb/compile/compile-object-load.c         |  7 ++---
>  gdb/dwarf2/read.c                         | 29 ++++++++++++++------
>  gdb/testsuite/ChangeLog                   | 10 +++++++
>  gdb/testsuite/gdb.base/c-linkage-name.exp |  2 +-
>  gdb/testsuite/gdb.cp/print-demangle.exp   | 32 +++++++++++++++++++++++
>  gdb/testsuite/gdb.guile/scm-symbol.exp    |  4 +--
>  gdb/testsuite/gdb.python/py-symbol.exp    |  2 +-
>  8 files changed, 79 insertions(+), 16 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.cp/print-demangle.exp
> 
> diff --git a/gdb/compile/compile-object-load.c b/gdb/compile/compile-object-load.c
> index 3fe95183e32..be4fa767142 100644
> --- a/gdb/compile/compile-object-load.c
> +++ b/gdb/compile/compile-object-load.c
> @@ -402,6 +402,9 @@ get_out_value_type (struct symbol *func_sym, struct objfile *objfile,
>    int nblocks = 0;
>    int block_loop = 0;
>  
> +  lookup_name_info func_matcher (GCC_FE_WRAPPER_FUNCTION,
> +				 symbol_name_match_type::SEARCH_NAME);
> +
>    bv = SYMTAB_BLOCKVECTOR (func_sym->owner.symtab);
>    nblocks = BLOCKVECTOR_NBLOCKS (bv);
>  
> @@ -433,9 +436,7 @@ get_out_value_type (struct symbol *func_sym, struct objfile *objfile,
>        if (function != NULL
>  	  && (BLOCK_SUPERBLOCK (function_block)
>  	      == BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK))
> -	  && (strcmp_iw (function->linkage_name (),
> -			 GCC_FE_WRAPPER_FUNCTION)
> -	      == 0))
> +	  && symbol_matches_search_name (function, func_matcher))
>  	break;
>      }
>    if (block_loop == nblocks)
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index 2bc7b521f2e..1e824ca399d 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -8370,7 +8370,14 @@ add_partial_symbol (struct partial_die_info *pdi, struct dwarf2_cu *cu)
>      {
>        if (built_actual_name != nullptr)
>  	actual_name = objfile->intern (actual_name);
> -      psymbol.ginfo.set_linkage_name (actual_name);
> +      if (pdi->linkage_name == nullptr || cu->language == language_ada)
> +	psymbol.ginfo.set_linkage_name (actual_name);
> +      else
> +	{
> +	  psymbol.ginfo.set_demangled_name (actual_name,
> +					    &objfile->objfile_obstack);
> +	  psymbol.ginfo.set_linkage_name (pdi->linkage_name);
> +	}
>        add_psymbol_to_list (psymbol, *where, objfile);
>      }
>  }
> @@ -20550,7 +20557,6 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
>    name = dwarf2_name (die, cu);
>    if (name)
>      {
> -      const char *linkagename;
>        int suppress_add = 0;
>  
>        if (space)
> @@ -20561,14 +20567,21 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
>  
>        /* Cache this symbol's name and the name's demangled form (if any).  */
>        sym->set_language (cu->language, &objfile->objfile_obstack);
> -      linkagename = dwarf2_physname (name, die, cu);
> -      sym->compute_and_set_names (linkagename, false, objfile->per_bfd);
> -
>        /* Fortran does not have mangling standard and the mangling does differ
>  	 between gfortran, iFort etc.  */
> -      if (cu->language == language_fortran
> -          && symbol_get_demangled_name (sym) == NULL)
> -	sym->set_demangled_name (dwarf2_full_name (name, die, cu), NULL);
> +      const char *physname
> +	= (cu->language == language_fortran
> +	   ? dwarf2_full_name (name, die, cu)
> +	   : dwarf2_physname (name, die, cu));
> +      const char *linkagename = dw2_linkage_name (die, cu);
> +
> +      if (linkagename == nullptr || cu->language == language_ada)
> +	sym->set_linkage_name (physname);
> +      else
> +	{
> +	  sym->set_demangled_name (physname, &objfile->objfile_obstack);
> +	  sym->set_linkage_name (linkagename);
> +	}
>  
>        /* Default assumptions.
>           Use the passed type or decode it from the die.  */
> diff --git a/gdb/testsuite/gdb.base/c-linkage-name.exp b/gdb/testsuite/gdb.base/c-linkage-name.exp
> index 4551c8dcac2..cf3e3780bb6 100644
> --- a/gdb/testsuite/gdb.base/c-linkage-name.exp
> +++ b/gdb/testsuite/gdb.base/c-linkage-name.exp
> @@ -48,7 +48,7 @@ verify_psymtab_expanded c-linkage-name-2.c no
>  # Try to print MUNDANE, but using its linkage name.
>  
>  gdb_test "print symada__cS" \
> -         "'symada__cS' has unknown type; cast it to its declared type" \
> +         " = {a = 100829103}" \
>           "print symada__cS before partial symtab expansion"
>  
>  # Force the symbols to be expanded for the unit that contains
> diff --git a/gdb/testsuite/gdb.cp/print-demangle.exp b/gdb/testsuite/gdb.cp/print-demangle.exp
> new file mode 100644
> index 00000000000..9e0f706febf
> --- /dev/null
> +++ b/gdb/testsuite/gdb.cp/print-demangle.exp
> @@ -0,0 +1,32 @@
> +# Copyright (C) 2013, 2020 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +if { [skip_cplus_tests] } { continue }
> +
> +standard_testfile bool.cc
> +
> +if {[prepare_for_testing $testfile.exp $testfile $srcfile {debug c++}]} {
> +    return -1
> +}
> +
> +runto_main
> +
> +gdb_breakpoint "return_true"
> +
> +gdb_continue_to_breakpoint "return_true"
> +
> +gdb_test_no_output "set print demangle off"
> +
> +gdb_test "frame" " _Z11return_truev .*"
> diff --git a/gdb/testsuite/gdb.guile/scm-symbol.exp b/gdb/testsuite/gdb.guile/scm-symbol.exp
> index 4addc0d10d0..486fc8fcfdb 100644
> --- a/gdb/testsuite/gdb.guile/scm-symbol.exp
> +++ b/gdb/testsuite/gdb.guile/scm-symbol.exp
> @@ -169,10 +169,8 @@ gdb_test "guile (print (symbol-name cplusfunc))" \
>      "= SimpleClass::valueofi().*" "test method.name"
>  gdb_test "guile (print (symbol-print-name cplusfunc))" \
>      "= SimpleClass::valueofi().*" "test method.print_name"
> -# FIXME: GDB is broken here and we're verifying broken behaviour.
> -# (linkage-name should be the mangled name)
>  gdb_test "guile (print (symbol-linkage-name cplusfunc))" \
> -    "SimpleClass::valueofi().*" "test method.linkage_name"
> +    "_ZN11SimpleClass8valueofiEv.*" "test method.linkage_name"
>  gdb_test "guile (print (= (symbol-addr-class cplusfunc) SYMBOL_LOC_BLOCK))" "= #t"
>  
>  # Test is_valid when the objfile is unloaded.  This must be the last
> diff --git a/gdb/testsuite/gdb.python/py-symbol.exp b/gdb/testsuite/gdb.python/py-symbol.exp
> index c4bae9f07f2..caa7ddc8007 100644
> --- a/gdb/testsuite/gdb.python/py-symbol.exp
> +++ b/gdb/testsuite/gdb.python/py-symbol.exp
> @@ -211,7 +211,7 @@ gdb_test "python print (cplusfunc.is_function)" \
>  
>  gdb_test "python print (cplusfunc.name)" "SimpleClass::valueofi().*" "test method.name"
>  gdb_test "python print (cplusfunc.print_name)" "SimpleClass::valueofi().*" "test method.print_name"
> -gdb_test "python print (cplusfunc.linkage_name)" "SimpleClass::valueofi().*" "test method.linkage_name"
> +gdb_test "python print (cplusfunc.linkage_name)" "_ZN11SimpleClass8valueofiEv.*" "test method.linkage_name"
>  gdb_test "python print (cplusfunc.addr_class == gdb.SYMBOL_LOC_BLOCK)" "True" "test method.addr_class"
>  
>  # Test is_valid when the objfile is unloaded.  This must be the last
>