[RFC] gdb/debuginfod: Support on-demand downloading of debuginfo

  At the beginning of a session, GDB attempts to download debuginfo for
every shared library used by the process being debugged.  One disadvantage
of this is that time may be spent downloading debuginfo that ultimately
isn't needed during the current session.

This patch helps address the issue by adding support for on-demand
downloading and reading of debuginfo.  The basic approach it takes is
to use debuginfod to download just the .gdb_index of a debuginfo file
as soon as the corresponding library is linked.  GDB then relies on the
information in the index for as long as possible.  When the index isn't
enough then debuginfo is downloaded and read. This helps avoid unnecessary
downloads.

Although this patch specifically uses .gdb_index, other indexes such
as .debug_names could be supported in much the same way.

Also with this patch, GDB will not build with the latest release of
debuginfod, hence the RFC. The debuginfod ELF/DWARF section query
function used in this patch to fetch .gdb_index is still in development
and will be included in a future release.

This patch implements basic on-demand functionality but more work needs
to be done to fully take advantage of this feature.  Currently GDB does
not attempt to download debuginfo when generating backtraces.  In some
situations backtraces may lack information for libraries that we ought
to be able to download debuginfo for.  If the user attempts to print a
non-existant symbol, GDB will start expanding symtabs one-by-one causing
all debuginfo to be downloaded. Some uses of the 'list' command also trigger
the downloading of all debuginfo, such as 'list 1' and 'list printf.c:33'.

Now I'll describe the implementation in more detail.

This patch adds a command 'set debuginfod enabled lazy' which enables
on-demand debuginfo downloading/reading.  If this 'lazy' mode is enabled
and a solib's debuginfo cannot be found locally, the new function
dwarf2_has_separate_index is called in elf_symfile_read. This function
queries debuginfod servers for the .gdb_index matching the build-id
of the solib.  If it's found, a new objfile is created to hold the .gdb_index
information.  The new objfile flag OBJF_INDEX_READLATER is used to indicate
that the objfile contains quick_symbols_functions for an index has deferred
debuginfo reading.

When GDB tries and fails to perform
dwarf2_base_index_functions::find_pc_sect_compunit_symtab or
dwarf2_gdb_index::expand_symtabs_matching, the new function
read_full_dwarf_from_debuginfod downloads the actual debuginfo file and
updates the objfile using the new function objfile::reinit. Symtab expansion
then proceeds as if the debuginfo was present all along.

Any feedback is appreciated.

Aaron
---
 gdb/debuginfod-support.c |  56 +++++++
 gdb/debuginfod-support.h |  23 +++
 gdb/dwarf2/index-cache.c |  33 +++++
 gdb/dwarf2/index-cache.h |  13 ++
 gdb/dwarf2/public.h      |   2 +
 gdb/dwarf2/read.c        | 308 ++++++++++++++++++++++++++++++++++++---
 gdb/dwarf2/section.c     |   4 +-
 gdb/elfread.c            |   2 +
 gdb/objfile-flags.h      |   4 +
 gdb/objfiles.c           |  16 ++
 gdb/objfiles.h           |   5 +
 gdb/symfile.c            |  82 +++++++++++
 gdb/symfile.h            |   3 +
 gdb/symtab.h             |   2 +
 14 files changed, 532 insertions(+), 21 deletions(-)
  

Ping

Thanks,
Aaron

On Wed, Oct 5, 2022 at 10:24 PM Aaron Merey <amerey@redhat.com> wrote:
>
> At the beginning of a session, GDB attempts to download debuginfo for
> every shared library used by the process being debugged.  One disadvantage
> of this is that time may be spent downloading debuginfo that ultimately
> isn't needed during the current session.
>
> This patch helps address the issue by adding support for on-demand
> downloading and reading of debuginfo.  The basic approach it takes is
> to use debuginfod to download just the .gdb_index of a debuginfo file
> as soon as the corresponding library is linked.  GDB then relies on the
> information in the index for as long as possible.  When the index isn't
> enough then debuginfo is downloaded and read. This helps avoid unnecessary
> downloads.
>
> Although this patch specifically uses .gdb_index, other indexes such
> as .debug_names could be supported in much the same way.
>
> Also with this patch, GDB will not build with the latest release of
> debuginfod, hence the RFC. The debuginfod ELF/DWARF section query
> function used in this patch to fetch .gdb_index is still in development
> and will be included in a future release.
>
> This patch implements basic on-demand functionality but more work needs
> to be done to fully take advantage of this feature.  Currently GDB does
> not attempt to download debuginfo when generating backtraces.  In some
> situations backtraces may lack information for libraries that we ought
> to be able to download debuginfo for.  If the user attempts to print a
> non-existant symbol, GDB will start expanding symtabs one-by-one causing
> all debuginfo to be downloaded. Some uses of the 'list' command also trigger
> the downloading of all debuginfo, such as 'list 1' and 'list printf.c:33'.
>
> Now I'll describe the implementation in more detail.
>
> This patch adds a command 'set debuginfod enabled lazy' which enables
> on-demand debuginfo downloading/reading.  If this 'lazy' mode is enabled
> and a solib's debuginfo cannot be found locally, the new function
> dwarf2_has_separate_index is called in elf_symfile_read. This function
> queries debuginfod servers for the .gdb_index matching the build-id
> of the solib.  If it's found, a new objfile is created to hold the .gdb_index
> information.  The new objfile flag OBJF_INDEX_READLATER is used to indicate
> that the objfile contains quick_symbols_functions for an index has deferred
> debuginfo reading.
>
> When GDB tries and fails to perform
> dwarf2_base_index_functions::find_pc_sect_compunit_symtab or
> dwarf2_gdb_index::expand_symtabs_matching, the new function
> read_full_dwarf_from_debuginfod downloads the actual debuginfo file and
> updates the objfile using the new function objfile::reinit. Symtab expansion
> then proceeds as if the debuginfo was present all along.
>
> Any feedback is appreciated.
>
> Aaron
> ---
>  gdb/debuginfod-support.c |  56 +++++++
>  gdb/debuginfod-support.h |  23 +++
>  gdb/dwarf2/index-cache.c |  33 +++++
>  gdb/dwarf2/index-cache.h |  13 ++
>  gdb/dwarf2/public.h      |   2 +
>  gdb/dwarf2/read.c        | 308 ++++++++++++++++++++++++++++++++++++---
>  gdb/dwarf2/section.c     |   4 +-
>  gdb/elfread.c            |   2 +
>  gdb/objfile-flags.h      |   4 +
>  gdb/objfiles.c           |  16 ++
>  gdb/objfiles.h           |   5 +
>  gdb/symfile.c            |  82 +++++++++++
>  gdb/symfile.h            |   3 +
>  gdb/symtab.h             |   2 +
>  14 files changed, 532 insertions(+), 21 deletions(-)
>
> diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> index 5f04a2b38ca..efcdb309522 100644
> --- a/gdb/debuginfod-support.c
> +++ b/gdb/debuginfod-support.c
> @@ -33,12 +33,14 @@ static cmd_list_element *show_debuginfod_prefix_list;
>  static const char debuginfod_on[] = "on";
>  static const char debuginfod_off[] = "off";
>  static const char debuginfod_ask[] = "ask";
> +static const char debuginfod_lazy[] = "lazy";
>
>  static const char *debuginfod_enabled_enum[] =
>  {
>    debuginfod_on,
>    debuginfod_off,
>    debuginfod_ask,
> +  debuginfod_lazy,
>    nullptr
>  };
>
> @@ -79,6 +81,15 @@ debuginfod_exec_query (const unsigned char *build_id,
>    return scoped_fd (-ENOSYS);
>  }
>
> +scoped_fd
> +debuginfod_section_query (const unsigned char *build_id,
> +                         int build_id_len,
> +                         const char *filename,
> +                         const char *section_name,
> +                         gdb::unique_xmalloc_ptr<char> *destname)
> +{
> +  return scoped_fd (-ENOSYS);
> +}
>  #define NO_IMPL _("Support for debuginfod is not compiled into GDB.")
>
>  #else
> @@ -358,6 +369,51 @@ debuginfod_exec_query (const unsigned char *build_id,
>
>    return fd;
>  }
> +
> +/* See debuginfod-support.h  */
> +
> +scoped_fd
> +debuginfod_section_query (const unsigned char *build_id,
> +                         int build_id_len,
> +                         const char *filename,
> +                         const char *section_name,
> +                         gdb::unique_xmalloc_ptr<char> *destname)
> +{
> +  if (debuginfod_enabled != debuginfod_lazy || !debuginfod_is_enabled ())
> +    return scoped_fd (-ENOSYS);
> +
> +  debuginfod_client *c = get_debuginfod_client ();
> +
> +  if (c == nullptr)
> +    return scoped_fd (-ENOMEM);
> +
> +  char *dname = nullptr;
> +  std::string desc = std::string ("section ") + section_name + " for";
> +  user_data data (desc.c_str (), filename);
> +
> +  debuginfod_set_user_data (c, &data);
> +  gdb::optional<target_terminal::scoped_restore_terminal_state> term_state;
> +  if (target_supports_terminal_ours ())
> +    {
> +      term_state.emplace ();
> +      target_terminal::ours ();
> +    }
> +
> +  scoped_fd fd (debuginfod_find_section (c, build_id, build_id_len,
> +                                        section_name, true, &dname));
> +  debuginfod_set_user_data (c, nullptr);
> +
> +  if (fd.get () < 0 && fd.get () != -ENOENT)
> +    gdb_printf (_("Download failed: %s. " \
> +                 "Continuing without section %s for %ps.\n"),
> +               safe_strerror (-fd.get ()), section_name,
> +               styled_string (file_name_style.style (),  filename));
> +
> +  if (fd.get () >= 0 && destname != nullptr)
> +    destname->reset (dname);
> +
> +  return fd;
> +}
>  #endif
>
>  /* Set callback for "set debuginfod enabled".  */
> diff --git a/gdb/debuginfod-support.h b/gdb/debuginfod-support.h
> index 5b1c1cb91f4..5294b65cac4 100644
> --- a/gdb/debuginfod-support.h
> +++ b/gdb/debuginfod-support.h
> @@ -78,4 +78,27 @@ extern scoped_fd debuginfod_exec_query (const unsigned char *build_id,
>                                         const char *filename,
>                                         gdb::unique_xmalloc_ptr<char>
>                                           *destname);
> +
> +/* Query debuginfod servers for the binary contents of a ELF/DWARF section
> +   from a file matching 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 file matching
> +   BUILD_ID.  It is used for printing messages to the user.
> +
> +   SECTION_NAME should be the name of an ELF/DWARF section beginning
> +   with '.'.
> +
> +   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_section_query (const unsigned char *build_id,
> +                                          int build_id_len,
> +                                          const char *filename,
> +                                          const char *section_name,
> +                                          gdb::unique_xmalloc_ptr<char>
> +                                            *destname);
>  #endif /* DEBUGINFOD_SUPPORT_H */
> diff --git a/gdb/dwarf2/index-cache.c b/gdb/dwarf2/index-cache.c
> index 6de58592050..cc170e8abfe 100644
> --- a/gdb/dwarf2/index-cache.c
> +++ b/gdb/dwarf2/index-cache.c
> @@ -222,6 +222,33 @@ index_cache::lookup_gdb_index (const bfd_build_id *build_id,
>    return {};
>  }
>
> +/* See dwarf-index-cache.h.  */
> +
> +gdb::array_view<const gdb_byte>
> +index_cache::lookup_gdb_index_debuginfod (const char *index_path,
> +                                         std::unique_ptr<index_cache_resource> *resource)
> +{
> +  try
> +    {
> +      /* Try to map that file.  */
> +      index_cache_resource_mmap *mmap_resource
> +       = new index_cache_resource_mmap (index_path);
> +
> +      /* Hand the resource to the caller.  */
> +      resource->reset (mmap_resource);
> +
> +      return gdb::array_view<const gdb_byte>
> +         ((const gdb_byte *) mmap_resource->mapping.get (),
> +          mmap_resource->mapping.size ());
> +    }
> +  catch (const gdb_exception_error &except)
> +    {
> +      warning (_("Unable to read %s: %s"), index_path, except.what ());
> +    }
> +
> +  return {};
> +}
> +
>  #else /* !HAVE_SYS_MMAN_H */
>
>  /* See dwarf-index-cache.h.  This is a no-op on unsupported systems.  */
> @@ -233,6 +260,12 @@ index_cache::lookup_gdb_index (const bfd_build_id *build_id,
>    return {};
>  }
>
> +gdb::array_view<const gdb_byte>
> +index_cache::lookup_gdb_index_debuginfod (const char *index_path,
> +                                         std::unique_ptr<index_cache_resource> *resource)
> +{
> +  return {};
> +}
>  #endif
>
>  /* See dwarf-index-cache.h.  */
> diff --git a/gdb/dwarf2/index-cache.h b/gdb/dwarf2/index-cache.h
> index 6366a9a9360..ecf74684c67 100644
> --- a/gdb/dwarf2/index-cache.h
> +++ b/gdb/dwarf2/index-cache.h
> @@ -65,6 +65,19 @@ class index_cache
>    lookup_gdb_index (const bfd_build_id *build_id,
>                     std::unique_ptr<index_cache_resource> *resource);
>
> +  /* Look for an index file located at INDEX_PATH in the debuginfod cache.
> +     Unlike lookup_gdb_index, this function does not exit early if the
> +     index cache has not been enabled.
> +
> +     If found, return the contents as an array_view and store the underlying
> +     resources (allocated memory, mapped file, etc) in RESOURCE.  The returned
> +     array_view is valid as long as RESOURCE is not destroyed.
> +
> +     If no matching index file is found, return an empty array view.  */
> +  gdb::array_view<const gdb_byte>
> +  lookup_gdb_index_debuginfod (const char *index_path,
> +                              std::unique_ptr<index_cache_resource> *resource);
> +
>    /* Return the number of cache hits.  */
>    unsigned int n_hits () const
>    { return m_n_hits; }
> diff --git a/gdb/dwarf2/public.h b/gdb/dwarf2/public.h
> index a9d4682c856..9971654c62f 100644
> --- a/gdb/dwarf2/public.h
> +++ b/gdb/dwarf2/public.h
> @@ -40,4 +40,6 @@ extern void dwarf2_initialize_objfile (struct objfile *objfile);
>
>  extern void dwarf2_build_frame_info (struct objfile *);
>
> +extern bool dwarf2_has_separate_index (struct objfile *);
> +
>  #endif /* DWARF2_PUBLIC_H */
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index 8e42c0f4d8d..1df63eba6f5 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -93,6 +93,9 @@
>  #include "split-name.h"
>  #include "gdbsupport/parallel-for.h"
>  #include "gdbsupport/thread-pool.h"
> +#include "symfile.h"
> +#include "inferior.h"
> +#include "debuginfod-support.h"
>
>  /* When == 1, print basic high level tracing messages.
>     When > 1, be more verbose.
> @@ -1846,6 +1849,10 @@ struct dwarf2_base_index_functions : public quick_symbol_functions
>       CORE_ADDR pc, struct obj_section *section, int warn_if_readin)
>         override final;
>
> +  struct compunit_symtab *_find_pc_sect_compunit_symtab
> +    (struct objfile *objfile, struct bound_minimal_symbol msymbol,
> +     CORE_ADDR pc, struct obj_section *section, int warn_if_readin);
> +
>    struct compunit_symtab *find_compunit_symtab_by_address
>      (struct objfile *objfile, CORE_ADDR address) override
>    {
> @@ -1911,6 +1918,16 @@ struct dwarf2_gdb_index : public dwarf2_base_index_functions
>       block_search_flags search_flags,
>       domain_enum domain,
>       enum search_domain kind) override;
> +
> +  bool _expand_symtabs_matching
> +    (struct objfile *objfile,
> +     gdb::function_view<expand_symtabs_file_matcher_ftype> file_matcher,
> +     const lookup_name_info *lookup_name,
> +     gdb::function_view<expand_symtabs_symbol_matcher_ftype> symbol_matcher,
> +     gdb::function_view<expand_symtabs_exp_notify_ftype> expansion_notify,
> +     block_search_flags search_flags,
> +     domain_enum domain,
> +     enum search_domain kind);
>  };
>
>  struct dwarf2_debug_names_index : public dwarf2_base_index_functions
> @@ -2677,28 +2694,31 @@ dwarf2_read_gdb_index
>
>    /* If there is a .dwz file, read it so we can get its CU list as
>       well.  */
> -  dwz = dwarf2_get_dwz_file (per_bfd);
> -  if (dwz != NULL)
> +  if (get_gdb_index_contents_dwz != nullptr)
>      {
> -      struct mapped_index dwz_map;
> -      const gdb_byte *dwz_types_ignore;
> -      offset_type dwz_types_elements_ignore;
> +      dwz = dwarf2_get_dwz_file (per_bfd);
> +      if (dwz != NULL)
> +       {
> +         struct mapped_index dwz_map;
> +         const gdb_byte *dwz_types_ignore;
> +         offset_type dwz_types_elements_ignore;
>
> -      gdb::array_view<const gdb_byte> dwz_index_content
> -       = get_gdb_index_contents_dwz (objfile, dwz);
> +         gdb::array_view<const gdb_byte> dwz_index_content
> +           = get_gdb_index_contents_dwz (objfile, dwz);
>
> -      if (dwz_index_content.empty ())
> -       return 0;
> +         if (dwz_index_content.empty ())
> +           return 0;
>
> -      if (!read_gdb_index_from_buffer (bfd_get_filename (dwz->dwz_bfd.get ()),
> -                                      1, dwz_index_content, &dwz_map,
> -                                      &dwz_list, &dwz_list_elements,
> -                                      &dwz_types_ignore,
> -                                      &dwz_types_elements_ignore))
> -       {
> -         warning (_("could not read '.gdb_index' section from %s; skipping"),
> -                  bfd_get_filename (dwz->dwz_bfd.get ()));
> -         return 0;
> +         if (!read_gdb_index_from_buffer (bfd_get_filename (dwz->dwz_bfd.get ()),
> +                                          1, dwz_index_content, &dwz_map,
> +                                          &dwz_list, &dwz_list_elements,
> +                                          &dwz_types_ignore,
> +                                          &dwz_types_elements_ignore))
> +           {
> +             warning (_("could not read '.gdb_index' section from %s; skipping"),
> +                      bfd_get_filename (dwz->dwz_bfd.get ()));
> +             return 0;
> +           }
>         }
>      }
>
> @@ -4192,8 +4212,10 @@ dw_expand_symtabs_matching_file_matcher
>      }
>  }
>
> +static bool read_full_dwarf_from_debuginfod (struct objfile *);
> +
>  bool
> -dwarf2_gdb_index::expand_symtabs_matching
> +dwarf2_gdb_index::_expand_symtabs_matching
>      (struct objfile *objfile,
>       gdb::function_view<expand_symtabs_file_matcher_ftype> file_matcher,
>       const lookup_name_info *lookup_name,
> @@ -4242,6 +4264,44 @@ dwarf2_gdb_index::expand_symtabs_matching
>    return result;
>  }
>
> +bool
> +dwarf2_gdb_index::expand_symtabs_matching
> +    (struct objfile *objfile,
> +     gdb::function_view<expand_symtabs_file_matcher_ftype> file_matcher,
> +     const lookup_name_info *lookup_name,
> +     gdb::function_view<expand_symtabs_symbol_matcher_ftype> symbol_matcher,
> +     gdb::function_view<expand_symtabs_exp_notify_ftype> expansion_notify,
> +     block_search_flags search_flags,
> +     domain_enum domain,
> +     enum search_domain kind)
> +{
> +  if (objfile->flags & OBJF_READNEVER)
> +    return false;
> +
> +  try
> +    {
> +      return _expand_symtabs_matching (objfile, file_matcher, lookup_name,
> +                                      symbol_matcher, expansion_notify,
> +                                      search_flags, domain, kind);
> +    }
> +  catch (gdb_exception e)
> +    {
> +      if ((objfile->flags & OBJF_INDEX_READLATER) == 0)
> +       {
> +         exception_print (gdb_stderr, e);
> +         return false;
> +       }
> +
> +      /* Objfile is a stub holding only index information.  Try to reinitialize
> +        objfile with the full debuginfo.  */
> +      if (!read_full_dwarf_from_debuginfod (objfile))
> +         return false;
> +      return _expand_symtabs_matching (objfile, file_matcher, lookup_name,
> +                                      symbol_matcher, expansion_notify,
> +                                      search_flags, domain, kind);
> +    }
> +}
> +
>  /* A helper for dw2_find_pc_sect_compunit_symtab which finds the most specific
>     symtab.  */
>
> @@ -4281,7 +4341,7 @@ dwarf2_base_index_functions::find_per_cu (dwarf2_per_bfd *per_bfd,
>  }
>
>  struct compunit_symtab *
> -dwarf2_base_index_functions::find_pc_sect_compunit_symtab
> +dwarf2_base_index_functions::_find_pc_sect_compunit_symtab
>       (struct objfile *objfile,
>        struct bound_minimal_symbol msymbol,
>        CORE_ADDR pc,
> @@ -4312,6 +4372,40 @@ dwarf2_base_index_functions::find_pc_sect_compunit_symtab
>    return result;
>  }
>
> +struct compunit_symtab *
> +dwarf2_base_index_functions::find_pc_sect_compunit_symtab
> +     (struct objfile *objfile,
> +      struct bound_minimal_symbol msymbol,
> +      CORE_ADDR pc,
> +      struct obj_section *section,
> +      int warn_if_readin)
> +{
> +  if (objfile->flags & OBJF_READNEVER)
> +    return nullptr;
> +
> +  try
> +    {
> +      return _find_pc_sect_compunit_symtab (objfile, msymbol, pc,
> +                                           section, warn_if_readin);
> +    }
> +  catch (gdb_exception e)
> +    {
> +      if ((objfile->flags & OBJF_INDEX_READLATER) == 0)
> +       {
> +         exception_print (gdb_stderr, e);
> +         return nullptr;
> +       }
> +
> +      /* Objfile is a stub holding only index information.  Try to reinitialize
> +        objfile with the full debuginfo.  */
> +      if (!read_full_dwarf_from_debuginfod (objfile))
> +       return nullptr;
> +
> +      return _find_pc_sect_compunit_symtab (objfile, msymbol, pc,
> +                                           section, warn_if_readin);
> +    }
> +}
> +
>  void
>  dwarf2_base_index_functions::map_symbol_filenames
>       (struct objfile *objfile,
> @@ -5417,6 +5511,180 @@ dwarf2_initialize_objfile (struct objfile *objfile)
>    objfile->qf.push_front (make_cooked_index_funcs ());
>  }
>
> +/* Query debuginfod for the .gdb_index matching OBJFILE's build-id.  Return the
> +   contents if successful.  */
> +
> +static gdb::array_view<const gdb_byte>
> +get_gdb_index_contents_from_debuginfod (objfile *objfile, dwarf2_per_bfd *per_bfd)
> +{
> +  const bfd_build_id *build_id = build_id_bfd_get (objfile->obfd.get ());
> +  if (build_id == nullptr)
> +    return {};
> +
> +  gdb::unique_xmalloc_ptr<char> index_path;
> +  scoped_fd fd = debuginfod_section_query (build_id->data, build_id->size,
> +                                          bfd_get_filename
> +                                            (objfile->obfd.get ()),
> +                                          ".gdb_index",
> +                                          &index_path);
> +  if (fd.get () < 0)
> +    return {};
> +
> +  return global_index_cache.lookup_gdb_index_debuginfod
> +    (index_path.get (), &per_bfd->index_cache_res);
> +}
> +
> +/* If OBJFILE is a stub holding only information from a .gdb_index, then attempt
> +   to download the full debuginfo and reinitialize OBJFILE with it.  */
> +
> +static bool
> +read_full_dwarf_from_debuginfod (struct objfile *objfile)
> +{
> +  if ((objfile->flags & OBJF_INDEX_READLATER) == 0)
> +    return false;
> +
> +  const struct bfd_build_id *build_id = build_id_bfd_get (objfile->obfd.get ());
> +  if (build_id == nullptr)
> +    return false;
> +
> +  const char *filename = bfd_get_filename (objfile->obfd.get ());
> +  dwarf2_per_bfd *per_bfd;
> +  dwarf2_per_objfile *per_objfile;
> +  gdb_bfd_ref_ptr debug_bfd;
> +  gdb::unique_xmalloc_ptr<char> symfile_path;
> +
> +  scoped_fd fd (debuginfod_debuginfo_query (build_id->data,
> +                                           build_id->size,
> +                                           filename,
> +                                           &symfile_path));
> +
> +  if (fd.get () < 0)
> +    goto fail;
> +
> +  /* File successfully retrieved from server.  Open as a bfd.  */
> +  debug_bfd = symfile_bfd_open (symfile_path.get ());
> +
> +  if (debug_bfd == nullptr
> +      || !build_id_verify (debug_bfd.get (), build_id->size, build_id->data))
> +    {
> +      warning (_("File \"%s\" from debuginfod cannot be opened as bfd"),
> +              filename);
> +      goto fail;
> +    }
> +
> +  /* Fill in objfile's missing information using the debuginfo.  */
> +  objfile->reinit (debug_bfd.release ());
> +
> +  /* Create new per_bfd and per_objfile.  Placeholders based on the
> +     separate_debug_objfile_backlink were deleted during reinit.  */
> +  per_bfd = new dwarf2_per_bfd (objfile->obfd.get (), nullptr, false);
> +  dwarf2_per_bfd_objfile_data_key.set (objfile, per_bfd);
> +  per_objfile = dwarf2_objfile_data_key.emplace (objfile, objfile, per_bfd);
> +
> +  objfile->flags &= ~OBJF_INDEX_READLATER;
> +
> +  /* Have to attach the separate index again.  The dwz will be downloaded at
> +     this point if applicable.  */
> +  if (dwarf2_read_gdb_index (per_objfile,
> +                            get_gdb_index_contents_from_debuginfod,
> +                            get_gdb_index_contents_from_section<dwz_file>))
> +    {
> +      dwarf_read_debug_printf ("found gdb index from debuginfod");
> +      objfile->qf.push_front (per_objfile->per_bfd->index_table->make_quick_functions ());
> +
> +      objfile->flags &= ~OBJF_NOT_FILENAME;
> +      return true;
> +    }
> +
> +fail:
> +  objfile->flags |= OBJF_READNEVER;
> +  return false;
> +}
> +
> +/* Query debuginfod for the separate .gdb_index associated with OBJFILE.  If
> +   successful, create an objfile to hold the .gdb_index information and act
> +   as a placeholder until the full debuginfo needs to be downloaded.  */
> +
> +bool
> +dwarf2_has_separate_index (struct objfile *objfile)
> +{
> +  if (objfile->flags & OBJF_MAINLINE
> +      || objfile->separate_debug_objfile_backlink != nullptr)
> +    return 0;
> +
> +  if (objfile->flags & OBJF_INDEX_READLATER)
> +    return 1;
> +
> +  gdb::unique_xmalloc_ptr<char> index_path;
> +  const bfd_build_id *build_id = build_id_bfd_get (objfile->obfd.get ());
> +
> +  scoped_fd fd = debuginfod_section_query (build_id->data,
> +                                          build_id->size,
> +                                          bfd_get_filename
> +                                            (objfile->obfd.get ()),
> +                                          ".gdb_index",
> +                                          &index_path);
> +  if (fd.get () >= 0)
> +    {
> +      /* We have a separate .gdb_index file so a separate debuginfo file
> +        should exist.  We just don't want to read it until we really
> +        have to.  Create an objfile to own the index information and to
> +        act as a placeholder for the debuginfo that we have the option
> +        of aquiring later.  */
> +      gdb_bfd_ref_ptr abfd (gdb_bfd_open (objfile_filename (objfile), gnutarget));
> +      if (abfd == nullptr)
> +       return false;
> +
> +      dwarf2_per_bfd_objfile_data_key.clear (objfile);
> +      dwarf2_objfile_data_key.clear (objfile);
> +
> +      symbol_file_add_from_index
> +       (abfd, current_inferior ()->symfile_flags | SYMFILE_NO_READ, objfile);
> +
> +      dwarf2_per_bfd *per_bfd;
> +      dwarf2_per_objfile *per_objfile = get_dwarf2_per_objfile (objfile);
> +
> +      if (per_objfile == nullptr)
> +       {
> +         per_bfd = dwarf2_per_bfd_objfile_data_key.get (objfile);
> +         if (per_bfd == nullptr)
> +           {
> +             per_bfd = new dwarf2_per_bfd (objfile->obfd.get (), nullptr, false);
> +             dwarf2_per_bfd_objfile_data_key.set (objfile, per_bfd);
> +           }
> +         per_objfile = dwarf2_objfile_data_key.emplace (objfile, objfile, per_bfd);
> +       }
> +
> +      struct objfile *stub = objfile->separate_debug_objfile;
> +      per_objfile = get_dwarf2_per_objfile (stub);
> +      if (per_objfile == nullptr)
> +       {
> +         per_bfd = dwarf2_per_bfd_objfile_data_key.get (stub);
> +         if (per_bfd == nullptr)
> +           {
> +             per_bfd = new dwarf2_per_bfd (stub->obfd.get (), nullptr, false);
> +             dwarf2_per_bfd_objfile_data_key.set (stub, per_bfd);
> +           }
> +         per_objfile = dwarf2_objfile_data_key.emplace (stub, stub, per_bfd);
> +       }
> +
> +      if (dwarf2_read_gdb_index (per_objfile,
> +                                get_gdb_index_contents_from_debuginfod,
> +                                nullptr))
> +       {
> +         dwarf_read_debug_printf ("found .gdb_index from debuginfod");
> +         stub->qf.push_front (per_bfd->index_table->make_quick_functions ());
> +         return 1;
> +       }
> +
> +      /* Unable to use the index.  Delete the stub.  */
> +      objfile->flags &= ~OBJF_INDEX_READLATER;
> +      stub->unlink ();
> +    }
> +
> +  return 0;
> +}
> +
>
>
>  /* Build a partial symbol table.  */
> diff --git a/gdb/dwarf2/section.c b/gdb/dwarf2/section.c
> index 32c86cc5d8d..4156ed2fdcb 100644
> --- a/gdb/dwarf2/section.c
> +++ b/gdb/dwarf2/section.c
> @@ -54,7 +54,9 @@ dwarf2_section_info::get_bfd_owner () const
>        section = get_containing_section ();
>        gdb_assert (!section->is_virtual);
>      }
> -  gdb_assert (section->s.section != nullptr);
> +  if (section->s.section == nullptr)
> +    throw_error (NOT_FOUND_ERROR,
> +                _("Can't find owner of DWARF2 section."));
>    return section->s.section->owner;
>  }
>
> diff --git a/gdb/elfread.c b/gdb/elfread.c
> index 8aee634b44b..25bbab065b8 100644
> --- a/gdb/elfread.c
> +++ b/gdb/elfread.c
> @@ -1274,6 +1274,8 @@ elf_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
>           symbol_file_add_separate (debug_bfd, debugfile.c_str (),
>                                     symfile_flags, objfile);
>         }
> +      else if (dwarf2_has_separate_index (objfile))
> +       return;
>        else
>         {
>           has_dwarf2 = false;
> diff --git a/gdb/objfile-flags.h b/gdb/objfile-flags.h
> index b2e07110571..df9bf27d778 100644
> --- a/gdb/objfile-flags.h
> +++ b/gdb/objfile-flags.h
> @@ -68,6 +68,10 @@ enum objfile_flag : unsigned
>      /* User requested that we do not read this objfile's symbolic
>         information.  */
>      OBJF_READNEVER = 1 << 7,
> +
> +    /* This objfile only holds information from an index.  It should
> +       be reinitialized with full debuginfo before expanding symtabs.  */
> +    OBJF_INDEX_READLATER = 1 << 8,
>    };
>
>  DEF_ENUM_FLAGS_TYPE (enum objfile_flag, objfile_flags);
> diff --git a/gdb/objfiles.c b/gdb/objfiles.c
> index 09aba0f80f0..c2c16c97758 100644
> --- a/gdb/objfiles.c
> +++ b/gdb/objfiles.c
> @@ -53,6 +53,7 @@
>  #include "gdb_bfd.h"
>  #include "btrace.h"
>  #include "gdbsupport/pathstuff.h"
> +#include "symfile.h"
>
>  #include <algorithm>
>  #include <vector>
> @@ -299,6 +300,16 @@ build_objfile_section_table (struct objfile *objfile)
>                            objfile, 1);
>  }
>
> +void
> +objfile::reinit (struct bfd *abfd)
> +{
> +  if ((flags & OBJF_INDEX_READLATER) == 0)
> +    return;
> +
> +  this->obfd.reset (abfd);
> +  deferred_read_symbols (this, 0);
> +}
> +
>  /* Given a pointer to an initialized bfd (ABFD) and some flag bits,
>     initialize the new objfile as best we can and link it into the list
>     of all known objfiles.
> @@ -455,6 +466,11 @@ objfile::make (gdb_bfd_ref_ptr bfd_, const char *name_, objfile_flags flags_,
>    if (parent != nullptr)
>      add_separate_debug_objfile (result, parent);
>
> +  /* Objfile was initialized using only an index.  Borrow offsets from the
> +     parent until debuginfo is read.  */
> +  if (flags_ & OBJF_INDEX_READLATER)
> +    result->section_offsets = parent->section_offsets;
> +
>    current_program_space->add_objfile (std::unique_ptr<objfile> (result),
>                                       parent);
>
> diff --git a/gdb/objfiles.h b/gdb/objfiles.h
> index 16dab0d2c69..0ebb16a30bd 100644
> --- a/gdb/objfiles.h
> +++ b/gdb/objfiles.h
> @@ -500,6 +500,11 @@ struct objfile
>    /* See quick_symbol_functions.  */
>    struct symtab *find_last_source_symtab ();
>
> +  /* Reinitialize this objfile using ABFD.  Objfile should have been originally
> +     initialized using a separate index from ABFD.  Updates this objfile with
> +     ABFD's symbols and section information.  */
> +  void reinit (struct bfd *abfd);
> +
>    /* See quick_symbol_functions.  */
>    void forget_cached_source_info ();
>
> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index eb27668f9d3..7edd50d92a2 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -1074,6 +1074,14 @@ symbol_file_add_with_addrs (const gdb_bfd_ref_ptr &abfd, const char *name,
>      flags |= OBJF_MAINLINE;
>    objfile = objfile::make (abfd, name, flags, parent);
>
> +  if (objfile->flags & OBJF_INDEX_READLATER)
> +    {
> +      /* objfile was initialized only using a separate index so don't
> +        try to read symbols yet.  */
> +      gdb::observers::new_objfile.notify (objfile);
> +      return objfile;
> +    }
> +
>    /* We either created a new mapped symbol table, mapped an existing
>       symbol table file which has not had initial symbol reading
>       performed, or need to read an unmapped symbol table.  */
> @@ -1135,6 +1143,29 @@ symbol_file_add_with_addrs (const gdb_bfd_ref_ptr &abfd, const char *name,
>    return (objfile);
>  }
>
> +/* We know a separate debuginfo file should exist, but we don't want to
> +   read it yet.  Infer some of it's properties from the parent objfile.  */
> +
> +void
> +symbol_file_add_from_index (const gdb_bfd_ref_ptr &bfd,
> +                           symfile_add_flags symfile_flags,
> +                           struct objfile *parent)
> +{
> +  section_addr_info sap = build_section_addr_info_from_objfile (parent);
> +
> +  symbol_file_add_with_addrs
> +    (bfd, "<separate .gdb_index stub>", symfile_flags, &sap,
> +     (parent->flags & (OBJF_REORDERED | OBJF_SHARED | OBJF_READNOW
> +                     | OBJF_USERLOADED | OBJF_MAINLINE | OBJF_PSYMTABS_READ))
> +                  | OBJF_INDEX_READLATER | OBJF_NOT_FILENAME,
> +     parent);
> +
> +  objfile *result = parent->separate_debug_objfile;
> +  init_objfile_sect_indices (result);
> +
> +  return;
> +}
> +
>  /* Add BFD as a separate debug file for OBJFILE.  For NAME description
>     see the objfile constructor.  */
>
> @@ -2415,6 +2446,57 @@ remove_symbol_file_command (const char *args, int from_tty)
>    clear_symtab_users (0);
>  }
>
> +/* Read a separate debuginfo OBJFILE that was originally initialized using
> +   only an index and section information from its parent file.  */
> +
> +void
> +deferred_read_symbols (struct objfile *objfile, int from_tty)
> +{
> +  gdb_assert (objfile->flags & OBJF_INDEX_READLATER);
> +
> +  /* Nuke all the state that we will re-read.  */
> +  objfile->registry_fields.clear_registry ();
> +
> +  objfile->sections = NULL;
> +  objfile->sect_index_bss = -1;
> +  objfile->sect_index_data = -1;
> +  objfile->sect_index_rodata = -1;
> +  objfile->sect_index_text = -1;
> +  objfile->compunit_symtabs = NULL;
> +  objfile->template_symbols = NULL;
> +
> +  {
> +    gdb_bfd_ref_ptr obfd = objfile->obfd;
> +    const char *obfd_filename;
> +
> +    obfd_filename = bfd_get_filename (objfile->obfd.get ());
> +    /* Open the new BFD before freeing the old one, so that
> +       the filename remains live.  */
> +    gdb_bfd_ref_ptr temp (gdb_bfd_open (obfd_filename, gnutarget));
> +    objfile->obfd = std::move (temp);
> +    if (objfile->obfd == NULL)
> +      error (_("Can't open %s to read symbols."), obfd_filename);
> +  }
> +
> +  std::string original_name = objfile->original_name;
> +
> +  /* bfd_openr sets cacheable to true, which is what we want.  */
> +  if (!bfd_check_format (objfile->obfd.get (), bfd_object))
> +    error (_("Can't read symbols from %s: %s."), objfile_name (objfile),
> +          bfd_errmsg (bfd_get_error ()));
> +
> +  objfile->original_name
> +    = obstack_strdup (&objfile->objfile_obstack, original_name);
> +
> +  objfile_set_sym_fns (objfile, find_sym_fns (objfile->obfd.get ()));
> +  build_objfile_section_table (objfile);
> +  (*objfile->sf->sym_init) (objfile);
> +  init_objfile_sect_indices (objfile);
> +
> +  read_symbols (objfile, 0);
> +  objfile->mtime = bfd_get_mtime (objfile->obfd.get ());
> +}
> +
>  /* Re-read symbols if a symbol-file has changed.  */
>
>  void
> diff --git a/gdb/symfile.h b/gdb/symfile.h
> index 1d13e82502b..724e156fc4b 100644
> --- a/gdb/symfile.h
> +++ b/gdb/symfile.h
> @@ -241,6 +241,9 @@ extern struct objfile *symbol_file_add_from_bfd (const gdb_bfd_ref_ptr &,
>  extern void symbol_file_add_separate (const gdb_bfd_ref_ptr &, const char *,
>                                       symfile_add_flags, struct objfile *);
>
> +extern void symbol_file_add_from_index (const gdb_bfd_ref_ptr &,
> +                                       symfile_add_flags, struct objfile *);
> +
>  extern std::string find_separate_debug_file_by_debuglink (struct objfile *);
>
>  /* Build (allocate and populate) a section_addr_info struct from an
> diff --git a/gdb/symtab.h b/gdb/symtab.h
> index 89d7a183ff3..380387162de 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -2207,6 +2207,8 @@ extern bool find_pc_line_pc_range (CORE_ADDR, CORE_ADDR *, CORE_ADDR *);
>
>  extern void reread_symbols (int from_tty);
>
> +extern void deferred_read_symbols (struct objfile *, int from_tty);
> +
>  /* Look up a type named NAME in STRUCT_DOMAIN in the current language.
>     The type returned must not be opaque -- i.e., must have at least one field
>     defined.  */
> --
> 2.37.3
>
  

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

Aaron> This patch helps address the issue by adding support for on-demand
Aaron> downloading and reading of debuginfo.  The basic approach it takes is
Aaron> to use debuginfod to download just the .gdb_index of a debuginfo file
Aaron> as soon as the corresponding library is linked.  GDB then relies on the
Aaron> information in the index for as long as possible.  When the index isn't
Aaron> enough then debuginfo is downloaded and read. This helps avoid unnecessary
Aaron> downloads.

Makes sense to me.

Aaron> Although this patch specifically uses .gdb_index, other indexes such
Aaron> as .debug_names could be supported in much the same way.

I mentioned this in bugzilla, but .debug_names will be worse here
because it requires .debug_str and .debug_aranges to be downloaded as
well, and the former in particular will contain a lot of data that's not
immediately needed.

Aaron> This patch adds a command 'set debuginfod enabled lazy' which enables
Aaron> on-demand debuginfo downloading/reading.  If this 'lazy' mode is enabled
Aaron> and a solib's debuginfo cannot be found locally, the new function
Aaron> dwarf2_has_separate_index is called in elf_symfile_read. This function
Aaron> queries debuginfod servers for the .gdb_index matching the build-id
Aaron> of the solib.  If it's found, a new objfile is created to hold the .gdb_index
Aaron> information.  The new objfile flag OBJF_INDEX_READLATER is used to indicate
Aaron> that the objfile contains quick_symbols_functions for an index has deferred
Aaron> debuginfo reading.

It seems to me that OBJF_INDEX_READLATER should not be needed.

Instead, what I'd propose is creating the separate debuginfo objfile and
providing it with an instance of dwarf2_gdb_index -- but one that
arranges to download the remaining data when necessary.

It's fine to add new virtual methods to dwarf2_base_index_functions or
wherever if this helps.  E.g, dw2_instantiate_symtab could be a virtual
method in the base class, and then the subclass could override it to
first download the remaining sections.

You'd have to consider how to handle errors in this situation, but
probably just printing a warning (once per file) and carrying on would
be good enough.  The idea behind warn-and-carry-on is that the user
asked for this lazy behavior, if their network goes down then the kind
of asked for it; and anyway maybe they can rescue the situation with
"noshared" or something.

Maybe you tried something like this already and ran into problems?  I'm
handwaving a lot, maybe there are hidden difficulties.

TBH the current gdb design where separate debug files are their own
objfile seems like a mistake to me.  I haven't ever looked at changing
this though.  But, maybe it doesn't have to really be changed,
dwarf2/read.c could just provide one of these subclasses attached to the
parent objfile.

Aaron> Also with this patch, GDB will not build with the latest release of
Aaron> debuginfod, hence the RFC. The debuginfod ELF/DWARF section query

Is it possible to detect the non-existence of debuginfod_find_section at
compile time so that we can avoid bumping the minimum requirement?

Tom
  

Hi Tom,

On Wed, Jan 11, 2023 at 4:25 PM Tom Tromey <tom@tromey.com> wrote:
> >>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Aaron> This patch helps address the issue by adding support for on-demand
> Aaron> downloading and reading of debuginfo.  The basic approach it takes is
> Aaron> to use debuginfod to download just the .gdb_index of a debuginfo file
> Aaron> as soon as the corresponding library is linked.  GDB then relies on the
> Aaron> information in the index for as long as possible.  When the index isn't
> Aaron> enough then debuginfo is downloaded and read. This helps avoid unnecessary
> Aaron> downloads.
>
> Makes sense to me.
>
> Aaron> Although this patch specifically uses .gdb_index, other indexes such
> Aaron> as .debug_names could be supported in much the same way.
>
> I mentioned this in bugzilla, but .debug_names will be worse here
> because it requires .debug_str and .debug_aranges to be downloaded as
> well, and the former in particular will contain a lot of data that's not
> immediately needed.

In the case of libxul, the combined size of its .debug_names, .debug_str and
.debug_aranges is about 72% larger than the size of its .gdb_index (690 MB
vs. 400 MB).  At least for this library we are much better off sticking with
.gdb_index.

> Aaron> This patch adds a command 'set debuginfod enabled lazy' which enables
> Aaron> on-demand debuginfo downloading/reading.  If this 'lazy' mode is enabled
> Aaron> and a solib's debuginfo cannot be found locally, the new function
> Aaron> dwarf2_has_separate_index is called in elf_symfile_read. This function
> Aaron> queries debuginfod servers for the .gdb_index matching the build-id
> Aaron> of the solib.  If it's found, a new objfile is created to hold the .gdb_index
> Aaron> information.  The new objfile flag OBJF_INDEX_READLATER is used to indicate
> Aaron> that the objfile contains quick_symbols_functions for an index has deferred
> Aaron> debuginfo reading.
>
> It seems to me that OBJF_INDEX_READLATER should not be needed.
>
> Instead, what I'd propose is creating the separate debuginfo objfile and
> providing it with an instance of dwarf2_gdb_index -- but one that
> arranges to download the remaining data when necessary.
>
> It's fine to add new virtual methods to dwarf2_base_index_functions or
> wherever if this helps.  E.g, dw2_instantiate_symtab could be a virtual
> method in the base class, and then the subclass could override it to
> first download the remaining sections.

Sure.

> You'd have to consider how to handle errors in this situation, but
> probably just printing a warning (once per file) and carrying on would
> be good enough.  The idea behind warn-and-carry-on is that the user
> asked for this lazy behavior, if their network goes down then the kind
> of asked for it; and anyway maybe they can rescue the situation with
> "noshared" or something.
>
> Maybe you tried something like this already and ran into problems?  I'm
> handwaving a lot, maybe there are hidden difficulties.

Currently if the index downloads successfully but the debuginfo download
fails, the usual "Download failed" message is printed and it indicates
that gdb will continue without that library's debuginfo.  So far I
haven't seen any issues in these cases but making use of "noshared"
is a good idea.

>
> TBH the current gdb design where separate debug files are their own
> objfile seems like a mistake to me.  I haven't ever looked at changing
> this though.  But, maybe it doesn't have to really be changed,
> dwarf2/read.c could just provide one of these subclasses attached to the
> parent objfile.
>
> Aaron> Also with this patch, GDB will not build with the latest release of
> Aaron> debuginfod, hence the RFC. The debuginfod ELF/DWARF section query
>
> Is it possible to detect the non-existence of debuginfod_find_section at
> compile time so that we can avoid bumping the minimum requirement?

Yes I will change this.  debuginfod_find_section is supported as of elfutils
0.188 but it's still a good idea to avoid bumping the minimum requirement.

Thanks,
Aaron
  

>>>>> "Aaron" == Aaron Merey <amerey@redhat.com> writes:

Aaron> Currently if the index downloads successfully but the debuginfo download
Aaron> fails, the usual "Download failed" message is printed and it indicates
Aaron> that gdb will continue without that library's debuginfo.

Yeah, this seems fine to me.

Aaron>  So far I
Aaron> haven't seen any issues in these cases but making use of "noshared"
Aaron> is a good idea.

If it works :).  It would probably be useful for users if we had some
way for them to say "try again to find some debuginfo".  But maybe
noshared+sharedlibrary works enough for that, I am not sure.

Tom