[PATCH v2] [PR gdb/27570] missing support for debuginfod in core_target::build_file_mappings
Simon Marchi
simon.marchi@polymtl.ca
Thu Jul 15 02:21:31 GMT 2021
> We populate a map of sonames to build-ids during core_target::build_file_mappings
> by using the new function debuginfod_set_soname_buildid. The filenames used in
> solib_map_sections originate from each library's DT_SONAME tag in its .dynamic
> segment so we added another function scan_soname_tag (which is modelled after
> solib-svr4.c:scan_dyntag) that will lookup the value of DT_SONAME in the dynamic
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.
> string table segment of the library. This name gets mapped to the corresponding
> build-id. Then using the new function debuginfod_get_soname_buildid we can
> retrieve the desired build-id in solib_map_sections and check the debuginfod
> cache in case the library can't otherwise be found.
>
> Tested on Fedora 33 x86_64.
>
> gdb/ChangeLog:
>
> PR gdb/27570
> * arch-utils.c (default_read_core_file_mappings): Add build_id
> parameter.
> * arch-utils.h (default_read_core_file_mappings): Add build_id
> parameter.
> * corelow.c (core_target::build_file_mappings): Add debuginfod
> executable query and map library soname to build-id.
> (core_target::close): Call debuginfod_clear_soname_buildids.
> (locate_exec_from_corefile_build_id): Add debuginfod executable query
> * debuginfod-support.c (debuginfod_exec_query): New function.
> (debuginfod_set_soname_buildid): New function.
> (debuginfod_get_soname_buildid): New function.
> (debuginfod_clear_soname_buildids): New function.
> (scan_soname_tag): New function.
> * debuginfod-support.h (debuginfod_exec_query): New function.
> (debuginfod_set_soname_buildid): New function.
> (debuginfod_get_soname_buildid): New function.
> (debuginfod_clear_soname_buildids): New function.
> * gcore.in: unset $DEBUGINFOD_URLS.
> * gdbarch.c: Regenerate.
> * gdbarch.h: Regenerate.
> * gdbarch.sh (read_core_file_mappings): Add build_id parameter.
> * linux-tdep.c (linux_read_core_file_mappings): Map shared library
> filenames in core file mappings to build-id.
> (linux_core_info_proc_mappings): Add build-id parameter.
> * solib.c (solib_map_sections): Add debuginfod executable query.
>
> gdb/testsuite/ChangeLog:
>
> PR gdb/27570
> * gdb.debuginfod/fetch_src_and_symbols.exp: Add new testcases.
Note that you can now get rid of the ChangeLog entries (instead of
updating them).
> ---
> gdb/arch-utils.c | 4 +-
> gdb/arch-utils.h | 4 +-
> gdb/corelow.c | 40 +++-
> gdb/debuginfod-support.c | 210 ++++++++++++++++++
> gdb/debuginfod-support.h | 41 ++++
> gdb/gcore.in | 3 +
> gdb/gdbarch.c | 2 +-
> gdb/gdbarch.h | 4 +-
> gdb/gdbarch.sh | 2 +-
> gdb/linux-tdep.c | 34 ++-
> gdb/solib.c | 19 ++
> .../gdb.debuginfod/fetch_src_and_symbols.exp | 25 ++-
> 12 files changed, 376 insertions(+), 12 deletions(-)
>
> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
> index c75a79757ea..44ffaa2c2c3 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -1104,7 +1104,9 @@ default_read_core_file_mappings (struct gdbarch *gdbarch,
> ULONGEST start,
> ULONGEST end,
> ULONGEST file_ofs,
> - const char *filename)>
> + const char *filename,
> + const bfd_build_id
> + *build_id)>
> loop_cb)
When it gets unreasonably crammed on the right border like that, I
suggest doing like this:
extern void default_read_core_file_mappings
(struct gdbarch *gdbarch,
struct bfd *cbfd,
...
That gives plenty of space for the parameters.
> @@ -194,4 +227,181 @@ 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"),
This line is > 80 columns. There are a few others in the patch.
> + safe_strerror (-fd.get ()),
> + styled_string (file_name_style.style (), filename));
> +
> + if (fd.get () >= 0)
> + destname->reset (dname);
> +
> + return fd;
> +}
> +
> +/* Attempt to locate the DT_SONAME tag within ABFD's .dynamic section.
> + If found copy the string referred to by the tag to SONAME and return 1.
> + Otherwise return 0. */
> +
> +static int
> +scan_soname_tag (struct bfd *abfd, std::string &soname)
Instead of returning 1 / 0 for work / did not work and returning the
soname by parameter, I suggest returning a gdb::optional<std::string>.
This way, both informations (whether it worked and if it did, the value)
are encoded in the return value, it's more straightforward.
> +{
> + if (abfd == nullptr)
> + return 0;
If abfd being nullptr is a programmer error, then use:
gdb_assert (abfd != nullptr);
> +
> + if (bfd_get_flavour (abfd) != bfd_target_elf_flavour)
> + return 0;
This was already checked at the level above, so if you want to keep it,
then make it an assert:
gdb_assert (bfd_get_flavour (abfd) == bfd_target_elf_flavour);
> +
> + int arch_size = bfd_get_arch_size (abfd);
> + if (arch_size == -1)
> + return 0;
> +
> + /* Find the start address of the .dynamic section. */
> + struct bfd_section *sect = bfd_get_section_by_name (abfd, ".dynamic");
> + if (sect == nullptr)
> + return 0;
> +
> + /* Read in .dynamic from the BFD. */
> + int sect_size = bfd_section_size (sect);
> + gdb::byte_vector bufstart (sect_size);
> +
> + if (!bfd_get_section_contents (abfd, sect, bufstart.data (), 0, sect_size))
> + return 0;
You could use gdb_bfd_get_full_section_contents.
Side-note: I did write gdb_bfd_get_full_section_contents, and now I
wonder why I made it return a bool instead of
gdb::optional<gdb::byte_vector>... it would be much nicer to return an
optional :).
> +
> + gdb_byte *buf = bufstart.data ();
> + struct bfd_section *dynstr = nullptr;
> + CORE_ADDR soname_idx = 0;
> + int step = (arch_size == 32) ? sizeof (Elf32_External_Dyn)
> + : sizeof (Elf64_External_Dyn);
> +
> + /* Iterate over BUF and search for DT_SONAME and DT_STRTAB. */
> + for (gdb_byte *bufend = buf + sect_size;
> + (buf < bufend) && (soname_idx == 0 || dynstr == nullptr);
> + buf += step)
> + {
> + long current_dyntag;
> + CORE_ADDR dyn_ptr;
> +
> + if (arch_size == 32)
> + {
> + Elf32_External_Dyn *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
> + {
> + Elf64_External_Dyn *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)
> + buf = bufend;
> + else if (current_dyntag == DT_SONAME)
> + soname_idx = dyn_ptr;
> + else if (current_dyntag == DT_STRTAB)
> + for (struct bfd_section *s = abfd->sections; s != nullptr; s = s->next)
> + if (s->vma == dyn_ptr)
> + dynstr = s;
> + }
> +
> + if (soname_idx == 0 || dynstr == nullptr)
> + return 0;
> +
> + sect_size = bfd_section_size (dynstr);
> + if (sect_size <= soname_idx)
> + return 0;
> +
> + /* Read the soname from the string table. */
> + gdb::char_vector dynstr_buf (sect_size);
> + if (!bfd_get_section_contents (abfd, dynstr, dynstr_buf.data (), 0, sect_size))
Use gdb::byte_vector. But like above, you can use
gdb_bfd_get_full_section_contents.
> + return 0;
> +
> + soname = dynstr_buf.data () + soname_idx;
> + return 1;
> +}
> +
> +/* See debuginfod-support.h */
> +
> +int
> +debuginfod_set_soname_buildid (struct bfd *bfd, const bfd_build_id *build_id)
There's one caller of this and it doesn't care about the return value,
so make this function return void.
> +{
> + if (bfd == nullptr || build_id == nullptr)
> + return 0;
Is there a legitimate use case to passing a nullptr bfd or build_id? I
would prefer if it were:
gdb_assert (bfd != nullptr);
gdb_assert (build_id != nullptr);
> +
> + gdb_bfd_ref_ptr abfd (gdb_bfd_open (bfd->filename, gnutarget));
A nit, but we prefer the style:
gdb_bfd_ref_ptr abfd = gdb_bfd_open (bfd->filename, gnutarget);
(same when using unique_ptrs)
> +
> + if (abfd == nullptr)
> + return 0;
> +
> + /* Check that bfd is an ET_DYN ELF file. */
> + bfd_check_format (abfd.get (), bfd_object);
What's the point of calling bfd_check_format without checking the
result? It looks like a function without side-effects.
> + if (bfd_get_flavour (abfd.get ()) != bfd_target_elf_flavour
> + || !(bfd_get_file_flags (abfd.get ()) & DYNAMIC))
> + return 0;
> +
> + std::string soname;
> + std::string build_id_hex;
Move the declaration of build_id_hex to the scope where it's used, so we
don't construct/destroy it if we don't need it.
> +
> + /* Determine soname of shared library. If found map soname to build-id. */
> + if (scan_soname_tag (abfd.get (), soname))
> + {
> + for (int i = 0; i < build_id->size; i++)
> + {
> + build_id_hex += "0123456789abcdef"[build_id->data[i] >> 4];
> + build_id_hex += "0123456789abcdef"[build_id->data[i] & 0xf];
> + }
Would the existing build_id_to_string function work here?
> +
> + soname_to_buildid[soname] = build_id_hex;
I'm not completely sure I understand the flow of all of this yet, but
since the map is global, can there be collisions in soname? For
example, if we debug two core files in the same GDB, and both have a
foo.so library, but different versions with different build-ids. What
will happen here?
It sounds like something needs to be per-inferior or per-program-space
here.
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.
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +/* See debuginfod-support.h */
> +
> +int
> +debuginfod_get_soname_buildid (char *soname, gdb::unique_xmalloc_ptr<char> *build_id_hex)
> +{
> + auto it = soname_to_buildid.find (basename (soname));
> + if (it == soname_to_buildid.end ())
> + return 0;
> +
> + build_id_hex->reset (xstrdup (it->second.c_str ()));
If we're going to return a copy, then let's make it an std::string. But
do we really need to return a copy? If not, I would suggest making it:
const char *
debuginfod_get_soname_buildid (const char *soname)
{
auto it = soname_to_buildid.find (basename (soname));
if (it == soname_to_buildid.end ())
return nullptr;
return it->second.c_str ();
}
> + return 1;
> +}
> +
> +/* See debuginfod-support.h */
> +
> +void debuginfod_clear_soname_buildids ()
"void" on its own line.
> +{
> + soname_to_buildid.clear ();
> + return;
> +}
> +
> #endif
> diff --git a/gdb/debuginfod-support.h b/gdb/debuginfod-support.h
> index 5e5aab56e74..185c5be22fd 100644
> --- a/gdb/debuginfod-support.h
> +++ b/gdb/debuginfod-support.h
> @@ -61,4 +61,45 @@ 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);
> +
> +/* If BFD is of an ELF shared library then associate its soname with
> + BUILD_ID so that it can be retrieved with debuginfod_get_soname_buildid().
> +
> + Return 1 if the soname was successully associated with BUILD-ID, otherwise
> + return 0. */
> +
> +extern int debuginfod_set_soname_buildid (struct bfd *bfd,
> + const bfd_build_id *build_id);
I think the return type should be bool.
> +
> +/* If SONAME had a build-id associated with it by a previous call to
> + debuginfod_set_soname_buildid() then store the build-id in BUILD_ID_HEX
Remove the ().
> + as a NULL-terminated string.
> +
> + Return 1 if SONAME's build-id was found and stored in BUILD_ID_HEX,
> + otherwise return 0. */
> +
> +extern int debuginfod_get_soname_buildid (char *soname,
> + gdb::unique_xmalloc_ptr<char> *build_id_hex);
soname should probably be `const char *`? And the return type bool as
well.
> @@ -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 = nullptr;
> +
> + /* 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 = vma_map[start]) != nullptr)
Please split the assignment on its own line.
> + 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)
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.
> {
> if (gdbarch_addr_bit (gdbarch) == 32)
> printf_filtered ("\t%10s %10s %10s %10s %s\n",
> diff --git a/gdb/solib.c b/gdb/solib.c
> index 2df52118143..5f96d463aa4 100644
> --- a/gdb/solib.c
> +++ b/gdb/solib.c
> @@ -46,6 +46,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"
>
> @@ -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?
Simon
More information about the Gdb-patches
mailing list