[PATCH 1/1] Add debuginfod support to GDB

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/1] Add debuginfod support to GDB

Aaron Merey


0001-Add-debuginfod-support-to-GDB.patch (48K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] Add debuginfod support to GDB

Eli Zaretskii
> From: Aaron Merey <[hidden email]>
> Date: Wed, 8 Jan 2020 22:48:11 -0500
>
> +* GDB now supports debuginfod, an HTTP server for distributing
> +  ELF/DWARF debugging information as well as source code. When built with
> +  debuginfod, GDB can automatically query debuginfod servers for the
> +  separate debug files and source code of the executable being debugged.
> +  To build GDB with debuginfod, pass --with-debuginfod to configure.
> +  This requires libdebuginfod, the debuginfod client library.
> +  debuginfod is distributed with elfutils, starting with version 0.178.
> +  You can get the latest version from https://sourceware.org/elfutils.

This part is OK, but please leave 2 spaces between sentences, per our
conventions.

Thanks.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] Add debuginfod support to GDB

Tom Tromey-2
In reply to this post by Aaron Merey
>>>>> "Aaron" == Aaron Merey <[hidden email]> writes:

Aaron> ChangeLog:
Aaron> 2020-01-08  Aaron Merey  <[hidden email]>

Aaron>         * config/debuginfod.m4: New file. Add macro AC_DEBUGINFOD.
Aaron>         Adds new configure option --with-debuginfod.
Aaron>         * configure: Regenerate.
Aaron>         * configure.ac: Call AC_DEBUGINFOD.

Does the top-level configure really need AC_DEBUGINFOD?

If so, then this part of the patch has to go to gcc-patches first.
But, I suspect it's not needed, in which case dropping it is more
convenient, because we can have debuginfod.m4 locally and not involve
gcc at all.

Aaron> +if test "${with_debuginfod}" = no; then
Aaron> +  AC_MSG_WARN([debuginfod support disabled; some features may be unavailable.])
Aaron> +else
Aaron> +  AC_CHECK_LIB([debuginfod], [debuginfod_begin], [have_debuginfod_lib=yes])

Does and/or should libdebuginfod use pkg-config for this kind of thing?

Aaron> diff --git a/gdb/Makefile.in b/gdb/Makefile.in

Aaron> @@ -612,7 +615,7 @@ CLIBS = $(SIM) $(READLINE) $(OPCODES) $(BFD) $(LIBCTF) $(ZLIB) \
Aaron>   @LIBS@ @GUILE_LIBS@ @PYTHON_LIBS@ \
Aaron>   $(LIBEXPAT) $(LIBLZMA) $(LIBBABELTRACE) $(LIBIPT) \
Aaron>   $(LIBIBERTY) $(WIN32LIBS) $(LIBGNU) $(LIBICONV) $(LIBMPFR) \
Aaron> - $(SRCHIGH_LIBS) $(LIBXXHASH) $(PTHREAD_LIBS)
Aaron> + $(SRCHIGH_LIBS) $(LIBXXHASH) $(PTHREAD_LIBS) $(LIBDEBUGINFOD)

Should it be before PTHREAD_LIBS?
If not, this is fine.

Aaron> +          /* Allow debuginfod to abort the download if SIGINT is raised.  */
Aaron> +          debuginfod_set_progressfn(

gdb style puts a space before the paren, and usually the paren is kept
next to the arguments if a newline is needed.

Aaron> +            client,
Aaron> +            [] (debuginfod_client *c, long a, long b)
Aaron> +              { return 1 ? check_quit_flag () : 0; }

This looks weird, because there's no need for the "1 ?" part.  Maybe it
should be just:

    return check_quit_flag ();

A lot of this code is duplicated in 3 places.  I think it would be
better to have a helper function to consolidate the shared code.

Aaron> +# Copyright 2010-2019 Free Software Foundation, Inc.

Probably just 2020.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] Add debuginfod support to GDB

Aaron Merey
On Thu, Jan 9, 2020 at 10:27 AM Tom Tromey <[hidden email]> wrote:

> Aaron>         * config/debuginfod.m4: New file. Add macro AC_DEBUGINFOD.
> Aaron>         Adds new configure option --with-debuginfod.
> Aaron>         * configure: Regenerate.
> Aaron>         * configure.ac: Call AC_DEBUGINFOD.
>
> Does the top-level configure really need AC_DEBUGINFOD?
>
> If so, then this part of the patch has to go to gcc-patches first.
> But, I suspect it's not needed, in which case dropping it is more
> convenient, because we can have debuginfod.m4 locally and not involve
> gcc at all.

The reason for the top-level AC_DEBUGINFOD is to prevent the top-level
configure check from succeeding in cases where --with-debuginfod is
given but the debuginfod library or header cannot be found.

I should also mention that these same top level changes were also
included in a patch that adds debuginfod support to binutils and that
patch has been merged.

https://sourceware.org/ml/binutils/2020-01/msg00090.html

(I wasn't sure which list to post top-level changes to so I included
them in both patches so that each will work independently.)

> Aaron> +if test "${with_debuginfod}" = no; then
> Aaron> +  AC_MSG_WARN([debuginfod support disabled; some features may be unavailable.])
> Aaron> +else
> Aaron> +  AC_CHECK_LIB([debuginfod], [debuginfod_begin], [have_debuginfod_lib=yes])
>
> Does and/or should libdebuginfod use pkg-config for this kind of thing?

I can change this to use pkg-config.

>
> Aaron> @@ -612,7 +615,7 @@ CLIBS = $(SIM) $(READLINE) $(OPCODES) $(BFD) $(LIBCTF) $(ZLIB) \
> Aaron>          @LIBS@ @GUILE_LIBS@ @PYTHON_LIBS@ \
> Aaron>          $(LIBEXPAT) $(LIBLZMA) $(LIBBABELTRACE) $(LIBIPT) \
> Aaron>          $(LIBIBERTY) $(WIN32LIBS) $(LIBGNU) $(LIBICONV) $(LIBMPFR) \
> Aaron> -        $(SRCHIGH_LIBS) $(LIBXXHASH) $(PTHREAD_LIBS)
> Aaron> +        $(SRCHIGH_LIBS) $(LIBXXHASH) $(PTHREAD_LIBS) $(LIBDEBUGINFOD)
>
> Should it be before PTHREAD_LIBS?
> If not, this is fine.

This should be ok.

> Aaron> +          /* Allow debuginfod to abort the download if SIGINT is raised.  */
> Aaron> +          debuginfod_set_progressfn(
>
> gdb style puts a space before the paren, and usually the paren is kept
> next to the arguments if a newline is needed.
>
> Aaron> +            client,
> Aaron> +            [] (debuginfod_client *c, long a, long b)
> Aaron> +              { return 1 ? check_quit_flag () : 0; }
>
> This looks weird, because there's no need for the "1 ?" part.  Maybe it
> should be just:
>
>     return check_quit_flag ();
>
> A lot of this code is duplicated in 3 places.  I think it would be
> better to have a helper function to consolidate the shared code.

Agreed, is gdb/gdbsupport/ a good place for this helper?

> Aaron> +# Copyright 2010-2019 Free Software Foundation, Inc.
>
> Probably just 2020.

Sure.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] Add debuginfod support to GDB

Sourceware - gdb-patches mailing list
On Thu, Jan 9, 2020 at 4:41 PM Aaron Merey <[hidden email]> wrote:

>
> On Thu, Jan 9, 2020 at 10:27 AM Tom Tromey <[hidden email]> wrote:
> > Aaron>         * config/debuginfod.m4: New file. Add macro AC_DEBUGINFOD.
> > Aaron>         Adds new configure option --with-debuginfod.
> > Aaron>         * configure: Regenerate.
> > Aaron>         * configure.ac: Call AC_DEBUGINFOD.
> >
> > Does the top-level configure really need AC_DEBUGINFOD?
> >
> > If so, then this part of the patch has to go to gcc-patches first.
> > But, I suspect it's not needed, in which case dropping it is more
> > convenient, because we can have debuginfod.m4 locally and not involve
> > gcc at all.
>
> The reason for the top-level AC_DEBUGINFOD is to prevent the top-level
> configure check from succeeding in cases where --with-debuginfod is
> given but the debuginfod library or header cannot be found.

Why is debuginfo special in that way? There are a lot of other
libraries in the same situation.


Other comments:
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 04979f3d12..f9c06b8cc6 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -2708,6 +2711,45 @@ dwarf2_get_dwz_file (struct dwarf2_per_objfile
*dwarf2_per_objfile)
+      char *alt_filename;
+      debuginfod_client *client;
+
+      client = debuginfod_begin ();

Now that GDB is C++, I'd move the declarations to where the variables
are used, e.g.:
  debuginfod_client *client = debuginfod_begin ();

+      if (client != NULL)

nullptr

+++ b/gdb/elfread.c

same here

Don't you also need to update gdb/README (the `configure' options
section) and doc/gdb.texinfo (@node Configure Options)?

Christian
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] Add debuginfod support to GDB

Tom Tromey-2
In reply to this post by Aaron Merey
>>>>> "Aaron" == Aaron Merey <[hidden email]> writes:

Aaron> The reason for the top-level AC_DEBUGINFOD is to prevent the top-level
Aaron> configure check from succeeding in cases where --with-debuginfod is
Aaron> given but the debuginfod library or header cannot be found.

As Christian pointed out, this isn't done in other cases.

Aaron> I should also mention that these same top level changes were also
Aaron> included in a patch that adds debuginfod support to binutils and that
Aaron> patch has been merged.

Ok.  Well, if you want to keep that, then you'll also have to submit the
top-level changes to GCC and get them approved there.  gcc and
binutils-gdb share the top-level configury.  It's canonically maintained
in the gcc repository; on occasion we do land a patch in binutils-gdb
first, but normally the process is to land there first.

>> A lot of this code is duplicated in 3 places.  I think it would be
>> better to have a helper function to consolidate the shared code.

Aaron> Agreed, is gdb/gdbsupport/ a good place for this helper?

No, since gdbserver won't use this code.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] Add debuginfod support to GDB

Aaron Merey
In reply to this post by Sourceware - gdb-patches mailing list
On Thu, Jan 9, 2020 at 6:28 PM Christian Biesinger
<[hidden email]> wrote:
> On Thu, Jan 9, 2020 at 4:41 PM Aaron Merey <[hidden email]> wrote:
> > The reason for the top-level AC_DEBUGINFOD is to prevent the top-level
> > configure check from succeeding in cases where --with-debuginfod is
> > given but the debuginfod library or header cannot be found.
>
> Why is debuginfo special in that way? There are a lot of other
> libraries in the same situation.

It was recommended to me on binutils@ that the top-level configure check
should fail if --with-debuginfod is given but not installed.

https://sourceware.org/ml/binutils/2019-11/msg00371.html

Despite that I can remove the top-level AC_DEBUGINFOD to match the
behavior of other configure options.

> Now that GDB is C++, I'd move the declarations to where the variables
> are used, e.g.:
>   debuginfod_client *client = debuginfod_begin ();
>
> +      if (client != NULL)
>
> nullptr
>
> +++ b/gdb/elfread.c
>
> same here

Ok.

> Don't you also need to update gdb/README (the `configure' options
> section) and doc/gdb.texinfo (@node Configure Options)?

gdb.texinfo was updated in this patch but not README, will fix that.

Aaron

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] Add debuginfod support to GDB

Aaron Merey
In reply to this post by Tom Tromey-2
On Fri, Jan 10, 2020 at 9:23 AM Tom Tromey <[hidden email]> wrote:
> Ok.  Well, if you want to keep that, then you'll also have to submit the
> top-level changes to GCC and get them approved there.  gcc and
> binutils-gdb share the top-level configury.  It's canonically maintained
> in the gcc repository; on occasion we do land a patch in binutils-gdb
> first, but normally the process is to land there first.

Ok good to know. Like you said it's more convenient to have debuginfod.m4
local to gdb. But since the binutils configury uses it too, maybe it's worth
submitting this change to GCC to minimize duplication.

> >> A lot of this code is duplicated in 3 places.  I think it would be
> >> better to have a helper function to consolidate the shared code.
>
> Aaron> Agreed, is gdb/gdbsupport/ a good place for this helper?
>
> No, since gdbserver won't use this code.

Ok how about gdb/debuginfod-support.h then?

Aaron

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] Add debuginfod support to GDB

Tom Tromey-2
>>>>> "Aaron" == Aaron Merey <[hidden email]> writes:

Aaron> Ok how about gdb/debuginfod-support.h then?

I think a .c file is more appropriate.

Tom