Re: [PATCH 3/4 v5] gdb/debuginfod: Support on-demand debuginfo downloading

[Aaron Merey <amerey@redhat.com>]

Hi Andrew,

On Wed, Mar 27, 2024 at 6:58 AM Andrew Burgess <aburgess@redhat.com> wrote:
> > +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 {};
> > +}
>
> I wonder if the core of this function should be merged with
> lookup_gdb_index?  The core functionality should be identical, but
> there's already a little divergence.
>
> I'd be tempted to just rename lookup_gdb_index_debuginfod to
> lookup_gdb_index and have it be an overload, one that finds the index in
> a named file, and the other that takes a build-id, creates a filename,
> and then calls your new version.

Ok I'll change this.

> > diff --git a/gdb/dwarf2/public.h b/gdb/dwarf2/public.h
> > index efb754b5fe8..4d3776465c3 100644
> > --- a/gdb/dwarf2/public.h
> > +++ b/gdb/dwarf2/public.h
> > @@ -44,4 +44,11 @@ extern bool dwarf2_initialize_objfile
> >
> >  extern void dwarf2_build_frame_info (struct objfile *);
> >
> > +/* Query debuginfod for the .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.  */
> > +
> > +extern bool dwarf2_has_separate_index (struct objfile *);
>
> Unless I'm missing something then this comment is not correct.  This
> function doesn't create an objfile to held the .gdb_index does it?

My mistake, that comment should instead say something like "associate
the .gdb_index information with OBJFILE until the full debuginfo is
downloaded".  In earlier revisions of the patch, this function did
create a new objfile but the comment wasn't updated when the function
changed.

> > +void
> > +dwarf2_gdb_index::expand_all_symtabs (struct objfile *objfile)
> > +{
> > +  try
> > +    {
> > +      dwarf2_base_index_functions::expand_all_symtabs (objfile);
> > +    }
> > +  catch (const gdb_exception &e)
> > +    {
> > +      if ((objfile->flags & OBJF_DOWNLOAD_DEFERRED) == 0)
> > +     exception_print (gdb_stderr, e);
> > +      else
> > +     read_full_dwarf_from_debuginfod (objfile, this);
>
> This is the part that I don't really understand about this patch, why is
> the download and read of the full dwarf done in the exception path?
>
> How do we know that the exception that was thrown indicates we should go
> fetch the dwarf and not that some other error has occurred?
>
> I couldn't find any obvious path from which
> read_full_dwarf_from_debuginfod might throw an exception, but if it does
> then I guess we're not going to get the behaviour we expect.
>
> I guess my expectation, going into this patch, was that you would have
> generalised the index checking code in some way so that when we check
> the index and find a match, instead of reading the full DWARF, we'd
> download the info, and then read the full DWARF.
>
> I'm not saying that what you have is wrong, or that it needs to change,
> just that I found this approach surprising, and I don't really
> understand the design -- but I'm open to being convinced.

Performing the download and read in the exception path helps decouple
dwarf2_base_index_functions and dwarf2_gdb_index functions from
the lazy debuginfo logic.  This design also facilitates an early return
from these functions so that the next objfile in the progspace objfile
list can be searched for a match.  If the deferred download was
successful, this next objfile holds the actual DWARF that gdb is looking for.

It is possible that some unrelated error during the initial index search
triggers an unnecessary debuginfo download.  FWIW I have not come across
such a case.  If this turned out to be an issue later on, we could for
instance introduce a special exception type indicating whether or not
gdb should attempt to download the deferred debuginfo.

In any case, the chance of an unnecessary download with this patch is
vastly lower than the current approach of immediately downloading all
debuginfo.

Aaron