gdb: unify build-id to objfile lookup code

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_objfile_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_objfile_by_build_id
does this for us.

One slight weird thing is in core_target::build_file_mappings, here we
call find_objfile_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_objfile_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_objfile_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.

7 files changed, 136 insertions, 55 deletions

diff --git a/gdb/build-id.c b/gdb/build-id.c
index 32fbe17..6266be1 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.  */
 
@@ -291,9 +293,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, "");
@@ -335,3 +339,37 @@ find_separate_debug_file_by_buildid (struct objfile *objfile,
 
   return std::string ();
 }
+
+/* See build-id.h.  */
+
+gdb_bfd_ref_ptr
+find_objfile_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 c5f20f8..1d2e789 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,21 @@ 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 objfile (executable or shared library) that matches BUILD_ID.
+   This is done by first checking in the debug-file-directory for a
+   suitable .build-id/ sub-directory, and looking for a file with the
+   required build-id (usually a symbolic link or hard link to the actual
+   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_objfile_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 e23d7d7..2e9253d 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -48,7 +48,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"
@@ -469,13 +468,20 @@ 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_objfile_by_build_id (file_data.build_id,
+					   filename.c_str ());
+
+	  if (abfd != nullptr)
 	    {
+	      /* The find_objfile_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);
 	    }
 	}
@@ -833,28 +839,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_objfile_by_build_id (build_id, abfd->filename);
 
   if (execbfd != nullptr)
     {
diff --git a/gdb/solib.c b/gdb/solib.c
index 184e190..b123be3 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -46,7 +46,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"
 
@@ -526,21 +525,15 @@ solib_map_sections (solib &so)
 	    abfd = nullptr;
 
 	  if (abfd == nullptr)
+	    abfd = find_objfile_by_build_id (mapped_file_info->build_id (),
+					     so.so_name.c_str ());
+
+	  if (abfd == nullptr && mismatch)
 	    {
-	      scoped_fd fd = debuginfod_exec_query
-		(mapped_file_info->build_id ()->data,
-		 mapped_file_info->build_id ()->size,
-		 so.so_name.c_str (), &filename);
-
-	      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;
-		}
+	      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 6e3301e..b5dee22 100644
--- a/gdb/testsuite/gdb.debuginfod/corefile-mapped-file.exp
+++ b/gdb/testsuite/gdb.debuginfod/corefile-mapped-file.exp
@@ -296,6 +296,30 @@ 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 build "mkdir -p [file dirname $build_id_filename]"
+remote_exec build "ln -sf $library_backup_filename $build_id_filename"
+
+clean_restart $binfile
+
+gdb_test_no_output "set debug-file-directory $debugdir" \
+    "set debug-file-directory"
+
+load_core_file "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 5bf6817..9ef1204 100644
--- a/gdb/testsuite/gdb.debuginfod/solib-with-soname.exp
+++ b/gdb/testsuite/gdb.debuginfod/solib-with-soname.exp
@@ -126,10 +126,19 @@ if {$corefile eq ""} {
 # If EXPECT_DOWNLOAD is true then we require a line indicating that
 # the shared library is being downloaded from debuginfod, otherwise
 # the shared library should not be downloaded.
-proc load_exec_and_core_file { expect_warning expect_download testname } {
+#
+# If DEBUGDIR is not the empty string then 'debug-file-directory' is
+# set to the value of DEBUGDIR.
+proc load_exec_and_core_file { expect_warning expect_download 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
 	set saw_download false
 	set saw_generated false
@@ -223,6 +232,27 @@ gdb_assert { [lindex $status 0] == 0 } \
 load_exec_and_core_file true false \
     "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 build \
+	 "mkdir -p [file dirname $build_id_filename]"]
+gdb_assert { [lindex $status 0] == 0 } \
+    "create sub-directory within the debug directory"
+set status \
+    [remote_exec build \
+	 "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 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 f58174d..0174aae 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -8306,14 +8306,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