* [PATCH v3 0/3] Add debuginfod core file support
@ 2021-08-12 4:24 Aaron Merey
2021-08-12 4:24 ` [PATCH 1/3] gdb/solib: Refactor scan_dyntag Aaron Merey
` (2 more replies)
0 siblings, 3 replies; 25+ messages in thread
From: Aaron Merey @ 2021-08-12 4:24 UTC (permalink / raw)
To: gdb-patches, simon.marchi, tom
Hello,
This patch series has been updated based on feedback from
https://sourceware.org/pipermail/gdb-patches/2021-July/180956.html
I want to address some points from Simon's review.
> Instead of duplicating scan_dyntag, can we please extract a common
> base? It is some non-trivial code, so it would be really good to
> avoid having two copies.
>
> Everything until setting the dynptr looks like it could be
> commonized. The common function could work like scan_dyntag, where you
> pass a desired dyntag. But since you are actually looking for two
> values, that would require two calls, so two scans. So instead I would
> suggest making a "foreach_dyntag" function that takes a callback (a
> gdb::function_view), where you invoke the callback for each dyntag.
> The rest could then easily be implemented on top of that, and dyntag
> parsing code would be at a single place.
The first patch in this series moves the multiple implementations of
scan_dyntag to solib.c so that duplication is avoided. I decided not
to add a foreach_dyntag function because the reason for having more
than one call to scan_dyntag was just to get the value of the DT_STRTAB
tag. I don't think it's really necessary to get this value because,
as far as I know, the value of DT_STRTAB always refers to the .dynstr
section, which is easily found with bfd_get_section_by_name. Unless
there are cases where DT_STRTAB doesn't refer to .dynstr, a
foreach_dyntag shouldn't be needed.
> What's the point of calling bfd_check_format without checking the
> result? It looks like a function without side-effects.
For some reason this function appears to update the flags and build-id
fields of the bfd with the correct values.
> In fact, that soname to buildid mapping doesn't seem related to
> debuginfod at all, it's just a generic soname to buildid mapping. That
> could exist and be useful even if debuginfod didn't exist, right? I'm
> not sure what is the best place. But that information is produced by
> the core target and consumed by the solib subsystem... so it should
> probably be close to one of them.
The second patch in this series implements a per-program_space soname to
build-id mapping that is independent of debuginfod.
> Not related to your patch but... would it be a good idea to show the
> build-ids in "info proc mappings"? It might sound useful to have that
> information in order to determine / find the right binaries that your
> process was using.
I think it would be useful in some circumstances. The changes to
linux_read_core_file_mappings in the second patch of this series should
make it easy to include build-ids in 'info proc mappings'.
> > @@ -536,6 +538,23 @@ 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 ()));
> >
> > + if (abfd == NULL)
> > + {
> > + gdb::unique_xmalloc_ptr<char> build_id_hex;
> > +
> > + /* If a build-id was previously associated with this soname
> > + then use it to query debuginfod for the library. */
> > + if (debuginfod_get_soname_buildid (so->so_name, &build_id_hex))
> > + {
> > + scoped_fd fd = debuginfod_exec_query
> > + ((const unsigned char*) build_id_hex.get (),
> > + 0, so->so_name, &filename);
> > +
> > + if (fd.get () >= 0)
> > + abfd = ops->bfd_open (filename.get ());
> > + }
> > + }
>
> I have a question about the order of the operations here. Let's say I
> generate a core on my ARM machine and bring it back to debug it on my
> x86-64 machine. And let's say I have a /usr/lib/foo.so on both
> machines. Will the first ops->bfd_open manage to open the one of my
> development machine (the wrong one), and abfd will be != NULL?
>
> I am not sure what happens in that case, but if we could somehow
> determine that we didn't open the right library (for example, seeing
> that the lib's build-id doesn't match the expected build-id), and
> instead try downloading the right one using debuginfod... would that
> make sense?
If the solib installed at the time of debugging has the same soname
mentioned in the core file but a different build-id, then gdb will
silently use this installed solib even though it's the wrong build.
At least this is what happens when the arch stays the same. I have
not tested this senario when using multiple architectures. The third
patch in this series prints a warning message when a build-id mismatch
happens. If debuginfod is enabled then gdb will also query debuginfod
for the correct build of the solib.
One thing I should point out is that if we have the wrong build of the
solib installed locally and the debuginfod query fails because of some
error, the user will see something like the following:
warning: Build-id of /lib64/libc.so.6 does not match core file.
Download failed: No route to host. Continuing without executable for /lib64/libc.so.6.
However gdb is continuing with libc.so.6, although it is the wrong
build. We could modify solib_map_sections so that if there is
a build-id mismatch gdb refuses to use the solib. Or we could
add a parameter to debuginfod_exec_query that replaces "Continuing
without executable..." with some other string indicating that
gdb will use the local solib despite the mismatch.
Thanks,
Aaron
Aaron Merey (3):
gdb/solib: Refactor scan_dyntag
gdb: Add soname to build-id mapping for corefiles
PR gdb/27570: missing support for debuginfod in
core_target::build_file_mappings
gdb/arch-utils.c | 21 ++-
gdb/arch-utils.h | 21 ++-
gdb/build-id.h | 2 +
gdb/corelow.c | 46 ++++-
gdb/debuginfod-support.c | 46 +++++
gdb/debuginfod-support.h | 16 ++
gdb/gcore.in | 3 +
gdb/gdbarch.c | 2 +-
gdb/gdbarch.h | 4 +-
gdb/gdbarch.sh | 2 +-
gdb/linux-tdep.c | 52 ++++--
gdb/progspace.c | 36 ++++
gdb/progspace.h | 17 ++
gdb/solib-dsbt.c | 104 +----------
gdb/solib-svr4.c | 118 +-----------
gdb/solib.c | 174 ++++++++++++++++++
gdb/solib.h | 11 ++
.../gdb.debuginfod/fetch_src_and_symbols.exp | 25 ++-
18 files changed, 451 insertions(+), 249 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 1/3] gdb/solib: Refactor scan_dyntag
2021-08-12 4:24 [PATCH v3 0/3] Add debuginfod core file support Aaron Merey
@ 2021-08-12 4:24 ` Aaron Merey
2021-08-17 13:28 ` Simon Marchi
2021-08-12 4:24 ` [PATCH 2/3] gdb: Add soname to build-id mapping for corefiles Aaron Merey
2021-08-12 4:24 ` [PATCH 3/3] PR gdb/27570: missing support for debuginfod in core_target::build_file_mappings Aaron Merey
2 siblings, 1 reply; 25+ messages in thread
From: Aaron Merey @ 2021-08-12 4:24 UTC (permalink / raw)
To: gdb-patches, simon.marchi, tom
scan_dyntag is unnecessarily duplicated in solib-svr4.c and solib-dsbt.c.
Move this function to solib.c and rename it to gdb_bfd_scan_elf_dyntag.
Also add it to solib.h so it is included in both solib-svr4 and solib-dsbt.
---
gdb/solib-dsbt.c | 104 ++---------------------------------------
gdb/solib-svr4.c | 118 ++++-------------------------------------------
gdb/solib.c | 104 +++++++++++++++++++++++++++++++++++++++++
gdb/solib.h | 6 +++
4 files changed, 122 insertions(+), 210 deletions(-)
diff --git a/gdb/solib-dsbt.c b/gdb/solib-dsbt.c
index 803467dd489..d7f4b252eee 100644
--- a/gdb/solib-dsbt.c
+++ b/gdb/solib-dsbt.c
@@ -396,106 +396,6 @@ fetch_loadmap (CORE_ADDR ldmaddr)
static void dsbt_relocate_main_executable (void);
static int enable_break (void);
-/* Scan for DYNTAG in .dynamic section of ABFD. If DYNTAG is found 1 is
- returned and the corresponding PTR is set. */
-
-static int
-scan_dyntag (int dyntag, bfd *abfd, CORE_ADDR *ptr)
-{
- int arch_size, step, sect_size;
- long dyn_tag;
- CORE_ADDR dyn_ptr, dyn_addr;
- gdb_byte *bufend, *bufstart, *buf;
- Elf32_External_Dyn *x_dynp_32;
- Elf64_External_Dyn *x_dynp_64;
- struct bfd_section *sect;
-
- if (abfd == NULL)
- return 0;
-
- if (bfd_get_flavour (abfd) != bfd_target_elf_flavour)
- return 0;
-
- arch_size = bfd_get_arch_size (abfd);
- if (arch_size == -1)
- return 0;
-
- /* Find the start address of the .dynamic section. */
- sect = bfd_get_section_by_name (abfd, ".dynamic");
- if (sect == NULL)
- return 0;
-
- bool found = false;
- for (const target_section &target_section
- : current_program_space->target_sections ())
- if (sect == target_section.the_bfd_section)
- {
- dyn_addr = target_section.addr;
- found = true;
- break;
- }
- if (!found)
- {
- /* ABFD may come from OBJFILE acting only as a symbol file without being
- loaded into the target (see add_symbol_file_command). This case is
- such fallback to the file VMA address without the possibility of
- having the section relocated to its actual in-memory address. */
-
- dyn_addr = bfd_section_vma (sect);
- }
-
- /* Read in .dynamic from the BFD. We will get the actual value
- from memory later. */
- sect_size = bfd_section_size (sect);
- buf = bufstart = (gdb_byte *) alloca (sect_size);
- if (!bfd_get_section_contents (abfd, sect,
- buf, 0, sect_size))
- return 0;
-
- /* Iterate over BUF and scan for DYNTAG. If found, set PTR and return. */
- step = (arch_size == 32) ? sizeof (Elf32_External_Dyn)
- : sizeof (Elf64_External_Dyn);
- for (bufend = buf + sect_size;
- buf < bufend;
- buf += step)
- {
- if (arch_size == 32)
- {
- x_dynp_32 = (Elf32_External_Dyn *) buf;
- dyn_tag = bfd_h_get_32 (abfd, (bfd_byte *) x_dynp_32->d_tag);
- dyn_ptr = bfd_h_get_32 (abfd, (bfd_byte *) x_dynp_32->d_un.d_ptr);
- }
- else
- {
- x_dynp_64 = (Elf64_External_Dyn *) buf;
- dyn_tag = bfd_h_get_64 (abfd, (bfd_byte *) x_dynp_64->d_tag);
- dyn_ptr = bfd_h_get_64 (abfd, (bfd_byte *) x_dynp_64->d_un.d_ptr);
- }
- if (dyn_tag == DT_NULL)
- return 0;
- if (dyn_tag == dyntag)
- {
- /* If requested, try to read the runtime value of this .dynamic
- entry. */
- if (ptr)
- {
- struct type *ptr_type;
- gdb_byte ptr_buf[8];
- CORE_ADDR ptr_addr;
-
- ptr_type = builtin_type (target_gdbarch ())->builtin_data_ptr;
- ptr_addr = dyn_addr + (buf - bufstart) + arch_size / 8;
- if (target_read_memory (ptr_addr, ptr_buf, arch_size / 8) == 0)
- dyn_ptr = extract_typed_address (ptr_buf, ptr_type);
- *ptr = dyn_ptr;
- }
- return 1;
- }
- }
-
- return 0;
-}
-
/* See solist.h. */
static int
@@ -565,7 +465,9 @@ lm_base (void)
"lm_base: get addr %x by _GLOBAL_OFFSET_TABLE_.\n",
(unsigned int) addr);
}
- else if (scan_dyntag (DT_PLTGOT, current_program_space->exec_bfd (), &addr))
+ else if (gdb_bfd_scan_elf_dyntag (DT_PLTGOT,
+ current_program_space->exec_bfd (),
+ &addr, NULL))
{
struct int_elf32_dsbt_loadmap *ldm;
diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index a8a7d1171dc..3de1bb9c7f7 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -582,109 +582,6 @@ find_program_interpreter (void)
}
-/* Scan for DESIRED_DYNTAG in .dynamic section of ABFD. If DESIRED_DYNTAG is
- found, 1 is returned and the corresponding PTR is set. */
-
-static int
-scan_dyntag (const int desired_dyntag, bfd *abfd, CORE_ADDR *ptr,
- CORE_ADDR *ptr_addr)
-{
- int arch_size, step, sect_size;
- long current_dyntag;
- CORE_ADDR dyn_ptr, dyn_addr;
- gdb_byte *bufend, *bufstart, *buf;
- Elf32_External_Dyn *x_dynp_32;
- Elf64_External_Dyn *x_dynp_64;
- struct bfd_section *sect;
-
- if (abfd == NULL)
- return 0;
-
- if (bfd_get_flavour (abfd) != bfd_target_elf_flavour)
- return 0;
-
- arch_size = bfd_get_arch_size (abfd);
- if (arch_size == -1)
- return 0;
-
- /* Find the start address of the .dynamic section. */
- sect = bfd_get_section_by_name (abfd, ".dynamic");
- if (sect == NULL)
- return 0;
-
- bool found = false;
- for (const target_section &target_section
- : current_program_space->target_sections ())
- if (sect == target_section.the_bfd_section)
- {
- dyn_addr = target_section.addr;
- found = true;
- break;
- }
- if (!found)
- {
- /* ABFD may come from OBJFILE acting only as a symbol file without being
- loaded into the target (see add_symbol_file_command). This case is
- such fallback to the file VMA address without the possibility of
- having the section relocated to its actual in-memory address. */
-
- dyn_addr = bfd_section_vma (sect);
- }
-
- /* Read in .dynamic from the BFD. We will get the actual value
- from memory later. */
- sect_size = bfd_section_size (sect);
- buf = bufstart = (gdb_byte *) alloca (sect_size);
- if (!bfd_get_section_contents (abfd, sect,
- buf, 0, sect_size))
- return 0;
-
- /* Iterate over BUF and scan for DYNTAG. If found, set PTR and return. */
- step = (arch_size == 32) ? sizeof (Elf32_External_Dyn)
- : sizeof (Elf64_External_Dyn);
- for (bufend = buf + sect_size;
- buf < bufend;
- buf += step)
- {
- if (arch_size == 32)
- {
- x_dynp_32 = (Elf32_External_Dyn *) buf;
- current_dyntag = bfd_h_get_32 (abfd, (bfd_byte *) x_dynp_32->d_tag);
- dyn_ptr = bfd_h_get_32 (abfd, (bfd_byte *) x_dynp_32->d_un.d_ptr);
- }
- else
- {
- x_dynp_64 = (Elf64_External_Dyn *) buf;
- current_dyntag = bfd_h_get_64 (abfd, (bfd_byte *) x_dynp_64->d_tag);
- dyn_ptr = bfd_h_get_64 (abfd, (bfd_byte *) x_dynp_64->d_un.d_ptr);
- }
- if (current_dyntag == DT_NULL)
- return 0;
- if (current_dyntag == desired_dyntag)
- {
- /* If requested, try to read the runtime value of this .dynamic
- entry. */
- if (ptr)
- {
- struct type *ptr_type;
- gdb_byte ptr_buf[8];
- CORE_ADDR ptr_addr_1;
-
- ptr_type = builtin_type (target_gdbarch ())->builtin_data_ptr;
- ptr_addr_1 = dyn_addr + (buf - bufstart) + arch_size / 8;
- if (target_read_memory (ptr_addr_1, ptr_buf, arch_size / 8) == 0)
- dyn_ptr = extract_typed_address (ptr_buf, ptr_type);
- *ptr = dyn_ptr;
- if (ptr_addr)
- *ptr_addr = dyn_addr + (buf - bufstart);
- }
- return 1;
- }
- }
-
- return 0;
-}
-
/* Scan for DESIRED_DYNTAG in .dynamic section of the target's main executable,
found by consulting the OS auxillary vector. If DESIRED_DYNTAG is found, 1
is returned and the corresponding PTR is set. */
@@ -768,8 +665,9 @@ elf_locate_base (void)
/* Look for DT_MIPS_RLD_MAP first. MIPS executables use this
instead of DT_DEBUG, although they sometimes contain an unused
DT_DEBUG. */
- if (scan_dyntag (DT_MIPS_RLD_MAP, current_program_space->exec_bfd (),
- &dyn_ptr, NULL)
+ if (gdb_bfd_scan_elf_dyntag (DT_MIPS_RLD_MAP,
+ current_program_space->exec_bfd (),
+ &dyn_ptr, NULL)
|| scan_dyntag_auxv (DT_MIPS_RLD_MAP, &dyn_ptr, NULL))
{
struct type *ptr_type = builtin_type (target_gdbarch ())->builtin_data_ptr;
@@ -787,8 +685,9 @@ elf_locate_base (void)
/* Then check DT_MIPS_RLD_MAP_REL. MIPS executables now use this form
because of needing to support PIE. DT_MIPS_RLD_MAP will also exist
in non-PIE. */
- if (scan_dyntag (DT_MIPS_RLD_MAP_REL, current_program_space->exec_bfd (),
- &dyn_ptr, &dyn_ptr_addr)
+ if (gdb_bfd_scan_elf_dyntag (DT_MIPS_RLD_MAP_REL,
+ current_program_space->exec_bfd (),
+ &dyn_ptr, &dyn_ptr_addr)
|| scan_dyntag_auxv (DT_MIPS_RLD_MAP_REL, &dyn_ptr, &dyn_ptr_addr))
{
struct type *ptr_type = builtin_type (target_gdbarch ())->builtin_data_ptr;
@@ -804,7 +703,8 @@ elf_locate_base (void)
}
/* Find DT_DEBUG. */
- if (scan_dyntag (DT_DEBUG, current_program_space->exec_bfd (), &dyn_ptr, NULL)
+ if (gdb_bfd_scan_elf_dyntag (DT_DEBUG, current_program_space->exec_bfd (),
+ &dyn_ptr, NULL)
|| scan_dyntag_auxv (DT_DEBUG, &dyn_ptr, NULL))
return dyn_ptr;
@@ -3258,7 +3158,7 @@ svr4_iterate_over_objfiles_in_search_order
abfd = current_objfile->obfd;
if (abfd != nullptr
- && scan_dyntag (DT_SYMBOLIC, abfd, nullptr, nullptr) == 1)
+ && gdb_bfd_scan_elf_dyntag (DT_SYMBOLIC, abfd, nullptr, nullptr) == 1)
{
checked_current_objfile = true;
if (cb (current_objfile, cb_data) != 0)
diff --git a/gdb/solib.c b/gdb/solib.c
index 317f7eb485e..e30affbb7e7 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -35,6 +35,8 @@
#include "language.h"
#include "gdbcmd.h"
#include "completer.h"
+#include "elf/external.h"
+#include "elf/common.h"
#include "filenames.h" /* for DOSish file names */
#include "exec.h"
#include "solist.h"
@@ -1481,6 +1483,108 @@ gdb_bfd_lookup_symbol_from_symtab (bfd *abfd,
return symaddr;
}
+/* See solib.h. */
+
+int
+gdb_bfd_scan_elf_dyntag (const int desired_dyntag, bfd *abfd, CORE_ADDR *ptr,
+ CORE_ADDR *ptr_addr)
+{
+ int arch_size, step, sect_size;
+ long current_dyntag;
+ CORE_ADDR dyn_ptr, dyn_addr;
+ gdb_byte *bufend, *bufstart, *buf;
+ Elf32_External_Dyn *x_dynp_32;
+ Elf64_External_Dyn *x_dynp_64;
+ struct bfd_section *sect;
+
+ if (abfd == NULL)
+ return 0;
+
+ if (bfd_get_flavour (abfd) != bfd_target_elf_flavour)
+ return 0;
+
+ arch_size = bfd_get_arch_size (abfd);
+ if (arch_size == -1)
+ return 0;
+
+ /* Find the start address of the .dynamic section. */
+ sect = bfd_get_section_by_name (abfd, ".dynamic");
+ if (sect == NULL)
+ return 0;
+
+ bool found = false;
+ for (const target_section &target_section
+ : current_program_space->target_sections ())
+ if (sect == target_section.the_bfd_section)
+ {
+ dyn_addr = target_section.addr;
+ found = true;
+ break;
+ }
+ if (!found)
+ {
+ /* ABFD may come from OBJFILE acting only as a symbol file without being
+ loaded into the target (see add_symbol_file_command). This case is
+ such fallback to the file VMA address without the possibility of
+ having the section relocated to its actual in-memory address. */
+
+ dyn_addr = bfd_section_vma (sect);
+ }
+
+ /* Read in .dynamic from the BFD. We will get the actual value
+ from memory later. */
+ sect_size = bfd_section_size (sect);
+ buf = bufstart = (gdb_byte *) alloca (sect_size);
+ if (!bfd_get_section_contents (abfd, sect,
+ buf, 0, sect_size))
+ return 0;
+
+ /* Iterate over BUF and scan for DYNTAG. If found, set PTR and return. */
+ step = (arch_size == 32) ? sizeof (Elf32_External_Dyn)
+ : sizeof (Elf64_External_Dyn);
+ for (bufend = buf + sect_size;
+ buf < bufend;
+ buf += step)
+ {
+ if (arch_size == 32)
+ {
+ x_dynp_32 = (Elf32_External_Dyn *) buf;
+ current_dyntag = bfd_h_get_32 (abfd, (bfd_byte *) x_dynp_32->d_tag);
+ dyn_ptr = bfd_h_get_32 (abfd, (bfd_byte *) x_dynp_32->d_un.d_ptr);
+ }
+ else
+ {
+ x_dynp_64 = (Elf64_External_Dyn *) buf;
+ current_dyntag = bfd_h_get_64 (abfd, (bfd_byte *) x_dynp_64->d_tag);
+ dyn_ptr = bfd_h_get_64 (abfd, (bfd_byte *) x_dynp_64->d_un.d_ptr);
+ }
+ if (current_dyntag == DT_NULL)
+ return 0;
+ if (current_dyntag == desired_dyntag)
+ {
+ /* If requested, try to read the runtime value of this .dynamic
+ entry. */
+ if (ptr)
+ {
+ struct type *ptr_type;
+ gdb_byte ptr_buf[8];
+ CORE_ADDR ptr_addr_1;
+
+ ptr_type = builtin_type (target_gdbarch ())->builtin_data_ptr;
+ ptr_addr_1 = dyn_addr + (buf - bufstart) + arch_size / 8;
+ if (target_read_memory (ptr_addr_1, ptr_buf, arch_size / 8) == 0)
+ dyn_ptr = extract_typed_address (ptr_buf, ptr_type);
+ *ptr = dyn_ptr;
+ if (ptr_addr)
+ *ptr_addr = dyn_addr + (buf - bufstart);
+ }
+ return 1;
+ }
+ }
+
+ return 0;
+}
+
/* 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 a94e9d3cd9e..c50f74e06bf 100644
--- a/gdb/solib.h
+++ b/gdb/solib.h
@@ -112,6 +112,12 @@ extern CORE_ADDR gdb_bfd_lookup_symbol_from_symtab (bfd *abfd,
const void *),
const void *data);
+/* Scan for DESIRED_DYNTAG in .dynamic section of ABFD. If DESIRED_DYNTAG is
+ found, 1 is returned and the corresponding PTR and PTR_ADDR are set. */
+
+extern int gdb_bfd_scan_elf_dyntag (const int desired_dyntag, bfd *abfd,
+ CORE_ADDR *ptr, CORE_ADDR *ptr_addr);
+
/* Enable or disable optional solib event breakpoints as appropriate. */
extern void update_solib_breakpoints (void);
--
2.31.1
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 2/3] gdb: Add soname to build-id mapping for corefiles
2021-08-12 4:24 [PATCH v3 0/3] Add debuginfod core file support Aaron Merey
2021-08-12 4:24 ` [PATCH 1/3] gdb/solib: Refactor scan_dyntag Aaron Merey
@ 2021-08-12 4:24 ` Aaron Merey
2021-08-15 14:51 ` Lancelot SIX
2021-08-12 4:24 ` [PATCH 3/3] PR gdb/27570: missing support for debuginfod in core_target::build_file_mappings Aaron Merey
2 siblings, 1 reply; 25+ messages in thread
From: Aaron Merey @ 2021-08-12 4:24 UTC (permalink / raw)
To: gdb-patches, simon.marchi, tom
Since commit aa2d5a422 gdb has been able to read executable and shared
library build-ids within core files.
Expand this functionality so that each program_space maintains a map of
sonames to build-ids for each shared library referenced in the program_space's
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/arch-utils.c | 21 +++++++++----------
gdb/arch-utils.h | 21 +++++++++----------
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 | 52 +++++++++++++++++++++++++++++++++++++-----------
gdb/progspace.c | 36 +++++++++++++++++++++++++++++++++
gdb/progspace.h | 17 ++++++++++++++++
gdb/solib.c | 35 ++++++++++++++++++++++++++++++++
gdb/solib.h | 5 +++++
12 files changed, 173 insertions(+), 37 deletions(-)
diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index 4290d637ce1..4c7497e6b4c 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -1072,16 +1072,17 @@ 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,
+ gdb::function_view<void (int num,
+ ULONGEST start,
+ ULONGEST end,
+ ULONGEST file_ofs,
+ const char *filename,
+ const bfd_build_id *build_id)>
+ loop_cb)
{
}
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index 03e9082f6d7..9139438c5fd 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -295,14 +295,15 @@ extern std::string default_get_pc_address_flags (frame_info *frame,
CORE_ADDR pc);
/* 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,
+ gdb::function_view<void (int num,
+ ULONGEST start,
+ ULONGEST end,
+ ULONGEST file_ofs,
+ const char *filename,
+ const bfd_build_id *build_id)>
+ 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..97eadceed84 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 (soname->data (),
+ 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 830a86df89f..b6472bb36d5 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 7db3e36d76a..dbd1fa0afc7 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 9bc9de91c30..56679b8fee6 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 637d3d36a0b..eb35a2b5297 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -43,6 +43,7 @@
#include "gcore-elf.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
@@ -1096,16 +1097,17 @@ 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,
+ gdb::function_view<void (int num,
+ ULONGEST start,
+ ULONGEST end,
+ ULONGEST file_ofs,
+ const char *filename,
+ const bfd_build_id *build_id)>
+ loop_cb)
{
/* Ensure that ULONGEST is big enough for reading 64-bit core files. */
gdb_static_assert (sizeof (ULONGEST) >= 8);
@@ -1174,6 +1176,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++)
@@ -1187,8 +1206,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);
}
}
@@ -1217,7 +1245,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..d39bd45fcf4 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,41 @@ 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)
+{
+ std::string build_id_hex = build_id_to_string (build_id);
+ cbfd_soname_to_build_id[soname] = build_id_hex;
+
+ return;
+}
+
+/* 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 ();
+ return;
+}
+
/* 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..8b92cf7db53 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 gdb::optional<std::string> ();
+
+ /* 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 gdb::optional<std::string> ();
+
+ /* 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 gdb::optional<std::string> ();
+
+ struct bfd_section *dynstr = bfd_get_section_by_name (abfd.get (), ".dynstr");
+ if (dynstr == nullptr)
+ return gdb::optional<std::string> ();
+
+ /* 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 gdb::optional<std::string> ();
+
+ return gdb::optional<std::string> ((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
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 3/3] PR gdb/27570: missing support for debuginfod in core_target::build_file_mappings
2021-08-12 4:24 [PATCH v3 0/3] Add debuginfod core file support Aaron Merey
2021-08-12 4:24 ` [PATCH 1/3] gdb/solib: Refactor scan_dyntag Aaron Merey
2021-08-12 4:24 ` [PATCH 2/3] gdb: Add soname to build-id mapping for corefiles Aaron Merey
@ 2021-08-12 4:24 ` Aaron Merey
2021-09-29 1:13 ` Aaron Merey
2021-11-04 1:37 ` [PATCH " Simon Marchi
2 siblings, 2 replies; 25+ messages in thread
From: Aaron Merey @ 2021-08-12 4:24 UTC (permalink / raw)
To: gdb-patches, simon.marchi, tom
Add debuginfod support to core_target::build_file_mappings and
locate_exec_from_corefile_build_id to enable the downloading of
missing core file executables and shared libraries.
Also add debuginfod support to solib_map_sections so that previously
downloaded shared libraries can be retrieved from the debuginfod
cache.
When core file shared libraries are found locally, verify
that their build-ids match the corresponding build-id found in
the core file. If there is a mismatch, print a warning and attempt
to query debuginfod for the correct build of the shared library:
warning: Build-id of /lib64/libc.so.6 does not match core file.
Downloading XY.Z MB executable for /lib64/libc.so.6
---
gdb/corelow.c | 33 +++++++++++++
gdb/debuginfod-support.c | 46 +++++++++++++++++++
gdb/debuginfod-support.h | 16 +++++++
gdb/gcore.in | 3 ++
gdb/solib.c | 35 ++++++++++++++
.../gdb.debuginfod/fetch_src_and_symbols.exp | 25 +++++++++-
6 files changed, 156 insertions(+), 2 deletions(-)
diff --git a/gdb/corelow.c b/gdb/corelow.c
index 97eadceed84..2c9ea1044b7 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);
@@ -416,6 +423,32 @@ locate_exec_from_corefile_build_id (bfd *abfd, int from_tty)
symbol_file_add_main (bfd_get_filename (execbfd.get ()),
symfile_add_flag (from_tty ? SYMFILE_VERBOSE : 0));
}
+ else
+ {
+ 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);
+ else
+ {
+ /* Successful download */
+ exec_file_attach (execpath.get (), from_tty);
+ symbol_file_add_main (execpath.get (),
+ symfile_add_flag (from_tty ?
+ SYMFILE_VERBOSE : 0));
+ }
+ }
+ }
}
/* See gdbcore.h. */
diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 2d626e335a0..04b39b57f6f 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -41,6 +41,16 @@ 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);
+}
+
#else
#include <elfutils/debuginfod.h>
@@ -194,4 +204,40 @@ 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)
+{
+ const char *urls_env_var = getenv (DEBUGINFOD_URLS_ENV_VAR);
+ if (urls_env_var == NULL || urls_env_var[0] == '\0')
+ 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);
+ scoped_fd fd (debuginfod_find_executable (c, build_id, build_id_len, &dname));
+ debuginfod_set_user_data (c, nullptr);
+
+ if (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
diff --git a/gdb/debuginfod-support.h b/gdb/debuginfod-support.h
index 5e5aab56e74..c29a07d2abe 100644
--- a/gdb/debuginfod-support.h
+++ b/gdb/debuginfod-support.h
@@ -61,4 +61,20 @@ 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 24354a79e27..25b24c3cd3d 100644
--- a/gdb/gcore.in
+++ b/gdb/gcore.in
@@ -89,6 +89,9 @@ if [ ! -f "$binary_path/@GDB_TRANSFORM_NAME@" ]; then
exit 1
fi
+# Prevent unnecessary debuginfod queries during core file generation.
+unset DEBUGINFOD_URLS
+
# Initialise return code.
rc=0
diff --git a/gdb/solib.c b/gdb/solib.c
index 8b92cf7db53..3a7bf59e64a 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"
@@ -538,6 +540,39 @@ 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 ()));
+ const char *build_id_hexstr =
+ current_program_space->get_cbfd_soname_build_id (so->so_name);
+
+ /* If we already have core file build-id information for this solib,
+ 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 != NULL)
+ {
+ bool mismatch = false;
+
+ if (abfd != NULL && abfd->build_id != NULL)
+ {
+ std::string build_id = build_id_to_string (abfd->build_id);
+ if (build_id != build_id_hexstr)
+ {
+ warning (_("Build-id of %ps does not match core file."),
+ styled_string (file_name_style.style (),
+ filename.get ()));
+ mismatch = true;
+ }
+ }
+
+ if (abfd == NULL || mismatch)
+ {
+ scoped_fd fd = debuginfod_exec_query ((const unsigned char*)
+ build_id_hexstr,
+ 0, so->so_name, &filename);
+
+ if (fd.get () >= 0)
+ abfd = ops->bfd_open (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 81d4791eb6d..79ec4e6f853 100644
--- a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
+++ b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
@@ -58,6 +58,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} {
@@ -109,8 +114,10 @@ proc write_dwarf_file {filename buildid {value 99}} {
}
}
+set corefile "corefile"
+
proc no_url { } {
- global binfile outputdir debugdir
+ global binfile corefile outputdir debugdir
setenv DEBUGINFOD_URLS ""
@@ -162,10 +169,20 @@ 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 rename -force ${binfile}2 $debugdir
+
+ clean_restart
+ gdb_test "core $corefile" ".*in ?? ().*"
}
proc local_url { } {
- global binfile outputdir db debugdir
+ global binfile corefile outputdir db debugdir
# Find an unused port
set port 7999
@@ -228,6 +245,10 @@ proc local_url { } {
gdb_test "file ${binfile}_alt.o" \
".*Reading symbols from ${binfile}_alt.o\.\.\.*" \
"file [file tail ${binfile}_alt.o]"
+
+ # gdb should now find the executable file
+ clean_restart
+ gdb_test "core $corefile" ".*return 0.*"
}
set envlist \
--
2.31.1
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/3] gdb: Add soname to build-id mapping for corefiles
2021-08-12 4:24 ` [PATCH 2/3] gdb: Add soname to build-id mapping for corefiles Aaron Merey
@ 2021-08-15 14:51 ` Lancelot SIX
2021-08-17 13:58 ` Simon Marchi
0 siblings, 1 reply; 25+ messages in thread
From: Lancelot SIX @ 2021-08-15 14:51 UTC (permalink / raw)
To: Aaron Merey; +Cc: gdb-patches, simon.marchi, tom
Hi,
I have a few comments I placed bellow.
On Thu, Aug 12, 2021 at 12:24:05AM -0400, Aaron Merey via Gdb-patches wrote:
> Since commit aa2d5a422 gdb has been able to read executable and shared
> library build-ids within core files.
>
> Expand this functionality so that each program_space maintains a map of
> sonames to build-ids for each shared library referenced in the program_space's
> 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/arch-utils.c | 21 +++++++++----------
> gdb/arch-utils.h | 21 +++++++++----------
> 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 | 52 +++++++++++++++++++++++++++++++++++++-----------
> gdb/progspace.c | 36 +++++++++++++++++++++++++++++++++
> gdb/progspace.h | 17 ++++++++++++++++
> gdb/solib.c | 35 ++++++++++++++++++++++++++++++++
> gdb/solib.h | 5 +++++
> 12 files changed, 173 insertions(+), 37 deletions(-)
>
> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
> index 4290d637ce1..4c7497e6b4c 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -1072,16 +1072,17 @@ 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,
> + gdb::function_view<void (int num,
> + ULONGEST start,
> + ULONGEST end,
> + ULONGEST file_ofs,
> + const char *filename,
> + const bfd_build_id *build_id)>
> + loop_cb)
It looks like 'loop_cb' could go on the previous line.
If the type of the function callbacks are too big, I guess it could be
possible to give them a name before declaring the function. Something
like
using loop_cb_ftype = gdb::function_view<void (...)>;
> {
> }
>
> diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
> index 03e9082f6d7..9139438c5fd 100644
> --- a/gdb/arch-utils.h
> +++ b/gdb/arch-utils.h
> @@ -295,14 +295,15 @@ extern std::string default_get_pc_address_flags (frame_info *frame,
> CORE_ADDR pc);
>
> /* 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,
> + gdb::function_view<void (int num,
> + ULONGEST start,
> + ULONGEST end,
> + ULONGEST file_ofs,
> + const char *filename,
> + const bfd_build_id *build_id)>
> + loop_cb);
loop_cb could also go up one line here.
> #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..97eadceed84 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 (soname->data (),
> + build_id);
Here, since set_cbfd_soname_build_id's first argument is a std::string,
you could just use '*soname' instead of 'soname->data ()'.
> + }
> });
>
> 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 830a86df89f..b6472bb36d5 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 7db3e36d76a..dbd1fa0afc7 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 9bc9de91c30..56679b8fee6 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 637d3d36a0b..eb35a2b5297 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -43,6 +43,7 @@
> #include "gcore-elf.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
> @@ -1096,16 +1097,17 @@ 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,
> + gdb::function_view<void (int num,
> + ULONGEST start,
> + ULONGEST end,
> + ULONGEST file_ofs,
> + const char *filename,
> + const bfd_build_id *build_id)>
> + loop_cb)
'loop_cb' could be on the line above.
> {
> /* Ensure that ULONGEST is big enough for reading 64-bit core files. */
> gdb_static_assert (sizeof (ULONGEST) >= 8);
> @@ -1174,6 +1176,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++)
> @@ -1187,8 +1206,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);
> }
> }
>
> @@ -1217,7 +1245,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..d39bd45fcf4 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,41 @@ print_program_space (struct ui_out *uiout, int requested)
> }
> }
>
> +/* See progspace.h. */
> +
> +void
> +program_space::set_cbfd_soname_build_id (std::string soname,
This parameter could be 'std::string const &' or...
> + const bfd_build_id *build_id)
> +{
> + std::string build_id_hex = build_id_to_string (build_id);
> + cbfd_soname_to_build_id[soname] = build_id_hex;
... use 'std::move (soname)' here.
I guess the more 'usual' approach would be to have the argument as a
const reference (but to be honest, the implication of calling one more
ctor and copying the soname is negligible, to say the least).
> +
> + return;
I am not sure if the GNU coding standard says something about this, but
'return;' as the last statement of a void function is redundant.
> +}
> +
> +/* See progspace.h. */
> +
> +const char *
> +program_space::get_cbfd_soname_build_id (const char *soname)
With set_cbfd_soname_build_id using a std::string, I would find it more
consistent to use std::string here also. Any reason not to use it I
missed?
You could use 'basename (soname.c_str ())' bellow.
The return type could also be 'const std::string *' (the map stores
std::string internally), but keeping a const char * is pretty similar.
> +{
> + 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 ();
> + return;
Same here, I guess 'return;' could be removed.
> +}
> +
> /* 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..8b92cf7db53 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 gdb::optional<std::string> ();
> +
> + /* 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 gdb::optional<std::string> ();
> +
> + /* 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 gdb::optional<std::string> ();
> +
> + struct bfd_section *dynstr = bfd_get_section_by_name (abfd.get (), ".dynstr");
> + if (dynstr == nullptr)
> + return gdb::optional<std::string> ();
> +
> + /* 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 gdb::optional<std::string> ();
> +
> + return gdb::optional<std::string> ((char *)dynstr_buf.data () + idx);
This will not change much, but you could cast to 'const char *' (this
is the type the std::string constructor expects).
> +}
> +
> /* 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
>
I hope the comments are helpful.
Best,
Lancelot.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] gdb/solib: Refactor scan_dyntag
2021-08-12 4:24 ` [PATCH 1/3] gdb/solib: Refactor scan_dyntag Aaron Merey
@ 2021-08-17 13:28 ` Simon Marchi
2021-08-19 2:30 ` Aaron Merey
0 siblings, 1 reply; 25+ messages in thread
From: Simon Marchi @ 2021-08-17 13:28 UTC (permalink / raw)
To: Aaron Merey, gdb-patches, tom
On 2021-08-12 12:24 a.m., Aaron Merey wrote:
> scan_dyntag is unnecessarily duplicated in solib-svr4.c and solib-dsbt.c.
>
> Move this function to solib.c and rename it to gdb_bfd_scan_elf_dyntag.
> Also add it to solib.h so it is included in both solib-svr4 and solib-dsbt.
Thanks, this is OK, and I think it can be merged on its own.
Simon
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/3] gdb: Add soname to build-id mapping for corefiles
2021-08-15 14:51 ` Lancelot SIX
@ 2021-08-17 13:58 ` Simon Marchi
2021-08-19 2:22 ` Aaron Merey
0 siblings, 1 reply; 25+ messages in thread
From: Simon Marchi @ 2021-08-17 13:58 UTC (permalink / raw)
To: Lancelot SIX, Aaron Merey; +Cc: gdb-patches, tom
I did not do a full review, since Lancelot's comments were already a
good start. I just added some complements to his comments.
>> /* 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,
>> + gdb::function_view<void (int num,
>> + ULONGEST start,
>> + ULONGEST end,
>> + ULONGEST file_ofs,
>> + const char *filename,
>> + const bfd_build_id *build_id)>
>> + loop_cb)
>
> It looks like 'loop_cb' could go on the previous line.
>
> If the type of the function callbacks are too big, I guess it could be
> possible to give them a name before declaring the function. Something
> like
>
> using loop_cb_ftype = gdb::function_view<void (...)>;
Agreed, and giving a name to the function type helps readability too.
>> @@ -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 (soname->data (),
>> + build_id);
>
> Here, since set_cbfd_soname_build_id's first argument is a std::string,
> you could just use '*soname' instead of 'soname->data ()'.
Since the caller (here) doesn't need to keep the string, I would suggest
`std::move (*soname)` (see below).
>> @@ -358,6 +359,41 @@ print_program_space (struct ui_out *uiout, int requested)
>> }
>> }
>>
>> +/* See progspace.h. */
>> +
>> +void
>> +program_space::set_cbfd_soname_build_id (std::string soname,
>
> This parameter could be 'std::string const &' or...
>
>> + const bfd_build_id *build_id)
>> +{
>> + std::string build_id_hex = build_id_to_string (build_id);
>> + cbfd_soname_to_build_id[soname] = build_id_hex;
>
> ... use 'std::move (soname)' here.
>
> I guess the more 'usual' approach would be to have the argument as a
> const reference (but to be honest, the implication of calling one more
> ctor and copying the soname is negligible, to say the least).
Since set_cbfd_soname_build_id needs to keep its own copy of the object,
I think it's better to take it by value like this, and have the caller
std::move its copy if it doesn't need to keep it around. And have
set_cbfd_soname_build_id std::move it into the map:
cbfd_soname_to_build_id[std::move (soname)] = build_id_hex;
This way, soname is never copied if it doesn't need to. Taking by
const-reference means that set_cbfd_soname_build_id will have no choice
but to copy the string into the map.
And in fact, I would suggest doing:
cbfd_soname_to_build_id[std::move (soname)]
= build_id_to_string (build_id);
... to avoid copying the build_id_hex string. Alternatively, you could
do:
std::string build_id_hex = build_id_to_string (build_id);
cbfd_soname_to_build_id[std::move (soname)] = std::move (build_id_hex);
... but I think the first version is fine.
>> +
>> + return;
>
> I am not sure if the GNU coding standard says something about this, but
> 'return;' as the last statement of a void function is redundant.
Indeed.
>> +}
>> +
>> +/* See progspace.h. */
>> +
>> +const char *
>> +program_space::get_cbfd_soname_build_id (const char *soname)
>
> With set_cbfd_soname_build_id using a std::string, I would find it more
> consistent to use std::string here also. Any reason not to use it I
> missed?
>
> You could use 'basename (soname.c_str ())' bellow.
>
> The return type could also be 'const std::string *' (the map stores
> std::string internally), but keeping a const char * is pretty similar.
We wouldn't want to return an std::string, that would make an
unnecessary copy. If returning a `const char *` doesn't make life
difficult for the caller, I think it's perfectly fine. Previously, I
would have suggested returning string_view, because that abstracts how
the string is stored. But that isn't right, since the string_view
interface doesn't require the view to be null-terminated (which we
usually need). We would need something like cstring_view instead:
http://open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1402r0.pdf
But in the mean time, I think `const char *` is fine.
>> +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 gdb::optional<std::string> ();
>> +
>> + /* 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.
>> + if (!(bfd_get_file_flags (abfd.get ()) & DYNAMIC))
>> + return gdb::optional<std::string> ();
>> +
>> + /* 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 gdb::optional<std::string> ();
>> +
>> + struct bfd_section *dynstr = bfd_get_section_by_name (abfd.get (), ".dynstr");
>> + if (dynstr == nullptr)
>> + return gdb::optional<std::string> ();
>> +
>> + /* 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 gdb::optional<std::string> ();
>> +
>> + return gdb::optional<std::string> ((char *)dynstr_buf.data () + idx);
Do we need bounds checking to protect against bad ELF files? What if
DT_SONAME points outside .dynstr's size? And ideally that a
null-terminator is found somewhere between idx and the end of the
section. Otherwise, I could craft a .dynstr section that does not
contain a null terminator, that would make GDB read out of bounds.
> This will not change much, but you could cast to 'const char *' (this
> is the type the std::string constructor expects).
And to shorten things up a bit, you can drop "gdb::optional":
return std::string ((const char *) dynstr_buf.data () + idx);
While at it, instead of returning "gdb::optional<std::string> ()" on
failure, you can simply "return {}".
Simon
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/3] gdb: Add soname to build-id mapping for corefiles
2021-08-17 13:58 ` Simon Marchi
@ 2021-08-19 2:22 ` Aaron Merey
2021-09-29 1:12 ` Aaron Merey
2021-11-04 1:32 ` [PATCH " Simon Marchi
0 siblings, 2 replies; 25+ messages in thread
From: Aaron Merey @ 2021-08-19 2:22 UTC (permalink / raw)
To: lsix, simon.marchi; +Cc: gdb-patches, tom
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] gdb/solib: Refactor scan_dyntag
2021-08-17 13:28 ` Simon Marchi
@ 2021-08-19 2:30 ` Aaron Merey
0 siblings, 0 replies; 25+ messages in thread
From: Aaron Merey @ 2021-08-19 2:30 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches, Tom Tromey
On Tue, Aug 17, 2021 at 9:29 AM Simon Marchi <simon.marchi@polymtl.ca> wrote:
> Thanks, this is OK, and I think it can be merged on its own.
Thanks, pushed as commit 8ddf46454aa
Aaron
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/3] gdb: Add soname to build-id mapping for corefiles
2021-08-19 2:22 ` Aaron Merey
@ 2021-09-29 1:12 ` Aaron Merey
2021-10-18 23:06 ` [PING**2][PATCH " Aaron Merey
2021-11-04 1:32 ` [PATCH " Simon Marchi
1 sibling, 1 reply; 25+ messages in thread
From: Aaron Merey @ 2021-09-29 1:12 UTC (permalink / raw)
To: Aaron Merey; +Cc: lsix, Simon Marchi, Tom Tromey, gdb-patches
Ping.
Thanks,
Aaron
On Wed, Aug 18, 2021 at 10:22 PM Aaron Merey via Gdb-patches
<gdb-patches@sourceware.org> wrote:
>
> 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
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/3] PR gdb/27570: missing support for debuginfod in core_target::build_file_mappings
2021-08-12 4:24 ` [PATCH 3/3] PR gdb/27570: missing support for debuginfod in core_target::build_file_mappings Aaron Merey
@ 2021-09-29 1:13 ` Aaron Merey
2021-10-18 23:05 ` [PING**2][PATCH " Aaron Merey
2021-11-04 1:37 ` [PATCH " Simon Marchi
1 sibling, 1 reply; 25+ messages in thread
From: Aaron Merey @ 2021-09-29 1:13 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi, Tom Tromey
Ping.
Thanks,
Aaron
On Thu, Aug 12, 2021 at 12:25 AM Aaron Merey via Gdb-patches
<gdb-patches@sourceware.org> wrote:
>
> Add debuginfod support to core_target::build_file_mappings and
> locate_exec_from_corefile_build_id to enable the downloading of
> missing core file executables and shared libraries.
>
> Also add debuginfod support to solib_map_sections so that previously
> downloaded shared libraries can be retrieved from the debuginfod
> cache.
>
> When core file shared libraries are found locally, verify
> that their build-ids match the corresponding build-id found in
> the core file. If there is a mismatch, print a warning and attempt
> to query debuginfod for the correct build of the shared library:
>
> warning: Build-id of /lib64/libc.so.6 does not match core file.
> Downloading XY.Z MB executable for /lib64/libc.so.6
> ---
> gdb/corelow.c | 33 +++++++++++++
> gdb/debuginfod-support.c | 46 +++++++++++++++++++
> gdb/debuginfod-support.h | 16 +++++++
> gdb/gcore.in | 3 ++
> gdb/solib.c | 35 ++++++++++++++
> .../gdb.debuginfod/fetch_src_and_symbols.exp | 25 +++++++++-
> 6 files changed, 156 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/corelow.c b/gdb/corelow.c
> index 97eadceed84..2c9ea1044b7 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);
> @@ -416,6 +423,32 @@ locate_exec_from_corefile_build_id (bfd *abfd, int from_tty)
> symbol_file_add_main (bfd_get_filename (execbfd.get ()),
> symfile_add_flag (from_tty ? SYMFILE_VERBOSE : 0));
> }
> + else
> + {
> + 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);
> + else
> + {
> + /* Successful download */
> + exec_file_attach (execpath.get (), from_tty);
> + symbol_file_add_main (execpath.get (),
> + symfile_add_flag (from_tty ?
> + SYMFILE_VERBOSE : 0));
> + }
> + }
> + }
> }
>
> /* See gdbcore.h. */
> diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> index 2d626e335a0..04b39b57f6f 100644
> --- a/gdb/debuginfod-support.c
> +++ b/gdb/debuginfod-support.c
> @@ -41,6 +41,16 @@ 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);
> +}
> +
> #else
> #include <elfutils/debuginfod.h>
>
> @@ -194,4 +204,40 @@ 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)
> +{
> + const char *urls_env_var = getenv (DEBUGINFOD_URLS_ENV_VAR);
> + if (urls_env_var == NULL || urls_env_var[0] == '\0')
> + 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);
> + scoped_fd fd (debuginfod_find_executable (c, build_id, build_id_len, &dname));
> + debuginfod_set_user_data (c, nullptr);
> +
> + if (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
> diff --git a/gdb/debuginfod-support.h b/gdb/debuginfod-support.h
> index 5e5aab56e74..c29a07d2abe 100644
> --- a/gdb/debuginfod-support.h
> +++ b/gdb/debuginfod-support.h
> @@ -61,4 +61,20 @@ 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 24354a79e27..25b24c3cd3d 100644
> --- a/gdb/gcore.in
> +++ b/gdb/gcore.in
> @@ -89,6 +89,9 @@ if [ ! -f "$binary_path/@GDB_TRANSFORM_NAME@" ]; then
> exit 1
> fi
>
> +# Prevent unnecessary debuginfod queries during core file generation.
> +unset DEBUGINFOD_URLS
> +
> # Initialise return code.
> rc=0
>
> diff --git a/gdb/solib.c b/gdb/solib.c
> index 8b92cf7db53..3a7bf59e64a 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"
>
> @@ -538,6 +540,39 @@ 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 ()));
> + const char *build_id_hexstr =
> + current_program_space->get_cbfd_soname_build_id (so->so_name);
> +
> + /* If we already have core file build-id information for this solib,
> + 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 != NULL)
> + {
> + bool mismatch = false;
> +
> + if (abfd != NULL && abfd->build_id != NULL)
> + {
> + std::string build_id = build_id_to_string (abfd->build_id);
> + if (build_id != build_id_hexstr)
> + {
> + warning (_("Build-id of %ps does not match core file."),
> + styled_string (file_name_style.style (),
> + filename.get ()));
> + mismatch = true;
> + }
> + }
> +
> + if (abfd == NULL || mismatch)
> + {
> + scoped_fd fd = debuginfod_exec_query ((const unsigned char*)
> + build_id_hexstr,
> + 0, so->so_name, &filename);
> +
> + if (fd.get () >= 0)
> + abfd = ops->bfd_open (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 81d4791eb6d..79ec4e6f853 100644
> --- a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
> +++ b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
> @@ -58,6 +58,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} {
> @@ -109,8 +114,10 @@ proc write_dwarf_file {filename buildid {value 99}} {
> }
> }
>
> +set corefile "corefile"
> +
> proc no_url { } {
> - global binfile outputdir debugdir
> + global binfile corefile outputdir debugdir
>
> setenv DEBUGINFOD_URLS ""
>
> @@ -162,10 +169,20 @@ 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 rename -force ${binfile}2 $debugdir
> +
> + clean_restart
> + gdb_test "core $corefile" ".*in ?? ().*"
> }
>
> proc local_url { } {
> - global binfile outputdir db debugdir
> + global binfile corefile outputdir db debugdir
>
> # Find an unused port
> set port 7999
> @@ -228,6 +245,10 @@ proc local_url { } {
> gdb_test "file ${binfile}_alt.o" \
> ".*Reading symbols from ${binfile}_alt.o\.\.\.*" \
> "file [file tail ${binfile}_alt.o]"
> +
> + # gdb should now find the executable file
> + clean_restart
> + gdb_test "core $corefile" ".*return 0.*"
> }
>
> set envlist \
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PING**2][PATCH 3/3] PR gdb/27570: missing support for debuginfod in core_target::build_file_mappings
2021-09-29 1:13 ` Aaron Merey
@ 2021-10-18 23:05 ` Aaron Merey
2021-11-03 18:11 ` [PING**3][PATCH " Aaron Merey
0 siblings, 1 reply; 25+ messages in thread
From: Aaron Merey @ 2021-10-18 23:05 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi, Tom Tromey
Ping
Thanks,
Aaron
On Tue, Sep 28, 2021 at 9:13 PM Aaron Merey <amerey@redhat.com> wrote:
>
> Ping.
>
> Thanks,
> Aaron
>
> On Thu, Aug 12, 2021 at 12:25 AM Aaron Merey via Gdb-patches
> <gdb-patches@sourceware.org> wrote:
> >
> > Add debuginfod support to core_target::build_file_mappings and
> > locate_exec_from_corefile_build_id to enable the downloading of
> > missing core file executables and shared libraries.
> >
> > Also add debuginfod support to solib_map_sections so that previously
> > downloaded shared libraries can be retrieved from the debuginfod
> > cache.
> >
> > When core file shared libraries are found locally, verify
> > that their build-ids match the corresponding build-id found in
> > the core file. If there is a mismatch, print a warning and attempt
> > to query debuginfod for the correct build of the shared library:
> >
> > warning: Build-id of /lib64/libc.so.6 does not match core file.
> > Downloading XY.Z MB executable for /lib64/libc.so.6
> > ---
> > gdb/corelow.c | 33 +++++++++++++
> > gdb/debuginfod-support.c | 46 +++++++++++++++++++
> > gdb/debuginfod-support.h | 16 +++++++
> > gdb/gcore.in | 3 ++
> > gdb/solib.c | 35 ++++++++++++++
> > .../gdb.debuginfod/fetch_src_and_symbols.exp | 25 +++++++++-
> > 6 files changed, 156 insertions(+), 2 deletions(-)
> >
> > diff --git a/gdb/corelow.c b/gdb/corelow.c
> > index 97eadceed84..2c9ea1044b7 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);
> > @@ -416,6 +423,32 @@ locate_exec_from_corefile_build_id (bfd *abfd, int from_tty)
> > symbol_file_add_main (bfd_get_filename (execbfd.get ()),
> > symfile_add_flag (from_tty ? SYMFILE_VERBOSE : 0));
> > }
> > + else
> > + {
> > + 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);
> > + else
> > + {
> > + /* Successful download */
> > + exec_file_attach (execpath.get (), from_tty);
> > + symbol_file_add_main (execpath.get (),
> > + symfile_add_flag (from_tty ?
> > + SYMFILE_VERBOSE : 0));
> > + }
> > + }
> > + }
> > }
> >
> > /* See gdbcore.h. */
> > diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> > index 2d626e335a0..04b39b57f6f 100644
> > --- a/gdb/debuginfod-support.c
> > +++ b/gdb/debuginfod-support.c
> > @@ -41,6 +41,16 @@ 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);
> > +}
> > +
> > #else
> > #include <elfutils/debuginfod.h>
> >
> > @@ -194,4 +204,40 @@ 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)
> > +{
> > + const char *urls_env_var = getenv (DEBUGINFOD_URLS_ENV_VAR);
> > + if (urls_env_var == NULL || urls_env_var[0] == '\0')
> > + 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);
> > + scoped_fd fd (debuginfod_find_executable (c, build_id, build_id_len, &dname));
> > + debuginfod_set_user_data (c, nullptr);
> > +
> > + if (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
> > diff --git a/gdb/debuginfod-support.h b/gdb/debuginfod-support.h
> > index 5e5aab56e74..c29a07d2abe 100644
> > --- a/gdb/debuginfod-support.h
> > +++ b/gdb/debuginfod-support.h
> > @@ -61,4 +61,20 @@ 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 24354a79e27..25b24c3cd3d 100644
> > --- a/gdb/gcore.in
> > +++ b/gdb/gcore.in
> > @@ -89,6 +89,9 @@ if [ ! -f "$binary_path/@GDB_TRANSFORM_NAME@" ]; then
> > exit 1
> > fi
> >
> > +# Prevent unnecessary debuginfod queries during core file generation.
> > +unset DEBUGINFOD_URLS
> > +
> > # Initialise return code.
> > rc=0
> >
> > diff --git a/gdb/solib.c b/gdb/solib.c
> > index 8b92cf7db53..3a7bf59e64a 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"
> >
> > @@ -538,6 +540,39 @@ 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 ()));
> > + const char *build_id_hexstr =
> > + current_program_space->get_cbfd_soname_build_id (so->so_name);
> > +
> > + /* If we already have core file build-id information for this solib,
> > + 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 != NULL)
> > + {
> > + bool mismatch = false;
> > +
> > + if (abfd != NULL && abfd->build_id != NULL)
> > + {
> > + std::string build_id = build_id_to_string (abfd->build_id);
> > + if (build_id != build_id_hexstr)
> > + {
> > + warning (_("Build-id of %ps does not match core file."),
> > + styled_string (file_name_style.style (),
> > + filename.get ()));
> > + mismatch = true;
> > + }
> > + }
> > +
> > + if (abfd == NULL || mismatch)
> > + {
> > + scoped_fd fd = debuginfod_exec_query ((const unsigned char*)
> > + build_id_hexstr,
> > + 0, so->so_name, &filename);
> > +
> > + if (fd.get () >= 0)
> > + abfd = ops->bfd_open (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 81d4791eb6d..79ec4e6f853 100644
> > --- a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
> > +++ b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
> > @@ -58,6 +58,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} {
> > @@ -109,8 +114,10 @@ proc write_dwarf_file {filename buildid {value 99}} {
> > }
> > }
> >
> > +set corefile "corefile"
> > +
> > proc no_url { } {
> > - global binfile outputdir debugdir
> > + global binfile corefile outputdir debugdir
> >
> > setenv DEBUGINFOD_URLS ""
> >
> > @@ -162,10 +169,20 @@ 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 rename -force ${binfile}2 $debugdir
> > +
> > + clean_restart
> > + gdb_test "core $corefile" ".*in ?? ().*"
> > }
> >
> > proc local_url { } {
> > - global binfile outputdir db debugdir
> > + global binfile corefile outputdir db debugdir
> >
> > # Find an unused port
> > set port 7999
> > @@ -228,6 +245,10 @@ proc local_url { } {
> > gdb_test "file ${binfile}_alt.o" \
> > ".*Reading symbols from ${binfile}_alt.o\.\.\.*" \
> > "file [file tail ${binfile}_alt.o]"
> > +
> > + # gdb should now find the executable file
> > + clean_restart
> > + gdb_test "core $corefile" ".*return 0.*"
> > }
> >
> > set envlist \
> > --
> > 2.31.1
> >
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PING**2][PATCH 2/3] gdb: Add soname to build-id mapping for corefiles
2021-09-29 1:12 ` Aaron Merey
@ 2021-10-18 23:06 ` Aaron Merey
2021-11-03 18:12 ` [PING**3][PATCH " Aaron Merey
0 siblings, 1 reply; 25+ messages in thread
From: Aaron Merey @ 2021-10-18 23:06 UTC (permalink / raw)
To: Aaron Merey; +Cc: lsix, Simon Marchi, Tom Tromey, gdb-patches
Ping
Thanks,
Aaron
On Tue, Sep 28, 2021 at 9:12 PM Aaron Merey <amerey@redhat.com> wrote:
>
> Ping.
>
> Thanks,
> Aaron
>
> On Wed, Aug 18, 2021 at 10:22 PM Aaron Merey via Gdb-patches
> <gdb-patches@sourceware.org> wrote:
> >
> > 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
> >
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PING**3][PATCH 3/3] PR gdb/27570: missing support for debuginfod in core_target::build_file_mappings
2021-10-18 23:05 ` [PING**2][PATCH " Aaron Merey
@ 2021-11-03 18:11 ` Aaron Merey
0 siblings, 0 replies; 25+ messages in thread
From: Aaron Merey @ 2021-11-03 18:11 UTC (permalink / raw)
To: gdb-patches
Ping
Thanks,
Aaron
On Mon, Oct 18, 2021 at 7:05 PM Aaron Merey <amerey@redhat.com> wrote:
>
> Ping
>
> Thanks,
> Aaron
>
> On Tue, Sep 28, 2021 at 9:13 PM Aaron Merey <amerey@redhat.com> wrote:
> >
> > Ping.
> >
> > Thanks,
> > Aaron
> >
> > On Thu, Aug 12, 2021 at 12:25 AM Aaron Merey via Gdb-patches
> > <gdb-patches@sourceware.org> wrote:
> > >
> > > Add debuginfod support to core_target::build_file_mappings and
> > > locate_exec_from_corefile_build_id to enable the downloading of
> > > missing core file executables and shared libraries.
> > >
> > > Also add debuginfod support to solib_map_sections so that previously
> > > downloaded shared libraries can be retrieved from the debuginfod
> > > cache.
> > >
> > > When core file shared libraries are found locally, verify
> > > that their build-ids match the corresponding build-id found in
> > > the core file. If there is a mismatch, print a warning and attempt
> > > to query debuginfod for the correct build of the shared library:
> > >
> > > warning: Build-id of /lib64/libc.so.6 does not match core file.
> > > Downloading XY.Z MB executable for /lib64/libc.so.6
> > > ---
> > > gdb/corelow.c | 33 +++++++++++++
> > > gdb/debuginfod-support.c | 46 +++++++++++++++++++
> > > gdb/debuginfod-support.h | 16 +++++++
> > > gdb/gcore.in | 3 ++
> > > gdb/solib.c | 35 ++++++++++++++
> > > .../gdb.debuginfod/fetch_src_and_symbols.exp | 25 +++++++++-
> > > 6 files changed, 156 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/gdb/corelow.c b/gdb/corelow.c
> > > index 97eadceed84..2c9ea1044b7 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);
> > > @@ -416,6 +423,32 @@ locate_exec_from_corefile_build_id (bfd *abfd, int from_tty)
> > > symbol_file_add_main (bfd_get_filename (execbfd.get ()),
> > > symfile_add_flag (from_tty ? SYMFILE_VERBOSE : 0));
> > > }
> > > + else
> > > + {
> > > + 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);
> > > + else
> > > + {
> > > + /* Successful download */
> > > + exec_file_attach (execpath.get (), from_tty);
> > > + symbol_file_add_main (execpath.get (),
> > > + symfile_add_flag (from_tty ?
> > > + SYMFILE_VERBOSE : 0));
> > > + }
> > > + }
> > > + }
> > > }
> > >
> > > /* See gdbcore.h. */
> > > diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> > > index 2d626e335a0..04b39b57f6f 100644
> > > --- a/gdb/debuginfod-support.c
> > > +++ b/gdb/debuginfod-support.c
> > > @@ -41,6 +41,16 @@ 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);
> > > +}
> > > +
> > > #else
> > > #include <elfutils/debuginfod.h>
> > >
> > > @@ -194,4 +204,40 @@ 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)
> > > +{
> > > + const char *urls_env_var = getenv (DEBUGINFOD_URLS_ENV_VAR);
> > > + if (urls_env_var == NULL || urls_env_var[0] == '\0')
> > > + 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);
> > > + scoped_fd fd (debuginfod_find_executable (c, build_id, build_id_len, &dname));
> > > + debuginfod_set_user_data (c, nullptr);
> > > +
> > > + if (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
> > > diff --git a/gdb/debuginfod-support.h b/gdb/debuginfod-support.h
> > > index 5e5aab56e74..c29a07d2abe 100644
> > > --- a/gdb/debuginfod-support.h
> > > +++ b/gdb/debuginfod-support.h
> > > @@ -61,4 +61,20 @@ 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 24354a79e27..25b24c3cd3d 100644
> > > --- a/gdb/gcore.in
> > > +++ b/gdb/gcore.in
> > > @@ -89,6 +89,9 @@ if [ ! -f "$binary_path/@GDB_TRANSFORM_NAME@" ]; then
> > > exit 1
> > > fi
> > >
> > > +# Prevent unnecessary debuginfod queries during core file generation.
> > > +unset DEBUGINFOD_URLS
> > > +
> > > # Initialise return code.
> > > rc=0
> > >
> > > diff --git a/gdb/solib.c b/gdb/solib.c
> > > index 8b92cf7db53..3a7bf59e64a 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"
> > >
> > > @@ -538,6 +540,39 @@ 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 ()));
> > > + const char *build_id_hexstr =
> > > + current_program_space->get_cbfd_soname_build_id (so->so_name);
> > > +
> > > + /* If we already have core file build-id information for this solib,
> > > + 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 != NULL)
> > > + {
> > > + bool mismatch = false;
> > > +
> > > + if (abfd != NULL && abfd->build_id != NULL)
> > > + {
> > > + std::string build_id = build_id_to_string (abfd->build_id);
> > > + if (build_id != build_id_hexstr)
> > > + {
> > > + warning (_("Build-id of %ps does not match core file."),
> > > + styled_string (file_name_style.style (),
> > > + filename.get ()));
> > > + mismatch = true;
> > > + }
> > > + }
> > > +
> > > + if (abfd == NULL || mismatch)
> > > + {
> > > + scoped_fd fd = debuginfod_exec_query ((const unsigned char*)
> > > + build_id_hexstr,
> > > + 0, so->so_name, &filename);
> > > +
> > > + if (fd.get () >= 0)
> > > + abfd = ops->bfd_open (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 81d4791eb6d..79ec4e6f853 100644
> > > --- a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
> > > +++ b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
> > > @@ -58,6 +58,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} {
> > > @@ -109,8 +114,10 @@ proc write_dwarf_file {filename buildid {value 99}} {
> > > }
> > > }
> > >
> > > +set corefile "corefile"
> > > +
> > > proc no_url { } {
> > > - global binfile outputdir debugdir
> > > + global binfile corefile outputdir debugdir
> > >
> > > setenv DEBUGINFOD_URLS ""
> > >
> > > @@ -162,10 +169,20 @@ 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 rename -force ${binfile}2 $debugdir
> > > +
> > > + clean_restart
> > > + gdb_test "core $corefile" ".*in ?? ().*"
> > > }
> > >
> > > proc local_url { } {
> > > - global binfile outputdir db debugdir
> > > + global binfile corefile outputdir db debugdir
> > >
> > > # Find an unused port
> > > set port 7999
> > > @@ -228,6 +245,10 @@ proc local_url { } {
> > > gdb_test "file ${binfile}_alt.o" \
> > > ".*Reading symbols from ${binfile}_alt.o\.\.\.*" \
> > > "file [file tail ${binfile}_alt.o]"
> > > +
> > > + # gdb should now find the executable file
> > > + clean_restart
> > > + gdb_test "core $corefile" ".*return 0.*"
> > > }
> > >
> > > set envlist \
> > > --
> > > 2.31.1
> > >
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PING**3][PATCH 2/3] gdb: Add soname to build-id mapping for corefiles
2021-10-18 23:06 ` [PING**2][PATCH " Aaron Merey
@ 2021-11-03 18:12 ` Aaron Merey
0 siblings, 0 replies; 25+ messages in thread
From: Aaron Merey @ 2021-11-03 18:12 UTC (permalink / raw)
To: gdb-patches; +Cc: lsix, Simon Marchi
Ping.
Thanks,
Aaron
On Mon, Oct 18, 2021 at 7:06 PM Aaron Merey <amerey@redhat.com> wrote:
>
> Ping
>
> Thanks,
> Aaron
>
> On Tue, Sep 28, 2021 at 9:12 PM Aaron Merey <amerey@redhat.com> wrote:
> >
> > Ping.
> >
> > Thanks,
> > Aaron
> >
> > On Wed, Aug 18, 2021 at 10:22 PM Aaron Merey via Gdb-patches
> > <gdb-patches@sourceware.org> wrote:
> > >
> > > 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
> > >
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/3] gdb: Add soname to build-id mapping for corefiles
2021-08-19 2:22 ` Aaron Merey
2021-09-29 1:12 ` Aaron Merey
@ 2021-11-04 1:32 ` Simon Marchi
1 sibling, 0 replies; 25+ messages in thread
From: Simon Marchi @ 2021-11-04 1:32 UTC (permalink / raw)
To: Aaron Merey, lsix; +Cc: gdb-patches, tom
> 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)
> {
> }
Two spaces before the parenthesis:
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)
{
}
>
> 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)>;
I think this could use a better name, especially since it's in a header
visible throughout GDB. read_core_file_mappings_loop_ftype, maybe, to
follow what we use often (ftype meaning function type). And while at
it, let's add read_core_file_mappings_pre_loop_ftype, while at it. I
wouldn't mind if adding the named types for the callbacks was done in
its own preparatory patch, it should be fairly obvious.
> 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>
Don't include defs.h here, see:
https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Include_Files
> 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
Replace the long gdb::function_view types here with the new named types,
it will help readability.
> @@ -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];
Here, use the find method of unordered_map. Using [] will cause an
insertion if the entry does not exist, which is unnecessary work.
> 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"
Move the build-id.h include below, at least below defs.h, which must be
the first one.
> @@ -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;
Prefix this member with `m_`.
Simon
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/3] PR gdb/27570: missing support for debuginfod in core_target::build_file_mappings
2021-08-12 4:24 ` [PATCH 3/3] PR gdb/27570: missing support for debuginfod in core_target::build_file_mappings Aaron Merey
2021-09-29 1:13 ` Aaron Merey
@ 2021-11-04 1:37 ` Simon Marchi
1 sibling, 0 replies; 25+ messages in thread
From: Simon Marchi @ 2021-11-04 1:37 UTC (permalink / raw)
To: Aaron Merey, gdb-patches, tom
On 2021-08-12 00:24, Aaron Merey wrote:
> Add debuginfod support to core_target::build_file_mappings and
> locate_exec_from_corefile_build_id to enable the downloading of
> missing core file executables and shared libraries.
>
> Also add debuginfod support to solib_map_sections so that previously
> downloaded shared libraries can be retrieved from the debuginfod
> cache.
>
> When core file shared libraries are found locally, verify
> that their build-ids match the corresponding build-id found in
> the core file. If there is a mismatch, print a warning and attempt
> to query debuginfod for the correct build of the shared library:
>
> warning: Build-id of /lib64/libc.so.6 does not match core file.
> Downloading XY.Z MB executable for /lib64/libc.so.6
Since this is getting a bit old, it doesn't apply anymore. Can you send
a new version of the series (including fixes to patch 2)?
Thanks
Simon
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/3] gdb: Add soname to build-id mapping for corefiles
2021-11-17 21:16 ` Aaron Merey
2022-01-26 1:40 ` Aaron Merey
@ 2022-02-17 16:01 ` Andrew Burgess
1 sibling, 0 replies; 25+ messages in thread
From: Andrew Burgess @ 2022-02-17 16:01 UTC (permalink / raw)
To: Aaron Merey; +Cc: Tom Tromey, lsix, Aaron Merey via Gdb-patches
* Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> [2021-11-17 16:16:13 -0500]:
> Hi Tom,
>
> On Wed, Nov 17, 2021 at 9:29 AM Tom Tromey <tom@tromey.com> wrote:
> > If re-opening and then calling bfd_check_format sets the flags
> > correctly, then I suppose the question is just why the call in
> > build_file_mappings isn't doing that:
> >
> > if (bfd == nullptr || !bfd_check_format (bfd, bfd_object))
> >
> > Possibly the answer is that corelow passes "binary" as the target, but
> > your code does:
> >
> > Aaron> + gdb_bfd_ref_ptr abfd = gdb_bfd_open (bfd->filename, gnutarget);
> >
> > But then I wonder why "binary" is appropriate in corelow.
>
> Yes it would be much better if "bfd" was simply initialised with the proper
> fields. Corelow treats bfds in a target-agnostic manner but this shouldn't
> preclude having accurate information in "bfd".
>
> We could force "bfd" to have a format of bfd_unknown and let
> bfd_check_format figure out the correct format and fields. This is
> basically what I do in gdb_bfd_read_elf_soname. But really there
> should be a way to just initialise "bfd" with completely accurate
> contents at the time of creation. If this doesn't exist maybe now
> is the time to add it.
As I understand it, the code in corelow is making a deliberate choice
_not_ to initialise the bfd with any of the "correct" information.
We want something that can handle both structured files, like shared
libraries, but also unstructured, raw data files that are mapped into
memory.
Once we've opened the bfd in core_target::build_file_mappings, we then
add sections to the bfd to cover those regions that were mapped into
memory.
One option would be to modify core_target::build_file_mappings so that
we first try to open the bfd using `gnutarget` as the type, and only
if that fails (returns nullptr), try opening the bfd with type
binary. I think in that way you'd find the bfd had all the data you
needed.
The other option, which I might be inclined to go with, would be to
change the API for the new gdb_bfd_read_elf_soname function, make it
instead something like:
gdb::optional<std::string>
gdb_bfd_read_elf_soname (const char *filename)
{ ... }
Then you remove the whole ugly, pull the filename from the bfd and
reopen the bfd logic, and instead, you're just opening a file as a
bfd, and reading content from it. You already have the filename
available in core_target::build_file_mappings.
I have some minor style issues which I've highlighted below. But I've
only really had a good look at the one part of the patch I discuss
above, (around gdb_bfd_read_elf_soname). I can try to look at the
rest next week, if nobody else picks this up.
Thanks,
Andrew
> From 275e69ce65aa3d5ee3b017175c73f8536a1fb559 Mon Sep 17 00:00:00 2001
> From: Aaron Merey <amerey@redhat.com>
> Date: Tue, 16 Nov 2021 17:33:52 -0500
> Subject: [PATCH 2/3] gdb: Add soname to build-id mapping for core files
>
> Since commit aa2d5a422 gdb has been able to read executable and shared
> library build-ids within core files.
>
> Expand this functionality so that each program_space maintains a map of
> soname to build-id for each shared library referenced in the program_space's
> 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/progspace.c | 32 ++++++++++++++++++++++++++++++++
> gdb/progspace.h | 18 ++++++++++++++++++
> gdb/solib.c | 35 +++++++++++++++++++++++++++++++++++
> gdb/solib.h | 5 +++++
> 6 files changed, 138 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/corelow.c b/gdb/corelow.c
> index 10942e6af01..d1256a9e99b 100644
> --- a/gdb/corelow.c
> +++ b/gdb/corelow.c
> @@ -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)
if (soname.has_value ())
> + 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/linux-tdep.c b/gdb/linux-tdep.c
> index e2cff83086a..7325ba67208 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
> @@ -1170,6 +1171,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++)
> @@ -1183,8 +1201,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. */
Two spaces after the '.'.
> + 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/progspace.c b/gdb/progspace.c
> index 7080bf8ee27..680c5ba8f0a 100644
> --- a/gdb/progspace.c
> +++ b/gdb/progspace.c
> @@ -18,6 +18,7 @@
> along with this program. If not, see <http://www.gnu.org/licenses/>. */
>
> #include "defs.h"
> +#include "build-id.h"
> #include "gdbcmd.h"
> #include "objfiles.h"
> #include "arch-utils.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)
> +{
> + m_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);
gdb_assert (soname != nullptr);
> +
> + auto it = m_cbfd_soname_to_build_id.find (basename (soname));
> + if (it == m_cbfd_soname_to_build_id.end ())
> + return nullptr;
> +
> + return it->second.c_str ();
> +}
> +
> +/* See progspace.h. */
> +
> +void
> +program_space::clear_cbfd_soname_build_ids ()
> +{
> + m_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..01af5a387e3 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,10 @@ 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 shared library sonames to their
> + respective build-ids. */
> + std::unordered_map<std::string, std::string> m_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 3947c2d1d2e..c0beea19d7b 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"
> @@ -1586,6 +1587,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);
Missing a space after the cast.
> +}
> +
> /* 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);
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/3] gdb: Add soname to build-id mapping for corefiles
2022-01-26 1:40 ` Aaron Merey
@ 2022-02-09 2:31 ` Aaron Merey
0 siblings, 0 replies; 25+ messages in thread
From: Aaron Merey @ 2022-02-09 2:31 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey, Simon Marchi, lsix
Ping ** 2
Thanks,
Aaron
On Tue, Jan 25, 2022 at 8:40 PM Aaron Merey <amerey@redhat.com> wrote:
>
> Ping
>
> Thanks,
> Aaron
>
> On Wed, Nov 17, 2021 at 4:16 PM Aaron Merey <amerey@redhat.com> wrote:
> >
> > Hi Tom,
> >
> > On Wed, Nov 17, 2021 at 9:29 AM Tom Tromey <tom@tromey.com> wrote:
> > > If re-opening and then calling bfd_check_format sets the flags
> > > correctly, then I suppose the question is just why the call in
> > > build_file_mappings isn't doing that:
> > >
> > > if (bfd == nullptr || !bfd_check_format (bfd, bfd_object))
> > >
> > > Possibly the answer is that corelow passes "binary" as the target, but
> > > your code does:
> > >
> > > Aaron> + gdb_bfd_ref_ptr abfd = gdb_bfd_open (bfd->filename, gnutarget);
> > >
> > > But then I wonder why "binary" is appropriate in corelow.
> >
> > Yes it would be much better if "bfd" was simply initialised with the proper
> > fields. Corelow treats bfds in a target-agnostic manner but this shouldn't
> > preclude having accurate information in "bfd".
> >
> > We could force "bfd" to have a format of bfd_unknown and let
> > bfd_check_format figure out the correct format and fields. This is
> > basically what I do in gdb_bfd_read_elf_soname. But really there
> > should be a way to just initialise "bfd" with completely accurate
> > contents at the time of creation. If this doesn't exist maybe now
> > is the time to add it.
> >
> > Aaron
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/3] gdb: Add soname to build-id mapping for corefiles
2021-11-17 21:16 ` Aaron Merey
@ 2022-01-26 1:40 ` Aaron Merey
2022-02-09 2:31 ` Aaron Merey
2022-02-17 16:01 ` Andrew Burgess
1 sibling, 1 reply; 25+ messages in thread
From: Aaron Merey @ 2022-01-26 1:40 UTC (permalink / raw)
To: Tom Tromey; +Cc: Aaron Merey via Gdb-patches, Simon Marchi, lsix
Ping
Thanks,
Aaron
On Wed, Nov 17, 2021 at 4:16 PM Aaron Merey <amerey@redhat.com> wrote:
>
> Hi Tom,
>
> On Wed, Nov 17, 2021 at 9:29 AM Tom Tromey <tom@tromey.com> wrote:
> > If re-opening and then calling bfd_check_format sets the flags
> > correctly, then I suppose the question is just why the call in
> > build_file_mappings isn't doing that:
> >
> > if (bfd == nullptr || !bfd_check_format (bfd, bfd_object))
> >
> > Possibly the answer is that corelow passes "binary" as the target, but
> > your code does:
> >
> > Aaron> + gdb_bfd_ref_ptr abfd = gdb_bfd_open (bfd->filename, gnutarget);
> >
> > But then I wonder why "binary" is appropriate in corelow.
>
> Yes it would be much better if "bfd" was simply initialised with the proper
> fields. Corelow treats bfds in a target-agnostic manner but this shouldn't
> preclude having accurate information in "bfd".
>
> We could force "bfd" to have a format of bfd_unknown and let
> bfd_check_format figure out the correct format and fields. This is
> basically what I do in gdb_bfd_read_elf_soname. But really there
> should be a way to just initialise "bfd" with completely accurate
> contents at the time of creation. If this doesn't exist maybe now
> is the time to add it.
>
> Aaron
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/3] gdb: Add soname to build-id mapping for corefiles
2021-11-17 14:17 ` Tom Tromey
@ 2021-11-17 21:16 ` Aaron Merey
2022-01-26 1:40 ` Aaron Merey
2022-02-17 16:01 ` Andrew Burgess
0 siblings, 2 replies; 25+ messages in thread
From: Aaron Merey @ 2021-11-17 21:16 UTC (permalink / raw)
To: Tom Tromey; +Cc: Aaron Merey via Gdb-patches, Simon Marchi, lsix
Hi Tom,
On Wed, Nov 17, 2021 at 9:29 AM Tom Tromey <tom@tromey.com> wrote:
> If re-opening and then calling bfd_check_format sets the flags
> correctly, then I suppose the question is just why the call in
> build_file_mappings isn't doing that:
>
> if (bfd == nullptr || !bfd_check_format (bfd, bfd_object))
>
> Possibly the answer is that corelow passes "binary" as the target, but
> your code does:
>
> Aaron> + gdb_bfd_ref_ptr abfd = gdb_bfd_open (bfd->filename, gnutarget);
>
> But then I wonder why "binary" is appropriate in corelow.
Yes it would be much better if "bfd" was simply initialised with the proper
fields. Corelow treats bfds in a target-agnostic manner but this shouldn't
preclude having accurate information in "bfd".
We could force "bfd" to have a format of bfd_unknown and let
bfd_check_format figure out the correct format and fields. This is
basically what I do in gdb_bfd_read_elf_soname. But really there
should be a way to just initialise "bfd" with completely accurate
contents at the time of creation. If this doesn't exist maybe now
is the time to add it.
Aaron
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/3] gdb: Add soname to build-id mapping for corefiles
2021-11-17 3:24 ` Aaron Merey
@ 2021-11-17 14:17 ` Tom Tromey
2021-11-17 21:16 ` Aaron Merey
0 siblings, 1 reply; 25+ messages in thread
From: Tom Tromey @ 2021-11-17 14:17 UTC (permalink / raw)
To: Aaron Merey via Gdb-patches; +Cc: simon.marchi, Aaron Merey, tom, lsix
>>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
>> Can you explain why we are opening bfd->filename? Doesn't "abfd"
>> represent the exact same thing than "bfd"? So why can't we use "bfd"
>> directly to look up the soname?
Aaron> bfd->flags does not contain the correct value. This causes the check
Aaron> for ET_DYN to fail even for shared libraries. Also bfd->build_id always
Aaron> seems to be NULL. Opening "abfd" with gdb_bfd_open and passing it to
Aaron> bfd_check_format populates flags and build_id with the correct values.
Aaron> Passing "bfd" to bfd_check_format still does not populate these fields
Aaron> with the correct values.
Aaron> It looks like this happens because the format of "abfd" is initially
Aaron> set to bfd_unknown. In this case bfd_check_format rereads the file and
Aaron> updates some fields with the correct values, including flags and build_id.
Aaron> Since format of "bfd" is instead bfd_object, bfd_check_format returns
Aaron> early and skips updating any fields.
This seems strange to me.
At first I was surprised that re-opening the BFD like this wouldn't
simply return the same BFD, since gdb uses a BFD cache. However, now I
see that corelow isn't using the cache:
bfd = bfd_map[filename] = bfd_openr (expanded_fname.get (),
"binary");
(as opposed to using gdb_bfd_openr here.)
If re-opening and then calling bfd_check_format sets the flags
correctly, then I suppose the question is just why the call in
build_file_mappings isn't doing that:
if (bfd == nullptr || !bfd_check_format (bfd, bfd_object))
Possibly the answer is that corelow passes "binary" as the target, but
your code does:
Aaron> + gdb_bfd_ref_ptr abfd = gdb_bfd_open (bfd->filename, gnutarget);
But then I wonder why "binary" is appropriate in corelow.
Tom
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/3] gdb: Add soname to build-id mapping for corefiles
2021-11-14 2:36 ` Simon Marchi
@ 2021-11-17 3:24 ` Aaron Merey
2021-11-17 14:17 ` Tom Tromey
0 siblings, 1 reply; 25+ messages in thread
From: Aaron Merey @ 2021-11-17 3:24 UTC (permalink / raw)
To: simon.marchi; +Cc: gdb-patches, tom, lsix
Hi Simon,
On Sat, Nov 13, 2021 at 9:37 PM Simon Marchi <simon.marchi@polymtl.ca> wrote:
> > + filename_map[filename] = build_id = vma_map_it->second;
>
> Please avoid doing two assignments in one statement.
Fixed.
> > +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);
>
> Can you explain why we are opening bfd->filename? Doesn't "abfd"
> represent the exact same thing than "bfd"? So why can't we use "bfd"
> directly to look up the soname?
bfd->flags does not contain the correct value. This causes the check
for ET_DYN to fail even for shared libraries. Also bfd->build_id always
seems to be NULL. Opening "abfd" with gdb_bfd_open and passing it to
bfd_check_format populates flags and build_id with the correct values.
Passing "bfd" to bfd_check_format still does not populate these fields
with the correct values.
It looks like this happens because the format of "abfd" is initially
set to bfd_unknown. In this case bfd_check_format rereads the file and
updates some fields with the correct values, including flags and build_id.
Since format of "bfd" is instead bfd_object, bfd_check_format returns
early and skips updating any fields.
Thanks,
Aaron
From 275e69ce65aa3d5ee3b017175c73f8536a1fb559 Mon Sep 17 00:00:00 2001
From: Aaron Merey <amerey@redhat.com>
Date: Tue, 16 Nov 2021 17:33:52 -0500
Subject: [PATCH 2/3] gdb: Add soname to build-id mapping for core files
Since commit aa2d5a422 gdb has been able to read executable and shared
library build-ids within core files.
Expand this functionality so that each program_space maintains a map of
soname to build-id for each shared library referenced in the program_space's
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/progspace.c | 32 ++++++++++++++++++++++++++++++++
gdb/progspace.h | 18 ++++++++++++++++++
gdb/solib.c | 35 +++++++++++++++++++++++++++++++++++
gdb/solib.h | 5 +++++
6 files changed, 138 insertions(+), 1 deletion(-)
diff --git a/gdb/corelow.c b/gdb/corelow.c
index 10942e6af01..d1256a9e99b 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -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/linux-tdep.c b/gdb/linux-tdep.c
index e2cff83086a..7325ba67208 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
@@ -1170,6 +1171,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++)
@@ -1183,8 +1201,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/progspace.c b/gdb/progspace.c
index 7080bf8ee27..680c5ba8f0a 100644
--- a/gdb/progspace.c
+++ b/gdb/progspace.c
@@ -18,6 +18,7 @@
along with this program. If not, see <http://www.gnu.org/licenses/>. */
#include "defs.h"
+#include "build-id.h"
#include "gdbcmd.h"
#include "objfiles.h"
#include "arch-utils.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)
+{
+ m_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 = m_cbfd_soname_to_build_id.find (basename (soname));
+ if (it == m_cbfd_soname_to_build_id.end ())
+ return nullptr;
+
+ return it->second.c_str ();
+}
+
+/* See progspace.h. */
+
+void
+program_space::clear_cbfd_soname_build_ids ()
+{
+ m_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..01af5a387e3 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,10 @@ 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 shared library sonames to their
+ respective build-ids. */
+ std::unordered_map<std::string, std::string> m_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 3947c2d1d2e..c0beea19d7b 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"
@@ -1586,6 +1587,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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/3] gdb: Add soname to build-id mapping for corefiles
2021-11-10 1:47 ` [PATCH 2/3] gdb: Add soname to build-id mapping for corefiles Aaron Merey
@ 2021-11-14 2:36 ` Simon Marchi
2021-11-17 3:24 ` Aaron Merey
0 siblings, 1 reply; 25+ messages in thread
From: Simon Marchi @ 2021-11-14 2:36 UTC (permalink / raw)
To: Aaron Merey, gdb-patches; +Cc: tom, lsix
> @@ -1183,8 +1201,23 @@ 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 ())
> + filename_map[filename] = build_id = vma_map_it->second;
Please avoid doing two assignments in one statement.
> @@ -1586,6 +1587,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);
Can you explain why we are opening bfd->filename? Doesn't "abfd"
represent the exact same thing than "bfd"? So why can't we use "bfd"
directly to look up the soname?
Simon
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 2/3] gdb: Add soname to build-id mapping for corefiles
2021-11-10 1:47 [PATCH v4 0/3] Add debuginfod core file support Aaron Merey
@ 2021-11-10 1:47 ` Aaron Merey
2021-11-14 2:36 ` Simon Marchi
0 siblings, 1 reply; 25+ messages in thread
From: Aaron Merey @ 2021-11-10 1:47 UTC (permalink / raw)
To: gdb-patches; +Cc: simon.marchi, tom, lsix
Since commit aa2d5a422 gdb has been able to read executable and shared
library build-ids within core files.
Expand this functionality so that each program_space maintains a map of
soname to build-id for each shared library referenced in the program_space's
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 | 35 ++++++++++++++++++++++++++++++++++-
gdb/progspace.c | 32 ++++++++++++++++++++++++++++++++
gdb/progspace.h | 18 ++++++++++++++++++
gdb/solib.c | 35 +++++++++++++++++++++++++++++++++++
gdb/solib.h | 5 +++++
6 files changed, 135 insertions(+), 1 deletion(-)
diff --git a/gdb/corelow.c b/gdb/corelow.c
index 10942e6af01..d1256a9e99b 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -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/linux-tdep.c b/gdb/linux-tdep.c
index e2cff83086a..a86ede6623e 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
@@ -1170,6 +1171,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++)
@@ -1183,8 +1201,23 @@ 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 ())
+ filename_map[filename] = build_id = vma_map_it->second;
+ 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/progspace.c b/gdb/progspace.c
index 7080bf8ee27..680c5ba8f0a 100644
--- a/gdb/progspace.c
+++ b/gdb/progspace.c
@@ -18,6 +18,7 @@
along with this program. If not, see <http://www.gnu.org/licenses/>. */
#include "defs.h"
+#include "build-id.h"
#include "gdbcmd.h"
#include "objfiles.h"
#include "arch-utils.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)
+{
+ m_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 = m_cbfd_soname_to_build_id.find (basename (soname));
+ if (it == m_cbfd_soname_to_build_id.end ())
+ return nullptr;
+
+ return it->second.c_str ();
+}
+
+/* See progspace.h. */
+
+void
+program_space::clear_cbfd_soname_build_ids ()
+{
+ m_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..01af5a387e3 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,10 @@ 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 shared library sonames to their
+ respective build-ids. */
+ std::unordered_map<std::string, std::string> m_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 3947c2d1d2e..c0beea19d7b 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"
@@ -1586,6 +1587,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
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2022-02-17 16:01 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-12 4:24 [PATCH v3 0/3] Add debuginfod core file support Aaron Merey
2021-08-12 4:24 ` [PATCH 1/3] gdb/solib: Refactor scan_dyntag Aaron Merey
2021-08-17 13:28 ` Simon Marchi
2021-08-19 2:30 ` Aaron Merey
2021-08-12 4:24 ` [PATCH 2/3] gdb: Add soname to build-id mapping for corefiles Aaron Merey
2021-08-15 14:51 ` Lancelot SIX
2021-08-17 13:58 ` Simon Marchi
2021-08-19 2:22 ` Aaron Merey
2021-09-29 1:12 ` Aaron Merey
2021-10-18 23:06 ` [PING**2][PATCH " Aaron Merey
2021-11-03 18:12 ` [PING**3][PATCH " Aaron Merey
2021-11-04 1:32 ` [PATCH " Simon Marchi
2021-08-12 4:24 ` [PATCH 3/3] PR gdb/27570: missing support for debuginfod in core_target::build_file_mappings Aaron Merey
2021-09-29 1:13 ` Aaron Merey
2021-10-18 23:05 ` [PING**2][PATCH " Aaron Merey
2021-11-03 18:11 ` [PING**3][PATCH " Aaron Merey
2021-11-04 1:37 ` [PATCH " Simon Marchi
2021-11-10 1:47 [PATCH v4 0/3] Add debuginfod core file support Aaron Merey
2021-11-10 1:47 ` [PATCH 2/3] gdb: Add soname to build-id mapping for corefiles Aaron Merey
2021-11-14 2:36 ` Simon Marchi
2021-11-17 3:24 ` Aaron Merey
2021-11-17 14:17 ` Tom Tromey
2021-11-17 21:16 ` Aaron Merey
2022-01-26 1:40 ` Aaron Merey
2022-02-09 2:31 ` Aaron Merey
2022-02-17 16:01 ` Andrew Burgess
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).