[PATCH 4/4] gdb: unify build-id to objfile lookup code

[Andrew Burgess <aburgess@redhat.com>]

There are 3 places where we currently call debuginfod_exec_query to
lookup an objfile for a given build-id.

In one of these places we first call build_id_to_exec_bfd which also
looks up an objfile given a build-id, but this function looks on disk
for a symlink in the .build-id/ sub-directory (within the
debug-file-directory).

I can't think of any reason why we shouldn't call build_id_to_exec_bfd
before every call to debuginfod_exec_query.

So, in this commit I have added a new function in build-id.c,
find_exec_by_build_id, this function calls build_id_to_exec_bfd, and
if that fails, then calls debuginfod_exec_query.

Everywhere we call debuginfod_exec_query is updated to call the new
function, and in locate_exec_from_corefile_build_id, the existing call
to build_id_to_exec_bfd is removed as calling find_exec_by_build_id
does this for us.

One slight weird thing is in core_target::build_file_mappings, here we
call find_exec_by_build_id which returns a gdb_bfd_ref_ptr for the
opened file, however we immediately reopen the file as "binary".  The
reason for this is that all the bfds opened in ::build_file_mappings
need to be opened as "binary" (see the function comments for why).

I did consider passing a target type into find_exec_by_build_id, which
could then be forwarded to build_id_to_exec_bfd and used to open the
BFD as "binary", however, if you follow the call chain you'll end up
in build_id_to_debug_bfd_1, where we actually open the bfd.  Notice in
here that we call build_id_verify to double check the build-id of the
file we found, this requires that the bfd not be opened as "binary".

What this means is that we always have to first open the bfd using the
gnutarget target type (for the build-id check), and then we would have
to reopen it as "binary".  There seems little point pushing the reopen
logic into find_exec_by_build_id, so we just do this in the
::build_file_mappings function.

I've extended the tests to cover the two cases which actually changed
in this commit.
---
 gdb/build-id.c                                | 42 ++++++++++++++++++-
 gdb/build-id.h                                | 21 ++++++----
 gdb/corelow.c                                 | 40 ++++++------------
 gdb/solib.c                                   | 22 ++++------
 .../gdb.debuginfod/corefile-mapped-file.exp   | 31 ++++++++++++++
 .../gdb.debuginfod/solib-with-soname.exp      | 28 ++++++++++++-
 gdb/testsuite/lib/gdb.exp                     |  7 +++-
 7 files changed, 137 insertions(+), 54 deletions(-)

diff --git a/gdb/build-id.c b/gdb/build-id.c
index 41667d5e5cf..27642b58d56 100644
--- a/gdb/build-id.c
+++ b/gdb/build-id.c
@@ -26,6 +26,8 @@
 #include "filenames.h"
 #include "gdbcore.h"
 #include "cli/cli-style.h"
+#include "gdbsupport/scoped_fd.h"
+#include "debuginfod-support.h"
 
 /* See build-id.h.  */
 
@@ -198,9 +200,11 @@ build_id_to_debug_bfd (size_t build_id_len, const bfd_byte *build_id)
   return build_id_to_bfd_suffix (build_id_len, build_id, ".debug");
 }
 
-/* See build-id.h.  */
+/* Find and open a BFD for an executable file given a build-id.  If no BFD
+   can be found, return NULL.  The returned reference to the BFD must be
+   released by the caller.  */
 
-gdb_bfd_ref_ptr
+static gdb_bfd_ref_ptr
 build_id_to_exec_bfd (size_t build_id_len, const bfd_byte *build_id)
 {
   return build_id_to_bfd_suffix (build_id_len, build_id, "");
@@ -243,3 +247,37 @@ find_separate_debug_file_by_buildid (struct objfile *objfile,
 
   return std::string ();
 }
+
+/* See build-id.h.  */
+
+gdb_bfd_ref_ptr
+find_exec_by_build_id (const bfd_build_id *build_id,
+		       const char *expected_filename)
+{
+  /* Try to find the executable (or shared object) by looking for a
+     (sym)link on disk from the build-id to the object file.  */
+  gdb_bfd_ref_ptr abfd = build_id_to_exec_bfd (build_id->size,
+					       build_id->data);
+
+  if (abfd != nullptr)
+    return abfd;
+
+  /* Attempt to query debuginfod for the executable.  */
+  gdb::unique_xmalloc_ptr<char> path;
+  scoped_fd fd = debuginfod_exec_query (build_id->data, build_id->size,
+					expected_filename, &path);
+  if (fd.get () >= 0)
+    {
+      abfd = gdb_bfd_open (path.get (), gnutarget);
+
+      if (abfd == nullptr)
+	warning (_("\"%ps\" from debuginfod cannot be opened as bfd: %s"),
+		 styled_string (file_name_style.style (), path.get ()),
+		 gdb_bfd_errmsg (bfd_get_error (), nullptr).c_str ());
+      else if (!build_id_verify (abfd.get (), build_id->size,
+				 build_id->data))
+	abfd = nullptr;
+    }
+
+  return abfd;
+}
diff --git a/gdb/build-id.h b/gdb/build-id.h
index c5f20f8782e..3df122a0cbf 100644
--- a/gdb/build-id.h
+++ b/gdb/build-id.h
@@ -40,13 +40,6 @@ extern int build_id_verify (bfd *abfd,
 extern gdb_bfd_ref_ptr build_id_to_debug_bfd (size_t build_id_len,
 					      const bfd_byte *build_id);
 
-/* Find and open a BFD for an executable file given a build-id.  If no BFD
-   can be found, return NULL.  The returned reference to the BFD must be
-   released by the caller.  */
-
-extern gdb_bfd_ref_ptr build_id_to_exec_bfd (size_t build_id_len,
-					     const bfd_byte *build_id);
-
 /* Find the separate debug file for OBJFILE, by using the build-id
    associated with OBJFILE's BFD.  If successful, returns the file name for the
    separate debug file, otherwise, return an empty string.
@@ -60,6 +53,20 @@ extern gdb_bfd_ref_ptr build_id_to_exec_bfd (size_t build_id_len,
 extern std::string find_separate_debug_file_by_buildid
   (struct objfile *objfile, deferred_warnings *warnings);
 
+/* Find an executable (or shared library) that matches BUILD_ID.  This is
+   done by first checking in the debug-file-directory for a .build-id/
+   sub-directory, and looking for a symlink in there that points to the
+   required file.
+
+   If that doesn't find us a file then we call to debuginfod to see if it
+   can provide the required file.
+
+   EXPECTED_FILENAME is used in output messages from debuginfod, this
+   should be the file we were looking for but couldn't find.  */
+
+extern gdb_bfd_ref_ptr find_exec_by_build_id (const bfd_build_id *build_id,
+					      const char *expected_filename);
+
 /* Return an hex-string representation of BUILD_ID.  */
 
 static inline std::string
diff --git a/gdb/corelow.c b/gdb/corelow.c
index 85bc3c26bea..6975719c1f2 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -47,7 +47,6 @@
 #include "gdbsupport/pathstuff.h"
 #include "gdbsupport/scoped_fd.h"
 #include "gdbsupport/x86-xstate.h"
-#include "debuginfod-support.h"
 #include <unordered_map>
 #include <unordered_set>
 #include "cli/cli-cmds.h"
@@ -407,13 +406,19 @@ core_target::build_file_mappings ()
 	   || !bfd_check_format (abfd.get (), bfd_object))
 	  && file_data.build_id != nullptr)
 	{
-	  expanded_fname = nullptr;
-	  debuginfod_exec_query (file_data.build_id->data,
-				 file_data.build_id->size,
-				 filename.c_str (), &expanded_fname);
-	  if (expanded_fname != nullptr)
+	  abfd = find_exec_by_build_id (file_data.build_id,
+					filename.c_str ());
+
+	  if (abfd != nullptr)
 	    {
+	      /* The find_exec_by_build_id will have opened ABFD using the
+		 GNUTARGET global bfd type, however, we need the bfd opened
+		 as the binary type (see the function's header comment), so
+		 now we reopen ABFD with the desired binary type.  */
+	      expanded_fname
+		= make_unique_xstrdup (bfd_get_filename (abfd.get ()));
 	      struct bfd *b = bfd_openr (expanded_fname.get (), "binary");
+	      gdb_assert (b != nullptr);
 	      abfd = gdb_bfd_ref_ptr::new_reference (b);
 	    }
 	}
@@ -774,28 +779,7 @@ locate_exec_from_corefile_build_id (bfd *abfd, int from_tty)
     return;
 
   gdb_bfd_ref_ptr execbfd
-    = build_id_to_exec_bfd (build_id->size, build_id->data);
-
-  if (execbfd == nullptr)
-    {
-      /* Attempt to query debuginfod for the executable.  */
-      gdb::unique_xmalloc_ptr<char> execpath;
-      scoped_fd fd = debuginfod_exec_query (build_id->data, build_id->size,
-					    abfd->filename, &execpath);
-
-      if (fd.get () >= 0)
-	{
-	  execbfd = gdb_bfd_open (execpath.get (), gnutarget);
-
-	  if (execbfd == nullptr)
-	    warning (_("\"%s\" from debuginfod cannot be opened as bfd: %s"),
-		     execpath.get (),
-		     gdb_bfd_errmsg (bfd_get_error (), nullptr).c_str ());
-	  else if (!build_id_verify (execbfd.get (), build_id->size,
-				     build_id->data))
-	    execbfd.reset (nullptr);
-	}
-    }
+    = find_exec_by_build_id (build_id, abfd->filename);
 
   if (execbfd != nullptr)
     {
diff --git a/gdb/solib.c b/gdb/solib.c
index c39dfbcc78e..3292f361176 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -45,7 +45,6 @@
 #include "gdb_bfd.h"
 #include "gdbsupport/filestuff.h"
 #include "gdbsupport/scoped_fd.h"
-#include "debuginfod-support.h"
 #include "source.h"
 #include "cli/cli-style.h"
 
@@ -826,20 +825,15 @@ solib_map_sections (solib &so)
 	    abfd = nullptr;
 
 	  if (abfd == nullptr)
-	    {
-	      scoped_fd fd = debuginfod_exec_query
-		(expected_build_id->data, expected_build_id->size,
-		 so.so_name.c_str (), &filename);
+	    abfd = find_exec_by_build_id (expected_build_id,
+					  so.so_name.c_str ());
 
-	      if (fd.get () >= 0)
-		abfd = ops->bfd_open (filename.get ());
-	      else if (mismatch)
-		{
-		  warning (_ ("Build-id of %ps does not match core file."),
-			   styled_string (file_name_style.style (),
-					  filename.get ()));
-		  abfd = nullptr;
-		}
+	  if (abfd == nullptr && mismatch)
+	    {
+	      warning (_ ("Build-id of %ps does not match core file."),
+		       styled_string (file_name_style.style (),
+				      filename.get ()));
+	      abfd = nullptr;
 	    }
 	}
     }
diff --git a/gdb/testsuite/gdb.debuginfod/corefile-mapped-file.exp b/gdb/testsuite/gdb.debuginfod/corefile-mapped-file.exp
index c789be87ba3..553d68e4332 100644
--- a/gdb/testsuite/gdb.debuginfod/corefile-mapped-file.exp
+++ b/gdb/testsuite/gdb.debuginfod/corefile-mapped-file.exp
@@ -230,6 +230,37 @@ set ptr_value [read_ptr_value]
 gdb_assert { $ptr_value eq "unavailable" } \
     "check value of pointer is unavailable with library file missing"
 
+# Now symlink the .build-id/xx/xxx...xxx filename within the debug
+# directory to library we just moved aside.  Restart GDB and setup the
+# debug-file-directory before loading the core file.
+#
+# GDB should lookup the file to map via the build-id link in the
+# .build-id/ directory.
+set debugdir [standard_output_file "debugdir"]
+set build_id_filename \
+    $debugdir/[build_id_debug_filename_get $library_backup_filename ""]
+
+remote_exec host "mkdir -p [file dirname $build_id_filename]"
+remote_exec host "ln -sf $library_backup_filename $build_id_filename"
+
+clean_restart $binfile
+
+gdb_test_no_output "set debug-file-directory $debugdir" \
+    "set debug-file-directory"
+
+gdb_test "core-file $corefile" \
+    [multi_line \
+	 "\\\[\[^\r\n\]+\\\]" \
+	 "Core was generated by \[^\r\n\]+" \
+	 "Program terminated with signal SIGSEGV, Segmentation fault\\." \
+	 "#0  main \\(\\) at \[^\r\n\]+" \
+	 "$decimal\\s+\[^\r\n\]+/\\* Undefined behaviour here\\.  \\*/"] \
+    "load corefile, lookup in debug-file-directory"
+
+set ptr_value [read_ptr_value]
+gdb_assert { $ptr_value == $ptr_expected_value } \
+    "check value of pointer variable from core-file, lookup in debug-file-directory"
+
 # Build a new version of the shared library, keep the library the same size,
 # but change the contents so the build-id changes.  Then restart GDB and load
 # the core-file again.  GDB should spot that the build-id for the shared
diff --git a/gdb/testsuite/gdb.debuginfod/solib-with-soname.exp b/gdb/testsuite/gdb.debuginfod/solib-with-soname.exp
index dfc78923436..9d311be8ebc 100644
--- a/gdb/testsuite/gdb.debuginfod/solib-with-soname.exp
+++ b/gdb/testsuite/gdb.debuginfod/solib-with-soname.exp
@@ -106,10 +106,15 @@ if {$corefile eq ""} {
 # unable to load the shared library symbols, otherwise, EXPECT_WARNING
 # is false and we expect no warnings about loading shared library
 # symbols.
-proc load_exec_and_core_file { expect_warning testname } {
+proc load_exec_and_core_file { expect_warning testname {debugdir ""}} {
     with_test_prefix $testname {
 	clean_restart $::binfile
 
+	if { $debugdir ne "" } {
+	    gdb_test_no_output "set debug-file-directory $debugdir" \
+		"set debug directory"
+	}
+
 	set saw_warning false
 	gdb_test_multiple "core-file $::corefile" "load core file" {
 	    -re "^core-file \[^\r\n\]+\r\n" {
@@ -205,6 +210,27 @@ gdb_assert { [lindex $status 0] == 0 } \
 load_exec_and_core_file true \
     "load core file, libfoo_1.so removed"
 
+# Symlink the .build-id/xx/xxx...xxx filename within the debug
+# directory to LIBRARY_1_BACKUP_FILENAME, now when we restart GDB it
+# should find the missing library within the debug directory.
+set debugdir [standard_output_file "debugdir"]
+set build_id_filename \
+    $debugdir/[build_id_debug_filename_get $library_1_backup_filename ""]
+set status \
+    [remote_exec host \
+	 "mkdir -p [file dirname $build_id_filename]"]
+gdb_assert { [lindex $status 0] == 0 } \
+    "create sub-directory within the debug directory"
+set status \
+    [remote_exec host \
+	 "ln -sf $library_1_backup_filename $build_id_filename"]
+gdb_assert { [lindex $status 0] == 0 } \
+    "create symlink within the debug directory "
+
+load_exec_and_core_file false \
+    "load core file, find libfoo_1.so through debug-file-directory" \
+    $debugdir
+
 # Setup a debuginfod server which can serve the original shared
 # library file.
 if {![allow_debuginfod_tests]} {
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index c958ff18d2a..e369b0be96a 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -8019,14 +8019,17 @@ proc get_build_id { filename } {
 
 # Return the build-id hex string (usually 160 bits as 40 hex characters)
 # converted to the form: .build-id/ab/cdef1234...89.debug
+#
+# The '.debug' suffix can be changed by passing the SUFFIX argument.
+#
 # Return "" if no build-id found.
-proc build_id_debug_filename_get { filename } {
+proc build_id_debug_filename_get { filename {suffix ".debug"} } {
     set data [get_build_id $filename]
     if { $data == "" } {
 	return ""
     }
     regsub {^..} $data {\0/} data
-    return ".build-id/${data}.debug"
+    return ".build-id/${data}${suffix}"
 }
 
 # DEST should be a file compiled with debug information.  This proc
-- 
2.25.4