[PATCH 3/4 v4] gdb/debuginfod: Support on-demand debuginfo downloading
Thiago Jung Bauermann
thiago.bauermann@linaro.org
Tue Dec 26 18:35:26 GMT 2023
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
More information about the Gdb-patches
mailing list