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

[Aaron Merey <amerey@redhat.com>]

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
>