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

[Simon Marchi <simon.marchi@efficios.com>]

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

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
+
+# 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\\).*"
-- 
2.26.2