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

[Tom Tromey <tom@tromey.com>]

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