diff mbox series

[V3,10/15] gdb: sim: buildsystem changes to accommodate libsframe

Message ID 20221030074450.1956074-11-indu.bhagat@oracle.com
State Not Applicable
Headers
Series Definition and support for SFrame unwind format |

Checks

Context Check Description
snail/binutils-gdb-check fail Git am fail log

Commit Message

Indu Bhagat Oct. 30, 2022, 7:44 a.m. UTC 
  [Changes in V3]
  - Additional diff in sim/ppc/Makefile.in to accommodate libsframe.
    This is needed to ensure --enable-targets=all continues to build.
  - Addressed review comments by Mike Frysinger.
[End of changes in V3]

[No changes in V2]

Both gdb and sim need buildsystem fixes to now include libsframe for a
successful build.
---
 gdb/Makefile.in           | 7 +++++--
 gdb/acinclude.m4          | 4 ++--
 gdb/configure             | 8 ++++----
 sim/common/Make-common.in | 7 +++++--
 sim/ppc/Makefile.in       | 5 +++--
 5 files changed, 19 insertions(+), 12 deletions(-)
 mode change 100755 => 100644 gdb/configure

Comments

Mike Frysinger Oct. 30, 2022, 2:15 p.m. UTC | #1 
On 30 Oct 2022 00:44, Indu Bhagat via Binutils wrote:
> [Changes in V3]
>   - Additional diff in sim/ppc/Makefile.in to accommodate libsframe.
>     This is needed to ensure --enable-targets=all continues to build.
>   - Addressed review comments by Mike Frysinger.

this doesn't seem to actually address my comments.  you're still poking
the internals of libtool by accessing files under .libs/.
-mike
Indu Bhagat Oct. 31, 2022, 8:39 p.m. UTC | #2 
On 10/30/22 07:15, Mike Frysinger wrote:
> On 30 Oct 2022 00:44, Indu Bhagat via Binutils wrote:
>> [Changes in V3]
>>    - Additional diff in sim/ppc/Makefile.in to accommodate libsframe.
>>      This is needed to ensure --enable-targets=all continues to build.
>>    - Addressed review comments by Mike Frysinger.
> 
> this doesn't seem to actually address my comments.  you're still poking
> the internals of libtool by accessing files under .libs/.
> -mike

gdb does not use libtool yet.

I see that opcodes/Makefile.am bfd/Makefile.am each create a 
opcodes/libopcodes.a and bfd/libbfd.a by copying over from the 
respective artifact from the .libs/ at build-time.
Mike Frysinger Nov. 2, 2022, 3:02 p.m. UTC | #3 
On 31 Oct 2022 13:39, Indu Bhagat wrote:
> On 10/30/22 07:15, Mike Frysinger wrote:
> > On 30 Oct 2022 00:44, Indu Bhagat via Binutils wrote:
> >> [Changes in V3]
> >>    - Additional diff in sim/ppc/Makefile.in to accommodate libsframe.
> >>      This is needed to ensure --enable-targets=all continues to build.
> >>    - Addressed review comments by Mike Frysinger.
> > 
> > this doesn't seem to actually address my comments.  you're still poking
> > the internals of libtool by accessing files under .libs/.
> 
> gdb does not use libtool yet.

you have access to the source.  you can change these things.

also, gdb & sim are sep projects.

> I see that opcodes/Makefile.am bfd/Makefile.am each create a 
> opcodes/libopcodes.a and bfd/libbfd.a by copying over from the 
> respective artifact from the .libs/ at build-time.

this forces static linking of libraries that are possibly available as
shared ones which isn't really what we want
-mike
Jose E. Marchesi Nov. 2, 2022, 7:11 p.m. UTC | #4 
Hi Mike, Indu.

>> > On 30 Oct 2022 00:44, Indu Bhagat via Binutils wrote:
>> >> [Changes in V3]
>> >>    - Additional diff in sim/ppc/Makefile.in to accommodate libsframe.
>> >>      This is needed to ensure --enable-targets=all continues to build.
>> >>    - Addressed review comments by Mike Frysinger.
>> > 
>> > this doesn't seem to actually address my comments.  you're still poking
>> > the internals of libtool by accessing files under .libs/.
>> 
>> gdb does not use libtool yet.
>
> you have access to the source.  you can change these things.
>
> also, gdb & sim are sep projects.

I see gdb/configure.ac uses the same strategy in order to locate the
in-tree libbacktrace.a and libctf:

  if test "${enable_libbacktrace}" = "yes"; then
    LIBBACKTRACE_INC="-I$srcdir/../libbacktrace/ -I../libbacktrace/"
    LIBBACKTRACE_LIB=../libbacktrace/.libs/libbacktrace.a
    AC_DEFINE(HAVE_LIBBACKTRACE, 1, [Define if libbacktrace is being used.])
  else
    LIBBACKTRACE_INC=
    LIBBACKTRACE_LIB=
  fi

  [...]

  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

With corresponding substitutions in gdb/Makefile.in.

I agree it would be better to have GDB libtoolized so it could refer to
the .la libraries directly thus avoiding internals, but could that be
done in a separated patch set, also covering the other cases?

In the meanwhile, Indu could change her patch in order to look for
libsframe.so in gdb/configure.ac instead of gdb/Makefile.in, as it is
done for the other libs.  Then we libtoolize.

WDYT?

>> I see that opcodes/Makefile.am bfd/Makefile.am each create a 
>> opcodes/libopcodes.a and bfd/libbfd.a by copying over from the 
>> respective artifact from the .libs/ at build-time.
>
> this forces static linking of libraries that are possibly available as
> shared ones which isn't really what we want
Mike Frysinger Nov. 3, 2022, 3:27 p.m. UTC | #5 
On 02 Nov 2022 20:11, Jose E. Marchesi wrote:
> 
> Hi Mike, Indu.
> 
> >> > On 30 Oct 2022 00:44, Indu Bhagat via Binutils wrote:
> >> >> [Changes in V3]
> >> >>    - Additional diff in sim/ppc/Makefile.in to accommodate libsframe.
> >> >>      This is needed to ensure --enable-targets=all continues to build.
> >> >>    - Addressed review comments by Mike Frysinger.
> >> > 
> >> > this doesn't seem to actually address my comments.  you're still poking
> >> > the internals of libtool by accessing files under .libs/.
> >> 
> >> gdb does not use libtool yet.
> >
> > you have access to the source.  you can change these things.
> >
> > also, gdb & sim are sep projects.
> 
> I see gdb/configure.ac uses the same strategy in order to locate the
> in-tree libbacktrace.a and libctf:
> 
>   if test "${enable_libbacktrace}" = "yes"; then
>     LIBBACKTRACE_INC="-I$srcdir/../libbacktrace/ -I../libbacktrace/"
>     LIBBACKTRACE_LIB=../libbacktrace/.libs/libbacktrace.a
>     AC_DEFINE(HAVE_LIBBACKTRACE, 1, [Define if libbacktrace is being used.])
>   else
>     LIBBACKTRACE_INC=
>     LIBBACKTRACE_LIB=
>   fi
> 
>   [...]
> 
>   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
> 
> With corresponding substitutions in gdb/Makefile.in.
> 
> I agree it would be better to have GDB libtoolized so it could refer to
> the .la libraries directly thus avoiding internals, but could that be
> done in a separated patch set, also covering the other cases?
> 
> In the meanwhile, Indu could change her patch in order to look for
> libsframe.so in gdb/configure.ac instead of gdb/Makefile.in, as it is
> done for the other libs.  Then we libtoolize.

"the code is already in bad shape, so let's add more kindle to the fire"
isn't a great strategy.  hoping someone else will come and clean up the
mess also isn't a great strategy ... usually that means it never gets
cleaned up, and the tech debt just continues to build.  so "let's do
this as a followup" almost always translates into "i don't want to do
it, and it's never actually going to happen, so let me merge anyways".
i'm not saying that's necessarily the intention of the person making
such a request, just that that's the practical result in my experience
in the vast majority of cases.  people, no matter how well intentioned,
are busy, so without any pressing leverage (like "this is required if
you want to merge"), it never improves.

to be clear, i'm not a global gdb maintainer, so if you can convince
one of them, then certainly they override.  i am NAKing adding any
such hacks to the sim code though.  although that's a bit moot since
i've already posted patches to clean up its libtool usage which means
it doesn't need any changes for libsframe logic.
-mike
Jose E. Marchesi Nov. 4, 2022, 12:14 p.m. UTC | #6 
> On 02 Nov 2022 20:11, Jose E. Marchesi wrote:
>> 
>> Hi Mike, Indu.
>> 
>> >> > On 30 Oct 2022 00:44, Indu Bhagat via Binutils wrote:
>> >> >> [Changes in V3]
>> >> >>    - Additional diff in sim/ppc/Makefile.in to accommodate libsframe.
>> >> >>      This is needed to ensure --enable-targets=all continues to build.
>> >> >>    - Addressed review comments by Mike Frysinger.
>> >> > 
>> >> > this doesn't seem to actually address my comments.  you're still poking
>> >> > the internals of libtool by accessing files under .libs/.
>> >> 
>> >> gdb does not use libtool yet.
>> >
>> > you have access to the source.  you can change these things.
>> >
>> > also, gdb & sim are sep projects.
>> 
>> I see gdb/configure.ac uses the same strategy in order to locate the
>> in-tree libbacktrace.a and libctf:
>> 
>>   if test "${enable_libbacktrace}" = "yes"; then
>>     LIBBACKTRACE_INC="-I$srcdir/../libbacktrace/ -I../libbacktrace/"
>>     LIBBACKTRACE_LIB=../libbacktrace/.libs/libbacktrace.a
>>     AC_DEFINE(HAVE_LIBBACKTRACE, 1, [Define if libbacktrace is being used.])
>>   else
>>     LIBBACKTRACE_INC=
>>     LIBBACKTRACE_LIB=
>>   fi
>> 
>>   [...]
>> 
>>   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
>> 
>> With corresponding substitutions in gdb/Makefile.in.
>> 
>> I agree it would be better to have GDB libtoolized so it could refer to
>> the .la libraries directly thus avoiding internals, but could that be
>> done in a separated patch set, also covering the other cases?
>> 
>> In the meanwhile, Indu could change her patch in order to look for
>> libsframe.so in gdb/configure.ac instead of gdb/Makefile.in, as it is
>> done for the other libs.  Then we libtoolize.
>
> "the code is already in bad shape, so let's add more kindle to the fire"
> isn't a great strategy.  hoping someone else will come and clean up the
> mess also isn't a great strategy ... usually that means it never gets
> cleaned up, and the tech debt just continues to build.  so "let's do
> this as a followup" almost always translates into "i don't want to do
> it, and it's never actually going to happen, so let me merge anyways".
> i'm not saying that's necessarily the intention of the person making
> such a request, just that that's the practical result in my experience
> in the vast majority of cases.  people, no matter how well intentioned,
> are busy, so without any pressing leverage (like "this is required if
> you want to merge"), it never improves.
>
> to be clear, i'm not a global gdb maintainer, so if you can convince
> one of them, then certainly they override.  i am NAKing adding any
> such hacks to the sim code though.  although that's a bit moot since
> i've already posted patches to clean up its libtool usage which means
> it doesn't need any changes for libsframe logic.
> -mike

I'm following up in gdb-patches@.
diff mbox series

Patch

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index c528ee5aa80..98157498fdb 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -161,6 +161,9 @@  LIBIBERTY = ../libiberty/libiberty.a
 LIBCTF = @LIBCTF@
 CTF_DEPS = @CTF_DEPS@
 
+# Where is the SFrame library?  Typically in ../libsframe.
+LIBSFRAME = ../libsframe/.libs/libsframe.a
+
 # Where is the BFD library?  Typically in ../bfd.
 BFD_DIR = ../bfd
 BFD = $(BFD_DIR)/libbfd.a
@@ -650,7 +653,7 @@  INTERNAL_LDFLAGS = \
 # Libraries and corresponding dependencies for compiling gdb.
 # XM_CLIBS, defined in *config files, have host-dependent libs.
 # LIBIBERTY appears twice on purpose.
-CLIBS = $(SIM) $(READLINE) $(OPCODES) $(LIBCTF) $(BFD) $(ZLIB) $(ZSTD_LIBS) \
+CLIBS = $(SIM) $(READLINE) $(OPCODES) $(LIBCTF) $(BFD) $(LIBSFRAME) $(ZLIB) $(ZSTD_LIBS) \
         $(LIBSUPPORT) $(INTL) $(LIBIBERTY) $(LIBDECNUMBER) \
 	$(XM_CLIBS) $(GDBTKLIBS)  $(LIBBACKTRACE_LIB) \
 	@LIBS@ @GUILE_LIBS@ @PYTHON_LIBS@ \
@@ -658,7 +661,7 @@  CLIBS = $(SIM) $(READLINE) $(OPCODES) $(LIBCTF) $(BFD) $(ZLIB) $(ZSTD_LIBS) \
 	$(WIN32LIBS) $(LIBGNU) $(LIBGNU_EXTRA_LIBS) $(LIBICONV) \
 	$(LIBMPFR) $(LIBGMP) $(SRCHIGH_LIBS) $(LIBXXHASH) $(PTHREAD_LIBS) \
 	$(DEBUGINFOD_LIBS) $(LIBBABELTRACE_LIB)
-CDEPS = $(NAT_CDEPS) $(SIM) $(BFD) $(READLINE_DEPS) $(CTF_DEPS) \
+CDEPS = $(NAT_CDEPS) $(SIM) $(BFD) $(LIBSFRAME) $(READLINE_DEPS) $(CTF_DEPS) \
 	$(OPCODES) $(INTL_DEPS) $(LIBIBERTY) $(CONFIG_DEPS) $(LIBGNU) \
 	$(LIBSUPPORT)
 
diff --git a/gdb/acinclude.m4 b/gdb/acinclude.m4
index 62fa66c7af3..8bbc5f0739e 100644
--- a/gdb/acinclude.m4
+++ b/gdb/acinclude.m4
@@ -234,9 +234,9 @@  AC_DEFUN([GDB_AC_CHECK_BFD], [
   # always want our bfd.
   CFLAGS="-I${srcdir}/../include -I../bfd -I${srcdir}/../bfd $CFLAGS"
   ZLIBDIR=`echo $zlibdir | sed 's,\$(top_builddir)/,,g'`
-  LDFLAGS="-L../bfd -L../libiberty $ZLIBDIR $LDFLAGS"
+  LDFLAGS="-L../bfd -L../libiberty -L../libsframe/.libs/ $ZLIBDIR $LDFLAGS"
   intl=`echo $LIBINTL | sed 's,${top_builddir}/,,g'`
-  LIBS="-lbfd -liberty -lz $ZSTD_LIBS $intl $LIBS"
+  LIBS="-lbfd -liberty -lz -lsframe $ZSTD_LIBS $intl $LIBS"
   AC_CACHE_CHECK(
     [$1],
     [$2],
diff --git a/gdb/configure b/gdb/configure
old mode 100755
new mode 100644
index 33677262783..71288533efd
--- a/gdb/configure
+++ b/gdb/configure
@@ -17412,9 +17412,9 @@  WIN32LIBS="$WIN32LIBS $WIN32APILIBS"
   # always want our bfd.
   CFLAGS="-I${srcdir}/../include -I../bfd -I${srcdir}/../bfd $CFLAGS"
   ZLIBDIR=`echo $zlibdir | sed 's,\$(top_builddir)/,,g'`
-  LDFLAGS="-L../bfd -L../libiberty $ZLIBDIR $LDFLAGS"
+  LDFLAGS="-L../bfd -L../libiberty -L../libsframe/.libs/ $ZLIBDIR $LDFLAGS"
   intl=`echo $LIBINTL | sed 's,${top_builddir}/,,g'`
-  LIBS="-lbfd -liberty -lz $ZSTD_LIBS $intl $LIBS"
+  LIBS="-lbfd -liberty -lz -lsframe $ZSTD_LIBS $intl $LIBS"
   { $as_echo "$as_me:${as_lineno-$LINENO}: checking for ELF support in BFD" >&5
 $as_echo_n "checking for ELF support in BFD... " >&6; }
 if ${gdb_cv_var_elf+:} false; then :
@@ -17527,9 +17527,9 @@  fi
   # always want our bfd.
   CFLAGS="-I${srcdir}/../include -I../bfd -I${srcdir}/../bfd $CFLAGS"
   ZLIBDIR=`echo $zlibdir | sed 's,\$(top_builddir)/,,g'`
-  LDFLAGS="-L../bfd -L../libiberty $ZLIBDIR $LDFLAGS"
+  LDFLAGS="-L../bfd -L../libiberty -L../libsframe/.libs/ $ZLIBDIR $LDFLAGS"
   intl=`echo $LIBINTL | sed 's,${top_builddir}/,,g'`
-  LIBS="-lbfd -liberty -lz $ZSTD_LIBS $intl $LIBS"
+  LIBS="-lbfd -liberty -lz -lsframe $ZSTD_LIBS $intl $LIBS"
   { $as_echo "$as_me:${as_lineno-$LINENO}: checking for Mach-O support in BFD" >&5
 $as_echo_n "checking for Mach-O support in BFD... " >&6; }
 if ${gdb_cv_var_macho+:} false; then :
diff --git a/sim/common/Make-common.in b/sim/common/Make-common.in
index b07ec96e147..8a49e0b4ef2 100644
--- a/sim/common/Make-common.in
+++ b/sim/common/Make-common.in
@@ -222,11 +222,14 @@  SIM_HW_DEVICES = cfi core pal glue $(SIM_EXTRA_HW_DEVICES)
 ZLIB = $(zlibdir) -lz
 LIBIBERTY_LIB = ../../libiberty/libiberty.a
 BFD_LIB = ../../bfd/libbfd.a
+LIBSFRAME_LIB = ../../libsframe/.libs/libsframe.a
 OPCODES_LIB = ../../opcodes/libopcodes.a
 CONFIG_LIBS = $(COMMON_LIBS) @LIBS@ $(ZLIB) $(ZSTD_LIBS)
-LIBDEPS = $(BFD_LIB) $(OPCODES_LIB) $(LIBINTL_DEP) $(LIBIBERTY_LIB)
+LIBDEPS = $(BFD_LIB) $(OPCODES_LIB) $(LIBINTL_DEP) $(LIBIBERTY_LIB) \
+	  $(LIBSFRAME_LIB)
 EXTRA_LIBS = $(BFD_LIB) $(OPCODES_LIB) $(LIBINTL) $(LIBIBERTY_LIB) \
-	$(CONFIG_LIBS) $(SIM_EXTRA_LIBS) $(LIBDL) $(LIBGNU) $(LIBGNU_EXTRA_LIBS)
+	     $(LIBSFRAME_LIB) $(CONFIG_LIBS) $(SIM_EXTRA_LIBS) \
+	     $(LIBDL) $(LIBGNU) $(LIBGNU_EXTRA_LIBS)
 
 COMMON_OBJS_NAMES = \
 	callback.o \
diff --git a/sim/ppc/Makefile.in b/sim/ppc/Makefile.in
index b0c073b8867..12123d36a66 100644
--- a/sim/ppc/Makefile.in
+++ b/sim/ppc/Makefile.in
@@ -133,6 +133,7 @@  INCLUDES	= -I. -I$(srcdir) $(LIB_INCLUDES) $(BFD_INCLUDES) $(GDB_INCLUDES) -I../
 
 LIBIBERTY_LIB	= ../../libiberty/libiberty.a
 BFD_LIB		= ../../bfd/libbfd.a
+SFRAME_LIB	= ../../libsframe/.libs/libsframe.a
 ZLIB		= $(zlibdir) -lz
 
 
@@ -521,8 +522,8 @@  PACKAGE_SRC = @sim_pk_src@
 PACKAGE_OBJ = @sim_pk_obj@
 
 
-psim$(EXEEXT): $(TARGETLIB) main.o $(LIBIBERTY_LIB) $(BFD_LIB) $(LIBINTL_DEP)
-	$(ECHO_CCLD) $(CC) $(CFLAGS) $(LDFLAGS) -o psim$(EXEEXT) main.o $(TARGETLIB) $(BFD_LIB) $(ZLIB) $(ZSTD_LIBS) $(LIBINTL) $(LIBIBERTY_LIB) $(LIBS)
+psim$(EXEEXT): $(TARGETLIB) main.o $(LIBIBERTY_LIB) $(BFD_LIB) $(SFRAME_LIB) $(LIBINTL_DEP)
+	$(ECHO_CCLD) $(CC) $(CFLAGS) $(LDFLAGS) -o psim$(EXEEXT) main.o $(TARGETLIB) $(BFD_LIB) $(SFRAME_LIB) $(ZLIB) $(ZSTD_LIBS) $(LIBINTL) $(LIBIBERTY_LIB) $(LIBS)
 
 run$(EXEEXT): psim$(EXEEXT)
 	$(SILENCE) rm -f $@