Re: [PATCH 2/3] gdb: Add soname to build-id mapping for corefiles

[Aaron Merey <amerey@redhat.com>]

Hi Lancelot and Simon,

Thanks for the reviews. The updated patch is included below.

On Tue, Aug 17, 2021 at 9:59 AM Simon Marchi <simon.marchi@polymtl.ca> wrote:
>>> +  /* Check that bfd is an ET_DYN ELF file.  */  
>>> +  bfd_check_format (abfd.get (), bfd_object);
>
> I asked this in my previous review, still applies here:
>
>   What's the point of calling bfd_check_format without checking the 
>   result?  It looks like a function without side-effects.

I included a few comments in the [PATCH 0/3] email for this series[1].
I mentioned that for some reason bfd_check_format appears to update
the bfd 'flags' field with the correct value. If bfd_check_format is not 
called here the check for the DYNAMIC flag fails even for ET_DYN files.

[1] https://sourceware.org/pipermail/gdb-patches/2021-August/181416.html

---
 gdb/arch-utils.c | 15 +++++----------
 gdb/arch-utils.h | 23 +++++++++++++----------
 gdb/build-id.h   |  2 ++
 gdb/corelow.c    | 13 ++++++++++++-
 gdb/gdbarch.c    |  2 +-
 gdb/gdbarch.h    |  4 ++--
 gdb/gdbarch.sh   |  2 +-
 gdb/linux-tdep.c | 46 ++++++++++++++++++++++++++++++++++------------
 gdb/progspace.c  | 32 ++++++++++++++++++++++++++++++++
 gdb/progspace.h  | 17 +++++++++++++++++
 gdb/solib.c      | 35 +++++++++++++++++++++++++++++++++++
 gdb/solib.h      |  5 +++++
 12 files changed, 159 insertions(+), 37 deletions(-)

diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index 862f26b6cf7..ffb32cb203f 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -1075,16 +1075,11 @@ default_get_pc_address_flags (frame_info *frame, CORE_ADDR pc)
 
 /* See arch-utils.h.  */
 void
-default_read_core_file_mappings (struct gdbarch *gdbarch,
-				 struct bfd *cbfd,
-				 gdb::function_view<void (ULONGEST count)>
-				   pre_loop_cb,
-				 gdb::function_view<void (int num,
-							  ULONGEST start,
-							  ULONGEST end,
-							  ULONGEST file_ofs,
-							  const char *filename)>
-				   loop_cb)
+default_read_core_file_mappings
+ (struct gdbarch *gdbarch,
+  struct bfd *cbfd,
+  gdb::function_view<void (ULONGEST count)> pre_loop_cb,
+  loop_cb_ftype loop_cb)
 {
 }
 
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index 03e9082f6d7..2243c3fb85b 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -294,15 +294,18 @@ extern ULONGEST default_type_align (struct gdbarch *gdbarch,
 extern std::string default_get_pc_address_flags (frame_info *frame,
 						 CORE_ADDR pc);
 
+
+using loop_cb_ftype = gdb::function_view<void (int num,
+					       ULONGEST start,
+					       ULONGEST end,
+					       ULONGEST file_ofs,
+					       const char *filename,
+					       const bfd_build_id *build_id)>;
+
 /* Default implementation of gdbarch read_core_file_mappings method.  */
-extern void default_read_core_file_mappings (struct gdbarch *gdbarch,
-					     struct bfd *cbfd,
-					     gdb::function_view<void (ULONGEST count)>
-					       pre_loop_cb,
-					     gdb::function_view<void (int num,
-								      ULONGEST start,
-								      ULONGEST end,
-								      ULONGEST file_ofs,
-								      const char *filename)>
-					       loop_cb);
+extern void default_read_core_file_mappings
+ (struct gdbarch *gdbarch,
+  struct bfd *cbfd,
+  gdb::function_view<void (ULONGEST count)> pre_loop_cb,
+  loop_cb_ftype loop_cb);
 #endif /* ARCH_UTILS_H */
diff --git a/gdb/build-id.h b/gdb/build-id.h
index 42f8d57ede1..3c9402ee71b 100644
--- a/gdb/build-id.h
+++ b/gdb/build-id.h
@@ -20,8 +20,10 @@
 #ifndef BUILD_ID_H
 #define BUILD_ID_H
 
+#include "defs.h"
 #include "gdb_bfd.h"
 #include "gdbsupport/rsp-low.h"
+#include <string>
 
 /* Locate NT_GNU_BUILD_ID from ABFD and return its content.  */
 
diff --git a/gdb/corelow.c b/gdb/corelow.c
index eb785a08633..31af0f22584 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -214,7 +214,7 @@ core_target::build_file_mappings ()
     /* read_core_file_mappings will invoke this lambda for each mapping
        that it finds.  */
     [&] (int num, ULONGEST start, ULONGEST end, ULONGEST file_ofs,
-	 const char *filename)
+	 const char *filename, const bfd_build_id *build_id)
       {
 	/* Architecture-specific read_core_mapping methods are expected to
 	   weed out non-file-backed mappings.  */
@@ -282,6 +282,16 @@ 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::optional<std::string> soname = gdb_bfd_read_elf_soname (bfd);
+	    if (soname)
+	      current_program_space->set_cbfd_soname_build_id 
+	       (std::move (*soname), build_id);
+	  }
       });
 
   normalize_mem_ranges (&m_core_unavailable_mappings);
@@ -305,6 +315,7 @@ core_target::close ()
 	 comments in clear_solib in solib.c.  */
       clear_solib ();
 
+      current_program_space->clear_cbfd_soname_build_ids ();
       current_program_space->cbfd.reset (nullptr);
     }
 
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index f89dcc57754..35464115039 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -5411,7 +5411,7 @@ set_gdbarch_get_pc_address_flags (struct gdbarch *gdbarch,
 }
 
 void
-gdbarch_read_core_file_mappings (struct gdbarch *gdbarch, struct bfd *cbfd, gdb::function_view<void (ULONGEST count)> pre_loop_cb, gdb::function_view<void (int num, ULONGEST start, ULONGEST end, ULONGEST file_ofs, const char *filename)> loop_cb)
+gdbarch_read_core_file_mappings (struct gdbarch *gdbarch, struct bfd *cbfd, gdb::function_view<void (ULONGEST count)> pre_loop_cb, gdb::function_view<void (int num, ULONGEST start, ULONGEST end, ULONGEST file_ofs, const char *filename, const bfd_build_id *build_id)> loop_cb)
 {
   gdb_assert (gdbarch != NULL);
   gdb_assert (gdbarch->read_core_file_mappings != NULL);
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 979159ba2f5..baf80580d60 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -1710,8 +1710,8 @@ extern void set_gdbarch_get_pc_address_flags (struct gdbarch *gdbarch, gdbarch_g
 
 /* Read core file mappings */
 
-typedef void (gdbarch_read_core_file_mappings_ftype) (struct gdbarch *gdbarch, struct bfd *cbfd, gdb::function_view<void (ULONGEST count)> pre_loop_cb, gdb::function_view<void (int num, ULONGEST start, ULONGEST end, ULONGEST file_ofs, const char *filename)> loop_cb);
-extern void gdbarch_read_core_file_mappings (struct gdbarch *gdbarch, struct bfd *cbfd, gdb::function_view<void (ULONGEST count)> pre_loop_cb, gdb::function_view<void (int num, ULONGEST start, ULONGEST end, ULONGEST file_ofs, const char *filename)> loop_cb);
+typedef void (gdbarch_read_core_file_mappings_ftype) (struct gdbarch *gdbarch, struct bfd *cbfd, gdb::function_view<void (ULONGEST count)> pre_loop_cb, gdb::function_view<void (int num, ULONGEST start, ULONGEST end, ULONGEST file_ofs, const char *filename, const bfd_build_id *build_id)> loop_cb);
+extern void gdbarch_read_core_file_mappings (struct gdbarch *gdbarch, struct bfd *cbfd, gdb::function_view<void (ULONGEST count)> pre_loop_cb, gdb::function_view<void (int num, ULONGEST start, ULONGEST end, ULONGEST file_ofs, const char *filename, const bfd_build_id *build_id)> loop_cb);
 extern void set_gdbarch_read_core_file_mappings (struct gdbarch *gdbarch, gdbarch_read_core_file_mappings_ftype *read_core_file_mappings);
 
 extern struct gdbarch_tdep *gdbarch_tdep (struct gdbarch *gdbarch);
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 39a99d0d5f3..c24079d34f3 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -1210,7 +1210,7 @@ m;ULONGEST;type_align;struct type *type;type;;default_type_align;;0
 f;std::string;get_pc_address_flags;frame_info *frame, CORE_ADDR pc;frame, pc;;default_get_pc_address_flags;;0
 
 # Read core file mappings
-m;void;read_core_file_mappings;struct bfd *cbfd, gdb::function_view<void (ULONGEST count)> pre_loop_cb, gdb::function_view<void (int num, ULONGEST start, ULONGEST end, ULONGEST file_ofs, const char *filename)> loop_cb;cbfd, pre_loop_cb, loop_cb;;default_read_core_file_mappings;;0
+m;void;read_core_file_mappings;struct bfd *cbfd, gdb::function_view<void (ULONGEST count)> pre_loop_cb, gdb::function_view<void (int num, ULONGEST start, ULONGEST end, ULONGEST file_ofs, const char *filename, const bfd_build_id *build_id)> loop_cb;cbfd, pre_loop_cb, loop_cb;;default_read_core_file_mappings;;0
 
 EOF
 }
diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index ae2f7c14f6d..812ca1f99bf 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
@@ -1097,16 +1098,11 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
    for each mapping.  */
 
 static void
-linux_read_core_file_mappings (struct gdbarch *gdbarch,
-			       struct bfd *cbfd,
-			       gdb::function_view<void (ULONGEST count)>
-				 pre_loop_cb,
-			       gdb::function_view<void (int num,
-							ULONGEST start,
-							ULONGEST end,
-							ULONGEST file_ofs,
-							const char *filename)>
-				 loop_cb)
+linux_read_core_file_mappings
+ (struct gdbarch *gdbarch,
+  struct bfd *cbfd,
+  gdb::function_view<void (ULONGEST count)> pre_loop_cb,
+  loop_cb_ftype  loop_cb)
 {
   /* Ensure that ULONGEST is big enough for reading 64-bit core files.  */
   gdb_static_assert (sizeof (ULONGEST) >= 8);
@@ -1175,6 +1171,23 @@ linux_read_core_file_mappings (struct gdbarch *gdbarch,
   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++)
@@ -1188,8 +1201,17 @@ linux_read_core_file_mappings (struct gdbarch *gdbarch,
       descdata += addr_size;
       char * filename = filenames;
       filenames += strlen ((char *) filenames) + 1;
+      const bfd_build_id *build_id = vma_map[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 (build_id != nullptr)
+	filename_map[filename] = build_id;
+      else
+	build_id = filename_map[filename];
 
-      loop_cb (i, start, end, file_ofs, filename);
+      loop_cb (i, start, end, file_ofs, filename, build_id);
     }
 }
 
@@ -1218,7 +1240,7 @@ linux_core_info_proc_mappings (struct gdbarch *gdbarch, const char *args)
 	  }
       },
     [=] (int num, ULONGEST start, ULONGEST end, ULONGEST file_ofs,
-	 const char *filename)
+	 const char *filename, const bfd_build_id *build_id)
       {
 	if (gdbarch_addr_bit (gdbarch) == 32)
 	  printf_filtered ("\t%10s %10s %10s %10s %s\n",
diff --git a/gdb/progspace.c b/gdb/progspace.c
index 7080bf8ee27..8b7b949d959 100644
--- a/gdb/progspace.c
+++ b/gdb/progspace.c
@@ -17,6 +17,7 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
+#include "build-id.h"
 #include "defs.h"
 #include "gdbcmd.h"
 #include "objfiles.h"
@@ -358,6 +359,37 @@ print_program_space (struct ui_out *uiout, int requested)
     }
 }
 
+/* See progspace.h.  */
+
+void
+program_space::set_cbfd_soname_build_id (std::string soname,
+					 const bfd_build_id *build_id)
+{
+  cbfd_soname_to_build_id[std::move (soname)] = build_id_to_string (build_id);
+}
+
+/* See progspace.h.  */
+
+const char *
+program_space::get_cbfd_soname_build_id (const char *soname)
+{
+  gdb_assert (soname);
+
+  auto it = cbfd_soname_to_build_id.find (basename (soname));
+  if (it == cbfd_soname_to_build_id.end ())
+    return nullptr;
+
+  return it->second.c_str ();
+}
+
+/* See progspace.h.  */
+
+void
+program_space::clear_cbfd_soname_build_ids ()
+{
+  cbfd_soname_to_build_id.clear ();
+}
+
 /* Boolean test for an already-known program space id.  */
 
 static int
diff --git a/gdb/progspace.h b/gdb/progspace.h
index fb348ca7539..b42b3ffc4f1 100644
--- a/gdb/progspace.h
+++ b/gdb/progspace.h
@@ -30,6 +30,7 @@
 #include "gdbsupport/safe-iterator.h"
 #include <list>
 #include <vector>
+#include <unordered_map>
 
 struct target_ops;
 struct bfd;
@@ -324,6 +325,19 @@ struct program_space
   /* Binary file diddling handle for the core file.  */
   gdb_bfd_ref_ptr cbfd;
 
+  /* 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 ();
+
   /* The address space attached to this program space.  More than one
      program space may be bound to the same address space.  In the
      traditional unix-like debugging scenario, this will usually
@@ -378,6 +392,9 @@ struct program_space
   /* The set of target sections matching the sections mapped into
      this program space.  Managed by both exec_ops and solib.c.  */
   target_section_table m_target_sections;
+
+  /* Mapping of a core file's library sonames to their respective build-ids.  */
+  std::unordered_map<std::string, std::string> cbfd_soname_to_build_id;
 };
 
 /* An address space.  It is used for comparing if
diff --git a/gdb/solib.c b/gdb/solib.c
index e30affbb7e7..1b99c8ab985 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"
@@ -1585,6 +1586,40 @@ gdb_bfd_scan_elf_dyntag (const int desired_dyntag, bfd *abfd, CORE_ADDR *ptr,
   return 0;
 }
 
+/* See solib.h.  */
+
+gdb::optional<std::string>
+gdb_bfd_read_elf_soname (struct bfd *bfd)
+{
+  gdb_assert (bfd != nullptr);
+
+  gdb_bfd_ref_ptr abfd = gdb_bfd_open (bfd->filename, gnutarget);
+
+  if (abfd == nullptr)
+    return {};
+
+  /* Check that bfd is an ET_DYN ELF file.  */
+  bfd_check_format (abfd.get (), bfd_object);
+  if (!(bfd_get_file_flags (abfd.get ()) & DYNAMIC))
+    return {};
+
+  /* Determine soname of shared library.  If found map soname to build-id.  */
+  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 std::string ((const 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 c50f74e06bf..51cc047463f 100644
--- a/gdb/solib.h
+++ b/gdb/solib.h
@@ -118,6 +118,11 @@ 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 BFD is an ELF shared object then attempt to return the string
+   referred to by its DT_SONAME tag.   */
+
+extern gdb::optional<std::string> gdb_bfd_read_elf_soname (struct bfd *bfd);
+
 /* Enable or disable optional solib event breakpoints as appropriate.  */
 
 extern void update_solib_breakpoints (void);
-- 
2.31.1