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

[Elena Zannoni <elena.zannoni@oracle.com>]

My obvious questions are:
1. Tromey approved already the gdb changes, so really we need to check with him.
2. I agree that any opportunity to clean up the code is good, but how much work will it be to do the libtoolization? And especially test it?

elena
 
On 11/4/22 06:02, Jose E. Marchesi wrote:
> 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
>
>
> Content-Type: text/plain; charset=utf-8
>
> Content-Disposition: inline
>
>
> 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
>
>
>
>
> -------------------- End of forwarded message --------------------