[RFC] Support debuginfo and source file fetching via debuginfo server

Message ID 20190820202809.25367-1-amerey@redhat.com
State New
Headers show
Series
  • [RFC] Support debuginfo and source file fetching via debuginfo server
Related show

Commit Message

Aaron Merey Aug. 20, 2019, 8:28 p.m.
Debuginfo server is a lightweight web service that indexes debuginfo
and source files by build-id and serves them over HTTP. Debuginfo server
is able to index unpackaged, locally-built software in addition to RPM
files. Debuginfo server is packaged with a shared library, libdbgserver,
that provides a small set of client functions for fetching files from
debuginfo server. This patch adds debuginfo server support to GDB. In
case a source file or separate debuginfo file cannot be found locally,
GDB can use libdbgserver to query debuginfo server for the file in
question, if enabled to do so.

We plan on packaging debuginfo server with elfutils. For more information
please see the 'dbgserver' branch of the elfutils git repo
(https://sourceware.org/git/?p=elfutils.git;a=shortlog;h=refs/heads/dbgserver).

This patch was tested on x86-64 Fedora 29. The testsuite was run both
with and without this patch applied and the summaries indicated no
regressions. Debuginfo server tests were also added to the testsuite
under gdb/testsuite/gdb.dbgserver/.

gdb/ChangeLog:

        * configure.ac: Add --with-dbgserver option.
        * config.in: Add libdbgserver.
        * elfread.c (elf_symfile_read): Query debuginfo server if
        separate debuginfo file cannot be found.
        * Makefile.in (CLIBS): Add libdbgserver.
        * source.c (open_source_file): Query debuginfo server if source
        file cannot be found.
        * top.c (print_gdb_configuration): Print "--with-dbgserver" or
        "--without-dbgserver" depending on whether GDB was
        configured with debuginfo server.

gdb/testsuite/ChangeLog:

        * gdb.dbgserver/: New directory for debuginfo server tests.
        * gdb.dbgserver/fetch_src_and_symbols.c: New file.
        * gdb.dbgserver/fetch_src_and_symbols.exp: Test debuginfo
        server's ability to fetch source and separate debuginfo files.
---
 gdb/Makefile.in                               |  5 +-
 gdb/config.in                                 |  3 +
 gdb/configure.ac                              | 26 ++++++
 gdb/elfread.c                                 | 27 ++++++
 gdb/source.c                                  | 56 +++++++++++
 .../gdb.dbgserver/fetch_src_and_symbols.c     |  1 +
 .../gdb.dbgserver/fetch_src_and_symbols.exp   | 93 +++++++++++++++++++
 gdb/top.c                                     |  9 ++
 8 files changed, 219 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.dbgserver/fetch_src_and_symbols.c
 create mode 100644 gdb/testsuite/gdb.dbgserver/fetch_src_and_symbols.exp

-- 
2.21.0

Comments

Tom Tromey Sept. 13, 2019, 9:03 p.m. | #1
>>>>> "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
Christian Biesinger via gdb-patches Sept. 22, 2019, 3:53 a.m. | #2
On Tue, Aug 20, 2019 at 4:28 PM Aaron Merey <amerey@redhat.com> wrote:
> Debuginfo server is a lightweight web service that indexes debuginfo

> and source files by build-id and serves them over HTTP. Debuginfo server

> is able to index unpackaged, locally-built software in addition to RPM

> files. Debuginfo server is packaged with a shared library, libdbgserver,

> that provides a small set of client functions for fetching files from

> debuginfo server. This patch adds debuginfo server support to GDB. In

> case a source file or separate debuginfo file cannot be found locally,

> GDB can use libdbgserver to query debuginfo server for the file in

> question, if enabled to do so.

>

[...]
> @@ -1296,6 +1299,30 @@ elf_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)

>           symbol_file_add_separate (debug_bfd.get (), debugfile.c_str (),

>                                     symfile_flags, objfile);

>         }

> +#if HAVE_LIBDBGSERVER

> +      else

> +        {

> +          const struct bfd_build_id *build_id;

> +          char *debugfile_path;

> +

> +          build_id = build_id_bfd_get (objfile->obfd);

> +          int fd = dbgserver_find_debuginfo (build_id->data,

> +                                             build_id->size,

> +                                             &debugfile_path);

> +

> +          if (fd >= 0)

> +            {

> +              /* debuginfo successfully retrieved from server, reopen

> +                 the file as a bfd instead.  */

> +              gdb_bfd_ref_ptr debug_bfd (symfile_bfd_open (debugfile_path));

> +

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

> +                                       symfile_flags, objfile);

> +              close(fd);

> +              free(debugfile_path);

> +            }

> +        }

> +#endif /* LIBDBGSERVER */

>      }

>  }

>


You wrote that the debuginfo server will download symbols over HTTP.
Does that mean that this call to dbgserver_find_debuginfo will block
as it downloads the file? (will ctrl+c work as it does that?)

If so, any way to do this download on a background thread?

Christian
Tom Tromey Oct. 9, 2019, 2:55 p.m. | #3
>>>>> "Christian" == Christian Biesinger via gdb-patches <gdb-patches@sourceware.org> writes:


>> Debuginfo server is a lightweight web service that indexes debuginfo

>> and source files by build-id and serves them over HTTP. 


Christian> You wrote that the debuginfo server will download symbols over HTTP.
Christian> Does that mean that this call to dbgserver_find_debuginfo will block
Christian> as it downloads the file? (will ctrl+c work as it does that?)

The control-c question is a good one -- I'd also like to know the
answer.  Most things in gdb are interruptible this way.

Christian> If so, any way to do this download on a background thread?

I think gdb can't support this without some additional re-architecting.
It's a good goal, but I think I would not require it in order to get the
patch in.

Tom
Aaron Merey Oct. 28, 2019, 8:34 p.m. | #4
On Wed, Oct 9, 2019 at 10:55 AM Tom Tromey <tom@tromey.com> wrote:
>

> >>>>> "Christian" == Christian Biesinger via gdb-patches <gdb-patches@sourceware.org> writes:

>

> >> Debuginfo server is a lightweight web service that indexes debuginfo

> >> and source files by build-id and serves them over HTTP.

>

> Christian> You wrote that the debuginfo server will download symbols over HTTP.

> Christian> Does that mean that this call to dbgserver_find_debuginfo will block

> Christian> as it downloads the file? (will ctrl+c work as it does that?)

>

> The control-c question is a good one -- I'd also like to know the

> answer.  Most things in gdb are interruptible this way.


We are now using the libcurl multi interface to fetch files over HTTP
and it is mostly non-blocking (although blocking can happen during
domain name resolution). However ctrl+c does not interrupt our symbol
downloading within GDB. This may be due to GDB's SIGINT handling,
which just sets a flag for the event loop when it is not safe to
throw an exception. Based on a comment in
gdb/event-top.c:handle_sigint(), symfile reading is one such region.

Also, we have an environment var $DEBUGINFOD_TIMEOUT that lets users
control how much time is spent attempting each download without having
to rely on ctrl+c.

Aaron
Tom Tromey Nov. 1, 2019, 7:31 p.m. | #5
Tom> The control-c question is a good one -- I'd also like to know the
Tom> answer.  Most things in gdb are interruptible this way.

Aaron> We are now using the libcurl multi interface to fetch files over HTTP
Aaron> and it is mostly non-blocking (although blocking can happen during
Aaron> domain name resolution). However ctrl+c does not interrupt our symbol
Aaron> downloading within GDB. This may be due to GDB's SIGINT handling,
Aaron> which just sets a flag for the event loop when it is not safe to
Aaron> throw an exception. Based on a comment in
Aaron> gdb/event-top.c:handle_sigint(), symfile reading is one such region.

Aaron> Also, we have an environment var $DEBUGINFOD_TIMEOUT that lets users
Aaron> control how much time is spent attempting each download without having
Aaron> to rely on ctrl+c.

I think an environment variable may be too late.  I realize gdb doesn't
generally allow interrupting the reading of symbol tables.  But, this
case is a little different, in that the main thing happening is the
download of the debug info.  Being able to interrupt this would be good,
because servers can wedge, download speeds can be low, etc.  Also, maybe
printing something if the download takes too long would be good to do.

Tom
Aaron Merey Nov. 1, 2019, 9:03 p.m. | #6
On Fri, Nov 1, 2019 at 3:31 PM Tom Tromey <tom@tromey.com> wrote:
> Aaron> domain name resolution). However ctrl+c does not interrupt our symbol

> Aaron> downloading within GDB. This may be due to GDB's SIGINT handling,

> Aaron> which just sets a flag for the event loop when it is not safe to

> Aaron> throw an exception. Based on a comment in

> Aaron> gdb/event-top.c:handle_sigint(), symfile reading is one such region.

>

> Aaron> Also, we have an environment var $DEBUGINFOD_TIMEOUT that lets users

> Aaron> control how much time is spent attempting each download without having

> Aaron> to rely on ctrl+c.

>

> I think an environment variable may be too late.  I realize gdb doesn't

> generally allow interrupting the reading of symbol tables.  But, this

> case is a little different, in that the main thing happening is the

> download of the debug info.  Being able to interrupt this would be good,

> because servers can wedge, download speeds can be low, etc.  Also, maybe

> printing something if the download takes too long would be good to do.


Ok makes sense. One approach would be for the debuginfod_find_*
functions to temporarily install their own SIGINT handler that cancels
any downloading. Though during this time ctrl+c won't have the same
gdb-wide effect that it currently has. When this happens maybe we
could print a message to indicate that the interrupt applied
specifically to the downloading. How does this sound to you?

Aaron
Frank Ch. Eigler Nov. 2, 2019, 2:05 p.m. | #7
amerey wrote:

> Ok makes sense. One approach would be for the debuginfod_find_*

> functions to temporarily install their own SIGINT handler that cancels

> any downloading. Though during this time ctrl+c won't have the same

> gdb-wide effect that it currently has. When this happens maybe we

> could print a message to indicate that the interrupt applied

> specifically to the downloading. How does this sound to you?


Actually we can probably do even better: trap the SIGINT ourselves in
libdebuginfod, but upon return to gdb with an -EINTR, the caller can
call set_quit_flag().  Then the rest of gdb can react the normal way.

- FChE
Frank Ch. Eigler Nov. 4, 2019, 3:03 p.m. | #8
fche@redhat.com (Frank Ch. Eigler) writes:

> Actually we can probably do even better: trap the SIGINT ourselves in

> libdebuginfod, but upon return to gdb with an -EINTR, the caller can

> call set_quit_flag().  Then the rest of gdb can react the normal way.


Actually2, prototyped a solution whereby the library temporarily hijacks
SIGINT to interrupt its ongoing transfers, then raises a SIGINT back to
gdb or other caller upon return.  If this works, then no changes at all
are required to the gdb/libdebuginfod interface code.

- FChe
Simon Marchi Nov. 4, 2019, 3:46 p.m. | #9
On 2019-11-04 10:03 a.m., Frank Ch. Eigler wrote:
> fche@redhat.com (Frank Ch. Eigler) writes:

> 

>> Actually we can probably do even better: trap the SIGINT ourselves in

>> libdebuginfod, but upon return to gdb with an -EINTR, the caller can

>> call set_quit_flag().  Then the rest of gdb can react the normal way.

> 

> Actually2, prototyped a solution whereby the library temporarily hijacks

> SIGINT to interrupt its ongoing transfers, then raises a SIGINT back to

> gdb or other caller upon return.  If this works, then no changes at all

> are required to the gdb/libdebuginfod interface code.

> 

> - FChe

> 


I haven't followed this thread closely, so it might be an obvious "no", but
I was wondering if the library could offer to install a callback that is call
relatively often, allowing GDB to interrupt the operation if it returns a
certain value.  GDB would check its quit_flag in there.  I would find that
cleaner and easier to understand than having the main program and libraries
competing for the same signal handler.

At the same time, that callback would be used to report progress, and GDB
could display a nice progress bar!

Simon
Frank Ch. Eigler Nov. 4, 2019, 4:26 p.m. | #10
Hi -

> I haven't followed this thread closely, so it might be an obvious "no", but

> I was wondering if the library could offer to install a callback that is call

> relatively often, allowing GDB to interrupt the operation [...]


That could also work, at the cost of extending the API.  Will play with it.

- FChE
Aaron Merey Nov. 5, 2019, 1:59 a.m. | #11
On Mon, Nov 4, 2019 at 11:26 AM Frank Ch. Eigler <fche@redhat.com> wrote:
>

> Hi -

>

> > I haven't followed this thread closely, so it might be an obvious "no", but

> > I was wondering if the library could offer to install a callback that is call

> > relatively often, allowing GDB to interrupt the operation [...]

>

> That could also work, at the cost of extending the API.  Will play with it.

>

> - FChE


Attached is the updated debuginfo and source lookup code, using the
callback idea that Simon suggested. Downloads are now cancelled upon
SIGINT and the callback can be modified with code that prints progress
messages (the a and b parameters in the callback represent the
numerator and denominator of the download's completion fraction).
gdb/elfread.c | 36 +++++++++++++++++++++++++++++++++++-
 gdb/source.c  | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 82 insertions(+), 1 deletion(-)

diff --git a/gdb/elfread.c b/gdb/elfread.c
index 226e3f09d3..e24bca610f 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -48,7 +48,11 @@
 #include "auxv.h"
 #include "mdebugread.h"
 #include "ctfread.h"
+#include "gdbsupport/scoped_fd.h"
 #include "gdbsupport/gdb_string_view.h"
+#if HAVE_LIBDEBUGINFOD
+#include <elfutils/debuginfod.h>
+#endif
 
 /* Forward declarations.  */
 extern const struct sym_fns elf_sym_fns_gdb_index;
@@ -1311,8 +1315,38 @@ elf_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
 	  symbol_file_add_separate (debug_bfd.get (), debugfile.c_str (),
 				    symfile_flags, objfile);
 	}
-	else
+      else
+        {
 	  has_dwarf2 = false;
+
+#if HAVE_LIBDEBUGINFOD
+          const struct bfd_build_id *build_id;
+          char *symfile_path;
+
+          build_id = build_id_bfd_get (objfile->obfd);
+
+          /* Allow debuginfod to abort the download if SIGINT is raised.  */
+          debuginfod_set_progressfn (
+            [] (long a, long b) { return 1 ? check_quit_flag () : 0; }
+          );
+
+          /* Query debuginfod servers for symfile.  */
+          scoped_fd fd (debuginfod_find_debuginfo (build_id->data,
+                                                   build_id->size,
+                                                   &symfile_path));
+
+          if (fd.get () >= 0)
+	     {
+              /* file successfully retrieved from server.  */
+              gdb_bfd_ref_ptr debug_bfd (symfile_bfd_open (symfile_path));
+
+              symbol_file_add_separate (debug_bfd.get (), symfile_path,
+                                        symfile_flags, objfile);
+              xfree (symfile_path);
+              has_dwarf2 = true;
+            }
+#endif /* LIBDEBUGINFOD */
+        }
     }
 
   /* Read the CTF section only if there is no DWARF info.  */
diff --git a/gdb/source.c b/gdb/source.c
index 9f53d654f3..4e8a6558b7 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -30,6 +30,10 @@
 
 #include <sys/types.h>
 #include <fcntl.h>
+#include "build-id.h"
+#ifdef HAVE_LIBDEBUGINFOD
+#include <elfutils/debuginfod.h>
+#endif
 #include "gdbcore.h"
 #include "gdb_regex.h"
 #include "symfile.h"
@@ -1127,6 +1131,49 @@ open_source_file (struct symtab *s)
   s->fullname = NULL;
   scoped_fd fd = find_and_open_source (s->filename, SYMTAB_DIRNAME (s),
 				       &fullname);
+
+#if HAVE_LIBDEBUGINFOD
+  if (fd.get () < 0 && SYMTAB_COMPUNIT (s) != NULL)
+    {
+      const struct bfd_build_id *build_id;
+      const objfile *ofp = COMPUNIT_OBJFILE (SYMTAB_COMPUNIT (s));
+
+      std::string srcpath;
+      if (IS_DIR_SEPARATOR (s->filename[0]))
+        srcpath = s->filename;
+      else
+        {
+          srcpath = SYMTAB_DIRNAME (s);
+          srcpath += SLASH_STRING;
+          srcpath += s->filename;
+        }
+
+      build_id = build_id_bfd_get (ofp->obfd);
+
+      /* Query debuginfod for the source file.  */
+      if (build_id != NULL)
+        {
+          char *name_in_cache;
+
+          /* Allow debuginfod to abort the download if SIGINT is raised.  */
+          debuginfod_set_progressfn (
+            [] (long a, long b) { return 1 ? check_quit_flag () : 0; }
+          );
+
+          scoped_fd src_fd (debuginfod_find_source (build_id->data,
+                                                    build_id->size,
+                                                    srcpath.c_str (),
+                                                    &name_in_cache));
+
+          if (src_fd.get () >= 0)
+            fullname.reset (name_in_cache);
+
+          s->fullname = fullname.release ();
+          return src_fd;
+        }
+    }
+#endif /* HAVE_LIBDEBUGINFOD */
+
   s->fullname = fullname.release ();
   return fd;
 }
Simon Marchi Nov. 5, 2019, 5:27 a.m. | #12
On 2019-11-04 8:59 p.m., Aaron Merey wrote:
> On Mon, Nov 4, 2019 at 11:26 AM Frank Ch. Eigler <fche@redhat.com> wrote:

>>

>> Hi -

>>

>>> I haven't followed this thread closely, so it might be an obvious "no", but

>>> I was wondering if the library could offer to install a callback that is call

>>> relatively often, allowing GDB to interrupt the operation [...]

>>

>> That could also work, at the cost of extending the API.  Will play with it.

>>

>> - FChE

> 

> Attached is the updated debuginfo and source lookup code, using the

> callback idea that Simon suggested. Downloads are now cancelled upon

> SIGINT and the callback can be modified with code that prints progress

> messages (the a and b parameters in the callback represent the

> numerator and denominator of the download's completion fraction).


Cool, thanks!

Maybe a user_data void pointer would be useful, to be able to get some
context in the callback?

I am noticing that debuginfod calls don't have any "context" or "handle"
parameters, which could suggest that the state of the library (if there
is any) is kept as global variables.  Is it safe to use the library to
do lookups in parallel, from different threads?  It is possible that
GDB or another program will want to do that some day.

Simon
Simon Marchi Nov. 5, 2019, 5:35 a.m. | #13
On 2019-11-04 8:59 p.m., Aaron Merey wrote:
> Attached is the updated debuginfo and source lookup code, using the

> callback idea that Simon suggested. Downloads are now cancelled upon

> SIGINT and the callback can be modified with code that prints progress

> messages (the a and b parameters in the callback represent the

> numerator and denominator of the download's completion fraction).


I took a peek at the debuginfod code and have one quick suggestion.  It would
help to understand what is what if the debuginfod was subdivided in "client" and
"server" directories, and perhaps a "common" directory, if there's any code
to share between them.

Simon
Frank Ch. Eigler Nov. 5, 2019, 10:25 a.m. | #14
Hi -

> Maybe a user_data void pointer would be useful, to be able to get some

> context in the callback?


In your case, with a single-threaded app with C++ lambda captured
variables, you wouldn't need the library to do that.  In a
multithreaded app's case, __thread variables and the promise to call
those functions back from the calling thread should again let one get
context if required.

> I am noticing that debuginfod calls don't have any "context" or "handle"

> parameters, which could suggest that the state of the library (if there

> is any) is kept as global variables.  [...]


It turns out it's the opposite: except for this callback function
pointer, it's practically all local variables therefore multithread-safe.

- FChE
Simon Marchi Nov. 5, 2019, 3:19 p.m. | #15
On 2019-11-05 5:25 a.m., Frank Ch. Eigler wrote:
> Hi -

> 

>> Maybe a user_data void pointer would be useful, to be able to get some

>> context in the callback?

> 

> In your case, with a single-threaded app with C++ lambda captured

> variables, you wouldn't need the library to do that.  In a

> multithreaded app's case, __thread variables and the promise to call

> those functions back from the calling thread should again let one get

> context if required.


Hmm does it actually work to capture some variables in the C++ lambda?

Given that the callback is declared as a simple function pointer:

  64 typedef int (*debuginfod_progressfn_t)(long a, long b);
  65 debuginfod_progressfn_t debuginfod_set_progressfn(debuginfod_progressfn_t fn);

I'm not sure that we can pass a lambda that captures stuff.  Just like this program:

    static void func(void (*callback)(void)) { callback(); }
    int main() { int a; func([a] { }); return 0; }

Generates this error:

    test.cpp: In function ‘int main()’:
    test.cpp:2:33: error: cannot convert ‘main()::<lambda()>’ to ‘void (*)()’
        2 | int main() { int a; func([a] { }); return 0; }
          |                                 ^
          |                                 |
          |                                 main()::<lambda()>
    test.cpp:1:25: note:   initializing argument 1 of ‘void func(void (*)())’
        1 | static void func(void (*callback)(void)) { callback(); }
          |                  ~~~~~~~^~~~~~~~~~~~~~~

>> I am noticing that debuginfod calls don't have any "context" or "handle"

>> parameters, which could suggest that the state of the library (if there

>> is any) is kept as global variables.  [...]


I think that providing a void pointer for context is pretty standard thing
for C libraries that accept callbacks.  I would use __thread and the promise
to call the function back in the same thread to "retro-fit" some thread-safety
in an existing API that doesn't provide a context pointer, and that can't be
changed.  But if the API is new, I think the context pointer solution is more
obvious for the user.

> It turns out it's the opposite: except for this callback function

> pointer, it's practically all local variables therefore multithread-safe.


Ok, well I think it would be a good reason to adopt an API of the style:

    debuginfod_client *client = debuginfod_create_client ();
    debuginfod_set_progressfn (client, my_progress_cb);

and where you pass `client` to all further calls.  That client object would
keep the callback for those calls, but also any other state that you might
want to keep across in the future.  Again, this is easy to add in the
beginning, but hard to retro-fit later on.

Simon
Frank Ch. Eigler Nov. 5, 2019, 3:50 p.m. | #16
Hi, Simon -

> [...]

> I think that providing a void pointer for context is pretty standard thing

> for C libraries that accept callbacks.  I would use __thread and the promise

> to call the function back in the same thread to "retro-fit" some thread-safety

> in an existing API that doesn't provide a context pointer, 


Remember that this is a batch file downloading tool.  We expect it to
be interrupted if a user gets impatient, in which case such a signal
would affect all downloads.  In the absence of a plausible user story
for the need for fine control over multiple different debuginfo
transfers in progress, I believe something simple is enough for now.


> Ok, well I think it would be a good reason to adopt an API of the

> style: [...]  Again, this is easy to add in the beginning, but hard

> to retro-fit later on.


Retro-fitting it later on is not that bad, when/if actual use cases
appear.  The default 'client context' can be the current global one.
A hypothetical future api can pass void* context parameters; we can
have two callback function types.  It being simple now does not
preclude necessary complexity later.  I appreciate the ideas, really,
but I am about as wary of overengineering as underengineering.

- FChE
Aaron Merey Nov. 7, 2019, 11:24 p.m. | #17
Here's a patch that adds debuginfod queries for dwz files when they
otherwise cannot be found. It's very similar to the normal debuginfo
lookup code. With this libdebuginfod now hooks into three functions
in gdb: dwarf2_get_dwz_file, open_source_file and elf_symfile_read.
Does anyone know of any code paths that gdb might take when
searching for dwz, source files, or separate DWARF debuginfo that
bypass all of the current libdebuginfod calls?

Aaron
---
 gdb/dwarf2read.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 0a7a033420..d0cc5f327b 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -77,6 +77,9 @@
 #include "gdbsupport/selftest.h"
 #include "rust-lang.h"
 #include "gdbsupport/pathstuff.h"
+#if HAVE_LIBDEBUGINFOD
+#include "elfutils/debuginfod.h"
+#endif
 
 /* When == 1, print basic high level tracing messages.
    When > 1, be more verbose.
@@ -2714,6 +2717,38 @@ dwarf2_get_dwz_file (struct dwarf2_per_objfile *dwarf2_per_objfile)
   if (dwz_bfd == NULL)
     dwz_bfd = build_id_to_debug_bfd (buildid_len, buildid);
 
+#if HAVE_LIBDEBUGINFOD
+  if (dwz_bfd == NULL)
+    {
+      /* Query debuginfod servers for the dwz file.  */
+      char *alt_filename;
+
+      /* Allow debuginfod to abort the download if SIGINT is raised.  */
+      debuginfod_set_progressfn(
+        [] (long a, long b) { return 1 ? check_quit_flag () : 0; }
+      );
+
+      /* Query debuginfod servers for symfile.  */
+      scoped_fd fd (debuginfod_find_debuginfo (buildid,
+                                               buildid_len,
+                                               &alt_filename));
+
+      if (fd.get () >= 0)
+        {
+          /* File successfully retrieved from server.  */
+          dwz_bfd = gdb_bfd_open (alt_filename, gnutarget, -1);
+
+          if(dwz_bfd != NULL)
+            {
+              if (!build_id_verify (dwz_bfd.get (), buildid_len, buildid))
+                dwz_bfd.reset (nullptr);
+            }
+
+          xfree (alt_filename);
+        }
+    }
+#endif /* HAVE_LIBDEBUGINFOD */
+
   if (dwz_bfd == NULL)
     error (_("could not find '.gnu_debugaltlink' file for %s"),
 	   objfile_name (dwarf2_per_objfile->objfile));

Patch

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index d5d095aae4..3149e0ea53 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -210,6 +210,9 @@  INTL = @LIBINTL@
 INTL_DEPS = @LIBINTL_DEP@
 INTL_CFLAGS = @INCINTL@
 
+# Where is the dbgserver library, if any?
+LIBDBGSERVER = @LIBDBGSERVER@
+
 # Where is the ICONV library?  This will be empty if in libc or not available.
 LIBICONV = @LIBICONV@
 
@@ -593,7 +596,7 @@  CLIBS = $(SIM) $(READLINE) $(OPCODES) $(BFD) $(ZLIB) $(INTL) $(LIBIBERTY) $(LIBD
 	@LIBS@ @GUILE_LIBS@ @PYTHON_LIBS@ \
 	$(LIBEXPAT) $(LIBLZMA) $(LIBBABELTRACE) $(LIBIPT) \
 	$(LIBIBERTY) $(WIN32LIBS) $(LIBGNU) $(LIBICONV) $(LIBMPFR) \
-	$(SRCHIGH_LIBS)
+	$(SRCHIGH_LIBS) $(LIBDBGSERVER)
 CDEPS = $(NAT_CDEPS) $(SIM) $(BFD) $(READLINE_DEPS) \
 	$(OPCODES) $(INTL_DEPS) $(LIBIBERTY) $(CONFIG_DEPS) $(LIBGNU)
 
diff --git a/gdb/config.in b/gdb/config.in
index 26ca02f6a3..f2f4b38bd7 100644
--- a/gdb/config.in
+++ b/gdb/config.in
@@ -231,6 +231,9 @@ 
 /* Define if you have the expat library. */
 #undef HAVE_LIBEXPAT
 
+/* Define if you have the dbgserver library. */
+#undef HAVE_LIBDBGSERVER
+
 /* Define to 1 if you have the `libiconvlist' function. */
 #undef HAVE_LIBICONVLIST
 
diff --git a/gdb/configure.ac b/gdb/configure.ac
index 5a18c16405..a224c7df81 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -327,6 +327,32 @@  case $host_os in
     enable_gdbtk=no ;;
 esac
 
+# Enable dbgserver
+AC_ARG_WITH([dbgserver],
+        AC_HELP_STRING([--with-dbgserver],
+                       [Enable debuginfo and source-file lookups with dbgserver (auto/yes/no)]),
+        [], [with_dbgserver=auto])
+AC_MSG_CHECKING([whether to use dbgserver])
+AC_MSG_RESULT([$with_dbgserver])
+
+if test "${with_dbgserver}" = no; then
+  AC_MSG_WARN([dbgserver support disabled; some features may be unavailable.])
+  HAVE_LIBDBGSERVER=no
+else
+  AC_LIB_HAVE_LINKFLAGS([dbgserver], [], [#include <elfutils/dbgserver-client.h>],
+                                     [const unsigned char buildid;
+                                     int buildid_len = 1;
+                                     char *path;
+                                     path = dbgserver_find_debuginfo (& buildid, buildid_len, &path);])
+  if test "$HAVE_LIBDBGSERVER" != yes; then
+    if test "$with_dbgserver" = yes; then
+      AC_MSG_ERROR([dbgserver is missing or unusable])
+    else
+      AC_MSG_WARN([dbgserver is missing or unusable; some features may be unavailable.])
+    fi
+  fi
+fi
+
 # Libunwind support for ia64.
 
 AC_ARG_WITH(libunwind-ia64,
diff --git a/gdb/elfread.c b/gdb/elfread.c
index 630550b80d..aa81ddf3c3 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -47,6 +47,9 @@ 
 #include "location.h"
 #include "auxv.h"
 #include "mdebugread.h"
+#if HAVE_LIBDBGSERVER
+#include <elfutils/dbgserver-client.h>
+#endif
 
 /* Forward declarations.  */
 extern const struct sym_fns elf_sym_fns_gdb_index;
@@ -1296,6 +1299,30 @@  elf_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
 	  symbol_file_add_separate (debug_bfd.get (), debugfile.c_str (),
 				    symfile_flags, objfile);
 	}
+#if HAVE_LIBDBGSERVER
+      else
+        {
+          const struct bfd_build_id *build_id;
+          char *debugfile_path;
+
+          build_id = build_id_bfd_get (objfile->obfd);
+          int fd = dbgserver_find_debuginfo (build_id->data,
+                                             build_id->size,
+                                             &debugfile_path);
+
+          if (fd >= 0)
+            {
+              /* debuginfo successfully retrieved from server, reopen
+                 the file as a bfd instead.  */
+              gdb_bfd_ref_ptr debug_bfd (symfile_bfd_open (debugfile_path));
+
+              symbol_file_add_separate(debug_bfd.get (), debugfile_path,
+                                       symfile_flags, objfile);
+              close(fd);
+              free(debugfile_path);
+            }
+        }
+#endif /* LIBDBGSERVER */
     }
 }
 
diff --git a/gdb/source.c b/gdb/source.c
index b27f210802..0150fd7c42 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -30,6 +30,10 @@ 
 
 #include <sys/types.h>
 #include <fcntl.h>
+#include "build-id.h"
+#ifdef HAVE_LIBDBGSERVER
+#include <elfutils/dbgserver-client.h>
+#endif
 #include "gdbcore.h"
 #include "gdb_regex.h"
 #include "symfile.h"
@@ -1060,6 +1064,58 @@  open_source_file (struct symtab *s)
   s->fullname = NULL;
   scoped_fd fd = find_and_open_source (s->filename, SYMTAB_DIRNAME (s),
 				       &fullname);
+
+#if HAVE_LIBDBGSERVER
+  if (fd.get() < 0)
+    {
+      if (SYMTAB_COMPUNIT(s) != NULL)
+        {
+          const struct bfd_build_id *build_id;
+          const objfile *ofp = COMPUNIT_OBJFILE (SYMTAB_COMPUNIT (s));
+
+          /* prefix the comp_dir to relative file names */
+          const char* dirname = SYMTAB_DIRNAME (s);
+          int suffname_len = strlen(dirname) + strlen(s->filename) + 2;
+          char *suffname = (char *) alloca(suffname_len);
+          if (IS_DIR_SEPARATOR (s->filename[0]))
+            xsnprintf (suffname, suffname_len, "%s", s->filename);
+          else
+            {
+              xsnprintf (suffname, suffname_len, "%s%s%s", dirname,
+                         SLASH_STRING, s->filename);
+            }
+
+          build_id = build_id_bfd_get (ofp->obfd);
+
+          /* Query debuginfo-server for the source file.  */
+          if (build_id != NULL)
+            {
+              char *name_in_cache;
+              int dbgsrv_rc = dbgserver_find_source (build_id->data,
+                                                     build_id->size,
+                                                     suffname,
+                                                     &name_in_cache);
+              if (dbgsrv_rc >= 0)
+                {
+                  fullname.reset (xstrdup(name_in_cache));
+                  free (name_in_cache);
+                }
+              else if (dbgsrv_rc == -ENOSYS)
+                {
+                  /* -ENOSYS indicates that libdbgserver could not find
+                     any dbgserver URLs to query due to $DBGSERVER_URLS
+                     not being defined. Replace -ENOSYS with -ENOENT so
+                     that users who have not configured dbgserver see the
+                     usual error message when a source file cannot be found.  */
+                  dbgsrv_rc = -ENOENT;
+                }
+              s->fullname = fullname.release ();
+              return scoped_fd(dbgsrv_rc);
+            }
+        }
+    }
+#endif /* HAVE_LIBDBGSERVER */
+
   s->fullname = fullname.release ();
   return fd;
 }
diff --git a/gdb/testsuite/gdb.dbgserver/fetch_src_and_symbols.c b/gdb/testsuite/gdb.dbgserver/fetch_src_and_symbols.c
new file mode 100644
index 0000000000..76e8197013
--- /dev/null
+++ b/gdb/testsuite/gdb.dbgserver/fetch_src_and_symbols.c
@@ -0,0 +1 @@ 
+int main() { return 0; }
diff --git a/gdb/testsuite/gdb.dbgserver/fetch_src_and_symbols.exp b/gdb/testsuite/gdb.dbgserver/fetch_src_and_symbols.exp
new file mode 100644
index 0000000000..9d21d6a96d
--- /dev/null
+++ b/gdb/testsuite/gdb.dbgserver/fetch_src_and_symbols.exp
@@ -0,0 +1,93 @@ 
+# Copyright 2010-2019 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# test dbgserver functionality
+
+standard_testfile
+
+# skip testing if dbgserver cannot be found
+if { [which dbgserver] == 0 } {
+    untested "cannot find dbgserver"
+    return -1
+}
+
+# skip testing if gdb was not configured with dbgserver
+if { [string first "with-dbgserver" [exec $GDB --configuration]] == -1 } {
+    untested "GDB not configured with dbgserver"
+    return -1
+}
+
+set cache [file join [standard_output_file {}] ".client_cache"]
+
+# make sure there isn't an old client cache lying around
+file delete -force $cache
+
+# make a copy source file that we can move around
+if { [catch {file copy -force ${srcdir}/${subdir}/${srcfile} \
+                              [standard_output_file tmp-${srcfile}]}] != 0 } {
+    error "Could not create temporary file"
+    return -1
+}
+
+set sourcetmp [standard_output_file tmp-${srcfile}]
+set outputdir [standard_output_file {}]
+
+if { [gdb_compile "${sourcetmp}" "${binfile}" executable {debug}] != "" } {
+    return -1
+}
+
+set port 58002
+set ::env(DBGSERVER_URLS) "localhost:$port"
+set ::env(DBGSERVER_TIMEOUT) 10
+set ::env(DBGSERVER_CACHE_PATH) $cache
+
+# test that gdb cannot find source without dbgserver
+gdb_start
+gdb_load $binfile
+gdb_test_no_output "set substitute-path $outputdir /dev/null"
+gdb_test "l" ".*Connection refused\."
+gdb_exit
+
+# strip symbols into separate file and move it so gdb cannot find it without dbgserver
+gdb_gnu_strip_debug $binfile
+
+set debugdir [file join [standard_output_file {}] "debug"]
+set debuginfo [file join [standard_output_file {}] "fetch_src_and_symbols.debug"]
+
+file mkdir -force $debugdir
+file copy -force $debuginfo $debugdir
+file delete -force $debuginfo
+
+# test that gdb cannot find symbols without dbgserver
+gdb_start
+gdb_load $binfile
+gdb_test "file" ".*No symbol file.*"
+gdb_exit
+
+# start up dbgserver
+
+set dbgserver_pid [exec dbgserver -p $port -F $debugdir >/dev/null 2>&1 &]
+sleep 5
+
+# gdb should now be able to find the symbol and source files
+gdb_start
+gdb_load $binfile
+gdb_test_no_output "set substitute-path $outputdir /dev/null"
+gdb_test "br main" "Breakpoint 1 at.*file.*"
+gdb_test "l" "int main().*return 0;.*"
+gdb_exit
+
+file delete -force $cache
+exec kill $dbgserver_pid
diff --git a/gdb/top.c b/gdb/top.c
index 9d4ce1fa3b..a1715a9910 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -1480,6 +1480,15 @@  This GDB was configured as follows:\n\
              --without-python\n\
 "));
 #endif
+#if HAVE_LIBDBGSERVER
+  fprintf_filtered (stream, _("\
+             --with-dbgserver\n\
+"));
+#else
+   fprintf_filtered (stream, _("\
+             --without-dbgserver\n\
+"));
+#endif
 #if HAVE_GUILE
   fprintf_filtered (stream, _("\
              --with-guile\n\