[Mike Frysinger] Re: [PATCH, V3 10/15] gdb: sim: buildsystem changes to accommodate libsframe

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

[-- Attachment #1: Type: text/plain, Size: 1535 bytes --]


Hello people!

What is the opinion of the GDB maintainers about the necessity of
libtoolizing GDB before accepting changes like the following:

In gdb/configure.ac:

  if test x${enable_static} = xno; then
    LIBSFRAME="-Wl,--rpath,../libsframe/.libs ../libsframe/libsframe.a"
    SFRAME_DEPS="../libsframe/.libs/libsframe.so"
  else
    LIBSFRAME="../libsframe/.libs/libsframe.a"
    SFRAME_DEPS="$LIBSFRAME"
  fi
  AC_SUBST([LIBSFRAME])
  AC_SUBST([SFRAME_DEPS])

In gdb/Makefile.in:

  LIBSFRAME = @LIBSFRAME@
  SFRAME_DEPS = @SFRAME_DEPS@

  [...]

  CLIBS += $(LIBSFRAME)
  CDEPS += $(SFRAME_DEPS)

Note that both the in-tree libs libctf and libbacktrace are currently
handled the same way.

I have to admit I find myself split here.

As a maintainer, I would share Mike's reservations (maybe not as
strongly, but I get it) expressed in the forwarded email below.

Corporate wise, however, we would really need to close the "add sframe
support to binutils" chapter before opening the "libtoolize GDB" chapter
or we can find ourselves in trouble.

But first things first: how do you people feel about libtoolizing the
rules in gdb/Makefile.in so GDB can handle the in-tree libtool libraries
in a more graceful way?

-------------------- Start of forwarded message --------------------
Date: Thu, 3 Nov 2022 22:27:34 +0700
From: Mike Frysinger <vapier@gentoo.org>
To: "Jose E. Marchesi" <jose.marchesi@oracle.com>
Cc: binutils@sourceware.org
Subject: Re: [PATCH, V3 10/15] gdb: sim: buildsystem changes to accommodate
 libsframe


[-- Attachment #2.1: Type: text/plain, Size: 2905 bytes --]

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

[-- Attachment #2.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #3: Type: text/plain, Size: 67 bytes --]

-------------------- End of forwarded message --------------------