From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 592903857C4E for ; Mon, 5 Oct 2020 18:34:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 592903857C4E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [172.16.0.95] (192-222-181-218.qc.cable.ebox.net [192.222.181.218]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id EE9911E58D; Mon, 5 Oct 2020 14:34:57 -0400 (EDT) Subject: Re: [PATCH] gnulib: Ensure all libraries are used when building gdb/gdbserver To: Andrew Burgess , gdb-patches@sourceware.org References: <20201005165134.1620549-1-andrew.burgess@embecosm.com> From: Simon Marchi Message-ID: <2feb66c2-8f65-7cbf-d1be-ae3d04b45d9a@simark.ca> Date: Mon, 5 Oct 2020 14:34:57 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <20201005165134.1620549-1-andrew.burgess@embecosm.com> Content-Type: text/plain; charset=utf-8 Content-Language: tl Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 05 Oct 2020 18:35:00 -0000 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 _LDADD when linking a program, or > in _a_LDFLAGS or _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