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

public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed

* [PATCH] gnulib: Ensure all libraries are used when building gdb/gdbserver
@ 2020-10-05 16:51 Andrew Burgess
  2020-10-05 18:34 ` Simon Marchi
  0 siblings, 1 reply; 13+ messages in thread
From: Andrew Burgess @ 2020-10-05 16:51 UTC (permalink / raw)
  To: gdb-patches

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.
---
 gdb/ChangeLog         |  5 +++++
 gdb/Makefile.in       |  5 ++---
 gdbserver/ChangeLog   |  5 +++++
 gdbserver/Makefile.in | 11 +++++------
 gnulib/ChangeLog      |  5 +++++
 gnulib/Makefile.am    | 36 ++++++++++++++++++++++++++++++++++++
 gnulib/Makefile.in    | 40 ++++++++++++++++++++++++++++++++++++++--
 7 files changed, 96 insertions(+), 11 deletions(-)

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index dbede7a9cfc..18a426dc225 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -239,8 +239,7 @@ GDBFLAGS =
 
 # Helper code from gnulib.
 GNULIB_BUILDDIR = ../gnulib
-LIBGNU = $(GNULIB_BUILDDIR)/import/libgnu.a
-INCGNU = -I$(srcdir)/../gnulib/import -I$(GNULIB_BUILDDIR)/import
+include $(GNULIB_BUILDDIR)/Makefile.gnulib.inc
 
 SUPPORT = ../gdbsupport
 LIBSUPPORT = $(SUPPORT)/libgdbsupport.a
@@ -627,7 +626,7 @@ CLIBS = $(SIM) $(READLINE) $(OPCODES) $(LIBCTF) $(BFD) $(ZLIB) \
 	$(XM_CLIBS) $(GDBTKLIBS) \
 	@LIBS@ @GUILE_LIBS@ @PYTHON_LIBS@ \
 	$(LIBEXPAT) $(LIBLZMA) $(LIBBABELTRACE) $(LIBIPT) \
-	$(WIN32LIBS) $(LIBGNU) $(LIBICONV) \
+	$(WIN32LIBS) $(LIBGNU) $(LIBGNU_EXTRA_LIBS) $(LIBICONV) \
 	$(LIBMPFR) $(SRCHIGH_LIBS) $(LIBXXHASH) $(PTHREAD_LIBS) \
 	$(DEBUGINFOD_LIBS)
 CDEPS = $(NAT_CDEPS) $(SIM) $(BFD) $(READLINE_DEPS) $(CTF_DEPS) \
diff --git a/gdbserver/Makefile.in b/gdbserver/Makefile.in
index b0bad0cdb09..54b3f3becc4 100644
--- a/gdbserver/Makefile.in
+++ b/gdbserver/Makefile.in
@@ -111,8 +111,7 @@ ustinc = @ustinc@
 
 # gnulib
 GNULIB_BUILDDIR = ../gnulib
-LIBGNU = $(GNULIB_BUILDDIR)/import/libgnu.a
-INCGNU = -I$(srcdir)/../gnulib/import -I$(GNULIB_BUILDDIR)/import
+include $(GNULIB_BUILDDIR)/Makefile.gnulib.inc
 
 # Where is the INTL library?  Typically in ../intl.
 INTL = @LIBINTL@
@@ -352,16 +351,16 @@ gdbserver$(EXEEXT): $(sort $(OBS)) ${CDEPS} $(LIBGNU) $(LIBIBERTY) \
 	$(SILENCE) rm -f gdbserver$(EXEEXT)
 	$(ECHO_CXXLD) $(CC_LD) $(INTERNAL_CFLAGS) $(INTERNAL_LDFLAGS) \
 		-o gdbserver$(EXEEXT) $(OBS) $(GDBSUPPORT) $(LIBGNU) \
-		$(LIBIBERTY) $(INTL) $(GDBSERVER_LIBS) $(XM_CLIBS) \
-		$(WIN32APILIBS)
+		$(LIBGNU_EXTRA_LIBS) $(LIBIBERTY) $(INTL) \
+		$(GDBSERVER_LIBS) $(XM_CLIBS) $(WIN32APILIBS)
 
 gdbreplay$(EXEEXT): $(sort $(GDBREPLAY_OBS)) $(LIBGNU) $(LIBIBERTY) \
 		$(INTL_DEPS) $(GDBSUPPORT)
 	$(SILENCE) rm -f gdbreplay$(EXEEXT)
 	$(ECHO_CXXLD) $(CC_LD) $(INTERNAL_CFLAGS) $(INTERNAL_LDFLAGS) \
 		-o gdbreplay$(EXEEXT) $(GDBREPLAY_OBS) $(XM_CLIBS) \
-		$(GDBSUPPORT) $(LIBGNU) $(LIBIBERTY) $(INTL) \
-		$(WIN32APILIBS)
+		$(GDBSUPPORT) $(LIBGNU) $(LIBGNU_EXTRA_LIBS) \
+		$(LIBIBERTY) $(INTL) $(WIN32APILIBS)
 
 IPA_OBJS = \
 	alloc-ipa.o \
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.
+#
+# 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
+
+all-local: Makefile.gnulib.inc
diff --git a/gnulib/Makefile.in b/gnulib/Makefile.in
index bdd3c3f3fbc..40ff899c943 100644
--- a/gnulib/Makefile.in
+++ b/gnulib/Makefile.in
@@ -1750,7 +1750,7 @@ distclean-tags:
 	-rm -f cscope.out cscope.in.out cscope.po.out cscope.files
 check-am: all-am
 check: check-recursive
-all-am: Makefile config.h
+all-am: Makefile config.h all-local
 installdirs: installdirs-recursive
 installdirs-am:
 install: install-recursive
@@ -1854,7 +1854,7 @@ uninstall-am:
 
 .MAKE: $(am__recursive_targets) all install-am install-strip
 
-.PHONY: $(am__recursive_targets) CTAGS GTAGS TAGS all all-am \
+.PHONY: $(am__recursive_targets) CTAGS GTAGS TAGS all all-am all-local \
 	am--refresh check check-am clean clean-cscope clean-generic \
 	cscope cscopelist-am ctags ctags-am distclean \
 	distclean-generic distclean-hdr distclean-tags dvi dvi-am html \
@@ -1870,6 +1870,42 @@ uninstall-am:
 .PRECIOUS: Makefile
 
 
+# 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.
+#
+# 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
+
+all-local: Makefile.gnulib.inc
+
 # Tell versions [3.59,3.63) of GNU make to not export all variables.
 # Otherwise a system limit (for SysV at least) may be exceeded.
 .NOEXPORT:
-- 
2.25.4


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] gnulib: Ensure all libraries are used when building gdb/gdbserver
  2020-10-05 16:51 [PATCH] gnulib: Ensure all libraries are used when building gdb/gdbserver Andrew Burgess
@ 2020-10-05 18:34 ` Simon Marchi
  2020-10-06 12:17   ` Andrew Burgess
  0 siblings, 1 reply; 13+ messages in thread
From: Simon Marchi @ 2020-10-05 18:34 UTC (permalink / raw)
  To: Andrew Burgess, gdb-patches

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] gnulib: Ensure all libraries are used when building gdb/gdbserver
  2020-10-05 18:34 ` Simon Marchi
@ 2020-10-06 12:17   ` Andrew Burgess
  2020-10-06 14:10     ` Simon Marchi
  0 siblings, 1 reply; 13+ messages in thread
From: Andrew Burgess @ 2020-10-06 12:17 UTC (permalink / raw)
  To: Simon Marchi; +Cc: gdb-patches

* Simon Marchi <simark@simark.ca> [2020-10-05 14:34:57 -0400]:

> 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'
>

The problem here was me unconditionally trying to include
Makefile.gnulib.inc from the gdb and gdbserver Makefile.  The very
first time you run 'make clean' after applying the patch
Makefile.gnulib.inc doesn't exist, but the gdb/gdbserver makefiles
still try to pull the file in unconditionally.

> 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.

Of course that's a better way to solve the problem.

The new patch (below) is a complete rewrite.  Now the makefile
fragment is generated by the configure process.  The include from
gdb/gdbserver is optional, so applying the patch shouldn't cause 'make
clean' to break.

> 
> > 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.

Done.

> 
> > +#
> > +# 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.

Nuts, I originally did this, but then removed it as I couldn't think
why it would be needed, and it seemed like overkill.  Of course an
interrupted build is exactly why it would be needed.

Anyway, the new approach removes the need for this completely.

> 
> 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).

Yeah, I saw this as a possible solution, initially I wondered about
having 'update-gnulib.sh' generate the library list automatically, but
because of the problems you pointed out above I discounted the idea.

I figured that once the infrastructure is in place if the library list
changes it's pretty easy to add an extra library to the list once we
discover it's missing.

Here's V2 of the patch.

  - The makefile fragment is generated at configure time,
  - gdb/gdbserver only include the fragment if it exists.

Thanks,
Andrew

---

From f4d167b8909667b4fda188e213d624b9b481af4c Mon Sep 17 00:00:00 2001
From: Andrew Burgess <andrew.burgess@embecosm.com>
Date: Tue, 6 Oct 2020 10:09:06 +0100
Subject: [PATCH] gnulib: Ensure all libraries are used when building
 gdb/gdbserver

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 configure script to install a new
makefile fragment in the gnulib build directory.  This new file will
have the usual variable substitutions applied to it, and so can
include the complete list (see above) of all the extra libraries that
are needed when linking against gnulib.

In fact the new makefile fragment defines three variables, these 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.  This list is taken from
       the output of gnulib-tool, which is run by our
       gnulib/update-gnulib.sh script.

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.

The makefile fragment relies on the variable GNULIB_BUILDDIR being
defined.  This is checked for in the fragment, and was already defined
in the makefiles of gdb and gdbserver.

gdb/ChangeLog:

	* Makefile.in: Include Makefile.gnulib.  Don't define LIBGNU or
	INCGNU.  Make use of LIBGNU_EXTRA_LIBS when linking.

gdbserver/ChangeLog:

	* Makefile.in: Include Makefile.gnulib.  Don't define LIBGNU or
	INCGNU.  Make use of LIBGNU_EXTRA_LIBS when linking.

gnulib/ChangeLog:

	* Makefile.gnulib.in: New file.
	* Makefile.in: Regenerate.
	* configure: Regenerate.
	* configure.ac: Install the new file.
---
 gdb/ChangeLog             |  5 +++++
 gdb/Makefile.in           |  5 ++---
 gdbserver/ChangeLog       |  5 +++++
 gdbserver/Makefile.in     | 11 +++++-----
 gnulib/ChangeLog          |  7 +++++++
 gnulib/Makefile.gnulib.in | 44 +++++++++++++++++++++++++++++++++++++++
 gnulib/Makefile.in        |  4 +++-
 gnulib/configure          |  4 ++++
 gnulib/configure.ac       |  2 ++
 9 files changed, 77 insertions(+), 10 deletions(-)
 create mode 100644 gnulib/Makefile.gnulib.in

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index dbede7a9cfc..bfcd8b0f545 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -239,8 +239,7 @@ GDBFLAGS =
 
 # Helper code from gnulib.
 GNULIB_BUILDDIR = ../gnulib
-LIBGNU = $(GNULIB_BUILDDIR)/import/libgnu.a
-INCGNU = -I$(srcdir)/../gnulib/import -I$(GNULIB_BUILDDIR)/import
+-include $(GNULIB_BUILDDIR)/Makefile.gnulib
 
 SUPPORT = ../gdbsupport
 LIBSUPPORT = $(SUPPORT)/libgdbsupport.a
@@ -627,7 +626,7 @@ CLIBS = $(SIM) $(READLINE) $(OPCODES) $(LIBCTF) $(BFD) $(ZLIB) \
 	$(XM_CLIBS) $(GDBTKLIBS) \
 	@LIBS@ @GUILE_LIBS@ @PYTHON_LIBS@ \
 	$(LIBEXPAT) $(LIBLZMA) $(LIBBABELTRACE) $(LIBIPT) \
-	$(WIN32LIBS) $(LIBGNU) $(LIBICONV) \
+	$(WIN32LIBS) $(LIBGNU) $(LIBGNU_EXTRA_LIBS) $(LIBICONV) \
 	$(LIBMPFR) $(SRCHIGH_LIBS) $(LIBXXHASH) $(PTHREAD_LIBS) \
 	$(DEBUGINFOD_LIBS)
 CDEPS = $(NAT_CDEPS) $(SIM) $(BFD) $(READLINE_DEPS) $(CTF_DEPS) \
diff --git a/gdbserver/Makefile.in b/gdbserver/Makefile.in
index b0bad0cdb09..b2c61ef069d 100644
--- a/gdbserver/Makefile.in
+++ b/gdbserver/Makefile.in
@@ -111,8 +111,7 @@ ustinc = @ustinc@
 
 # gnulib
 GNULIB_BUILDDIR = ../gnulib
-LIBGNU = $(GNULIB_BUILDDIR)/import/libgnu.a
-INCGNU = -I$(srcdir)/../gnulib/import -I$(GNULIB_BUILDDIR)/import
+-include $(GNULIB_BUILDDIR)/Makefile.gnulib
 
 # Where is the INTL library?  Typically in ../intl.
 INTL = @LIBINTL@
@@ -352,16 +351,16 @@ gdbserver$(EXEEXT): $(sort $(OBS)) ${CDEPS} $(LIBGNU) $(LIBIBERTY) \
 	$(SILENCE) rm -f gdbserver$(EXEEXT)
 	$(ECHO_CXXLD) $(CC_LD) $(INTERNAL_CFLAGS) $(INTERNAL_LDFLAGS) \
 		-o gdbserver$(EXEEXT) $(OBS) $(GDBSUPPORT) $(LIBGNU) \
-		$(LIBIBERTY) $(INTL) $(GDBSERVER_LIBS) $(XM_CLIBS) \
-		$(WIN32APILIBS)
+		$(LIBGNU_EXTRA_LIBS) $(LIBIBERTY) $(INTL) \
+		$(GDBSERVER_LIBS) $(XM_CLIBS) $(WIN32APILIBS)
 
 gdbreplay$(EXEEXT): $(sort $(GDBREPLAY_OBS)) $(LIBGNU) $(LIBIBERTY) \
 		$(INTL_DEPS) $(GDBSUPPORT)
 	$(SILENCE) rm -f gdbreplay$(EXEEXT)
 	$(ECHO_CXXLD) $(CC_LD) $(INTERNAL_CFLAGS) $(INTERNAL_LDFLAGS) \
 		-o gdbreplay$(EXEEXT) $(GDBREPLAY_OBS) $(XM_CLIBS) \
-		$(GDBSUPPORT) $(LIBGNU) $(LIBIBERTY) $(INTL) \
-		$(WIN32APILIBS)
+		$(GDBSUPPORT) $(LIBGNU) $(LIBGNU_EXTRA_LIBS) \
+		$(LIBIBERTY) $(INTL) $(WIN32APILIBS)
 
 IPA_OBJS = \
 	alloc-ipa.o \
diff --git a/gnulib/Makefile.gnulib.in b/gnulib/Makefile.gnulib.in
new file mode 100644
index 00000000000..f3fd72b5781
--- /dev/null
+++ b/gnulib/Makefile.gnulib.in
@@ -0,0 +1,44 @@
+# Copyright (C) 2020 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/>.
+
+# 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.
+#
+# 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.  This list is taken from
+#        the output of gnulib-tool, which is run by our
+#        gnulib/update-gnulib.sh script.
+#
+# INCGNU: A list of -I.... include paths that should be passed to the
+#        compiler, these are where the gnulib headers can be found.
+
+ifndef GNULIB_BUILDDIR
+$(error missing GNULIB_BUILDDIR)
+endif
+
+LIBGNU = $(GNULIB_BUILDDIR)/import/libgnu.a
+LIBGNU_EXTRA_LIBS = @FREXPL_LIBM@ @FREXP_LIBM@ @INET_NTOP_LIB@ \
+                    @LIBTHREAD@ @LIB_GETLOGIN@ @LIB_GETRANDOM@ \
+                    @LIB_HARD_LOCALE@ @LIB_MBRTOWC@ \
+                    @LIB_SETLOCALE_NULL@ @LIBINTL@
+INCGNU = -I$(srcdir)/../gnulib/import -I$(GNULIB_BUILDDIR)/import
diff --git a/gnulib/Makefile.in b/gnulib/Makefile.in
index bdd3c3f3fbc..f9ef98a6906 100644
--- a/gnulib/Makefile.in
+++ b/gnulib/Makefile.in
@@ -277,7 +277,7 @@ am__CONFIG_DISTCLEAN_FILES = config.status config.cache config.log \
  configure.lineno config.status.lineno
 mkinstalldirs = $(SHELL) $(top_srcdir)/../mkinstalldirs
 CONFIG_HEADER = config.h
-CONFIG_CLEAN_FILES =
+CONFIG_CLEAN_FILES = Makefile.gnulib
 CONFIG_CLEAN_VPATH_FILES =
 AM_V_P = $(am__v_P_@AM_V@)
 am__v_P_ = $(am__v_P_@AM_DEFAULT_V@)
@@ -1642,6 +1642,8 @@ $(srcdir)/config.in: @MAINTAINER_MODE_TRUE@ $(am__configure_deps)
 
 distclean-hdr:
 	-rm -f config.h stamp-h1
+Makefile.gnulib: $(top_builddir)/config.status $(srcdir)/Makefile.gnulib.in
+	cd $(top_builddir) && $(SHELL) ./config.status $@
 
 # This directory's subdirectories are mostly independent; you can cd
 # into them and run 'make' without going through this Makefile.
diff --git a/gnulib/configure b/gnulib/configure
index 5c6add6e371..d7358ed1a19 100644
--- a/gnulib/configure
+++ b/gnulib/configure
@@ -30802,6 +30802,9 @@ fi
 # Checks for libraries.  #
 # ---------------------- #
 
+ac_config_files="$ac_config_files Makefile.gnulib"
+
+
 ac_config_files="$ac_config_files Makefile import/Makefile"
 
 ac_config_commands="$ac_config_commands default"
@@ -31634,6 +31637,7 @@ do
   case $ac_config_target in
     "config.h") CONFIG_HEADERS="$CONFIG_HEADERS config.h:config.in" ;;
     "depfiles") CONFIG_COMMANDS="$CONFIG_COMMANDS depfiles" ;;
+    "Makefile.gnulib") CONFIG_FILES="$CONFIG_FILES Makefile.gnulib" ;;
     "Makefile") CONFIG_FILES="$CONFIG_FILES Makefile" ;;
     "import/Makefile") CONFIG_FILES="$CONFIG_FILES import/Makefile" ;;
     "default") CONFIG_COMMANDS="$CONFIG_COMMANDS default" ;;
diff --git a/gnulib/configure.ac b/gnulib/configure.ac
index acc1b1a322e..ecdad8608fa 100644
--- a/gnulib/configure.ac
+++ b/gnulib/configure.ac
@@ -56,6 +56,8 @@ AC_CHECK_TOOL(AR, ar)
 # Checks for libraries.  #
 # ---------------------- #
 
+AC_CONFIG_FILES(Makefile.gnulib)
+
 AC_OUTPUT(Makefile import/Makefile,
 [
 case x$CONFIG_HEADERS in
-- 
2.25.4


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] gnulib: Ensure all libraries are used when building gdb/gdbserver
  2020-10-06 12:17   ` Andrew Burgess
@ 2020-10-06 14:10     ` Simon Marchi
  2020-10-07 15:33       ` Andrew Burgess
  0 siblings, 1 reply; 13+ messages in thread
From: Simon Marchi @ 2020-10-06 14:10 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: gdb-patches

On 2020-10-06 8:17 a.m., Andrew Burgess wrote:
> The problem here was me unconditionally trying to include
> Makefile.gnulib.inc from the gdb and gdbserver Makefile.  The very
> first time you run 'make clean' after applying the patch
> Makefile.gnulib.inc doesn't exist, but the gdb/gdbserver makefiles
> still try to pull the file in unconditionally.

If the file is generated at configure-time, I think it would be ok for
Makefile.gnulib.inc to be included unconditionally.  As when you run
"make clean" after pulling some changes, you're supposed to do to it
from the top level, so Makefile.gnulib.inc will be created before "make
clean" runs in GDB.

Building GDB (i.e. running "make" in gdb/) requires some other
directories (like gnulib/) to have been configured first.  So if you
pull changes that affect these other directories and try to run "make"
just in gdb/ (instead of at the top-level), it's expected that some
things may not work (I think).

If there is no other legitimate situation where Makefile.gnulib.inc may
not exist, I'd slightly prefer for the include to be unconditional.
Simply because if Makefile.gnulib.inc doesn't exist, for some reason,
the error message will be much more straightforward.  This:

    $ make
    /bin/sh config.status Makefile
    config.status: creating Makefile
    Makefile:242: ../gnulib/Makefile.gnulib: No such file or directory
    make: *** No rule to make target '../gnulib/Makefile.gnulib'.  Stop.

vs

    $ make
    CXX    gdb.o
    cc1plus: warning: command-line option ‘-Wmissing-prototypes’ is valid for C/ObjC but not for C++
    In file included from /home/simark/src/binutils-gdb/gdb/defs.h:28,
                     from /home/simark/src/binutils-gdb/gdb/gdb.c:19:
    /home/simark/src/binutils-gdb/gdb/../gdbsupport/common-defs.h:120:10: fatal error: pathmax.h: No such file or directory
      120 | #include "pathmax.h"
          |          ^~~~~~~~~~~
    compilation terminated.
    make: *** [Makefile:1618: gdb.o] Error 1

Finally, I don't know if the change was intentional, but the file was
previously named Makefile.gnulib.inc and is now named Makefile.gnulib.
I liked Makefile.gnulib.inc, as it shows it's not a "top-level"
Makefile, it's meant to be included.

Otherwise, the patch LGTM, thanks!

Simon

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] gnulib: Ensure all libraries are used when building gdb/gdbserver
  2020-10-06 14:10     ` Simon Marchi
@ 2020-10-07 15:33       ` Andrew Burgess
  2020-10-07 15:34         ` Simon Marchi
  2020-10-09  8:34         ` Andrew Burgess
  0 siblings, 2 replies; 13+ messages in thread
From: Andrew Burgess @ 2020-10-07 15:33 UTC (permalink / raw)
  To: Simon Marchi; +Cc: gdb-patches

* Simon Marchi <simark@simark.ca> [2020-10-06 10:10:25 -0400]:

> On 2020-10-06 8:17 a.m., Andrew Burgess wrote:
> > The problem here was me unconditionally trying to include
> > Makefile.gnulib.inc from the gdb and gdbserver Makefile.  The very
> > first time you run 'make clean' after applying the patch
> > Makefile.gnulib.inc doesn't exist, but the gdb/gdbserver makefiles
> > still try to pull the file in unconditionally.
> 
> If the file is generated at configure-time, I think it would be ok for
> Makefile.gnulib.inc to be included unconditionally.  As when you run
> "make clean" after pulling some changes, you're supposed to do to it
> from the top level, so Makefile.gnulib.inc will be created before "make
> clean" runs in GDB.

OK, I made the include compulsory.

<snip>

> 
> Finally, I don't know if the change was intentional, but the file was
> previously named Makefile.gnulib.inc and is now named Makefile.gnulib.
> I liked Makefile.gnulib.inc, as it shows it's not a "top-level"
> Makefile, it's meant to be included.

I just thought 'Makefile.gnulib.inc.in' was a bit much (in the source
tree), but I don't really mind.  I changed back to use this filename.

The patch below has these two fixes.  I'll apply in a couple of days
unless anyone objects.

Thanks,
Andrew

--

From 573756867de5b836fc5418499e5037207d692207 Mon Sep 17 00:00:00 2001
From: Andrew Burgess <andrew.burgess@embecosm.com>
Date: Tue, 6 Oct 2020 10:09:06 +0100
Subject: [PATCH] gnulib: Ensure all libraries are used when building
 gdb/gdbserver

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 configure script to install a new
makefile fragment in the gnulib build directory.  This new file will
have the usual variable substitutions applied to it, and so can
include the complete list (see above) of all the extra libraries that
are needed when linking against gnulib.

In fact the new makefile fragment defines three variables, these 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.  This list is taken from
       the output of gnulib-tool, which is run by our
       gnulib/update-gnulib.sh script.

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.

The makefile fragment relies on the variable GNULIB_BUILDDIR being
defined.  This is checked for in the fragment, and was already defined
in the makefiles of 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.gnulib.inc.in: New file.
	* Makefile.in: Regenerate.
	* configure: Regenerate.
	* configure.ac: Install the new file.
---
 gdb/ChangeLog                 |  5 ++++
 gdb/Makefile.in               |  5 ++--
 gdbserver/ChangeLog           |  5 ++++
 gdbserver/Makefile.in         | 11 ++++-----
 gnulib/ChangeLog              |  7 ++++++
 gnulib/Makefile.gnulib.inc.in | 44 +++++++++++++++++++++++++++++++++++
 gnulib/Makefile.in            |  4 +++-
 gnulib/configure              |  4 ++++
 gnulib/configure.ac           |  2 ++
 9 files changed, 77 insertions(+), 10 deletions(-)
 create mode 100644 gnulib/Makefile.gnulib.inc.in

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index dbede7a9cfc..18a426dc225 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -239,8 +239,7 @@ GDBFLAGS =
 
 # Helper code from gnulib.
 GNULIB_BUILDDIR = ../gnulib
-LIBGNU = $(GNULIB_BUILDDIR)/import/libgnu.a
-INCGNU = -I$(srcdir)/../gnulib/import -I$(GNULIB_BUILDDIR)/import
+include $(GNULIB_BUILDDIR)/Makefile.gnulib.inc
 
 SUPPORT = ../gdbsupport
 LIBSUPPORT = $(SUPPORT)/libgdbsupport.a
@@ -627,7 +626,7 @@ CLIBS = $(SIM) $(READLINE) $(OPCODES) $(LIBCTF) $(BFD) $(ZLIB) \
 	$(XM_CLIBS) $(GDBTKLIBS) \
 	@LIBS@ @GUILE_LIBS@ @PYTHON_LIBS@ \
 	$(LIBEXPAT) $(LIBLZMA) $(LIBBABELTRACE) $(LIBIPT) \
-	$(WIN32LIBS) $(LIBGNU) $(LIBICONV) \
+	$(WIN32LIBS) $(LIBGNU) $(LIBGNU_EXTRA_LIBS) $(LIBICONV) \
 	$(LIBMPFR) $(SRCHIGH_LIBS) $(LIBXXHASH) $(PTHREAD_LIBS) \
 	$(DEBUGINFOD_LIBS)
 CDEPS = $(NAT_CDEPS) $(SIM) $(BFD) $(READLINE_DEPS) $(CTF_DEPS) \
diff --git a/gdbserver/Makefile.in b/gdbserver/Makefile.in
index b0bad0cdb09..54b3f3becc4 100644
--- a/gdbserver/Makefile.in
+++ b/gdbserver/Makefile.in
@@ -111,8 +111,7 @@ ustinc = @ustinc@
 
 # gnulib
 GNULIB_BUILDDIR = ../gnulib
-LIBGNU = $(GNULIB_BUILDDIR)/import/libgnu.a
-INCGNU = -I$(srcdir)/../gnulib/import -I$(GNULIB_BUILDDIR)/import
+include $(GNULIB_BUILDDIR)/Makefile.gnulib.inc
 
 # Where is the INTL library?  Typically in ../intl.
 INTL = @LIBINTL@
@@ -352,16 +351,16 @@ gdbserver$(EXEEXT): $(sort $(OBS)) ${CDEPS} $(LIBGNU) $(LIBIBERTY) \
 	$(SILENCE) rm -f gdbserver$(EXEEXT)
 	$(ECHO_CXXLD) $(CC_LD) $(INTERNAL_CFLAGS) $(INTERNAL_LDFLAGS) \
 		-o gdbserver$(EXEEXT) $(OBS) $(GDBSUPPORT) $(LIBGNU) \
-		$(LIBIBERTY) $(INTL) $(GDBSERVER_LIBS) $(XM_CLIBS) \
-		$(WIN32APILIBS)
+		$(LIBGNU_EXTRA_LIBS) $(LIBIBERTY) $(INTL) \
+		$(GDBSERVER_LIBS) $(XM_CLIBS) $(WIN32APILIBS)
 
 gdbreplay$(EXEEXT): $(sort $(GDBREPLAY_OBS)) $(LIBGNU) $(LIBIBERTY) \
 		$(INTL_DEPS) $(GDBSUPPORT)
 	$(SILENCE) rm -f gdbreplay$(EXEEXT)
 	$(ECHO_CXXLD) $(CC_LD) $(INTERNAL_CFLAGS) $(INTERNAL_LDFLAGS) \
 		-o gdbreplay$(EXEEXT) $(GDBREPLAY_OBS) $(XM_CLIBS) \
-		$(GDBSUPPORT) $(LIBGNU) $(LIBIBERTY) $(INTL) \
-		$(WIN32APILIBS)
+		$(GDBSUPPORT) $(LIBGNU) $(LIBGNU_EXTRA_LIBS) \
+		$(LIBIBERTY) $(INTL) $(WIN32APILIBS)
 
 IPA_OBJS = \
 	alloc-ipa.o \
diff --git a/gnulib/Makefile.gnulib.inc.in b/gnulib/Makefile.gnulib.inc.in
new file mode 100644
index 00000000000..f3fd72b5781
--- /dev/null
+++ b/gnulib/Makefile.gnulib.inc.in
@@ -0,0 +1,44 @@
+# Copyright (C) 2020 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/>.
+
+# 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.
+#
+# 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.  This list is taken from
+#        the output of gnulib-tool, which is run by our
+#        gnulib/update-gnulib.sh script.
+#
+# INCGNU: A list of -I.... include paths that should be passed to the
+#        compiler, these are where the gnulib headers can be found.
+
+ifndef GNULIB_BUILDDIR
+$(error missing GNULIB_BUILDDIR)
+endif
+
+LIBGNU = $(GNULIB_BUILDDIR)/import/libgnu.a
+LIBGNU_EXTRA_LIBS = @FREXPL_LIBM@ @FREXP_LIBM@ @INET_NTOP_LIB@ \
+                    @LIBTHREAD@ @LIB_GETLOGIN@ @LIB_GETRANDOM@ \
+                    @LIB_HARD_LOCALE@ @LIB_MBRTOWC@ \
+                    @LIB_SETLOCALE_NULL@ @LIBINTL@
+INCGNU = -I$(srcdir)/../gnulib/import -I$(GNULIB_BUILDDIR)/import
diff --git a/gnulib/Makefile.in b/gnulib/Makefile.in
index bdd3c3f3fbc..c1c21680944 100644
--- a/gnulib/Makefile.in
+++ b/gnulib/Makefile.in
@@ -277,7 +277,7 @@ am__CONFIG_DISTCLEAN_FILES = config.status config.cache config.log \
  configure.lineno config.status.lineno
 mkinstalldirs = $(SHELL) $(top_srcdir)/../mkinstalldirs
 CONFIG_HEADER = config.h
-CONFIG_CLEAN_FILES =
+CONFIG_CLEAN_FILES = Makefile.gnulib.inc
 CONFIG_CLEAN_VPATH_FILES =
 AM_V_P = $(am__v_P_@AM_V@)
 am__v_P_ = $(am__v_P_@AM_DEFAULT_V@)
@@ -1642,6 +1642,8 @@ $(srcdir)/config.in: @MAINTAINER_MODE_TRUE@ $(am__configure_deps)
 
 distclean-hdr:
 	-rm -f config.h stamp-h1
+Makefile.gnulib.inc: $(top_builddir)/config.status $(srcdir)/Makefile.gnulib.inc.in
+	cd $(top_builddir) && $(SHELL) ./config.status $@
 
 # This directory's subdirectories are mostly independent; you can cd
 # into them and run 'make' without going through this Makefile.
diff --git a/gnulib/configure b/gnulib/configure
index 5c6add6e371..6c58a46e42e 100644
--- a/gnulib/configure
+++ b/gnulib/configure
@@ -30802,6 +30802,9 @@ fi
 # Checks for libraries.  #
 # ---------------------- #
 
+ac_config_files="$ac_config_files Makefile.gnulib.inc"
+
+
 ac_config_files="$ac_config_files Makefile import/Makefile"
 
 ac_config_commands="$ac_config_commands default"
@@ -31634,6 +31637,7 @@ do
   case $ac_config_target in
     "config.h") CONFIG_HEADERS="$CONFIG_HEADERS config.h:config.in" ;;
     "depfiles") CONFIG_COMMANDS="$CONFIG_COMMANDS depfiles" ;;
+    "Makefile.gnulib.inc") CONFIG_FILES="$CONFIG_FILES Makefile.gnulib.inc" ;;
     "Makefile") CONFIG_FILES="$CONFIG_FILES Makefile" ;;
     "import/Makefile") CONFIG_FILES="$CONFIG_FILES import/Makefile" ;;
     "default") CONFIG_COMMANDS="$CONFIG_COMMANDS default" ;;
diff --git a/gnulib/configure.ac b/gnulib/configure.ac
index acc1b1a322e..052b8bd8dc5 100644
--- a/gnulib/configure.ac
+++ b/gnulib/configure.ac
@@ -56,6 +56,8 @@ AC_CHECK_TOOL(AR, ar)
 # Checks for libraries.  #
 # ---------------------- #
 
+AC_CONFIG_FILES(Makefile.gnulib.inc)
+
 AC_OUTPUT(Makefile import/Makefile,
 [
 case x$CONFIG_HEADERS in
-- 
2.25.4


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] gnulib: Ensure all libraries are used when building gdb/gdbserver
  2020-10-07 15:33       ` Andrew Burgess
@ 2020-10-07 15:34         ` Simon Marchi
  2020-10-09  8:34         ` Andrew Burgess
  1 sibling, 0 replies; 13+ messages in thread
From: Simon Marchi @ 2020-10-07 15:34 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: gdb-patches

On 2020-10-07 11:33 a.m., Andrew Burgess wrote:
> The patch below has these two fixes.  I'll apply in a couple of days
> unless anyone objects.

Thanks, then I don't think I need to take another look, I suppose it looks good to me :).

Simon

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] gnulib: Ensure all libraries are used when building gdb/gdbserver
  2020-10-07 15:33       ` Andrew Burgess
  2020-10-07 15:34         ` Simon Marchi
@ 2020-10-09  8:34         ` Andrew Burgess
  2020-10-12 11:41           ` Joel Brobecker
  1 sibling, 1 reply; 13+ messages in thread
From: Andrew Burgess @ 2020-10-09  8:34 UTC (permalink / raw)
  To: Simon Marchi; +Cc: gdb-patches

* Andrew Burgess <andrew.burgess@embecosm.com> [2020-10-07 16:33:19 +0100]:

> * Simon Marchi <simark@simark.ca> [2020-10-06 10:10:25 -0400]:
> 
> > On 2020-10-06 8:17 a.m., Andrew Burgess wrote:
> > > The problem here was me unconditionally trying to include
> > > Makefile.gnulib.inc from the gdb and gdbserver Makefile.  The very
> > > first time you run 'make clean' after applying the patch
> > > Makefile.gnulib.inc doesn't exist, but the gdb/gdbserver makefiles
> > > still try to pull the file in unconditionally.
> > 
> > If the file is generated at configure-time, I think it would be ok for
> > Makefile.gnulib.inc to be included unconditionally.  As when you run
> > "make clean" after pulling some changes, you're supposed to do to it
> > from the top level, so Makefile.gnulib.inc will be created before "make
> > clean" runs in GDB.
> 
> OK, I made the include compulsory.
> 
> <snip>
> 
> > 
> > Finally, I don't know if the change was intentional, but the file was
> > previously named Makefile.gnulib.inc and is now named Makefile.gnulib.
> > I liked Makefile.gnulib.inc, as it shows it's not a "top-level"
> > Makefile, it's meant to be included.
> 
> I just thought 'Makefile.gnulib.inc.in' was a bit much (in the source
> tree), but I don't really mind.  I changed back to use this filename.
> 
> The patch below has these two fixes.  I'll apply in a couple of days
> unless anyone objects.

I've now pushed this patch.

Thanks,
Andrew

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] gnulib: Ensure all libraries are used when building gdb/gdbserver
  2020-10-09  8:34         ` Andrew Burgess
@ 2020-10-12 11:41           ` Joel Brobecker
  2020-10-12 14:16             ` Simon Marchi
  0 siblings, 1 reply; 13+ messages in thread
From: Joel Brobecker @ 2020-10-12 11:41 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: Simon Marchi, gdb-patches

Hi Andrew,

On Fri, Oct 09, 2020 at 09:34:36AM +0100, Andrew Burgess wrote:
> * Andrew Burgess <andrew.burgess@embecosm.com> [2020-10-07 16:33:19 +0100]:
> 
> > * Simon Marchi <simark@simark.ca> [2020-10-06 10:10:25 -0400]:
> > 
> > > On 2020-10-06 8:17 a.m., Andrew Burgess wrote:
> > > > The problem here was me unconditionally trying to include
> > > > Makefile.gnulib.inc from the gdb and gdbserver Makefile.  The very
> > > > first time you run 'make clean' after applying the patch
> > > > Makefile.gnulib.inc doesn't exist, but the gdb/gdbserver makefiles
> > > > still try to pull the file in unconditionally.
> > > 
> > > If the file is generated at configure-time, I think it would be ok for
> > > Makefile.gnulib.inc to be included unconditionally.  As when you run
> > > "make clean" after pulling some changes, you're supposed to do to it
> > > from the top level, so Makefile.gnulib.inc will be created before "make
> > > clean" runs in GDB.
> > 
> > OK, I made the include compulsory.
> > 
> > <snip>
> > 
> > > 
> > > Finally, I don't know if the change was intentional, but the file was
> > > previously named Makefile.gnulib.inc and is now named Makefile.gnulib.
> > > I liked Makefile.gnulib.inc, as it shows it's not a "top-level"
> > > Makefile, it's meant to be included.
> > 
> > I just thought 'Makefile.gnulib.inc.in' was a bit much (in the source
> > tree), but I don't really mind.  I changed back to use this filename.
> > 
> > The patch below has these two fixes.  I'll apply in a couple of days
> > unless anyone objects.
> 
> I've now pushed this patch.

I got reports of the nightly source packaging failing soon after
you pushed this patch, and I was able to reproduce it by configuring
(out of tree), followed by a "make distclean". I did a "make -C gdb install"
as well, but I don't think it should have any influence on the outcome.

The error I get is the following:

     | Makefile:246: ../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 '/[...]/gdb'

Do you think you could look into it?

Thank you!
-- 
Joel

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] gnulib: Ensure all libraries are used when building gdb/gdbserver
  2020-10-12 11:41           ` Joel Brobecker
@ 2020-10-12 14:16             ` Simon Marchi
  2020-10-12 15:30               ` Andrew Burgess
  0 siblings, 1 reply; 13+ messages in thread
From: Simon Marchi @ 2020-10-12 14:16 UTC (permalink / raw)
  To: Joel Brobecker, Andrew Burgess; +Cc: gdb-patches

On 2020-10-12 7:41 a.m., Joel Brobecker wrote:
> I got reports of the nightly source packaging failing soon after
> you pushed this patch, and I was able to reproduce it by configuring
> (out of tree), followed by a "make distclean". I did a "make -C gdb install"
> as well, but I don't think it should have any influence on the outcome.
>
> The error I get is the following:
>
>      | Makefile:246: ../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 '/[...]/gdb'
>
> Do you think you could look into it?

Ah, that's a case I didn't think of.  When make distclean runs, it runs
first in gnulib, so erases gnulib/Makefile.gnulib.inc.  When it then
runs in gdb, the Makefile can't run.

So I guess we'll need to make the inclusion condition/non-fatal for this
reason, unless you see another way?

Simon

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] gnulib: Ensure all libraries are used when building gdb/gdbserver
  2020-10-12 14:16             ` Simon Marchi
@ 2020-10-12 15:30               ` Andrew Burgess
  2020-10-13 14:26                 ` Simon Marchi
  0 siblings, 1 reply; 13+ messages in thread
From: Andrew Burgess @ 2020-10-12 15:30 UTC (permalink / raw)
  To: Simon Marchi; +Cc: Joel Brobecker, gdb-patches

* Simon Marchi <simark@simark.ca> [2020-10-12 10:16:08 -0400]:

> On 2020-10-12 7:41 a.m., Joel Brobecker wrote:
> > I got reports of the nightly source packaging failing soon after
> > you pushed this patch, and I was able to reproduce it by configuring
> > (out of tree), followed by a "make distclean". I did a "make -C gdb install"
> > as well, but I don't think it should have any influence on the outcome.
> >
> > The error I get is the following:
> >
> >      | Makefile:246: ../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 '/[...]/gdb'
> >
> > Do you think you could look into it?
> 
> Ah, that's a case I didn't think of.  When make distclean runs, it runs
> first in gnulib, so erases gnulib/Makefile.gnulib.inc.  When it then
> runs in gdb, the Makefile can't run.
> 
> So I guess we'll need to make the inclusion condition/non-fatal for this
> reason, unless you see another way?

How about this?

I've added a module dependency to ensure that gdb/gdbserver are both
cleaned before gnulib.  My hope is that this should ensure
Makefile.gnulib.inc still exists when needed...

Thanks,
Andrew

---

commit 75b86a705adbb127962beb5ef8d9d7181afe3789
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Mon Oct 12 16:04:32 2020 +0100

    gdb/gdbserver: use '-include' to pull in Makefile.gnulib.inc
    
    After commit:
    
      commit 361cb219351d8b7e39e1962fe77f40aa80657b27
      Date:   Tue Oct 6 10:09:06 2020 +0100
    
          gnulib: Ensure all libraries are used when building gdb/gdbserver
    
    We now get an error when, at the top level of the build tree, we do
    'make distclean'.
    
    The reason for this is that the gnulib directory is cleaned before the
    gdb directory, cleaning gnulib deletes Makefile.gnulib.inc from the
    gnulib build directory, which is currently pulled in by the gdb
    Makefile.in using 'include'.
    
    This commit adds a dependency between distclean-gnulib and both
    distclean-gdb and distclean-gdbserver.  This means that gdb and
    gdbserver will be cleaned before gnulib, as a result the
    Makefile.gnulib.inc file should exist when needed.
    
    ChangeLog:
    
            * Makefile.in: Rebuild.
            * Makefile.def: Make distclean-gnulib depend on distclean-gdb and
            distclean-gdbserver.

diff --git a/Makefile.def b/Makefile.def
index 76d062bb671..089e70ae3ed 100644
--- a/Makefile.def
+++ b/Makefile.def
@@ -548,6 +548,12 @@ dependencies = { module=configure-libctf; on=all-intl; };
 dependencies = { module=configure-libctf; on=all-zlib; };
 dependencies = { module=configure-libctf; on=all-libiconv; };
 
+// The Makefiles in gdb and gdbserver pull in a file that configure
+// generates in the gnulib directory, so distclean gnulib only after
+// gdb and gdbserver.
+dependencies = { module=distclean-gnulib; on=distclean-gdb; };
+dependencies = { module=distclean-gnulib; on=distclean-gdbserver; };
+
 // Warning, these are not well tested.
 dependencies = { module=all-bison; on=all-intl; };
 dependencies = { module=all-bison; on=all-build-texinfo; };
diff --git a/Makefile.in b/Makefile.in
index 9dfd39fae13..fe34132f9e5 100644
--- a/Makefile.in
+++ b/Makefile.in
@@ -52366,6 +52366,8 @@ configure-stage3-libctf: maybe-all-stage3-libiconv
 configure-stage4-libctf: maybe-all-stage4-libiconv
 configure-stageprofile-libctf: maybe-all-stageprofile-libiconv
 configure-stagefeedback-libctf: maybe-all-stagefeedback-libiconv
+distclean-gnulib: maybe-distclean-gdb
+distclean-gnulib: maybe-distclean-gdbserver
 all-bison: maybe-all-build-texinfo
 all-flex: maybe-all-build-bison
 all-flex: maybe-all-m4

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] gnulib: Ensure all libraries are used when building gdb/gdbserver
  2020-10-12 15:30               ` Andrew Burgess
@ 2020-10-13 14:26                 ` Simon Marchi
  2020-10-14 14:09                   ` Andrew Burgess
  0 siblings, 1 reply; 13+ messages in thread
From: Simon Marchi @ 2020-10-13 14:26 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: Joel Brobecker, gdb-patches


On 2020-10-12 11:30 a.m., Andrew Burgess wrote:
> commit 75b86a705adbb127962beb5ef8d9d7181afe3789
> Author: Andrew Burgess <andrew.burgess@embecosm.com>
> Date:   Mon Oct 12 16:04:32 2020 +0100
>
>     gdb/gdbserver: use '-include' to pull in Makefile.gnulib.inc

The patch subject doesn't seem right.

>
>     After commit:
>
>       commit 361cb219351d8b7e39e1962fe77f40aa80657b27
>       Date:   Tue Oct 6 10:09:06 2020 +0100
>
>           gnulib: Ensure all libraries are used when building gdb/gdbserver
>
>     We now get an error when, at the top level of the build tree, we do
>     'make distclean'.
>
>     The reason for this is that the gnulib directory is cleaned before the
>     gdb directory, cleaning gnulib deletes Makefile.gnulib.inc from the
>     gnulib build directory, which is currently pulled in by the gdb
>     Makefile.in using 'include'.
>
>     This commit adds a dependency between distclean-gnulib and both
>     distclean-gdb and distclean-gdbserver.  This means that gdb and
>     gdbserver will be cleaned before gnulib, as a result the
>     Makefile.gnulib.inc file should exist when needed.
>
>     ChangeLog:
>
>             * Makefile.in: Rebuild.
>             * Makefile.def: Make distclean-gnulib depend on distclean-gdb and
>             distclean-gdbserver.
>
> diff --git a/Makefile.def b/Makefile.def
> index 76d062bb671..089e70ae3ed 100644
> --- a/Makefile.def
> +++ b/Makefile.def
> @@ -548,6 +548,12 @@ dependencies = { module=configure-libctf; on=all-intl; };
>  dependencies = { module=configure-libctf; on=all-zlib; };
>  dependencies = { module=configure-libctf; on=all-libiconv; };
>
> +// The Makefiles in gdb and gdbserver pull in a file that configure
> +// generates in the gnulib directory, so distclean gnulib only after
> +// gdb and gdbserver.
> +dependencies = { module=distclean-gnulib; on=distclean-gdb; };
> +dependencies = { module=distclean-gnulib; on=distclean-gdbserver; };
> +
>  // Warning, these are not well tested.
>  dependencies = { module=all-bison; on=all-intl; };
>  dependencies = { module=all-bison; on=all-build-texinfo; };
> diff --git a/Makefile.in b/Makefile.in
> index 9dfd39fae13..fe34132f9e5 100644
> --- a/Makefile.in
> +++ b/Makefile.in
> @@ -52366,6 +52366,8 @@ configure-stage3-libctf: maybe-all-stage3-libiconv
>  configure-stage4-libctf: maybe-all-stage4-libiconv
>  configure-stageprofile-libctf: maybe-all-stageprofile-libiconv
>  configure-stagefeedback-libctf: maybe-all-stagefeedback-libiconv
> +distclean-gnulib: maybe-distclean-gdb
> +distclean-gnulib: maybe-distclean-gdbserver
>  all-bison: maybe-all-build-texinfo
>  all-flex: maybe-all-build-bison
>  all-flex: maybe-all-m4
>

I haven't tested, but I presume that you were able to reproduce the
problem and test the fix.  If so, that LGTM.

Thanks!

Simon

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] gnulib: Ensure all libraries are used when building gdb/gdbserver
  2020-10-13 14:26                 ` Simon Marchi
@ 2020-10-14 14:09                   ` Andrew Burgess
  2020-10-15  8:35                     ` Joel Brobecker
  0 siblings, 1 reply; 13+ messages in thread
From: Andrew Burgess @ 2020-10-14 14:09 UTC (permalink / raw)
  To: Simon Marchi; +Cc: Joel Brobecker, gdb-patches

* Simon Marchi <simark@simark.ca> [2020-10-13 10:26:48 -0400]:

> 
> On 2020-10-12 11:30 a.m., Andrew Burgess wrote:
> > commit 75b86a705adbb127962beb5ef8d9d7181afe3789
> > Author: Andrew Burgess <andrew.burgess@embecosm.com>
> > Date:   Mon Oct 12 16:04:32 2020 +0100
> >
> >     gdb/gdbserver: use '-include' to pull in Makefile.gnulib.inc
> 
> The patch subject doesn't seem right.
> 
> >
> >     After commit:
> >
> >       commit 361cb219351d8b7e39e1962fe77f40aa80657b27
> >       Date:   Tue Oct 6 10:09:06 2020 +0100
> >
> >           gnulib: Ensure all libraries are used when building gdb/gdbserver
> >
> >     We now get an error when, at the top level of the build tree, we do
> >     'make distclean'.
> >
> >     The reason for this is that the gnulib directory is cleaned before the
> >     gdb directory, cleaning gnulib deletes Makefile.gnulib.inc from the
> >     gnulib build directory, which is currently pulled in by the gdb
> >     Makefile.in using 'include'.
> >
> >     This commit adds a dependency between distclean-gnulib and both
> >     distclean-gdb and distclean-gdbserver.  This means that gdb and
> >     gdbserver will be cleaned before gnulib, as a result the
> >     Makefile.gnulib.inc file should exist when needed.
> >
> >     ChangeLog:
> >
> >             * Makefile.in: Rebuild.
> >             * Makefile.def: Make distclean-gnulib depend on distclean-gdb and
> >             distclean-gdbserver.
> >
> > diff --git a/Makefile.def b/Makefile.def
> > index 76d062bb671..089e70ae3ed 100644
> > --- a/Makefile.def
> > +++ b/Makefile.def
> > @@ -548,6 +548,12 @@ dependencies = { module=configure-libctf; on=all-intl; };
> >  dependencies = { module=configure-libctf; on=all-zlib; };
> >  dependencies = { module=configure-libctf; on=all-libiconv; };
> >
> > +// The Makefiles in gdb and gdbserver pull in a file that configure
> > +// generates in the gnulib directory, so distclean gnulib only after
> > +// gdb and gdbserver.
> > +dependencies = { module=distclean-gnulib; on=distclean-gdb; };
> > +dependencies = { module=distclean-gnulib; on=distclean-gdbserver; };
> > +
> >  // Warning, these are not well tested.
> >  dependencies = { module=all-bison; on=all-intl; };
> >  dependencies = { module=all-bison; on=all-build-texinfo; };
> > diff --git a/Makefile.in b/Makefile.in
> > index 9dfd39fae13..fe34132f9e5 100644
> > --- a/Makefile.in
> > +++ b/Makefile.in
> > @@ -52366,6 +52366,8 @@ configure-stage3-libctf: maybe-all-stage3-libiconv
> >  configure-stage4-libctf: maybe-all-stage4-libiconv
> >  configure-stageprofile-libctf: maybe-all-stageprofile-libiconv
> >  configure-stagefeedback-libctf: maybe-all-stagefeedback-libiconv
> > +distclean-gnulib: maybe-distclean-gdb
> > +distclean-gnulib: maybe-distclean-gdbserver
> >  all-bison: maybe-all-build-texinfo
> >  all-flex: maybe-all-build-bison
> >  all-flex: maybe-all-m4
> >
> 
> I haven't tested, but I presume that you were able to reproduce the
> problem and test the fix.  If so, that LGTM.

Indeed.

I updated the commit message title to:

  gdb/gdbserver: add dependencies for distclean-gnulib

And pushed this patch.

Joel - I think everything should be good now, but do let me know if
you are still seeing problems.

Thanks,
Andrew

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] gnulib: Ensure all libraries are used when building gdb/gdbserver
  2020-10-14 14:09                   ` Andrew Burgess
@ 2020-10-15  8:35                     ` Joel Brobecker
  0 siblings, 0 replies; 13+ messages in thread
From: Joel Brobecker @ 2020-10-15  8:35 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: Simon Marchi, gdb-patches

> Joel - I think everything should be good now, but do let me know if
> you are still seeing problems.

Thanks Andrew. It's looking good; I didn't get an error email last
night, and today's gdb.tar.gz snapshot points to
gdb-11.0.50.20201015.tar.xz.

Thanks again!
-- 
Joel

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2020-10-15  8:35 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-05 16:51 [PATCH] gnulib: Ensure all libraries are used when building gdb/gdbserver Andrew Burgess
2020-10-05 18:34 ` Simon Marchi
2020-10-06 12:17   ` Andrew Burgess
2020-10-06 14:10     ` Simon Marchi
2020-10-07 15:33       ` Andrew Burgess
2020-10-07 15:34         ` Simon Marchi
2020-10-09  8:34         ` Andrew Burgess
2020-10-12 11:41           ` Joel Brobecker
2020-10-12 14:16             ` Simon Marchi
2020-10-12 15:30               ` Andrew Burgess
2020-10-13 14:26                 ` Simon Marchi
2020-10-14 14:09                   ` Andrew Burgess
2020-10-15  8:35                     ` Joel Brobecker

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).