[PATCH 0/2] Add debuginfod core file support

[PATCH 0/2] Add debuginfod core file support

[Aaron Merey]

For v4 of this series see
https://sourceware.org/pipermail/gdb-patches/2021-November/183291.html

The main change in v5 is that core file soname-to-build-id mappings are
no longer part of progspaces and are instead stored in the registry of
core file bfds.

This change was based on a suggestion from Andrew Burgess.  I've included
part of this discussion below since it appears that gdb-patches fell off
the cc list.  Originally we talked about adding the maps to progspace
registries but I decided to use bfd registries since the maps are
arguably more closely associated with the core file itself rather than
the progspace.

Aaron Merey (2):
  gdb: Add soname to build-id mapping for core files
  PR gdb/27570: missing support for debuginfod in
    core_target::build_file_mappings

 gdb/corelow.c                                 |  38 ++++++
 gdb/debuginfod-support.c                      |  51 +++++++
 gdb/debuginfod-support.h                      |  17 +++
 gdb/gcore.in                                  |   2 +-
 gdb/linux-tdep.c                              |  38 +++++-
 gdb/solib.c                                   | 128 ++++++++++++++++++
 gdb/solib.h                                   |  20 +++
 .../gdb.debuginfod/fetch_src_and_symbols.exp  |  22 +++
 8 files changed, 314 insertions(+), 2 deletions(-)

On Tue, Feb 22, 2022 at 8:45 PM Aaron Merey <amerey@redhat.com> wrote:
> On Thu, Feb 17, 2022 at 5:48 PM Andrew Burgess <aburgess@redhat.com> wrote:
> > Aaron Merey <amerey@redhat.com> writes:
> > > +  /* Associate a core file SONAME with BUILD_ID so that it can be retrieved
> > > +     with get_cbfd_soname_build_id.  */
> > > +  void set_cbfd_soname_build_id (std::string soname,
> > > +                              const bfd_build_id *build_id);
> > > +
> > > +  /* If a core file SONAME had a build-id associated with it by a previous
> > > +     call to set_cbfd_soname_build_id then return the build-id as a
> > > +     NULL-terminated hex string.  */
> > > +  const char *get_cbfd_soname_build_id (const char *soname);
> > > +
> > > +  /* Clear all core file soname to build-id mappings.  */
> > > +  void clear_cbfd_soname_build_ids ();
> > > +
> >
> > I don't know how you'd feel about it, but I have another suggestion,
> > that would remove the need to change progspace.* at all.
> >
> > So, I was always a little uncomfortable with this new bit of API because
> > it was so tied to core files.  I did wonder if we could rename it to
> > sound more generic.... but I think there's a better option:
> >
> >   register_program_space_data_with_cleanup (...)
> >
> > The program_space structure has a REGISTRY_FIELDS entry, which means it
> > supports carrying around arbitrary data payloads.  You can see this in
> > use in python/py-progspace.c.
> >
> > What this would allow you to do is move this new API off of progspace
> > and place it entirely within corelow.c.
> >
> > I don't know how you feel about this idea?
>
> It would be nice to avoid adding an extra API to progspace and I think
> the registry is a suitable alternative.  If I understand things correctly,
> we'll still use an unordered_map to associate sonames to build-ids but
> register this map with the current progspace so that the map is cleared
> when the progspace's dtor runs.

-- 
2.35.1

[PATCH 1/2] gdb: Add soname to build-id mapping for core files

[Aaron Merey]

Since commit aa2d5a422 gdb has been able to read executable and shared
library build-ids within core files.

Expand this functionality so that each core file bfd maintains a map of
soname to build-id for each shared library referenced in the core file.

This feature may be used to verify that gdb has found the correct shared
libraries for core files and to facilitate downloading shared libaries via
debuginfod.
---
 gdb/corelow.c    | 11 ++++++
 gdb/linux-tdep.c | 38 ++++++++++++++++++-
 gdb/solib.c      | 96 ++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/solib.h      | 20 ++++++++++
 4 files changed, 164 insertions(+), 1 deletion(-)

diff --git a/gdb/corelow.c b/gdb/corelow.c
index 1579e6bc2b8..9617597c95e 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -282,6 +282,17 @@ core_target::build_file_mappings ()
 
 	/* Set target_section fields.  */
 	m_core_file_mappings.emplace_back (start, end, sec);
+
+	/* If this is a bfd of a shared library, record its soname
+	   and build id.  */
+	if (build_id != nullptr)
+	  {
+	    gdb::unique_xmalloc_ptr<char> soname =
+	      gdb_bfd_read_elf_soname (bfd->filename);
+	    if (soname.get () != nullptr)
+	      set_cbfd_soname_build_id (current_program_space->cbfd,
+					soname.get (), build_id);
+	  }
       });
 
   normalize_mem_ranges (&m_core_unavailable_mappings);
diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index d4868902ac3..bde28e25f2a 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -44,6 +44,7 @@
 #include "solib-svr4.h"
 
 #include <ctype.h>
+#include <unordered_map>
 
 /* This enum represents the values that the user can choose when
    informing the Linux kernel about which memory mappings will be
@@ -1185,6 +1186,23 @@ linux_read_core_file_mappings
   if (f != descend)
     warning (_("malformed note - filename area is too big"));
 
+  const bfd_build_id *orig_build_id = cbfd->build_id;
+  std::unordered_map<ULONGEST, const bfd_build_id *> vma_map;
+  std::unordered_map<char *, const bfd_build_id *> filename_map;
+
+  /* Search for solib build-ids in the core file.  Each time one is found,
+     map the start vma of the corresponding elf header to the build-id.  */
+  for (bfd_section *sec = cbfd->sections; sec != nullptr; sec = sec->next)
+    {
+      cbfd->build_id = nullptr;
+
+      if (sec->flags & SEC_LOAD
+	  && get_elf_backend_data (cbfd)->elf_backend_core_find_build_id
+	       (cbfd, (bfd_vma) sec->filepos))
+	vma_map[sec->vma] = cbfd->build_id;
+    }
+
+  cbfd->build_id = orig_build_id;
   pre_loop_cb (count);
 
   for (int i = 0; i < count; i++)
@@ -1198,8 +1216,26 @@ linux_read_core_file_mappings
       descdata += addr_size;
       char * filename = filenames;
       filenames += strlen ((char *) filenames) + 1;
+      const bfd_build_id *build_id = nullptr;
+      auto vma_map_it = vma_map.find (start);
+
+      /* Map filename to the build-id associated with this start vma,
+	 if such a build-id was found.  Otherwise use the build-id
+	 already associated with this filename if it exists.  */
+      if (vma_map_it != vma_map.end ())
+	{
+	  build_id = vma_map_it->second;
+	  filename_map[filename] = build_id;
+	}
+      else
+	{
+	  auto filename_map_it = filename_map.find (filename);
+
+	  if (filename_map_it != filename_map.end ())
+	    build_id = filename_map_it->second;
+	}
 
-      loop_cb (i, start, end, file_ofs, filename, nullptr);
+      loop_cb (i, start, end, file_ofs, filename, build_id);
     }
 }
 
diff --git a/gdb/solib.c b/gdb/solib.c
index b9b1d037187..b8e64647afb 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -23,6 +23,7 @@
 #include <fcntl.h>
 #include "symtab.h"
 #include "bfd.h"
+#include "build-id.h"
 #include "symfile.h"
 #include "objfiles.h"
 #include "gdbcore.h"
@@ -519,6 +520,68 @@ solib_bfd_open (const char *pathname)
   return abfd;
 }
 
+/* Mapping of a core file's shared library sonames to their respective
+   build-ids.  Added to the registries of core file bfds.  */
+
+typedef std::unordered_map<std::string, std::string> soname_build_id_map;
+
+/* Key used to associate a soname_build_id_map to a core file bfd.  */
+
+static const struct bfd_data *cbfd_soname_build_id_data_key;
+
+/* See solib.h.  */
+
+void
+set_cbfd_soname_build_id (gdb_bfd_ref_ptr abfd,
+			  const char *soname,
+			  const bfd_build_id *build_id)
+{
+  gdb_assert (abfd.get () != nullptr);
+  gdb_assert (soname != nullptr);
+  gdb_assert (build_id != nullptr);
+
+  soname_build_id_map *mapptr = (soname_build_id_map *)
+    bfd_data (abfd.get (), cbfd_soname_build_id_data_key);
+
+  if (mapptr == nullptr)
+    {
+      mapptr = new soname_build_id_map ();
+      set_bfd_data (abfd.get (), cbfd_soname_build_id_data_key, mapptr);
+    }
+
+  (*mapptr)[soname] = build_id_to_string (build_id);
+}
+
+/* See solib.h.  */
+
+gdb::unique_xmalloc_ptr<char>
+get_cbfd_soname_build_id (gdb_bfd_ref_ptr abfd, const char *soname)
+{
+  if (abfd.get () == nullptr || soname == nullptr)
+    return {};
+
+  soname_build_id_map *mapptr = (soname_build_id_map *)
+    bfd_data (abfd.get (), cbfd_soname_build_id_data_key);
+
+  if (mapptr == nullptr)
+    return {};
+
+  auto it = mapptr->find (basename (soname));
+  if (it == mapptr->end ())
+    return {};
+
+  return make_unique_xstrdup (it->second.c_str ());
+}
+
+/* Free memory allocated for a soname_build_id_map.  */
+
+static void
+delete_soname_build_id_map (struct bfd *abfd, void *mapptr)
+{
+  if (mapptr != nullptr)
+    delete (soname_build_id_map *) mapptr;
+}
+
 /* Given a pointer to one of the shared objects in our list of mapped
    objects, use the recorded name to open a bfd descriptor for the
    object, build a section table, relocate all the section addresses
@@ -1586,6 +1649,37 @@ gdb_bfd_scan_elf_dyntag (const int desired_dyntag, bfd *abfd, CORE_ADDR *ptr,
   return 0;
 }
 
+/* See solib.h.  */
+
+gdb::unique_xmalloc_ptr<char>
+gdb_bfd_read_elf_soname (const char *filename)
+{
+  gdb_bfd_ref_ptr abfd = gdb_bfd_open (filename, gnutarget);
+
+  if (abfd == nullptr)
+    return {};
+
+  /* Check that ABFD is an ET_DYN ELF file.  */
+  if (!bfd_check_format (abfd.get (), bfd_object)
+      || !(bfd_get_file_flags (abfd.get ()) & DYNAMIC))
+    return {};
+
+  CORE_ADDR idx;
+  if (!gdb_bfd_scan_elf_dyntag (DT_SONAME, abfd.get (), &idx, nullptr))
+    return {};
+
+  struct bfd_section *dynstr = bfd_get_section_by_name (abfd.get (), ".dynstr");
+  if (dynstr == nullptr || bfd_section_size (dynstr) <= idx)
+    return {};
+
+  /* Read the soname from the string table.  */
+  gdb::byte_vector dynstr_buf;
+  if (!gdb_bfd_get_full_section_contents (abfd.get (), dynstr, &dynstr_buf))
+    return {};
+
+  return make_unique_xstrdup ((char *) dynstr_buf.data () + idx);
+}
+
 /* Lookup the value for a specific symbol from symbol table.  Look up symbol
    from ABFD.  MATCH_SYM is a callback function to determine whether to pick
    up a symbol.  DATA is the input of this callback function.  Return NULL
@@ -1665,6 +1759,8 @@ void
 _initialize_solib ()
 {
   solib_data = gdbarch_data_register_pre_init (solib_init);
+  cbfd_soname_build_id_data_key = (const struct bfd_data *)
+    register_bfd_data_with_cleanup (nullptr, delete_soname_build_id_map);
 
   gdb::observers::free_objfile.attach (remove_user_added_objfile,
 				       "solib");
diff --git a/gdb/solib.h b/gdb/solib.h
index 2258b0ba6a0..cd6c8a81104 100644
--- a/gdb/solib.h
+++ b/gdb/solib.h
@@ -118,6 +118,12 @@ extern CORE_ADDR gdb_bfd_lookup_symbol_from_symtab (bfd *abfd,
 extern int gdb_bfd_scan_elf_dyntag (const int desired_dyntag, bfd *abfd,
 				    CORE_ADDR *ptr, CORE_ADDR *ptr_addr);
 
+/* If FILENAME refers to an ELF shared object then attempt to return the
+   string referred to by its DT_SONAME tag.   */
+
+extern gdb::unique_xmalloc_ptr<char> gdb_bfd_read_elf_soname
+  (const char *filename);
+
 /* Enable or disable optional solib event breakpoints as appropriate.  */
 
 extern void update_solib_breakpoints (void);
@@ -126,4 +132,18 @@ extern void update_solib_breakpoints (void);
 
 extern void handle_solib_event (void);
 
+/* Associate SONAME with BUILD_ID in ABFD's registry so that it can be
+   retrieved with get_cbfd_soname_build_id.  */
+
+extern void set_cbfd_soname_build_id (gdb_bfd_ref_ptr abfd,
+				      const char *soname,
+				      const bfd_build_id *build_id);
+
+/* If SONAME had a build-id associated with it in ABFD's registry by a
+   previous call to set_cbfd_soname_build_id then return the build-id
+   as a NULL-terminated hex string.  */
+
+extern gdb::unique_xmalloc_ptr<char> get_cbfd_soname_build_id
+  (gdb_bfd_ref_ptr abfd, const char *soname);
+
 #endif /* SOLIB_H */
-- 
2.35.1

[PATCH 2/2] PR gdb/27570: missing support for debuginfod in core_target::build_file_mappings

[Aaron Merey]

Add debuginfod support to core_target::build_file_mappings and
locate_exec_from_corefile_build_id to enable the downloading of
missing executables and shared libraries referenced in core files.

Also add debuginfod support to solib_map_sections so that previously
downloaded shared libraries can be retrieved from the local debuginfod
cache.

When core file shared libraries are found locally, verify that their
build-ids match the corresponding build-ids found in the core file.
If there is a mismatch, attempt to query debuginfod for the correct
build and print a warning if unsuccessful:

  warning: Build-id of /lib64/libc.so.6 does not match core file.

Also disable debuginfod when gcore invokes gdb.  Debuginfo is not
needed for core file generation so debuginfod queries will slow down
gcore unnecessarily.
---
 gdb/corelow.c                                 | 27 ++++++++++
 gdb/debuginfod-support.c                      | 51 +++++++++++++++++++
 gdb/debuginfod-support.h                      | 17 +++++++
 gdb/gcore.in                                  |  2 +-
 gdb/solib.c                                   | 32 ++++++++++++
 .../gdb.debuginfod/fetch_src_and_symbols.exp  | 22 ++++++++
 6 files changed, 150 insertions(+), 1 deletion(-)

diff --git a/gdb/corelow.c b/gdb/corelow.c
index 9617597c95e..68ac1010b66 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -46,6 +46,8 @@
 #include "gdbsupport/filestuff.h"
 #include "build-id.h"
 #include "gdbsupport/pathstuff.h"
+#include "gdbsupport/scoped_fd.h"
+#include "debuginfod-support.h"
 #include <unordered_map>
 #include <unordered_set>
 #include "gdbcmd.h"
@@ -229,6 +231,11 @@ core_target::build_file_mappings ()
 	       canonical) pathname will be provided.  */
 	    gdb::unique_xmalloc_ptr<char> expanded_fname
 	      = exec_file_find (filename, NULL);
+
+	    if (expanded_fname == nullptr && build_id != nullptr)
+	      debuginfod_exec_query (build_id->data, build_id->size,
+				     filename, &expanded_fname);
+
 	    if (expanded_fname == nullptr)
 	      {
 		m_core_unavailable_mappings.emplace_back (start, end - start);
@@ -410,6 +417,26 @@ locate_exec_from_corefile_build_id (bfd *abfd, int from_tty)
   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 (_("File \"%s\" from debuginfod cannot be opened as bfd"),
+		     execpath.get ());
+	  else if (!build_id_verify (execbfd.get (), build_id->size,
+				     build_id->data))
+	    execbfd.reset (nullptr);
+	}
+    }
+
   if (execbfd != nullptr)
     {
       exec_file_attach (bfd_get_filename (execbfd.get ()), from_tty);
diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 56d8e7781c5..6dbe40a14e3 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -69,6 +69,15 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
   return scoped_fd (-ENOSYS);
 }
 
+scoped_fd
+debuginfod_exec_query (const unsigned char *build_id,
+		       int build_id_len,
+		       const char *filename,
+		       gdb::unique_xmalloc_ptr<char> *destname)
+{
+  return scoped_fd (-ENOSYS);
+}
+
 #define NO_IMPL _("Support for debuginfod is not compiled into GDB.")
 
 #else
@@ -271,6 +280,48 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
 
   return fd;
 }
+
+/* See debuginfod-support.h  */
+
+scoped_fd
+debuginfod_exec_query (const unsigned char *build_id,
+		       int build_id_len,
+		       const char *filename,
+		       gdb::unique_xmalloc_ptr<char> *destname)
+{
+  if (!debuginfod_is_enabled ())
+    return scoped_fd (-ENOSYS);
+
+  debuginfod_client *c = get_debuginfod_client ();
+
+  if (c == nullptr)
+    return scoped_fd (-ENOMEM);
+
+  char *dname = nullptr;
+  user_data data ("executable for", filename);
+
+  debuginfod_set_user_data (c, &data);
+  gdb::optional<target_terminal::scoped_restore_terminal_state> term_state;
+  if (target_supports_terminal_ours ())
+    {
+      term_state.emplace ();
+      target_terminal::ours ();
+    }
+
+  scoped_fd fd (debuginfod_find_executable (c, build_id, build_id_len, &dname));
+  debuginfod_set_user_data (c, nullptr);
+
+  if (debuginfod_verbose > 0 && fd.get () < 0 && fd.get () != -ENOENT)
+    printf_filtered (_("Download failed: %s. " \
+		       "Continuing without executable for %ps.\n"),
+		     safe_strerror (-fd.get ()),
+		     styled_string (file_name_style.style (),  filename));
+
+  if (fd.get () >= 0)
+    destname->reset (dname);
+
+  return fd;
+}
 #endif
 
 /* Set callback for "set debuginfod enabled".  */
diff --git a/gdb/debuginfod-support.h b/gdb/debuginfod-support.h
index 29e361bb76f..5b1c1cb91f4 100644
--- a/gdb/debuginfod-support.h
+++ b/gdb/debuginfod-support.h
@@ -61,4 +61,21 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
 			    const char *filename,
 			    gdb::unique_xmalloc_ptr<char> *destname);
 
+/* Query debuginfod servers for an executable file with BUILD_ID.
+   BUILD_ID can be given as a binary blob or a null-terminated string.
+   If given as a binary blob, BUILD_ID_LEN should be the number of bytes.
+   If given as a null-terminated string, BUILD_ID_LEN should be 0.
+
+   FILENAME should be the name or path associated with the executable.
+   It is used for printing messages to the user.
+
+   If the file is successfully retrieved, its path on the local machine
+   is stored in DESTNAME.  If GDB is not built with debuginfod, this
+   function returns -ENOSYS.  */
+
+extern scoped_fd debuginfod_exec_query (const unsigned char *build_id,
+					int build_id_len,
+					const char *filename,
+					gdb::unique_xmalloc_ptr<char>
+					  *destname);
 #endif /* DEBUGINFOD_SUPPORT_H */
diff --git a/gdb/gcore.in b/gdb/gcore.in
index 7038c0c394c..ed7abf2d29b 100644
--- a/gdb/gcore.in
+++ b/gdb/gcore.in
@@ -98,7 +98,7 @@ do
 	# `</dev/null' to avoid touching interactive terminal if it is
 	# available but not accessible as GDB would get stopped on SIGTTIN.
 	"$binary_path/@GDB_TRANSFORM_NAME@" </dev/null \
-	    --nx --batch --readnever \
+	    --nx --batch --readnever -iex 'set debuginfod enabled off' \
 	    -ex "set pagination off" -ex "set height 0" -ex "set width 0" \
 	    "${dump_all_cmds[@]}" \
 	    -ex "attach $pid" -ex "gcore $prefix.$pid" -ex detach -ex quit
diff --git a/gdb/solib.c b/gdb/solib.c
index b8e64647afb..06fe9584f2a 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -49,6 +49,8 @@
 #include "filesystem.h"
 #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"
 
@@ -601,6 +603,36 @@ solib_map_sections (struct so_list *so)
 
   gdb::unique_xmalloc_ptr<char> filename (tilde_expand (so->so_name));
   gdb_bfd_ref_ptr abfd (ops->bfd_open (filename.get ()));
+  gdb::unique_xmalloc_ptr<char> build_id_hexstr =
+    get_cbfd_soname_build_id (current_program_space->cbfd, so->so_name);
+
+  /* If we already know the build-id of this solib from a core file, verify
+     it matches ABFD's build-id.  If there is a mismatch or the solib wasn't
+     found, attempt to query debuginfod for the correct solib.  */
+  if (build_id_hexstr.get () != nullptr)
+    {
+      bool mismatch = false;
+
+      if (abfd != nullptr && abfd->build_id != nullptr)
+	{
+	  std::string build_id = build_id_to_string (abfd->build_id);
+
+	  if (build_id != build_id_hexstr.get ())
+	    mismatch = true;
+	}
+      if (abfd == nullptr || mismatch)
+	{
+	  scoped_fd fd = debuginfod_exec_query ((const unsigned char*)
+						build_id_hexstr.get (),
+						0, so->so_name, &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 ()));
+	}
+    }
 
   if (abfd == NULL)
     return 0;
diff --git a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
index 5912c38c266..f12ed7d486c 100644
--- a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
+++ b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
@@ -63,6 +63,11 @@ if { [gdb_compile "$sourcetmp" "$binfile" executable {debug}] != "" } {
     return -1
 }
 
+if { [gdb_compile "$sourcetmp" "${binfile}2" executable {debug}] != "" } {
+    fail "compile"
+    return -1
+}
+
 # Write some assembly that just has a .gnu_debugaltlink section.
 # Copied from testsuite/gdb.dwarf2/dwzbuildid.exp.
 proc write_just_debugaltlink {filename dwzname buildid} {
@@ -114,6 +119,8 @@ proc write_dwarf_file {filename buildid {value 99}} {
     }
 }
 
+set corefile [standard_output_file "corefile"]
+
 proc no_url { } {
     global binfile outputdir debugdir
 
@@ -167,6 +174,16 @@ proc no_url { } {
     gdb_test "file ${binfile}_alt.o" \
 	".*could not find '.gnu_debugaltlink'.*" \
 	"file [file tail ${binfile}_alt.o]"
+
+    # Generate a core file and test that gdb cannot find the executable
+    clean_restart ${binfile}2
+    gdb_test "start" "Temporary breakpoint.*"
+    gdb_test "generate-core-file $::corefile" "Saved corefile $::corefile" \
+	"file [file tail $::corefile] gen"
+    file rename -force ${binfile}2 $debugdir
+
+    clean_restart
+    gdb_test "core $::corefile" ".*in ?? ().*" "file [file tail $::corefile]"
 }
 
 proc local_url { } {
@@ -234,6 +251,11 @@ proc local_url { } {
     gdb_test "br main" "Breakpoint 1 at.*file.*"
     gdb_test "l" ".*This program is distributed in the hope.*"
 
+    # gdb should now find the executable file
+    clean_restart
+    gdb_test "core $::corefile" ".*return 0.*" "file [file tail $::corefile]" \
+	"Enable debuginfod?.*" "y"
+
     # gdb should now find the debugaltlink file
     clean_restart
     gdb_test "file ${binfile}_alt.o" \
-- 
2.35.1

Re: [PATCH 1/2] gdb: Add soname to build-id mapping for core files

[Tom Tromey]

>>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:

Aaron> Since commit aa2d5a422 gdb has been able to read executable and shared
Aaron> library build-ids within core files.

Aaron> Expand this functionality so that each core file bfd maintains a map of
Aaron> soname to build-id for each shared library referenced in the core file.

Aaron> This feature may be used to verify that gdb has found the correct shared
Aaron> libraries for core files and to facilitate downloading shared libaries via
Aaron> debuginfod.

Thanks for working on this.

Aaron> +	    gdb::unique_xmalloc_ptr<char> soname =
Aaron> +	      gdb_bfd_read_elf_soname (bfd->filename);

The '=' should go after the line break.

Aaron> +/* Mapping of a core file's shared library sonames to their respective
Aaron> +   build-ids.  Added to the registries of core file bfds.  */
Aaron> +
Aaron> +typedef std::unordered_map<std::string, std::string> soname_build_id_map;
Aaron> +
Aaron> +/* Key used to associate a soname_build_id_map to a core file bfd.  */
Aaron> +
Aaron> +static const struct bfd_data *cbfd_soname_build_id_data_key;

I think it's better to use the template form, like

static const struct bfd_key<soname_build_id_map> cbfd_soname_build_id_data_key;

More comments on this below.

Aaron> +  soname_build_id_map *mapptr = (soname_build_id_map *)
Aaron> +    bfd_data (abfd.get (), cbfd_soname_build_id_data_key);

... this cast won't be needed:

soname_build_id_map *mapptr = cbfd_soname_build_id_data_key.get (abfd.get ());

Aaron> +  if (mapptr == nullptr)
Aaron> +    {
Aaron> +      mapptr = new soname_build_id_map ();
Aaron> +      set_bfd_data (abfd.get (), cbfd_soname_build_id_data_key, mapptr);

mapptr = cbfd_soname_build_id_data_key.emplace (abfd.get ());

Aaron> +/* See solib.h.  */
Aaron> +
Aaron> +gdb::unique_xmalloc_ptr<char>
Aaron> +get_cbfd_soname_build_id (gdb_bfd_ref_ptr abfd, const char *soname)
Aaron> +{

Why does this return a unique_xmalloc_ptr?

Aaron> +/* Free memory allocated for a soname_build_id_map.  */
Aaron> +
Aaron> +static void
Aaron> +delete_soname_build_id_map (struct bfd *abfd, void *mapptr)
Aaron> +{
Aaron> +  if (mapptr != nullptr)
Aaron> +    delete (soname_build_id_map *) mapptr;
Aaron> +}

With the template form, this can go away entirely.

Aaron>  _initialize_solib ()
Aaron>  {
Aaron>    solib_data = gdbarch_data_register_pre_init (solib_init);
Aaron> +  cbfd_soname_build_id_data_key = (const struct bfd_data *)
Aaron> +    register_bfd_data_with_cleanup (nullptr, delete_soname_build_id_map);

This won't be needed with the template form.

Tom

Re: [PATCH 2/2] PR gdb/27570: missing support for debuginfod in core_target::build_file_mappings

[Tom Tromey]

>>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:

Aaron> When core file shared libraries are found locally, verify that their
Aaron> build-ids match the corresponding build-ids found in the core file.
Aaron> If there is a mismatch, attempt to query debuginfod for the correct
Aaron> build and print a warning if unsuccessful:

Aaron>   warning: Build-id of /lib64/libc.so.6 does not match core file.

Nice.

Aaron> +	  if (execbfd == nullptr)
Aaron> +	    warning (_("File \"%s\" from debuginfod cannot be opened as bfd"),
Aaron> +		     execpath.get ());

I suspect this should use gdb_bfd_errmsg.

Aaron> +  char *dname = nullptr;

Aaron> +  scoped_fd fd (debuginfod_find_executable (c, build_id, build_id_len, &dname));
Aaron> +  debuginfod_set_user_data (c, nullptr);

Aaron> +  if (fd.get () >= 0)
Aaron> +    destname->reset (dname);

Is it possible for fd.get()<0 and dname!=nullptr?  If so there's a
memory leak.

Aaron> +  gdb::unique_xmalloc_ptr<char> build_id_hexstr =
Aaron> +    get_cbfd_soname_build_id (current_program_space->cbfd, so->so_name);

'=' on the next line.

Tom

[PATCH 1/2] gdb: Add soname to build-id mapping for core files

[Aaron Merey]

Hi Tom,

On Fri, Mar 4, 2022 at 9:54 AM Tom Tromey <tom@tromey.com> wrote:
> Aaron> +            gdb::unique_xmalloc_ptr<char> soname =
> Aaron> +              gdb_bfd_read_elf_soname (bfd->filename);
>
> The '=' should go after the line break.

Fixed.

> Aaron> +/* Mapping of a core file's shared library sonames to their respective
> Aaron> +   build-ids.  Added to the registries of core file bfds.  */
> Aaron> +
> Aaron> +typedef std::unordered_map<std::string, std::string> soname_build_id_map;
> Aaron> +
> Aaron> +/* Key used to associate a soname_build_id_map to a core file bfd.  */
> Aaron> +
> Aaron> +static const struct bfd_data *cbfd_soname_build_id_data_key;
>
> I think it's better to use the template form, like
>
> static const struct bfd_key<soname_build_id_map> cbfd_soname_build_id_data_key;

Thanks for pointing this out, the template form is much better. 

> Aaron> +gdb::unique_xmalloc_ptr<char>
> Aaron> +get_cbfd_soname_build_id (gdb_bfd_ref_ptr abfd, const char *soname)
> Aaron> +{
>
> Why does this return a unique_xmalloc_ptr?

I wanted to make sure the pointer stays valid even after the bfd has
been freed.  Currently this isn't a concern but I wanted to prevent
any issues in case there were some future changes that stored the pointer
for later use.

Aaron

---
 gdb/corelow.c    | 11 +++++++
 gdb/linux-tdep.c | 38 ++++++++++++++++++++++-
 gdb/solib.c      | 81 ++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/solib.h      | 20 ++++++++++++
 4 files changed, 149 insertions(+), 1 deletion(-)

diff --git a/gdb/corelow.c b/gdb/corelow.c
index 1579e6bc2b8..83d3c89656f 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -282,6 +282,17 @@ core_target::build_file_mappings ()
 
 	/* Set target_section fields.  */
 	m_core_file_mappings.emplace_back (start, end, sec);
+
+	/* If this is a bfd of a shared library, record its soname
+	   and build id.  */
+	if (build_id != nullptr)
+	  {
+	    gdb::unique_xmalloc_ptr<char> soname
+	      = gdb_bfd_read_elf_soname (bfd->filename);
+	    if (soname.get () != nullptr)
+	      set_cbfd_soname_build_id (current_program_space->cbfd,
+					soname.get (), build_id);
+	  }
       });
 
   normalize_mem_ranges (&m_core_unavailable_mappings);
diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index d4868902ac3..bde28e25f2a 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -44,6 +44,7 @@
 #include "solib-svr4.h"
 
 #include <ctype.h>
+#include <unordered_map>
 
 /* This enum represents the values that the user can choose when
    informing the Linux kernel about which memory mappings will be
@@ -1185,6 +1186,23 @@ linux_read_core_file_mappings
   if (f != descend)
     warning (_("malformed note - filename area is too big"));
 
+  const bfd_build_id *orig_build_id = cbfd->build_id;
+  std::unordered_map<ULONGEST, const bfd_build_id *> vma_map;
+  std::unordered_map<char *, const bfd_build_id *> filename_map;
+
+  /* Search for solib build-ids in the core file.  Each time one is found,
+     map the start vma of the corresponding elf header to the build-id.  */
+  for (bfd_section *sec = cbfd->sections; sec != nullptr; sec = sec->next)
+    {
+      cbfd->build_id = nullptr;
+
+      if (sec->flags & SEC_LOAD
+	  && get_elf_backend_data (cbfd)->elf_backend_core_find_build_id
+	       (cbfd, (bfd_vma) sec->filepos))
+	vma_map[sec->vma] = cbfd->build_id;
+    }
+
+  cbfd->build_id = orig_build_id;
   pre_loop_cb (count);
 
   for (int i = 0; i < count; i++)
@@ -1198,8 +1216,26 @@ linux_read_core_file_mappings
       descdata += addr_size;
       char * filename = filenames;
       filenames += strlen ((char *) filenames) + 1;
+      const bfd_build_id *build_id = nullptr;
+      auto vma_map_it = vma_map.find (start);
+
+      /* Map filename to the build-id associated with this start vma,
+	 if such a build-id was found.  Otherwise use the build-id
+	 already associated with this filename if it exists.  */
+      if (vma_map_it != vma_map.end ())
+	{
+	  build_id = vma_map_it->second;
+	  filename_map[filename] = build_id;
+	}
+      else
+	{
+	  auto filename_map_it = filename_map.find (filename);
+
+	  if (filename_map_it != filename_map.end ())
+	    build_id = filename_map_it->second;
+	}
 
-      loop_cb (i, start, end, file_ofs, filename, nullptr);
+      loop_cb (i, start, end, file_ofs, filename, build_id);
     }
 }
 
diff --git a/gdb/solib.c b/gdb/solib.c
index b9b1d037187..11182b43dd0 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -23,6 +23,7 @@
 #include <fcntl.h>
 #include "symtab.h"
 #include "bfd.h"
+#include "build-id.h"
 #include "symfile.h"
 #include "objfiles.h"
 #include "gdbcore.h"
@@ -519,6 +520,55 @@ solib_bfd_open (const char *pathname)
   return abfd;
 }
 
+/* Mapping of a core file's shared library sonames to their respective
+   build-ids.  Added to the registries of core file bfds.  */
+
+typedef std::unordered_map<std::string, std::string> soname_build_id_map;
+
+/* Key used to associate a soname_build_id_map to a core file bfd.  */
+
+static const struct bfd_key<soname_build_id_map> cbfd_soname_build_id_data_key;
+
+/* See solib.h.  */
+
+void
+set_cbfd_soname_build_id (gdb_bfd_ref_ptr abfd,
+			  const char *soname,
+			  const bfd_build_id *build_id)
+{
+  gdb_assert (abfd.get () != nullptr);
+  gdb_assert (soname != nullptr);
+  gdb_assert (build_id != nullptr);
+
+  soname_build_id_map *mapptr = cbfd_soname_build_id_data_key.get (abfd.get ());
+
+  if (mapptr == nullptr)
+    mapptr = cbfd_soname_build_id_data_key.emplace (abfd.get ());
+
+  (*mapptr)[soname] = build_id_to_string (build_id);
+}
+
+/* See solib.h.  */
+
+gdb::unique_xmalloc_ptr<char>
+get_cbfd_soname_build_id (gdb_bfd_ref_ptr abfd, const char *soname)
+{
+  if (abfd.get () == nullptr || soname == nullptr)
+    return {};
+
+  soname_build_id_map *mapptr
+    = cbfd_soname_build_id_data_key.get (abfd.get ());
+
+  if (mapptr == nullptr)
+    return {};
+
+  auto it = mapptr->find (basename (soname));
+  if (it == mapptr->end ())
+    return {};
+
+  return make_unique_xstrdup (it->second.c_str ());
+}
+
 /* Given a pointer to one of the shared objects in our list of mapped
    objects, use the recorded name to open a bfd descriptor for the
    object, build a section table, relocate all the section addresses
@@ -1586,6 +1636,37 @@ gdb_bfd_scan_elf_dyntag (const int desired_dyntag, bfd *abfd, CORE_ADDR *ptr,
   return 0;
 }
 
+/* See solib.h.  */
+
+gdb::unique_xmalloc_ptr<char>
+gdb_bfd_read_elf_soname (const char *filename)
+{
+  gdb_bfd_ref_ptr abfd = gdb_bfd_open (filename, gnutarget);
+
+  if (abfd == nullptr)
+    return {};
+
+  /* Check that ABFD is an ET_DYN ELF file.  */
+  if (!bfd_check_format (abfd.get (), bfd_object)
+      || !(bfd_get_file_flags (abfd.get ()) & DYNAMIC))
+    return {};
+
+  CORE_ADDR idx;
+  if (!gdb_bfd_scan_elf_dyntag (DT_SONAME, abfd.get (), &idx, nullptr))
+    return {};
+
+  struct bfd_section *dynstr = bfd_get_section_by_name (abfd.get (), ".dynstr");
+  if (dynstr == nullptr || bfd_section_size (dynstr) <= idx)
+    return {};
+
+  /* Read the soname from the string table.  */
+  gdb::byte_vector dynstr_buf;
+  if (!gdb_bfd_get_full_section_contents (abfd.get (), dynstr, &dynstr_buf))
+    return {};
+
+  return make_unique_xstrdup ((char *) dynstr_buf.data () + idx);
+}
+
 /* Lookup the value for a specific symbol from symbol table.  Look up symbol
    from ABFD.  MATCH_SYM is a callback function to determine whether to pick
    up a symbol.  DATA is the input of this callback function.  Return NULL
diff --git a/gdb/solib.h b/gdb/solib.h
index 2258b0ba6a0..cd6c8a81104 100644
--- a/gdb/solib.h
+++ b/gdb/solib.h
@@ -118,6 +118,12 @@ extern CORE_ADDR gdb_bfd_lookup_symbol_from_symtab (bfd *abfd,
 extern int gdb_bfd_scan_elf_dyntag (const int desired_dyntag, bfd *abfd,
 				    CORE_ADDR *ptr, CORE_ADDR *ptr_addr);
 
+/* If FILENAME refers to an ELF shared object then attempt to return the
+   string referred to by its DT_SONAME tag.   */
+
+extern gdb::unique_xmalloc_ptr<char> gdb_bfd_read_elf_soname
+  (const char *filename);
+
 /* Enable or disable optional solib event breakpoints as appropriate.  */
 
 extern void update_solib_breakpoints (void);
@@ -126,4 +132,18 @@ extern void update_solib_breakpoints (void);
 
 extern void handle_solib_event (void);
 
+/* Associate SONAME with BUILD_ID in ABFD's registry so that it can be
+   retrieved with get_cbfd_soname_build_id.  */
+
+extern void set_cbfd_soname_build_id (gdb_bfd_ref_ptr abfd,
+				      const char *soname,
+				      const bfd_build_id *build_id);
+
+/* If SONAME had a build-id associated with it in ABFD's registry by a
+   previous call to set_cbfd_soname_build_id then return the build-id
+   as a NULL-terminated hex string.  */
+
+extern gdb::unique_xmalloc_ptr<char> get_cbfd_soname_build_id
+  (gdb_bfd_ref_ptr abfd, const char *soname);
+
 #endif /* SOLIB_H */
-- 
2.35.1

[PATCH 2/2] PR gdb/27570: missing support for debuginfod in core_target::build_file_mappings

[Aaron Merey]

Hi Tom,

On Fri, Mar 4, 2022 at 10:20 AM Tom Tromey <tom@tromey.com> wrote:
> Aaron> +          if (execbfd == nullptr)
> Aaron> +            warning (_("File \"%s\" from debuginfod cannot be opened as bfd"),
> Aaron> +                     execpath.get ());
>
> I suspect this should use gdb_bfd_errmsg.

Fixed.

> Aaron> +  char *dname = nullptr;
>
> Aaron> +  scoped_fd fd (debuginfod_find_executable (c, build_id, build_id_len, &dname));
> Aaron> +  debuginfod_set_user_data (c, nullptr);
>
> Aaron> +  if (fd.get () >= 0)
> Aaron> +    destname->reset (dname);
>
> Is it possible for fd.get()<0 and dname!=nullptr?  If so there's a
> memory leak.

If fd.get()<0 then dname is always nullptr.

> Aaron> +  gdb::unique_xmalloc_ptr<char> build_id_hexstr =
> Aaron> +    get_cbfd_soname_build_id (current_program_space->cbfd, so->so_name);
>
> '=' on the next line.

Fixed.

Aaron

---
 gdb/corelow.c                                 | 28 ++++++++++
 gdb/debuginfod-support.c                      | 51 +++++++++++++++++++
 gdb/debuginfod-support.h                      | 17 +++++++
 gdb/gcore.in                                  |  2 +-
 gdb/solib.c                                   | 32 ++++++++++++
 .../gdb.debuginfod/fetch_src_and_symbols.exp  | 22 ++++++++
 6 files changed, 151 insertions(+), 1 deletion(-)

diff --git a/gdb/corelow.c b/gdb/corelow.c
index 83d3c89656f..3ad03fadc4b 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -46,6 +46,8 @@
 #include "gdbsupport/filestuff.h"
 #include "build-id.h"
 #include "gdbsupport/pathstuff.h"
+#include "gdbsupport/scoped_fd.h"
+#include "debuginfod-support.h"
 #include <unordered_map>
 #include <unordered_set>
 #include "gdbcmd.h"
@@ -229,6 +231,11 @@ core_target::build_file_mappings ()
 	       canonical) pathname will be provided.  */
 	    gdb::unique_xmalloc_ptr<char> expanded_fname
 	      = exec_file_find (filename, NULL);
+
+	    if (expanded_fname == nullptr && build_id != nullptr)
+	      debuginfod_exec_query (build_id->data, build_id->size,
+				     filename, &expanded_fname);
+
 	    if (expanded_fname == nullptr)
 	      {
 		m_core_unavailable_mappings.emplace_back (start, end - start);
@@ -410,6 +417,27 @@ locate_exec_from_corefile_build_id (bfd *abfd, int from_tty)
   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);
+	}
+    }
+
   if (execbfd != nullptr)
     {
       exec_file_attach (bfd_get_filename (execbfd.get ()), from_tty);
diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 56d8e7781c5..6dbe40a14e3 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -69,6 +69,15 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
   return scoped_fd (-ENOSYS);
 }
 
+scoped_fd
+debuginfod_exec_query (const unsigned char *build_id,
+		       int build_id_len,
+		       const char *filename,
+		       gdb::unique_xmalloc_ptr<char> *destname)
+{
+  return scoped_fd (-ENOSYS);
+}
+
 #define NO_IMPL _("Support for debuginfod is not compiled into GDB.")
 
 #else
@@ -271,6 +280,48 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
 
   return fd;
 }
+
+/* See debuginfod-support.h  */
+
+scoped_fd
+debuginfod_exec_query (const unsigned char *build_id,
+		       int build_id_len,
+		       const char *filename,
+		       gdb::unique_xmalloc_ptr<char> *destname)
+{
+  if (!debuginfod_is_enabled ())
+    return scoped_fd (-ENOSYS);
+
+  debuginfod_client *c = get_debuginfod_client ();
+
+  if (c == nullptr)
+    return scoped_fd (-ENOMEM);
+
+  char *dname = nullptr;
+  user_data data ("executable for", filename);
+
+  debuginfod_set_user_data (c, &data);
+  gdb::optional<target_terminal::scoped_restore_terminal_state> term_state;
+  if (target_supports_terminal_ours ())
+    {
+      term_state.emplace ();
+      target_terminal::ours ();
+    }
+
+  scoped_fd fd (debuginfod_find_executable (c, build_id, build_id_len, &dname));
+  debuginfod_set_user_data (c, nullptr);
+
+  if (debuginfod_verbose > 0 && fd.get () < 0 && fd.get () != -ENOENT)
+    printf_filtered (_("Download failed: %s. " \
+		       "Continuing without executable for %ps.\n"),
+		     safe_strerror (-fd.get ()),
+		     styled_string (file_name_style.style (),  filename));
+
+  if (fd.get () >= 0)
+    destname->reset (dname);
+
+  return fd;
+}
 #endif
 
 /* Set callback for "set debuginfod enabled".  */
diff --git a/gdb/debuginfod-support.h b/gdb/debuginfod-support.h
index 29e361bb76f..5b1c1cb91f4 100644
--- a/gdb/debuginfod-support.h
+++ b/gdb/debuginfod-support.h
@@ -61,4 +61,21 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
 			    const char *filename,
 			    gdb::unique_xmalloc_ptr<char> *destname);
 
+/* Query debuginfod servers for an executable file with BUILD_ID.
+   BUILD_ID can be given as a binary blob or a null-terminated string.
+   If given as a binary blob, BUILD_ID_LEN should be the number of bytes.
+   If given as a null-terminated string, BUILD_ID_LEN should be 0.
+
+   FILENAME should be the name or path associated with the executable.
+   It is used for printing messages to the user.
+
+   If the file is successfully retrieved, its path on the local machine
+   is stored in DESTNAME.  If GDB is not built with debuginfod, this
+   function returns -ENOSYS.  */
+
+extern scoped_fd debuginfod_exec_query (const unsigned char *build_id,
+					int build_id_len,
+					const char *filename,
+					gdb::unique_xmalloc_ptr<char>
+					  *destname);
 #endif /* DEBUGINFOD_SUPPORT_H */
diff --git a/gdb/gcore.in b/gdb/gcore.in
index 7038c0c394c..ed7abf2d29b 100644
--- a/gdb/gcore.in
+++ b/gdb/gcore.in
@@ -98,7 +98,7 @@ do
 	# `</dev/null' to avoid touching interactive terminal if it is
 	# available but not accessible as GDB would get stopped on SIGTTIN.
 	"$binary_path/@GDB_TRANSFORM_NAME@" </dev/null \
-	    --nx --batch --readnever \
+	    --nx --batch --readnever -iex 'set debuginfod enabled off' \
 	    -ex "set pagination off" -ex "set height 0" -ex "set width 0" \
 	    "${dump_all_cmds[@]}" \
 	    -ex "attach $pid" -ex "gcore $prefix.$pid" -ex detach -ex quit
diff --git a/gdb/solib.c b/gdb/solib.c
index 11182b43dd0..e7fb7f788fb 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -49,6 +49,8 @@
 #include "filesystem.h"
 #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"
 
@@ -588,6 +590,36 @@ solib_map_sections (struct so_list *so)
 
   gdb::unique_xmalloc_ptr<char> filename (tilde_expand (so->so_name));
   gdb_bfd_ref_ptr abfd (ops->bfd_open (filename.get ()));
+  gdb::unique_xmalloc_ptr<char> build_id_hexstr
+    = get_cbfd_soname_build_id (current_program_space->cbfd, so->so_name);
+
+  /* If we already know the build-id of this solib from a core file, verify
+     it matches ABFD's build-id.  If there is a mismatch or the solib wasn't
+     found, attempt to query debuginfod for the correct solib.  */
+  if (build_id_hexstr.get () != nullptr)
+    {
+      bool mismatch = false;
+
+      if (abfd != nullptr && abfd->build_id != nullptr)
+	{
+	  std::string build_id = build_id_to_string (abfd->build_id);
+
+	  if (build_id != build_id_hexstr.get ())
+	    mismatch = true;
+	}
+      if (abfd == nullptr || mismatch)
+	{
+	  scoped_fd fd = debuginfod_exec_query ((const unsigned char*)
+						build_id_hexstr.get (),
+						0, so->so_name, &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 ()));
+	}
+    }
 
   if (abfd == NULL)
     return 0;
diff --git a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
index 5912c38c266..f12ed7d486c 100644
--- a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
+++ b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
@@ -63,6 +63,11 @@ if { [gdb_compile "$sourcetmp" "$binfile" executable {debug}] != "" } {
     return -1
 }
 
+if { [gdb_compile "$sourcetmp" "${binfile}2" executable {debug}] != "" } {
+    fail "compile"
+    return -1
+}
+
 # Write some assembly that just has a .gnu_debugaltlink section.
 # Copied from testsuite/gdb.dwarf2/dwzbuildid.exp.
 proc write_just_debugaltlink {filename dwzname buildid} {
@@ -114,6 +119,8 @@ proc write_dwarf_file {filename buildid {value 99}} {
     }
 }
 
+set corefile [standard_output_file "corefile"]
+
 proc no_url { } {
     global binfile outputdir debugdir
 
@@ -167,6 +174,16 @@ proc no_url { } {
     gdb_test "file ${binfile}_alt.o" \
 	".*could not find '.gnu_debugaltlink'.*" \
 	"file [file tail ${binfile}_alt.o]"
+
+    # Generate a core file and test that gdb cannot find the executable
+    clean_restart ${binfile}2
+    gdb_test "start" "Temporary breakpoint.*"
+    gdb_test "generate-core-file $::corefile" "Saved corefile $::corefile" \
+	"file [file tail $::corefile] gen"
+    file rename -force ${binfile}2 $debugdir
+
+    clean_restart
+    gdb_test "core $::corefile" ".*in ?? ().*" "file [file tail $::corefile]"
 }
 
 proc local_url { } {
@@ -234,6 +251,11 @@ proc local_url { } {
     gdb_test "br main" "Breakpoint 1 at.*file.*"
     gdb_test "l" ".*This program is distributed in the hope.*"
 
+    # gdb should now find the executable file
+    clean_restart
+    gdb_test "core $::corefile" ".*return 0.*" "file [file tail $::corefile]" \
+	"Enable debuginfod?.*" "y"
+
     # gdb should now find the debugaltlink file
     clean_restart
     gdb_test "file ${binfile}_alt.o" \
-- 
2.35.1

Re: [PATCH 1/2] gdb: Add soname to build-id mapping for core files

[Tom Tromey]

>>>>> Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:

Hi again.

> +	    if (soname.get () != nullptr)

I don't think you need the .get here.

> +  std::unordered_map<char *, const bfd_build_id *> filename_map;

I suspect this code is intended to use the contents of the string as the
key -- but this will actually use the pointer value as the key.

If the pointer value is intended, some comment somewhere to that effect
-- explaining why it's an ok approach -- would be good to have.

> +      if (sec->flags & SEC_LOAD
> +	  && get_elf_backend_data (cbfd)->elf_backend_core_find_build_id
> +	       (cbfd, (bfd_vma) sec->filepos))

The get_elf_backend_data call has to have parens around it according to
GNU / gdb style.

> +  /* Read the soname from the string table.  */
> +  gdb::byte_vector dynstr_buf;
> +  if (!gdb_bfd_get_full_section_contents (abfd.get (), dynstr, &dynstr_buf))
> +    return {};
> +
> +  return make_unique_xstrdup ((char *) dynstr_buf.data () + idx);

What if the data isn't \0-terminated?  Maybe some size checking (not
sure if needed) and make_unique_xstrndup would be safer.

Tom

Re: [PATCH 2/2] PR gdb/27570: missing support for debuginfod in core_target::build_file_mappings

[Tom Tromey]

>>>>> Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:

>  gdb/corelow.c                                 | 28 ++++++++++
>  gdb/debuginfod-support.c                      | 51 +++++++++++++++++++
>  gdb/debuginfod-support.h                      | 17 +++++++
>  gdb/gcore.in                                  |  2 +-
>  gdb/solib.c                                   | 32 ++++++++++++
>  .../gdb.debuginfod/fetch_src_and_symbols.exp  | 22 ++++++++
>  6 files changed, 151 insertions(+), 1 deletion(-)

This looks good to me.  Thanks.

> +  if (debuginfod_verbose > 0 && fd.get () < 0 && fd.get () != -ENOENT)
> +    printf_filtered (_("Download failed: %s. " \
> +		       "Continuing without executable for %ps.\n"),
> +		     safe_strerror (-fd.get ()),
> +		     styled_string (file_name_style.style (),  filename));

I can't remember if I mentioned this before but it seems like a failure
to download should probably always be reported.  Maybe this ought to be
an unconditional warning call.

Tom

Re: [PATCH 1/2] gdb: Add soname to build-id mapping for core files

[Aaron Merey]

On Fri, Mar 11, 2022 at 10:23 AM Tom Tromey <tom@tromey.com> wrote:
> > +         if (soname.get () != nullptr)
>
> I don't think you need the .get here.

Fixed.

> > +  std::unordered_map<char *, const bfd_build_id *> filename_map;
>
> I suspect this code is intended to use the contents of the string as the 
> key -- but this will actually use the pointer value as the key.
>
> If the pointer value is intended, some comment somewhere to that effect
> -- explaining why it's an ok approach -- would be good to have.

After taking another look at this I don't think filename_map is needed
at all.  The idea behind it was to use build-ids to reaquire downloaded
files in core_target::build_file_mappings from the debuginfod cache.
However this is unnecessary since bfd_map in build_file_mappings already
makes previously opened files (including those downloaded) available
using just a filename.

A build-id is only needed when trying to aquire the file for the first
time.  This is facilitated by vma_map in linux_read_core_file_mappings.

> > +      if (sec->flags & SEC_LOAD
> > +       && get_elf_backend_data (cbfd)->elf_backend_core_find_build_id
> > +            (cbfd, (bfd_vma) sec->filepos))
>
> The get_elf_backend_data call has to have parens around it according to
> GNU / gdb style.

Fixed.

> > +  /* Read the soname from the string table.  */  
> > +  gdb::byte_vector dynstr_buf;
> > +  if (!gdb_bfd_get_full_section_contents (abfd.get (), dynstr, &dynstr_buf))
> > +    return {}; 
> > + 
> > +  return make_unique_xstrdup ((char *) dynstr_buf.data () + idx);
>
> What if the data isn't \0-terminated?  Maybe some size checking (not
> sure if needed) and make_unique_xstrndup would be safer.

Added a check to make sure \0 exists between the start of the soname
and the end of the section.

I also replaced basename with lbasename in get_cbfd_soname_build_id.

Aaron

---
 gdb/corelow.c    | 11 ++++++
 gdb/linux-tdep.c | 24 ++++++++++++-
 gdb/solib.c      | 87 ++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/solib.h      | 20 +++++++++++
 4 files changed, 141 insertions(+), 1 deletion(-)

diff --git a/gdb/corelow.c b/gdb/corelow.c
index 1579e6bc2b8..9876f4d920e 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -282,6 +282,17 @@ core_target::build_file_mappings ()
 
 	/* Set target_section fields.  */
 	m_core_file_mappings.emplace_back (start, end, sec);
+
+	/* If this is a bfd of a shared library, record its soname
+	   and build id.  */
+	if (build_id != nullptr)
+	  {
+	    gdb::unique_xmalloc_ptr<char> soname
+	      = gdb_bfd_read_elf_soname (bfd->filename);
+	    if (soname != nullptr)
+	      set_cbfd_soname_build_id (current_program_space->cbfd,
+					soname.get (), build_id);
+	  }
       });
 
   normalize_mem_ranges (&m_core_unavailable_mappings);
diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index d4868902ac3..52cdaae034b 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -44,6 +44,7 @@
 #include "solib-svr4.h"
 
 #include <ctype.h>
+#include <unordered_map>
 
 /* This enum represents the values that the user can choose when
    informing the Linux kernel about which memory mappings will be
@@ -1185,6 +1186,22 @@ linux_read_core_file_mappings
   if (f != descend)
     warning (_("malformed note - filename area is too big"));
 
+  const bfd_build_id *orig_build_id = cbfd->build_id;
+  std::unordered_map<ULONGEST, const bfd_build_id *> vma_map;
+
+  /* Search for solib build-ids in the core file.  Each time one is found,
+     map the start vma of the corresponding elf header to the build-id.  */
+  for (bfd_section *sec = cbfd->sections; sec != nullptr; sec = sec->next)
+    {
+      cbfd->build_id = nullptr;
+
+      if (sec->flags & SEC_LOAD
+	  && (get_elf_backend_data (cbfd)->elf_backend_core_find_build_id
+	       (cbfd, (bfd_vma) sec->filepos)))
+	vma_map[sec->vma] = cbfd->build_id;
+    }
+
+  cbfd->build_id = orig_build_id;
   pre_loop_cb (count);
 
   for (int i = 0; i < count; i++)
@@ -1198,8 +1215,13 @@ linux_read_core_file_mappings
       descdata += addr_size;
       char * filename = filenames;
       filenames += strlen ((char *) filenames) + 1;
+      const bfd_build_id *build_id = nullptr;
+      auto vma_map_it = vma_map.find (start);
+
+      if (vma_map_it != vma_map.end ())
+	build_id = vma_map_it->second;
 
-      loop_cb (i, start, end, file_ofs, filename, nullptr);
+      loop_cb (i, start, end, file_ofs, filename, build_id);
     }
 }
 
diff --git a/gdb/solib.c b/gdb/solib.c
index 8bcbfa22df1..8ecea0f89e7 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -23,6 +23,7 @@
 #include <fcntl.h>
 #include "symtab.h"
 #include "bfd.h"
+#include "build-id.h"
 #include "symfile.h"
 #include "objfiles.h"
 #include "gdbcore.h"
@@ -519,6 +520,55 @@ solib_bfd_open (const char *pathname)
   return abfd;
 }
 
+/* Mapping of a core file's shared library sonames to their respective
+   build-ids.  Added to the registries of core file bfds.  */
+
+typedef std::unordered_map<std::string, std::string> soname_build_id_map;
+
+/* Key used to associate a soname_build_id_map to a core file bfd.  */
+
+static const struct bfd_key<soname_build_id_map> cbfd_soname_build_id_data_key;
+
+/* See solib.h.  */
+
+void
+set_cbfd_soname_build_id (gdb_bfd_ref_ptr abfd,
+			  const char *soname,
+			  const bfd_build_id *build_id)
+{
+  gdb_assert (abfd.get () != nullptr);
+  gdb_assert (soname != nullptr);
+  gdb_assert (build_id != nullptr);
+
+  soname_build_id_map *mapptr = cbfd_soname_build_id_data_key.get (abfd.get ());
+
+  if (mapptr == nullptr)
+    mapptr = cbfd_soname_build_id_data_key.emplace (abfd.get ());
+
+  (*mapptr)[soname] = build_id_to_string (build_id);
+}
+
+/* See solib.h.  */
+
+gdb::unique_xmalloc_ptr<char>
+get_cbfd_soname_build_id (gdb_bfd_ref_ptr abfd, const char *soname)
+{
+  if (abfd.get () == nullptr || soname == nullptr)
+    return {};
+
+  soname_build_id_map *mapptr
+    = cbfd_soname_build_id_data_key.get (abfd.get ());
+
+  if (mapptr == nullptr)
+    return {};
+
+  auto it = mapptr->find (lbasename (soname));
+  if (it == mapptr->end ())
+    return {};
+
+  return make_unique_xstrdup (it->second.c_str ());
+}
+
 /* Given a pointer to one of the shared objects in our list of mapped
    objects, use the recorded name to open a bfd descriptor for the
    object, build a section table, relocate all the section addresses
@@ -1586,6 +1636,43 @@ gdb_bfd_scan_elf_dyntag (const int desired_dyntag, bfd *abfd, CORE_ADDR *ptr,
   return 0;
 }
 
+/* See solib.h.  */
+
+gdb::unique_xmalloc_ptr<char>
+gdb_bfd_read_elf_soname (const char *filename)
+{
+  gdb_bfd_ref_ptr abfd = gdb_bfd_open (filename, gnutarget);
+
+  if (abfd == nullptr)
+    return {};
+
+  /* Check that ABFD is an ET_DYN ELF file.  */
+  if (!bfd_check_format (abfd.get (), bfd_object)
+      || !(bfd_get_file_flags (abfd.get ()) & DYNAMIC))
+    return {};
+
+  CORE_ADDR idx;
+  if (!gdb_bfd_scan_elf_dyntag (DT_SONAME, abfd.get (), &idx, nullptr))
+    return {};
+
+  struct bfd_section *dynstr = bfd_get_section_by_name (abfd.get (), ".dynstr");
+  int sect_size = bfd_section_size (dynstr);
+  if (dynstr == nullptr || sect_size <= idx)
+    return {};
+
+  /* Read soname from the string table.  */
+  gdb::byte_vector dynstr_buf;
+  if (!gdb_bfd_get_full_section_contents (abfd.get (), dynstr, &dynstr_buf))
+    return {};
+
+  /* Ensure soname is null-terminated before returning a copy.  */
+  char *soname = (char *) dynstr_buf.data () + idx;
+  if (strnlen (soname, sect_size - idx) == sect_size - idx)
+    return {};
+
+  return make_unique_xstrdup (soname);
+}
+
 /* Lookup the value for a specific symbol from symbol table.  Look up symbol
    from ABFD.  MATCH_SYM is a callback function to determine whether to pick
    up a symbol.  DATA is the input of this callback function.  Return NULL
diff --git a/gdb/solib.h b/gdb/solib.h
index 2258b0ba6a0..cd6c8a81104 100644
--- a/gdb/solib.h
+++ b/gdb/solib.h
@@ -118,6 +118,12 @@ extern CORE_ADDR gdb_bfd_lookup_symbol_from_symtab (bfd *abfd,
 extern int gdb_bfd_scan_elf_dyntag (const int desired_dyntag, bfd *abfd,
 				    CORE_ADDR *ptr, CORE_ADDR *ptr_addr);
 
+/* If FILENAME refers to an ELF shared object then attempt to return the
+   string referred to by its DT_SONAME tag.   */
+
+extern gdb::unique_xmalloc_ptr<char> gdb_bfd_read_elf_soname
+  (const char *filename);
+
 /* Enable or disable optional solib event breakpoints as appropriate.  */
 
 extern void update_solib_breakpoints (void);
@@ -126,4 +132,18 @@ extern void update_solib_breakpoints (void);
 
 extern void handle_solib_event (void);
 
+/* Associate SONAME with BUILD_ID in ABFD's registry so that it can be
+   retrieved with get_cbfd_soname_build_id.  */
+
+extern void set_cbfd_soname_build_id (gdb_bfd_ref_ptr abfd,
+				      const char *soname,
+				      const bfd_build_id *build_id);
+
+/* If SONAME had a build-id associated with it in ABFD's registry by a
+   previous call to set_cbfd_soname_build_id then return the build-id
+   as a NULL-terminated hex string.  */
+
+extern gdb::unique_xmalloc_ptr<char> get_cbfd_soname_build_id
+  (gdb_bfd_ref_ptr abfd, const char *soname);
+
 #endif /* SOLIB_H */
-- 
2.35.1

Re: [PATCH 2/2] PR gdb/27570: missing support for debuginfod in core_target::build_file_mappings

[Aaron Merey]

On Fri, Mar 11, 2022 at 10:27 AM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> >  gdb/corelow.c                                 | 28 ++++++++++
> >  gdb/debuginfod-support.c                      | 51 +++++++++++++++++++
> >  gdb/debuginfod-support.h                      | 17 +++++++
> >  gdb/gcore.in                                  |  2 +-
> >  gdb/solib.c                                   | 32 ++++++++++++
> >  .../gdb.debuginfod/fetch_src_and_symbols.exp  | 22 ++++++++
> >  6 files changed, 151 insertions(+), 1 deletion(-)
>
> This looks good to me.  Thanks.

Great, I'll merge it once patch 1/2 is merged.

>
> > +  if (debuginfod_verbose > 0 && fd.get () < 0 && fd.get () != -ENOENT)
> > +    printf_filtered (_("Download failed: %s. " \
> > +                    "Continuing without executable for %ps.\n"),
> > +                  safe_strerror (-fd.get ()),
> > +                  styled_string (file_name_style.style (),  filename));
>
> I can't remember if I mentioned this before but it seems like a failure
> to download should probably always be reported.  Maybe this ought to be
> an unconditional warning call.

I will fix this in a separate patch. I think this counts as an obvious fix.

Aaron

Re: [PATCH 1/2] gdb: Add soname to build-id mapping for core files

[Tom Tromey]

>>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:

Aaron> Added a check to make sure \0 exists between the start of the soname
Aaron> and the end of the section.

Aaron> I also replaced basename with lbasename in get_cbfd_soname_build_id.

Thanks, I think this is ok.

Tom

Re: [PATCH 1/2] gdb: Add soname to build-id mapping for core files

[Aaron Merey]

On Fri, Mar 18, 2022 at 3:03 PM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Aaron> Added a check to make sure \0 exists between the start of the soname
> Aaron> and the end of the section.
>
> Aaron> I also replaced basename with lbasename in get_cbfd_soname_build_id.
>
> Thanks, I think this is ok.

Thanks, pushed as commit 39f53acb41

Aaron

Re: [PATCH 2/2] PR gdb/27570: missing support for debuginfod in core_target::build_file_mappings

[Aaron Merey]

On Fri, Mar 11, 2022 at 4:49 PM Aaron Merey <amerey@redhat.com> wrote:
>
> On Fri, Mar 11, 2022 at 10:27 AM Tom Tromey <tom@tromey.com> wrote:
> >
> > >>>>> Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
> >
> > >  gdb/corelow.c                                 | 28 ++++++++++
> > >  gdb/debuginfod-support.c                      | 51 +++++++++++++++++++
> > >  gdb/debuginfod-support.h                      | 17 +++++++
> > >  gdb/gcore.in                                  |  2 +-
> > >  gdb/solib.c                                   | 32 ++++++++++++
> > >  .../gdb.debuginfod/fetch_src_and_symbols.exp  | 22 ++++++++
> > >  6 files changed, 151 insertions(+), 1 deletion(-)
> >
> > This looks good to me.  Thanks.
>
> Great, I'll merge it once patch 1/2 is merged.

Pushed as commit b91f93a02c9c

Aaron