Re: [PATCH] gnulib: Ensure all libraries are used when building gdb/gdbserver

Simon Marchi
2020-10-05T18:35:03Z

On 2020-10-05 12:51 p.m., Andrew Burgess wrote:
> An issue was reported here related to building GDB on MinGW:
>
> https://sourceware.org/pipermail/gdb/2020-September/048927.html
>
> It was suggested here:
>
> https://sourceware.org/pipermail/gdb/2020-September/048931.html
>
> that the solution might be to make use of $(LIB_GETRANDOM), a variable
> defined in the gnulib makefile, when linking GDB.
>
> In fact I think the issue is bigger than just LIB_GETRANDOM. When
> using the script binutils-gdb/gnulib/update-gnulib.sh to reimport
> gnulib there is a lot of output from gnulib's gnulib-tool. Part of
> that output is this:
>
> You may need to use the following makefile variables when linking.
> Use them in <program>_LDADD when linking a program, or
> in <library>_a_LDFLAGS or <library>_la_LDFLAGS when linking a library.
> $(FREXPL_LIBM)
> $(FREXP_LIBM)
> $(INET_NTOP_LIB)
> $(LIBTHREAD)
> $(LIB_GETLOGIN)
> $(LIB_GETRANDOM)
> $(LIB_HARD_LOCALE)
> $(LIB_MBRTOWC)
> $(LIB_SETLOCALE_NULL)
> $(LTLIBINTL) when linking with libtool, $(LIBINTL) otherwise
>
> What I think this is telling us is that we should be including the
> value of all these variables on the link line for gdb and gdbserver.
>
> The problem though is that these variables are define in gnulib's
> makefile, but are not (necessarily) defined in GDB's makefile.
>
> One solution would be to recreate the checks that gnulib performs in
> order to recreate these variables in both gdb's and gdbserver's
> makefile. Though this shouldn't be too hard, most (if not all) of
> these checks are in the form macros defined in m4 files in the gnulib
> tree, so we could just reference these as needed. However, in this
> commit I propose a different solution.
>
> Currently, in the top level makefile, we give gdb and gdbserver a
> dependency on gnulib. Once gnulib has finished building gdb and
> gdbserver can start, these projects then have a hard coded (relative)
> path to the compiled gnulib library in their makefiles.
>
> In this commit I extend the gnulib makefile so that after building the
> gnulib library we generate a makefile fragment that defines a number
> of variables, these variables are:
>
> LIBGNU: The path to the archive containing gnulib. Can be used as a
> dependency as when this file changes gdb/gdbserver should be
> relinked.
>
> LIBGNU_EXTRA_LIBS: A list of linker -l.... flags that should be
> included in the link line of gdb/gdbserver. These are
> libraries that $(LIBGNU) depends on.
>
> INCGNU: A list of -I.... include paths that should be passed to the
> compiler, these are where the gnulib headers can be found.
>
> Now both gdb and gdbserver can include the makefile fragment and make
> use of these variables. As the makefile fragment is generated from
> within gnulib's makefile, it has full access to the variable list
> given above.
>
> The generated makefile fragment relies on the variable GNULIB_BUILDDIR
> being defined. This is checked for in the fragment, and was already
> defined in gdb and gdbserver.
>
> gdb/ChangeLog:
>
> * Makefile.in: Include Makefile.gnulib.inc. Don't define LIBGNU
> or INCGNU. Make use of LIBGNU_EXTRA_LIBS when linking.
>
> gdbserver/ChangeLog:
>
> * Makefile.in: Include Makefile.gnulib.inc. Don't define LIBGNU
> or INCGNU. Make use of LIBGNU_EXTRA_LIBS when linking.
>
> gnulib/ChangeLog:
>
> * Makefile.am: Add rule to generate Makefile.gnulib.inc.
> * Makefile.in: Regenerate.

I really like the idea. I just have some minor comments on the
implementation:

I applied the patch, did "make clean && make -j N" as I always do, and
got this during the "make clean":

make[2]: Entering directory '/home/smarchi/build/binutils-gdb/gdbserver'
Makefile:114: ../gnulib/Makefile.gnulib.inc: No such file or directory
make[2]: *** No rule to make target '../gnulib/Makefile.gnulib.inc'. Stop.
make[2]: Leaving directory '/home/smarchi/build/binutils-gdb/gdbserver'

Would it be possible to generate Makefile.gnulib.inc during the
configure step of gnulib instead? It would ensure that the file is
generated at this moment.

> diff --git a/gnulib/Makefile.am b/gnulib/Makefile.am
> index 3732e4d0dc2..29ac3f209e4 100644
> --- a/gnulib/Makefile.am
> +++ b/gnulib/Makefile.am
> @@ -26,3 +26,39 @@
> MAKEOVERRIDES =
>
> SUBDIRS = import
> +
> +# Generate a makefile snippet that lists all of the libraries that
> +# should be pulled in when linking against gnulib. Both GDB and
> +# GDBSERVER will include this snippet.

I'd suggest adding to the comment that the list of libraries below is
scraped from the output of update-gnulib.sh. That will help people
understand where it comes from and how to keep it up to date.

> +#
> +# The defined variables are:
> +#
> +# LIBGNU: The path to the archive containing gnulib. Can be used as a
> +# dependency as when this file changes gdb/gdbserver should be
> +# relinked.
> +#
> +# LIBGNU_EXTRA_LIBS: A list of linker -l.... flags that should be
> +# included in the link line of gdb/gdbserver. These are
> +# libraries that $(LIBGNU) depends on.
> +#
> +# INCGNU: A list of -I.... include paths that should be passed to the
> +# compiler, these are where the gnulib headers can be found.
> +
> +Makefile.gnulib.inc: $(builddir)/Makefile
> + $(AM_V_GEN)
> + @rm -f Makefile.gnulib.inc
> + @echo "ifndef GNULIB_BUILDDIR" > Makefile.gnulib.inc
> + @echo "\$$(error missing GNULIB_BUILDDIR)" >> Makefile.gnulib.inc
> + @echo "endif" >> Makefile.gnulib.inc
> + @echo "" >> Makefile.gnulib.inc
> + @echo "LIBGNU = \$$(GNULIB_BUILDDIR)/import/libgnu.a" \
> + >> Makefile.gnulib.inc
> + @echo "LIBGNU_EXTRA_LIBS = $(FREXPL_LIBM) $(FREXP_LIBM) \
> + $(INET_NTOP_LIB) $(LIBTHREAD) $(LIB_GETLOGIN) \
> + $(LIB_GETRANDOM) $(LIB_HARD_LOCALE) $(LIB_MBRTOWC) \
> + $(LIB_SETLOCALE_NULL)" | sed "s/ \+$$//" \
> + >> Makefile.gnulib.inc
> + @echo "INCGNU = -I\$$(srcdir)/../gnulib/import -I\$$(GNULIB_BUILDDIR)/import" \
> + >> Makefile.gnulib.inc

With this, I think it's possible to end up with a partially written file
if you ^C at the wrong time, in the middle of these echos. We typically
write to a temporary file (e.g. Makefile.gnulib.inc.tmp) and move it (an
atomic operation) to the final location when done.

It would be nicer if this list was generated by gnulib instead. I
noticed it has an --extract-recursive-link-directive mode, which could
help for this. It gives:

$ /home/smarchi/src/gnulib/gnulib-tool --extract-recursive-link-directive --no-conditional-dependencies --no-libtool --macro-prefix=gl --no-vc-files alloca canonicalize-lgpl count-one-bits dirent dirfd errno fnmatch-gnu frexpl getcwd gettimeofday glob inet_ntop inttypes lstat limits-h memchr memmem mkdir mkdtemp mkostemp pathmax rawmemchr readlink rename setenv signal-h strchrnul strerror_r-posix strstr strtok_r sys_stat time_r unistd unsetenv update-copyright wchar wctype-h
$(LIB_MBRTOWC)
$(FREXPL_LIBM)
$(LTLIBINTL) when linking with libtool, $(LIBINTL) otherwise
$(LIB_MBRTOWC)
$(INET_NTOP_LIB)
$(LIB_GETRANDOM)
$(LIB_GETRANDOM)
$(LIBTHREAD)

It looks almost right, but some items that are in your list don't appear
here, I don't know why. Maybe I don't understand what
--extract-recursive-link-directive is supposed to do, or there's a bug.
There's also the "when linking with libtool" line that is a bit
annoying. We can always massage it, but also perhaps gnulib-tool should
understand that when you pass --no-libtool, you want $(LIBINTL).

Anyway, those are thoughts for future work, I think that what you've
done is already a step in the right direction.

Simon