[V3,10/15] gdb: sim: buildsystem changes to accommodate libsframe
Checks
Commit Message
[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
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
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.
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
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
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
> 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@.
@@ -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)
@@ -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],
old mode 100755
new mode 100644
@@ -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 :
@@ -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 \
@@ -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 $@