public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH gdb-10-branch candidate] gdb/dwarf: disable per-BFD resource sharing
@ 2021-03-15 20:03 Simon Marchi
  2021-03-30 15:43 ` [PATCH v2] gdb/dwarf: disable per-BFD resource sharing for -readnow objfiles Simon Marchi
  0 siblings, 1 reply; 5+ messages in thread
From: Simon Marchi @ 2021-03-15 20:03 UTC (permalink / raw)
  To: gdb-patches

As described in PR 27541, we hit an internal error when loading a binary
the standard way and then loading it with the -readnow option:

    $ ./gdb -nx -q --data-directory=data-directory ~/a.out -ex "set confirm off" -ex "file -readnow ~/a.out"
    Reading symbols from /home/simark/a.out...
    Reading symbols from ~/a.out...
    /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:8098: internal-error: void create_all_comp_units(dwarf2_per_objfile*): Assertion `per_objfile->per_bfd->all_comp_units.empty ()' failed.

This is a recurring problem that exposes a design issue in the DWARF
per-BFD sharing feature.  Things work well when loading a binary with
the same method (with/without index, with/without readnow) twice in a
row.  But they don't work so well when loading a binary with different
methods.  See this previous fix, for example:

    efb763a5ea35 ("gdb: check for partial symtab presence in dwarf2_initialize_objfile")

That one handled the case where the first load is normal (uses partial
symbols) and the second load uses an index.

The problem is that when loading an objfile with a method A, we create a
dwarf2_per_bfd and some dwarf2_per_cu_data and initialize them with the
data belonging to that method.  When loading another obfile sharing the
same BFD but with a different method B, it's not clear how to re-use the
dwarf2_per_bfd/dwarf2_per_cu_data previously created, because they
contain the data specific to method A.

I think the most sensible fix would be to not share a dwarf2_per_bfd
between two objfiles loaded with different methods.  That means that two
objfiles sharing the same BFD and loaded the same way would share a
dwarf2_per_bfd.  Two objfiles sharing the same BFD but loaded with
different methods would use two different dwarf2_per_bfd structures.

However, this isn't a trivial change.  So to fix the issue quickly
(including in the gdb 10 branche), this patch just disables all
dwarf2_per_bfd sharing, until we implement the idea described above or
another suitable solution in master.

Generalize the gdb.base/index-cache-load-twice.exp test to test all
the possible combinations of loading a file with partial symtabs, index
and readnow.  Move it to gdb.dwarf2, since it really exercises features
of the DWARF reader.

gdb/ChangeLog:

	* dwarf2/read.c (dwarf2_per_bfd_bfd_data_key,
	dwarf2_per_bfd_objfile_data_key): Remove.
	(dwarf2_has_info): Don't share dwarf2_per_bfd between objfiles.

gdb/testsuite/ChangeLog:

	* gdb.base/index-cache-load-twice.exp: Remove.
	* gdb.base/index-cache-load-twice.c: Remove.
	* gdb.dwarf2/per-bfd-sharing.exp: New.
	* gdb.dwarf2/per-bfd-sharing.c: New.

Change-Id: I9ffcf1e136f3e75242f70e4e58e4ba1fd3083389
---
 gdb/dwarf2/read.c                             | 38 +-------
 .../gdb.base/index-cache-load-twice.exp       | 42 ---------
 .../per-bfd-sharing.c}                        |  8 +-
 gdb/testsuite/gdb.dwarf2/per-bfd-sharing.exp  | 93 +++++++++++++++++++
 4 files changed, 103 insertions(+), 78 deletions(-)
 delete mode 100644 gdb/testsuite/gdb.base/index-cache-load-twice.exp
 rename gdb/testsuite/{gdb.base/index-cache-load-twice.c => gdb.dwarf2/per-bfd-sharing.c} (91%)
 create mode 100644 gdb/testsuite/gdb.dwarf2/per-bfd-sharing.exp

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index d6881300fa66..bc46cdba511b 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -120,17 +120,6 @@ static bool use_deprecated_index_sections = false;
 /* This is used to store the data that is always per objfile.  */
 static const objfile_key<dwarf2_per_objfile> dwarf2_objfile_data_key;
 
-/* These are used to store the dwarf2_per_bfd objects.
-
-   objfiles having the same BFD, which doesn't require relocations, are going to
-   share a dwarf2_per_bfd object, which is held in the _bfd_data_key version.
-
-   Other objfiles are not going to share a dwarf2_per_bfd with any other
-   objfiles, so they'll have their own version kept in the _objfile_data_key
-   version.  */
-static const struct bfd_key<dwarf2_per_bfd> dwarf2_per_bfd_bfd_data_key;
-static const struct objfile_key<dwarf2_per_bfd> dwarf2_per_bfd_objfile_data_key;
-
 /* The "aclass" indices for various kinds of computed DWARF symbols.  */
 
 static int dwarf2_locexpr_index;
@@ -1948,30 +1937,9 @@ dwarf2_has_info (struct objfile *objfile,
 
   if (per_objfile == NULL)
     {
-      dwarf2_per_bfd *per_bfd;
-
-      /* We can share a "dwarf2_per_bfd" with other objfiles if the BFD
-	 doesn't require relocations and if there aren't partial symbols
-	 from some other reader.  */
-      if (!objfile_has_partial_symbols (objfile)
-	  && !gdb_bfd_requires_relocations (objfile->obfd))
-	{
-	  /* See if one has been created for this BFD yet.  */
-	  per_bfd = dwarf2_per_bfd_bfd_data_key.get (objfile->obfd);
-
-	  if (per_bfd == nullptr)
-	    {
-	      /* No, create it now.  */
-	      per_bfd = new dwarf2_per_bfd (objfile->obfd, names, can_copy);
-	      dwarf2_per_bfd_bfd_data_key.set (objfile->obfd, per_bfd);
-	    }
-	}
-      else
-	{
-	  /* No sharing possible, create one specifically for this objfile.  */
-	  per_bfd = new dwarf2_per_bfd (objfile->obfd, names, can_copy);
-	  dwarf2_per_bfd_objfile_data_key.set (objfile, per_bfd);
-	}
+      /* No dwarf2_per_bfd sharing for now.  */
+      dwarf2_per_bfd *per_bfd
+	= new dwarf2_per_bfd (objfile->obfd, names, can_copy);
 
       per_objfile = dwarf2_objfile_data_key.emplace (objfile, objfile, per_bfd);
     }
diff --git a/gdb/testsuite/gdb.base/index-cache-load-twice.exp b/gdb/testsuite/gdb.base/index-cache-load-twice.exp
deleted file mode 100644
index f442527d81f8..000000000000
--- a/gdb/testsuite/gdb.base/index-cache-load-twice.exp
+++ /dev/null
@@ -1,42 +0,0 @@
-#   Copyright 2020-2021 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/>.
-
-# This test checks that loading a file twice with the index cache enabled does
-# not crash.
-
-standard_testfile
-
-lassign [remote_exec host mktemp -d] ret cache_dir
-
-# The output of mktemp contains an end of line, remove it.
-set cache_dir [string trimright $cache_dir \r\n]
-
-if { $ret != 0 } {
-    fail "couldn't create temporary cache dir"
-    return
-}
-
-save_vars { GDBFLAGS } {
-    set GDBFLAGS "$GDBFLAGS -iex \"set index-cache directory $cache_dir\""
-    set GDBFLAGS "$GDBFLAGS -iex \"set index-cache on\""
-
-    if { [prepare_for_testing "failed to prepare" $testfile $srcfile \
-	  {debug additional_flags=-Wl,--build-id}] } {
-	return
-    }
-
-    # This file command would generate an internal error.
-    gdb_file_cmd [standard_output_file $testfile]
-}
diff --git a/gdb/testsuite/gdb.base/index-cache-load-twice.c b/gdb/testsuite/gdb.dwarf2/per-bfd-sharing.c
similarity index 91%
rename from gdb/testsuite/gdb.base/index-cache-load-twice.c
rename to gdb/testsuite/gdb.dwarf2/per-bfd-sharing.c
index 76c0dea3f1de..455ea3d54074 100644
--- a/gdb/testsuite/gdb.base/index-cache-load-twice.c
+++ b/gdb/testsuite/gdb.dwarf2/per-bfd-sharing.c
@@ -15,8 +15,14 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
+static
+int foo (int a, int b)
+{
+  return a + b;
+}
+
 int
 main (void)
 {
-  return 0;
+  return foo (2, 3);
 }
diff --git a/gdb/testsuite/gdb.dwarf2/per-bfd-sharing.exp b/gdb/testsuite/gdb.dwarf2/per-bfd-sharing.exp
new file mode 100644
index 000000000000..22ab91f8f65e
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/per-bfd-sharing.exp
@@ -0,0 +1,93 @@
+#   Copyright 2020-2021 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/>.
+
+# This test checks that loading a file with different methods (partial symtabs,
+# index, readnow) does not crash.
+
+standard_testfile
+
+lassign [remote_exec host mktemp -d] ret cache_dir
+
+# The output of mktemp contains an end of line, remove it.
+set cache_dir [string trimright $cache_dir \r\n]
+
+if { $ret != 0 } {
+    fail "couldn't create temporary cache dir"
+    return
+}
+
+verbose -log "Index cache dir: $cache_dir"
+
+if { [build_executable "failed to prepare" $testfile $srcfile \
+	  {debug additional_flags=-Wl,--build-id}] == -1 } {
+    return
+}
+
+# Populate the index-cache.
+with_test_prefix "populate index cache" {
+    clean_restart
+
+    gdb_test_no_output "set index-cache directory $cache_dir" \
+	"set index-cache directory"
+    gdb_test_no_output "set index-cache on"
+    gdb_test "file $binfile" "Reading symbols from .*" "file"
+}
+
+proc load_binary { method } {
+    global binfile
+    global hex
+
+    if { $method == "standard" } {
+	gdb_test "file $binfile" "Reading symbols from .*" "file"
+    } elseif { $method == "index" } {
+	gdb_test_no_output "set index-cache on"
+	gdb_test "file $binfile" "Reading symbols from .*" "file index"
+	gdb_test_no_output "set index-cache off"
+    } elseif { $method == "readnow" } {
+	gdb_test "file -readnow $binfile" \
+	    "Reading symbols from .*Expanding full symbols from .*" \
+	    "file readnow"
+    } else {
+	error "unknown method"
+    }
+
+    # Print a static function: seeing it and its signature confirms GDB
+    # sees some symbols.
+    gdb_test "print foo" " = {int \\(int, int\\)} $hex <foo>"
+}
+
+set methods {standard index readnow}
+
+foreach_with_prefix first $methods {
+    foreach_with_prefix second $methods {
+	foreach_with_prefix third $methods {
+	    # Start with a clean GDB.
+	    clean_restart
+
+	    # Set the index cache dir, but don't enable the index-cache, it will
+	    # be enabled only when needed, when loading a file with the "index"
+	    # method.
+	    gdb_test_no_output "set index-cache directory $cache_dir" \
+		"set index-cache directory"
+
+	    # Avoid GDB asking whether we really want to load a new binary.
+	    gdb_test_no_output "set confirm off"
+
+	    with_test_prefix "load first" { load_binary $first }
+	    with_test_prefix "load second" { load_binary $second }
+	    with_test_prefix "load third" { load_binary $third }
+	}
+    }
+}
-- 
2.30.1


^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-03-30 19:55 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-15 20:03 [PATCH gdb-10-branch candidate] gdb/dwarf: disable per-BFD resource sharing Simon Marchi
2021-03-30 15:43 ` [PATCH v2] gdb/dwarf: disable per-BFD resource sharing for -readnow objfiles Simon Marchi
2021-03-30 16:13   ` Tom Tromey
2021-03-30 17:33     ` Simon Marchi
2021-03-30 19:55       ` Tom Tromey

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).