Re: [PATCH V2] gdb: link executables with libtool

["Jose E. Marchesi" <jose.marchesi@oracle.com>]

> On 11/5/22 10:02, Jose E. Marchesi via Gdb-patches wrote:
>> This patch changes the GDB build system in order to use libtool to
>> link the several built executables.  This makes it possible to refer
>> to libtool libraries (.la files) in CLIBS.
>> 
>> As an application of the above,
>> 
>>   LIBBFD           new refers to ../libbfd/libbfd.la
>
> new -> now
>
>>   LIBBACKTRACE_LIB now refers to ../libbacktrace/libbacktrace.la
>>   LIBCTF           now refers to ../libctf/libctf.la
>
> The patch linked in this bug:
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=29372
>
> also updated OPCODES to use $(OPCODES_DIR)/libopcodes.la, should you do
> it too?

Indeed I should.  Thanks for pointing that out :)


>> 
>> NOTE1: The addition of libtool adds a few new configure-time options
>>        to GDB.  Among these, --enable-shared and --disable-shared,
>>        which were previously ignored.  Now GDB shall honor these
>>        options when linking, picking up the right version of the
>>        referred libtool libraries automagically.
>> 
>> NOTE2: I have not tested the insight build.
>> 
>> NOTE3: For regenerating configure I used an environment with Autoconf
>>        2.69 and Automake 1.15.1.  This should match the previously
>>        used version as announced in the configure script.
>
> Your patch appears to be missing the changes to gdb/configure, however
> the diffstat in your email shows:
>
>  gdb/configure             | 31544 ++++++++++++++++++++++++------------
>
> When running `autoreconf -f` in the gdb/ directory, I get some changes
> that revert your changes to gdb/aclocal.m4, and makes some changes to
> gdb/configure.  But I don't think my changes are right, because LT_INIT
> stays unexpanded in the generated configure.
>
> Does the method of regenerating the build system change with this patch?
>
> If I run libtoolize in gdb/, it adds a bunch of .m4 files to
> $top_level/config, and then if I run autoreconf, LT_INIT gets properly
> expanded.  But these .m4 files already exist at the top-level such as
> $top_level/libtool.m4, I guess we don't want them twice.  Should
> gdb/configure.ac be modified to find the existing files?  For instance,
> this makes it work for me:
>
>
> diff --git a/gdb/configure.ac b/gdb/configure.ac
> index 4fd77e485762..12561d4d2de6 100644
> --- a/gdb/configure.ac
> +++ b/gdb/configure.ac
> @@ -19,7 +19,7 @@ dnl along with this program.  If not, see <http://www.gnu.org/licenses/>.
>  dnl Process this file with autoconf to produce a configure script.
>  
>  AC_INIT
> -AC_CONFIG_MACRO_DIRS([../config])
> +AC_CONFIG_MACRO_DIRS([.. ../config])
>  AC_CONFIG_SRCDIR([main.c])
>  AC_CONFIG_HEADERS(config.h:config.in, [echo > stamp-h])
>  AM_MAINTAINER_MODE

Hmm, the libtool that is in the binutils tree (in the top-level .m4
files, which are macros that generate the actual `libtool' scripts at
configure time) is actually:

1) A phantom release 2.2.7a that doesn't exist anyhwere else, or at
   least I haven't managed to find it.  It certainly doesn't exist in
   ftp.gnu.org nor in alpha.gnu.org.
   
2) Locally patched.

Indeed `autoreconf' runs libtoolize, so to avoid it to install other .m4
files your fix above makes sense.


>> NOTE4: Now the installed shared object libbfd.so is used by gdb
>>        if binutils is installed with --enable-shared.
>
> Do you mean bfd instead of binutils?  Or do you mean binutils in the
> sense that bfd is part of binutils?
>
>> Testing performed:
>> 
>> - --enable-shared and --disable-shared (the default in binutils) work
>>   as expected: the linked executables link with the archive or shared
>>   libraries transparently.
>> 
>> - Makefile.in modified for EXEEXT = .exe.  It installs the binaries
>>   just fine.  The installed gdb.exe runs fine.
>> 
>> - Native build regtested in x86_64. The installed gdb runs fine.
>> 
>>   In the regression testing I'm observing that the following tests
>>   doesn't seem to be deterministic:
>> 
>>     gdb.base/step-over-syscall.exp
>>     gdb.threads/process-dies-while-detaching.exp
>>     gdb.threads/process-dies-while-handling-bp.exp
>> 
>>   Sometimes some of the the tests in these files unexpectedly fail,
>>   like in:
>> 
>>   -PASS: gdb.threads/process-dies-while-detaching.exp: single-process: \
>>          continue: detach: continue
>>   +FAIL: gdb.threads/process-dies-while-detaching.exp: single-process: \
>>          continue: detach: continue
>> 
>>   Sometimes they unexpectedly pass:
>> 
>>   -KFAIL: gdb.base/step-over-syscall.exp: clone: displaced=on: \
>>           check_pc_after_cross_syscall: single step over clone \
>>           final pc (PRMS: gdb/19675)
>>   +PASS: gdb.base/step-over-syscall.exp: clone: displaced=on: \
>>          check_pc_after_cross_syscall: single step over clone final pc
>> 
>>   -KFAIL: gdb.threads/process-dies-while-handling-bp.exp: \
>>           non_stop=on: cond_bp_target=0: inferior 1 exited \
>>           (prompt) (PRMS: gdb/18749)
>>   +PASS: gdb.threads/process-dies-while-handling-bp.exp: \
>>           non_stop=on: cond_bp_target=0: inferior 1 exited
>
> It's unfortunately not the only ones.  It's not good, but it's known.

Ah ok.  I would say it it not bad at all, considering both the size and
character of the GDB testsuite.

>> - Cross build for aarch64-linux-gnu built to exercise
>>   program_transform_name and friends.  The installed
>>   aarch64-linux-gnu-gdb runs fine.
>
> Great, thanks for the details on the testing.
>
>> 
>> 2022-11-05  Jose E. Marchesi  <jose.marchesi@oracle.com>
>> 
>> 	* gdb/aclocal.m4: include libtool macros from the top-level
>> 	directory.
>> 	* gdb/Makefile.in (LIBTOOL): Define.
>> 	(CC_LD): Use libtool for linking.
>> 	(install-only): Use libtool for installing GDB$(EXEEXT).
>> 	(install-gdbtk): Likewise for insight$(EXEEXT).
>> 	* gdb/configure.ac: Call LT_INIT.
>> 	Refer to libctf.la and libbacktrace.la.
>> 	* gdb/configure: Regenerate.
>> 	* gdb/testsuite/lib/gdb.exp: Run uninstalled GDB with libtool
>> 	--mode=execute.
>
> Could you add the git trailer:
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29372
>
> ?

Will do.

>
>> ---
>>  ChangeLog                 |    15 +
>
> I don't think you should add gdb changes to the top-level ChangeLog.  We
> simply don't use ChangeLogs now.

Oops, that was Emacs C-x4a.  I saw ChangeLog entries in the buffer with
reasonably recent entries and assumed it was GDB's.  Sorry about that :)

>
>>  gdb/Makefile.in           |    12 +-
>>  gdb/aclocal.m4            |     5 +
>>  gdb/configure             | 31544 ++++++++++++++++++++++++------------
>>  gdb/configure.ac          |    15 +-
>>  gdb/testsuite/lib/gdb.exp |     2 +-
>>  6 files changed, 21142 insertions(+), 10451 deletions(-)
>> 
>> diff --git a/ChangeLog b/ChangeLog
>> index 73c3a006881..fea3f7c6877 100644
>> --- a/ChangeLog
>> +++ b/ChangeLog
>> @@ -1,3 +1,18 @@
>> +2022-11-05  Jose E. Marchesi  <jose.marchesi@oracle.com>
>> +
>> +	* gdb/aclocal.m4: include libtool macros from the top-level
>> +	directory.
>> +	* gdb/Makefile.in (LIBTOOL): Define.
>> +	(CC_LD): Use libtool for linking.
>> +	(install-only): Use libtool for installing GDB$(EXEEXT).
>> +	(install-gdbtk): Likewise for insight$(EXEEXT).
>> +	Use libbfd.la instead of libbfd.a.
>> +	* gdb/configure.ac: Call LT_INIT.
>> +	Refer to libctf.la and libbacktrace.la.
>> +	* gdb/configure: Regenerate.
>> +	* gdb/testsuite/lib/gdb.exp: Run uninstalled GDB with libtool
>> +	--mode=execute.
>> +
>>  2022-10-10  Nick Clifton  <nickc@redhat.com>
>>  
>>  	* src-release.sh: Add "-r <date>" option to create reproducible
>> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
>> index c528ee5aa80..b03eedd4d2f 100644
>> --- a/gdb/Makefile.in
>> +++ b/gdb/Makefile.in
>> @@ -143,10 +143,12 @@ MAKEINFO_CMD = $(MAKEINFO) $(MAKEINFOFLAGS) $(MAKEINFO_EXTRA_FLAGS)
>>  MAKEHTML = $(MAKEINFO_CMD) --html
>>  MAKEHTMLFLAGS =
>>  
>> +LIBTOOL = @LIBTOOL@
>> +
>>  # Set this up with gcc if you have gnu ld and the loader will print out
>>  # line numbers for undefined references.
>>  #CC_LD = g++ -static
>> -CC_LD = $(CXX) $(CXX_DIALECT)
>> +CC_LD = $(LIBTOOL) --mode=link $(CXX) $(CXX_DIALECT)
>>  
>>  # Where is our "include" directory?  Typically $(srcdir)/../include.
>>  # This is essentially the header file directory for the library
>> @@ -163,7 +165,7 @@ CTF_DEPS = @CTF_DEPS@
>>  
>>  # Where is the BFD library?  Typically in ../bfd.
>>  BFD_DIR = ../bfd
>> -BFD = $(BFD_DIR)/libbfd.a
>> +BFD = $(BFD_DIR)/libbfd.la
>>  BFD_SRC = $(srcdir)/$(BFD_DIR)
>>  BFD_CFLAGS = -I$(BFD_DIR) -I$(BFD_SRC)
>>  
>> @@ -2017,7 +2019,8 @@ install-only: $(CONFIG_INSTALL)
>>  		  true ; \
>>  		fi ; \
>>  		$(SHELL) $(srcdir)/../mkinstalldirs $(DESTDIR)$(bindir) ; \
>> -		$(INSTALL_PROGRAM_ENV) $(INSTALL_PROGRAM) gdb$(EXEEXT) \
>> +		$(LIBTOOL) --mode=install $(INSTALL_PROGRAM_ENV) $(INSTALL_PROGRAM) \
>> +			gdb$(EXEEXT) \
>>  			$(DESTDIR)$(bindir)/$$transformed_name$(EXEEXT) ; \
>>  		$(SHELL) $(srcdir)/../mkinstalldirs $(DESTDIR)$(includedir)/gdb ; \
>>  		$(INSTALL_DATA) jit-reader.h $(DESTDIR)$(includedir)/gdb/jit-reader.h
>> @@ -2529,7 +2532,8 @@ install-gdbtk:
>>  	  true ; \
>>  	fi ; \
>>  	$(SHELL) $(srcdir)/../mkinstalldirs $(DESTDIR)$(bindir); \
>> -	$(INSTALL_PROGRAM_ENV) $(INSTALL_PROGRAM) insight$(EXEEXT) \
>> +	$(LIBTOOL) --mode=install $(INSTALL_PROGRAM_ENV) $(INSTALL_PROGRAM) \
>> +		insight$(EXEEXT) \
>>  		$(DESTDIR)$(bindir)/$$transformed_name$(EXEEXT) ; \
>>  	$(SHELL) $(srcdir)/../mkinstalldirs \
>>  		$(DESTDIR)$(GDBTK_LIBRARY) ; \
>> diff --git a/gdb/aclocal.m4 b/gdb/aclocal.m4
>> index 3ed4a58d39f..4aac87b52ed 100644
>> --- a/gdb/aclocal.m4
>> +++ b/gdb/aclocal.m4
>> @@ -212,4 +212,9 @@ m4_include([../config/override.m4])
>>  m4_include([../config/pkg.m4])
>>  m4_include([../config/plugins.m4])
>>  m4_include([../config/tcl.m4])
>> +m4_include([../libtool.m4])
>> +m4_include([../ltoptions.m4])
>> +m4_include([../ltsugar.m4])
>> +m4_include([../ltversion.m4])
>> +m4_include([../lt~obsolete.m4])
>>  m4_include([acinclude.m4])
>> diff --git a/gdb/configure.ac b/gdb/configure.ac
>> index fceb80e8c9d..4fd77e48576 100644
>> --- a/gdb/configure.ac
>> +++ b/gdb/configure.ac
>> @@ -46,6 +46,10 @@ ACX_NONCANONICAL_TARGET
>>  
>>  AC_ARG_PROGRAM
>>  
>> +# We require libtool to link with the in-tree libtool libraries
>> +# the proper way.
>> +LT_INIT
>> +
>>  # We require a C++11 compiler.  Check if one is available, and if
>>  # necessary, set CXX_DIALECT to some -std=xxx switch.
>>  AX_CXX_COMPILE_STDCXX(11, , mandatory)
>> @@ -2092,7 +2096,7 @@ AC_ARG_ENABLE([libbacktrace],
>>  
>>  if test "${enable_libbacktrace}" = "yes"; then
>>    LIBBACKTRACE_INC="-I$srcdir/../libbacktrace/ -I../libbacktrace/"
>> -  LIBBACKTRACE_LIB=../libbacktrace/.libs/libbacktrace.a
>> +  LIBBACKTRACE_LIB=../libbacktrace/libbacktrace.la
>>    AC_DEFINE(HAVE_LIBBACKTRACE, 1, [Define if libbacktrace is being used.])
>>  else
>>    LIBBACKTRACE_INC=
>> @@ -2151,15 +2155,10 @@ AC_ARG_WITH(xxhash,
>>    [], [with_xxhash=auto])
>>  
>>  GCC_ENABLE([libctf], [yes], [], [Handle .ctf type-info sections])
>> -if test x${enable_static} = xno; then
>> -  LIBCTF="-Wl,--rpath,../libctf/.libs ../libctf/.libs/libctf.so"
>> -  CTF_DEPS="../libctf/.libs/libctf.so"
>> -else
>> -  LIBCTF="../libctf/.libs/libctf.a"
>> -  CTF_DEPS="$LIBCTF"
>> -fi
>>  if test "${enable_libctf}" = yes; then
>>    AC_DEFINE(ENABLE_LIBCTF, 1, [Handle .ctf type-info sections])
>> +  LIBCTF="../libctf/libctf.la"
>> +  CTF_DEPS="../libctf/libctf.la"
>>  else
>>    LIBCTF=
>>    CTF_DEPS=
>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>> index e2cda30b95a..63d57581d13 100644
>> --- a/gdb/testsuite/lib/gdb.exp
>> +++ b/gdb/testsuite/lib/gdb.exp
>> @@ -154,7 +154,7 @@ global GDB_DATA_DIRECTORY
>>  global inferior_spawn_id
>>  
>>  if [info exists TOOL_EXECUTABLE] {
>> -    set GDB $TOOL_EXECUTABLE
>> +    set GDB "libtool --mode=execute $TOOL_EXECUTABLE"
>
> Can you explain why this is needed?  My understanding is that
> TOOL_EXECUTABLE can be used to point to an arbitrary gdb executable to
> test.  If it's a gdb that has been "make install"ed, then libtool
> doesn't have to be involved.  If pointing to a gdb executable in its
> build directory, then "gdb" will be a libtool script that sets all the
> right runtime library paths.  The only moment I can imagine this would
> be useful would be if TOOL_EXECUTABLE consists of a wrapper around
> gdb, e.g. "valgrind /path/to/build/gdb".

Hm, I actually changed that to see if it would help with the failing
tests, since these (related to forking etc) may have been sensible to
the fact the libtool wrapper script introduced an additional process in
the chain.  I agree with you, will remove this thunk from the next
version of the patch.

Thanks for the review!