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

[Simon Marchi <simark@simark.ca>]

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