Re: [PATCH v2 00/42] Share DWARF partial symtabs between objfiles

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

On 02-06-2020 23:27, Simon Marchi wrote:
> On 2020-05-31 10:22 a.m., Tom de Vries wrote:
>> On 31-05-2020 06:16, Simon Marchi wrote:
>>> See this proof of concept fix, which sets per_bfd->partial_symtabs in
>>> dwarf2_read_gdb_index (when parsing the first objfile), and reads it back
>>> in dwarf2_initialize_objfile (when parsing the second objfile).
>>
>> I've tested this patch with the test-suite file-twice hack and target
>> board cc-with-gdb-index, and it fixes all regressions.
>>
>> Also, I've tested it with my ada .gdb_index patch series, and also there
>> it fixes the regressions.
>>
>> Thanks,
>> - Tom
>>
> 
> Ok, thanks for testing.  Here's a somewhat more complete patch.
> 
> 
> From 0b342d014459b86027aa9c877233caccac8d9446 Mon Sep 17 00:00:00 2001
> From: Simon Marchi <simon.marchi@efficios.com>
> Date: Sun, 31 May 2020 00:10:33 -0400
> Subject: [PATCH] gdb: really share partial symtabs when using .gdb_index or
>  .debug_names
> 
> Fix/follow-up to commit 17ee85fc2a ("Share DWARF partial symtabs").
> 
> In the non-index case, where GDB builds partial symbols from scratch,
> two objfiles around the same BFD correctly share partial symtabs.  The
> first objfile, which has to do all the work, saves a reference to the
> created partial symtabs in the shared per_bfd object (at the end of
> dwarf2_build_psymtabs).  The second objfile, when it reaches
> dwarf2_build_psymtabs, sees that there are already partial symtabs built
> for this BFD and just uses it.
> 
> However, that commit missed implementing the same sharing for cases
> where GDB uses .gdb_index or .debug_names to build the partial symtabs.
> 
> This patch fixes it by having the first objfile to use the BFD set
> per_bfd->partial_symtabs at the end of dwarf2_read_gdb_index /
> dwarf2_read_debug_names.  For the subsequent objfiles using that BFD,
> the partial symtabs are then picked up in dwarf2_initialize_objfile.
> 
> This patch adds a test that mimics how the issue was originally
> triggered:
> 
>   1. Load the test file twice, such that the second objfile re-uses the
>      per_bfd object created for the first objfile.
>   2. Run to some point where in the backtrace there is a frame for a
>      function that's in a CU that's not yet read in.
>   3. Check that this frame's information is complete in the "backtrace"
>      output.
> 
> Step 2 requires an address -> symbol lookup which uses the addrmap at
> objfile->partial_symtabs->psymtabs_addrmap.  If the
> objfile->partial_symtabs link is not properly setup (as is the case
> before this patch), the symbol for that frame won't be found and we'll
> get a frame with incomplete information.
> 
> The test fails without the fix when using boards "cc-with-gdb-index" and
> "cc-with-debug-names".
> 

I've also tested this with the gdb_cmd_file-twice patch with these two
target boards, and everything looks good.

[ FWIW, the gdb.base/index-cache.exp with the gdb_cmd_file-twice patch
and native is still there. ]

Some comments on the test-case below...

> gdb/ChangeLog:
> 
> 	* dwarf2/read.c (dwarf2_read_gdb_index): Save partial_symtabs in
> 	the per_bfd object.
> 	(dwarf2_read_debug_names): Likewise.
> 	(dwarf2_initialize_objfile): Use partial_symtabs from per_bfd
> 	object when re-using a per_bfd object with an index.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.dwarf2/share-psymtabs-bt.exp: New file.
> 	* gdb.dwarf2/share-psymtabs-bt.c: New file.
> 	* gdb.dwarf2/share-psymtabs-bt-2.c: New file.
> 
> Change-Id: Ibb26210e2dfc03b80ba9fa56b875ba4cc58c0352
> ---
>  gdb/dwarf2/read.c                            | 56 +++++++++++--------
>  gdb/testsuite/gdb.base/share-psymtabs-bt-2.c | 24 +++++++++
>  gdb/testsuite/gdb.base/share-psymtabs-bt.c   | 29 ++++++++++
>  gdb/testsuite/gdb.base/share-psymtabs-bt.exp | 57 ++++++++++++++++++++
>  4 files changed, 144 insertions(+), 22 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.base/share-psymtabs-bt-2.c
>  create mode 100644 gdb/testsuite/gdb.base/share-psymtabs-bt.c
>  create mode 100644 gdb/testsuite/gdb.base/share-psymtabs-bt.exp
> 
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index e6566f9649f8..f04bdd1ce58e 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -3068,9 +3068,10 @@ dwarf2_read_gdb_index
>    offset_type cu_list_elements, types_list_elements, dwz_list_elements = 0;
>    struct dwz_file *dwz;
>    struct objfile *objfile = per_objfile->objfile;
> +  dwarf2_per_bfd *per_bfd = per_objfile->per_bfd;
> 
>    gdb::array_view<const gdb_byte> main_index_contents
> -    = get_gdb_index_contents (objfile, per_objfile->per_bfd);
> +    = get_gdb_index_contents (objfile, per_bfd);
> 
>    if (main_index_contents.empty ())
>      return 0;
> @@ -3089,7 +3090,7 @@ dwarf2_read_gdb_index
> 
>    /* If there is a .dwz file, read it so we can get its CU list as
>       well.  */
> -  dwz = dwarf2_get_dwz_file (per_objfile->per_bfd);
> +  dwz = dwarf2_get_dwz_file (per_bfd);
>    if (dwz != NULL)
>      {
>        struct mapped_index dwz_map;
> @@ -3114,29 +3115,33 @@ dwarf2_read_gdb_index
>  	}
>      }
> 
> -  create_cus_from_index (per_objfile->per_bfd, cu_list, cu_list_elements,
> -			 dwz_list, dwz_list_elements);
> +  create_cus_from_index (per_bfd, cu_list, cu_list_elements, dwz_list,
> +			 dwz_list_elements);
> 
>    if (types_list_elements)
>      {
>        /* We can only handle a single .debug_types when we have an
>  	 index.  */
> -      if (per_objfile->per_bfd->types.size () != 1)
> +      if (per_bfd->types.size () != 1)
>  	return 0;
> 
> -      dwarf2_section_info *section = &per_objfile->per_bfd->types[0];
> +      dwarf2_section_info *section = &per_bfd->types[0];
> 
> -      create_signatured_type_table_from_index (per_objfile->per_bfd,
> -					       section, types_list,
> +      create_signatured_type_table_from_index (per_bfd, section, types_list,
>  					       types_list_elements);
>      }
> 
>    create_addrmap_from_index (per_objfile, map.get ());
> 
> -  per_objfile->per_bfd->index_table = std::move (map);
> -  per_objfile->per_bfd->using_index = 1;
> -  per_objfile->per_bfd->quick_file_names_table =
> -    create_quick_file_names_table (per_objfile->per_bfd->all_comp_units.size ());
> +  per_bfd->index_table = std::move (map);
> +  per_bfd->using_index = 1;
> +  per_bfd->quick_file_names_table =
> +    create_quick_file_names_table (per_bfd->all_comp_units.size ());
> +
> +  /* Save partial symtabs in the per_bfd object, for the benefit of subsequent
> +     objfiles using the same BFD.  */
> +  gdb_assert (per_bfd->partial_symtabs == nullptr);
> +  per_bfd->partial_symtabs = objfile->partial_symtabs;
> 
>    return 1;
>  }
> @@ -5205,6 +5210,7 @@ dwarf2_read_debug_names (dwarf2_per_objfile *per_objfile)
>    std::unique_ptr<mapped_debug_names> map (new mapped_debug_names);
>    mapped_debug_names dwz_map;
>    struct objfile *objfile = per_objfile->objfile;
> +  dwarf2_per_bfd *per_bfd = per_objfile->per_bfd;
> 
>    if (!read_debug_names_from_section (objfile, objfile_name (objfile),
>  				      &per_objfile->per_bfd->debug_names, *map))
> @@ -5216,7 +5222,7 @@ dwarf2_read_debug_names (dwarf2_per_objfile *per_objfile)
> 
>    /* If there is a .dwz file, read it so we can get its CU list as
>       well.  */
> -  dwz_file *dwz = dwarf2_get_dwz_file (per_objfile->per_bfd);
> +  dwz_file *dwz = dwarf2_get_dwz_file (per_bfd);
>    if (dwz != NULL)
>      {
>        if (!read_debug_names_from_section (objfile,
> @@ -5229,29 +5235,33 @@ dwarf2_read_debug_names (dwarf2_per_objfile *per_objfile)
>  	}
>      }
> 
> -  create_cus_from_debug_names (per_objfile->per_bfd, *map, dwz_map);
> +  create_cus_from_debug_names (per_bfd, *map, dwz_map);
> 
>    if (map->tu_count != 0)
>      {
>        /* We can only handle a single .debug_types when we have an
>  	 index.  */
> -      if (per_objfile->per_bfd->types.size () != 1)
> +      if (per_bfd->types.size () != 1)
>  	return false;
> 
> -      dwarf2_section_info *section = &per_objfile->per_bfd->types[0];
> +      dwarf2_section_info *section = &per_bfd->types[0];
> 
>        create_signatured_type_table_from_debug_names
> -	(per_objfile, *map, section, &per_objfile->per_bfd->abbrev);
> +	(per_objfile, *map, section, &per_bfd->abbrev);
>      }
> 
> -  create_addrmap_from_aranges (per_objfile,
> -			       &per_objfile->per_bfd->debug_aranges);
> +  create_addrmap_from_aranges (per_objfile, &per_bfd->debug_aranges);
> 
> -  per_objfile->per_bfd->debug_names_table = std::move (map);
> -  per_objfile->per_bfd->using_index = 1;
> -  per_objfile->per_bfd->quick_file_names_table =
> +  per_bfd->debug_names_table = std::move (map);
> +  per_bfd->using_index = 1;
> +  per_bfd->quick_file_names_table =
>      create_quick_file_names_table (per_objfile->per_bfd->all_comp_units.size ());
> 
> +  /* Save partial symtabs in the per_bfd object, for the benefit of subsequent
> +     objfiles using the same BFD.  */
> +  gdb_assert (per_bfd->partial_symtabs == nullptr);
> +  per_bfd->partial_symtabs = objfile->partial_symtabs;
> +
>    return true;
>  }
> 
> @@ -5972,6 +5982,7 @@ dwarf2_initialize_objfile (struct objfile *objfile, dw_index_kind *index_kind)
>    if (per_bfd->debug_names_table != nullptr)
>      {
>        *index_kind = dw_index_kind::DEBUG_NAMES;
> +      per_objfile->objfile->partial_symtabs = per_bfd->partial_symtabs;
>        per_objfile->resize_symtabs ();
>        return true;
>      }
> @@ -5981,6 +5992,7 @@ dwarf2_initialize_objfile (struct objfile *objfile, dw_index_kind *index_kind)
>    if (per_bfd->index_table != nullptr)
>      {
>        *index_kind = dw_index_kind::GDB_INDEX;
> +      per_objfile->objfile->partial_symtabs = per_bfd->partial_symtabs;
>        per_objfile->resize_symtabs ();
>        return true;
>      }
> diff --git a/gdb/testsuite/gdb.base/share-psymtabs-bt-2.c b/gdb/testsuite/gdb.base/share-psymtabs-bt-2.c
> new file mode 100644
> index 000000000000..c713eb22693c
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/share-psymtabs-bt-2.c
> @@ -0,0 +1,24 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 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/>.  */
> +
> +void bar (int x);
> +
> +void
> +foo (int x)
> +{
> +  bar (x);
> +}
> diff --git a/gdb/testsuite/gdb.base/share-psymtabs-bt.c b/gdb/testsuite/gdb.base/share-psymtabs-bt.c
> new file mode 100644
> index 000000000000..97ad3457905b
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/share-psymtabs-bt.c
> @@ -0,0 +1,29 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 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/>.  */
> +
> +void
> +bar (int x)
> +{}
> +
> +void foo (int x);
> +
> +int
> +main (void)
> +{
> +  foo (12345);
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/share-psymtabs-bt.exp b/gdb/testsuite/gdb.base/share-psymtabs-bt.exp
> new file mode 100644
> index 000000000000..f4ca58e30b96
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/share-psymtabs-bt.exp
> @@ -0,0 +1,57 @@
> +# Copyright 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/>.
> +
> +# Test that a backtrace is shown correctly for an objfile that uses partial
> +# symtabs created by another objfile sharing the same BFD.
> +#
> +# It mimics how a bug with psymtab sharing was initially found:
> +#
> +#  1. Load the test file twice, such that the second objfile re-uses the
> +#     per_bfd object created for the first objfile.
> +#  2. Run to some point where in the backtrace there is a frame for a
> +#     function that's in a CU that's not yet read in.
> +#  3. Check that this frame's information is complete in the "backtrace"
> +#     output.
> +
> +standard_testfile share-psymtabs-bt.c share-psymtabs-bt-2.c
> +
> +set sources [list \
> +    "${srcdir}/${subdir}/${srcfile}" \
> +    "${srcdir}/${subdir}/${srcfile2}" \
> +]
> +
> +if { [gdb_compile $sources $binfile executable {debug}] != "" } {
> +    untested "failed to compile"
> +    return -1
> +}
> +
> +clean_restart $binfile
> +

That looks more elaborate than necessary, how about:
...
diff --git a/gdb/testsuite/gdb.base/share-psymtabs-bt.exp
b/gdb/testsuite/gdb.base/share-psymtabs-bt.exp
index f4ca58e30b..65ea728f66 100644
--- a/gdb/testsuite/gdb.base/share-psymtabs-bt.exp
+++ b/gdb/testsuite/gdb.base/share-psymtabs-bt.exp
@@ -25,20 +25,14 @@
 #  3. Check that this frame's information is complete in the "backtrace"
 #     output.

-standard_testfile share-psymtabs-bt.c share-psymtabs-bt-2.c
+standard_testfile .c share-psymtabs-bt-2.c

-set sources [list \
-    "${srcdir}/${subdir}/${srcfile}" \
-    "${srcdir}/${subdir}/${srcfile2}" \
-]
-
-if { [gdb_compile $sources $binfile executable {debug}] != "" } {
+if { [prepare_for_testing "failed to prepare" $testfile "$srcfile
$srcfile2" \
+         {debug}] } {
     untested "failed to compile"
     return -1
 }

-clean_restart $binfile
-
 # Load $binfile a second time.  The second created objfile will re-use the
 # partial symtabs created by the first one.
 if { [gdb_file_cmd $binfile] != 0 } {
...
?

Thanks,
- Tom

> +# Load $binfile a second time.  The second created objfile will re-use the
> +# partial symtabs created by the first one.
> +if { [gdb_file_cmd $binfile] != 0 } {
> +    fail "file command failed"
> +    return -1
> +}
> +
> +gdb_breakpoint "bar"
> +if { ![runto "bar"] } {
> +    fail "failed to run to bar"
> +    return -1
> +}
> +
> +# A buggy GDB would fail to find the full symbol associated to this frame's
> +# address, so would just show "foo ()" (from the minimal symbol).
> +gdb_test "bt" "foo \\(x=12345\\).*"
>