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

[Andrew Burgess <aburgess@redhat.com>]

Aaron Merey <amerey@redhat.com> writes:

> diff --git a/gdb/dwarf2/frame.h b/gdb/dwarf2/frame.h
> index 5643e557513..2391e313e7c 100644
> --- a/gdb/dwarf2/frame.h
> +++ b/gdb/dwarf2/frame.h
> @@ -238,6 +238,10 @@ void dwarf2_append_unwinders (struct gdbarch *gdbarch);
>  extern const struct frame_base *
>    dwarf2_frame_base_sniffer (frame_info_ptr this_frame);
>  
> +/* Delete OBJFILEs comp_unit.  */
> +
> +extern void dwarf2_clear_frame_data (struct objfile * objfile);
> +
>  /* Compute the DWARF CFA for a frame.  */
>  
>  CORE_ADDR dwarf2_frame_cfa (frame_info_ptr this_frame);
> diff --git a/gdb/dwarf2/index-cache.c b/gdb/dwarf2/index-cache.c
> index 69f70642dc6..8c969ecd590 100644
> --- a/gdb/dwarf2/index-cache.c
> +++ b/gdb/dwarf2/index-cache.c
> @@ -240,6 +240,33 @@ index_cache::lookup_gdb_index (const bfd_build_id *build_id,
>    return {};
>  }
>  
> +/* See 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 {};
> +}

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.


> 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?

> +
>  #endif /* DWARF2_PUBLIC_H */
> diff --git a/gdb/dwarf2/read-gdb-index.c b/gdb/dwarf2/read-gdb-index.c
> index 4ca44540296..f4d77ed2d04 100644
> --- a/gdb/dwarf2/read-gdb-index.c
> +++ b/gdb/dwarf2/read-gdb-index.c
> @@ -139,6 +139,8 @@ struct dwarf2_gdb_index : public dwarf2_base_index_functions
>       gdb.dwarf2/gdb-index.exp testcase.  */
>    void dump (struct objfile *objfile) override;
>  
> +  /* Calls do_expand_symtabs_matching and triggers debuginfo downloading
> +     if necessary.  */
>    bool expand_symtabs_matching
>      (struct objfile *objfile,
>       gdb::function_view<expand_symtabs_file_matcher_ftype> file_matcher,
> @@ -148,8 +150,60 @@ struct dwarf2_gdb_index : public dwarf2_base_index_functions
>       block_search_flags search_flags,
>       domain_enum domain,
>       enum search_domain kind) override;
> +
> +  /* Expand symtabs matching a given symbol or file.  */
> +  bool do_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);
> +
> +  /* Calls dwarf2_base_index_functions::expand_all_symtabs and downloads
> +     debuginfo if necessary.  */
> +  void expand_all_symtabs (struct objfile *objfile) override;
> +
> +  /* Calls dwarf2_base_index_functions::find_last_source_symtab and downloads
> +     debuginfo if necessary.  */
> +  struct symtab *find_last_source_symtab (struct objfile *objfile) override;
>  };
>  
> +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.

Thanks,
Andrew