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

[Aaron Merey <amerey@redhat.com>]

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