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

[Aaron Merey <amerey@redhat.com>]

On Thu, Jan 9, 2020 at 10:27 AM Tom Tromey <tom@tromey.com> 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.