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

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

Aaron Merey <amerey@redhat.com> writes:

> +void
> +dwarf2_gdb_index::expand_matching_symbols
> +  (struct objfile *objfile,
> +   const lookup_name_info &lookup_name,
> +   domain_enum domain,
> +   int global,
> +   symbol_compare_ftype *ordered_compare)
> +{
> +  try
> +    {
> +      do_expand_matching_symbols (objfile, lookup_name, domain,
> +				  global, ordered_compare);
> +    }
> +  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);
> +      return;

"return" is redundant here. I suggest removing it.

> +    }
> +}
> +void
> +read_full_dwarf_from_debuginfod (struct objfile *objfile,
> +				 dwarf2_base_index_functions *fncs)
> +{
> +  gdb_assert (objfile->flags & OBJF_DOWNLOAD_DEFERRED);
> +
> +  const struct bfd_build_id *build_id = build_id_bfd_get (objfile->obfd.get ());
> +  const char *filename;
> +  gdb_bfd_ref_ptr debug_bfd;
> +  gdb::unique_xmalloc_ptr<char> symfile_path;
> +  scoped_fd fd;

The "goto unset"s in this function make me wonder whether it is
exception-safe. If SCOPE_EXIT is used at this point like so:

SCOPE_EXIT { objfile->remove_deferred_status (); };

can it correctly replace the gotos?

> +
> +  if (build_id == nullptr)
> +    goto unset;
> +
> +  filename = bfd_get_filename (objfile->obfd.get ());
> +  fd = debuginfod_debuginfo_query (build_id->data, build_id->size,
> +				   filename, &symfile_path);
> +  if (fd.get () < 0)
> +    goto unset;
> +
> +  /* Separate debuginfo successfully retrieved from server.  */
> +  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 unset;
> +    }
> +
> +  /* Clear frame data so it can be recalculated using DWARF.  */
> +  dwarf2_clear_frame_data (objfile);
> +
> +  /* This may also trigger a dwz download.  */
> +  symbol_file_add_separate (debug_bfd, symfile_path.get (),
> +			     current_inferior ()->symfile_flags, objfile);
> +
> +unset:
> +  objfile->remove_deferred_status ();
> +}
> +
> +/* See public.h.  */
> +
> +bool
> +dwarf2_has_separate_index (struct objfile *objfile)
> +{
> +  if (objfile->flags & OBJF_DOWNLOAD_DEFERRED)
> +    return true;
> +  if (objfile->flags & OBJF_MAINLINE)
> +    return false;
> +  if (!IS_DIR_SEPARATOR (*objfile_filename (objfile)))
> +    return false;
> +
> +  gdb::unique_xmalloc_ptr<char> index_path;

I hope this isn't too pedantic, but the index_path declaration can be
moved right above the call to debuginfod_section_query.

> +  const bfd_build_id *build_id = build_id_bfd_get (objfile->obfd.get ());
> +
> +  if (build_id == nullptr)
> +    return false;
> +
> +  scoped_fd fd = debuginfod_section_query (build_id->data,
> +					   build_id->size,
> +					   bfd_get_filename
> +					     (objfile->obfd.get ()),
> +					   ".gdb_index",
> +					   &index_path);

<snip>

> diff --git a/gdb/objfiles.h b/gdb/objfiles.h
> index c20b63ceadf..ea9bd2157dc 100644
> --- a/gdb/objfiles.h
> +++ b/gdb/objfiles.h
> @@ -612,6 +612,26 @@ struct objfile
>    /* See quick_symbol_functions.  */
>    void require_partial_symbols (bool verbose);
>  
> +  /* Indicate that the aquisition of this objfile's separate debug objfile
> +     is no longer deferred.  Used when the debug objfile has been aquired

*acquired

> +     or could not be found.  */
> +  void remove_deferred_status ()

<snip>

> +void
> +libsection1_test ()
> +{
> +  pthread_t thr;
> +
> +  printf ("In libsection1\n");
> +  libsection2_test ();
> +
> +  pthread_create (&thr, NULL, libsection2_thread_test, NULL);
> +
> +  /* Give the new thread a chance to actually enter libsection2_thread_test.  */
> +  sleep (3);

If the testcase is run in a slow simulator, it can take more than 3
seconds. Or in a heavily loaded machine. Can the test use
pthread_cond_wait/pthread_cond_signal instead?


> +  printf ("Cancelling thread\n");
> +
> +  pthread_cancel (thr);
> +}

<snip>

> +# Restart GDB and clear the debuginfod client cache. Then load BINFILE into
> +# GDB and start running it.  Match output with pattern RES and use TESTNAME
> +# as the test name.
> +proc_with_prefix clean_restart_with_prompt { binfile res testname } {

The res argument isn't used by the function.

> +    global cache
> +
> +    # Delete client cache so debuginfo downloads again.
> +    file delete -force $cache
> +    clean_restart
> +
> +    gdb_test "set debuginfod enabled on" "" "clean_restart enable $testname"
> +    gdb_load $binfile
> +
> +    if {![runto_main]} {
> +       return

Isn't this return redundant? Suggest ending the procedure with just
"[runto_main]".

> +    }
> +}

-- 
Thiago