Re: [RFC PATCH] Support debuginfo and source file fetching via debuginfo server

Tom Tromey
2019-09-13T21:03:55Z

>>>>> "Aaron" == Aaron Merey <amerey@redhat.com> writes:

Aaron> Debuginfo server is a lightweight web service that indexes debuginfo
Aaron> and source files by build-id and serves them over HTTP.

Thank you for the patch.

I think the idea is fine for gdb, so all that's left is some nits in the
patch.

Aaron> +#if HAVE_LIBDBGSERVER
Aaron> + else
Aaron> + {
Aaron> + const struct bfd_build_id *build_id;
Aaron> + char *debugfile_path;
Aaron> +
Aaron> + build_id = build_id_bfd_get (objfile->obfd);
Aaron> + int fd = dbgserver_find_debuginfo (build_id->data,
Aaron> + build_id->size,
Aaron> + &debugfile_path);

I was wondering what the fd represents. If it's open on the file, can
we simply reuse it rather than trying to reopen the file?

Instead of "int", using scoped_fd would be better.

Aaron> + symbol_file_add_separate(debug_bfd.get (), debugfile_path,

GNU style is a space before parens - there are a few instances.

Aaron> + free(debugfile_path);

xfree instead of free.

Aaron> +#if HAVE_LIBDBGSERVER
Aaron> + if (fd.get() < 0)
Aaron> + {
Aaron> + if (SYMTAB_COMPUNIT(s) != NULL)
Aaron> + {
Aaron> + const struct bfd_build_id *build_id;
Aaron> + const objfile *ofp = COMPUNIT_OBJFILE (SYMTAB_COMPUNIT (s));
Aaron> +
Aaron> + /* prefix the comp_dir to relative file names */
Aaron> + const char* dirname = SYMTAB_DIRNAME (s);
Aaron> + int suffname_len = strlen(dirname) + strlen(s->filename) + 2;
Aaron> + char *suffname = (char *) alloca(suffname_len);

I think it's better to just use std::string for this kind of thing.

Probably this area will need some refactoring since other patches have
touched this.

Aaron> + char *name_in_cache;
Aaron> + int dbgsrv_rc = dbgserver_find_source (build_id->data,
Aaron> + build_id->size,
Aaron> + suffname,
Aaron> + &name_in_cache);
Aaron> + if (dbgsrv_rc >= 0)
Aaron> + {
Aaron> + fullname.reset (xstrdup(name_in_cache));
Aaron> + free (name_in_cache);

It seems like you could just use

fullname.reset (name_in_cache);

here.

Aaron> + }
Aaron> + else if (dbgsrv_rc == -ENOSYS)
Aaron> + {
Aaron> + /* -ENOSYS indicates that libdbgserver could not find
Aaron> + any dbgserver URLs to query due to $DBGSERVER_URLS
Aaron> + not being defined. Replace -ENOSYS with -ENOENT so
Aaron> + that users who have not configured dbgserver see the
Aaron> + usual error message when a source file cannot be found. */
Aaron> + dbgsrv_rc = -ENOENT;

This assignment doesn't seem useful here.

Tom