[PATCH gdb-10-branch candidate] gdb/dwarf: disable per-BFD resource sharing

[PATCH gdb-10-branch candidate] gdb/dwarf: disable per-BFD resource sharing

[Simon Marchi]

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

[PATCH v2] gdb/dwarf: disable per-BFD resource sharing for -readnow objfiles

[Simon Marchi]

New in v2:

  - Disable sharing only for -readnow objfiles, not all objfiles.

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 known issue quickly
(including in the gdb 10 branch), this patch just disables all
dwarf2_per_bfd sharing for objfiles using READNOW.

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_has_info): Don't share dwarf2_per_bfd
	with objfiles using READNOW.

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                             | 11 ++-
 .../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, 108 insertions(+), 46 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 1b98b758c350..24183014cf4b 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -1951,9 +1951,14 @@ dwarf2_has_info (struct objfile *objfile,
     {
       dwarf2_per_bfd *per_bfd;
 
-      /* We can share a "dwarf2_per_bfd" with other objfiles if the BFD
-	 doesn't require relocations.  */
-      if (!gdb_bfd_requires_relocations (objfile->obfd))
+      /* We can share a "dwarf2_per_bfd" with other objfiles if the
+	 BFD doesn't require relocations.
+
+	 We don't share with objfiles for which -readnow was requested,
+	 because it would complicate things when loading the same BFD with
+	 -readnow and then without -readnow.  */
+      if (!gdb_bfd_requires_relocations (objfile->obfd)
+	  && (objfile->flags & OBJF_READNOW) == 0)
 	{
 	  /* See if one has been created for this BFD yet.  */
 	  per_bfd = dwarf2_per_bfd_bfd_data_key.get (objfile->obfd);
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

Re: [PATCH v2] gdb/dwarf: disable per-BFD resource sharing for -readnow objfiles

[Tom Tromey]

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

It seems to me that -readnow is a funny situation, because it is
intended to do something unusual.

For all other kinds of possible sharing, I think that deferring to
whatever was already done and recorded in the per-BFD object should work fine.
The idea here is just to bypass the reading step when possible.
So IMO it's fine to ignore the index cache if the psymtabs have already
been scanned -- reading from the cache won't give any benefits relative
to reusing the per-BFD object.

This seems like it would mainly be an issue for the ordering of checks
in dwarf2_initialize_objfile.  When I look there, it seems correct
already though -- the existing per-BFD is checked before trying to do
any scan or reading from the index cache.

Simon> 	* dwarf2/read.c (dwarf2_has_info): Don't share dwarf2_per_bfd
Simon> 	with objfiles using READNOW.

Thank you.  This looks good to me.  Thanks especially for the test case.

I think the ChangeLog entry should probably mention the PR though.

Tom

Re: [PATCH v2] gdb/dwarf: disable per-BFD resource sharing for -readnow objfiles

[Simon Marchi]

On 2021-03-30 12:13 p.m., Tom Tromey wrote:> Simon> I think the most sensible fix would be to not share a dwarf2_per_bfd
> Simon> between two objfiles loaded with different methods.  That means that two
> Simon> objfiles sharing the same BFD and loaded the same way would share a
> Simon> dwarf2_per_bfd.  Two objfiles sharing the same BFD but loaded with
> Simon> different methods would use two different dwarf2_per_bfd structures.
> 
> It seems to me that -readnow is a funny situation, because it is
> intended to do something unusual.
> 
> For all other kinds of possible sharing, I think that deferring to
> whatever was already done and recorded in the per-BFD object should work fine.
> The idea here is just to bypass the reading step when possible.
> So IMO it's fine to ignore the index cache if the psymtabs have already
> been scanned -- reading from the cache won't give any benefits relative
> to reusing the per-BFD object.
> 
> This seems like it would mainly be an issue for the ordering of checks
> in dwarf2_initialize_objfile.  When I look there, it seems correct
> already though -- the existing per-BFD is checked before trying to do
> any scan or reading from the index cache.

Yes, that case works fine (because it was broken at first and a PR was
reported :)).  I would be curious what you think about the (still
theoritical) index -> psymtabs case mentioned here:

    https://sourceware.org/pipermail/gdb-patches/2021-March/177366.html

> 
> Simon> 	* dwarf2/read.c (dwarf2_has_info): Don't share dwarf2_per_bfd
> Simon> 	with objfiles using READNOW.
> 
> Thank you.  This looks good to me.  Thanks especially for the test case.

> I think the ChangeLog entry should probably mention the PR though.

Thanks, I will add that and push to both branches.

Simon

Re: [PATCH v2] gdb/dwarf: disable per-BFD resource sharing for -readnow objfiles

[Tom Tromey]

Simon> Yes, that case works fine (because it was broken at first and a PR was
Simon> reported :)).  I would be curious what you think about the (still
Simon> theoritical) index -> psymtabs case mentioned here:

Simon>     https://sourceware.org/pipermail/gdb-patches/2021-March/177366.html

Ah, yeah, if we added some kind of "avoid the index" option, then there
would be a similar problem with these scenarios.

Tom