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

[Simon Marchi <simark@simark.ca>]

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?

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



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

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

?

> ---
>  ChangeLog                 |    15 +

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

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

Simon