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

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

[Jose E. Marchesi]

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

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

[Elena Zannoni]

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

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

[Simon Marchi]

> 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?
What does "libtoolizing the rules" mean, in practice?

Simon

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

[Jose E. Marchesi]

>> 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?
> What does "libtoolizing the rules" mean, in practice?

I would say to be able to refer to the libtool libraries (.la) directly
when linking, and let libtool do the magic of picking up the
corresponding archive or shared object.

This means that instead of the linker, you have to invoke something
like:

  libtool --mode=link foo.o bar.o baz.la ...

I have to confess I have never autotoolized a project that does not use
Automake.  Automake takes care of generating the right rules for linking
when you specify a .la library in either LIBADD or LDADD.

From the libtool manual:

  If you're not using Automake, and you don't know how to incorporate
  libtool into your package you need to do one of the following:

  1. Download the latest Automake distribution from your nearest GNU
     mirror, install it, and start using it.

  2. Learn how to write 'Makefile' rules by hand.  They're sometimes
     complex, but if you're clever enough to write rules for compiling
     your old libraries, then you should be able to figure out new rules
     for libtool libraries (hint: examine the 'Makefile.in' in the
     'tests/demo' subdirectory of the libtool distribution... note
     especially that it was automatically generated from the
     'Makefile.am' by Automake).

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

[Jose E. Marchesi]

>>> 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?
>> What does "libtoolizing the rules" mean, in practice?
>
> I would say to be able to refer to the libtool libraries (.la) directly
> when linking, and let libtool do the magic of picking up the
> corresponding archive or shared object.
>
> This means that instead of the linker, you have to invoke something
> like:
>
>   libtool --mode=link foo.o bar.o baz.la ...

It may be as easy as the following, plus start referring to libraries
like ../libbacktrace/libbacktrace.la in CLIBS, plus the bits in
configure.ac to search and SUBST LIBTOOL, etc.

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index c528ee5aa80..d554e62b698 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -146,7 +146,7 @@ MAKEHTMLFLAGS =
 # 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) --tag=CC --mode=link $(CXX) $(CXX_DIALECT)
 
 # Where is our "include" directory?  Typically $(srcdir)/../include.
 # This is essentially the header file directory for the library

> I have to confess I have never autotoolized a project that does not use
> Automake.  Automake takes care of generating the right rules for linking
> when you specify a .la library in either LIBADD or LDADD.
>
> From the libtool manual:
>
>   If you're not using Automake, and you don't know how to incorporate
>   libtool into your package you need to do one of the following:
>
>   1. Download the latest Automake distribution from your nearest GNU
>      mirror, install it, and start using it.
>
>   2. Learn how to write 'Makefile' rules by hand.  They're sometimes
>      complex, but if you're clever enough to write rules for compiling
>      your old libraries, then you should be able to figure out new rules
>      for libtool libraries (hint: examine the 'Makefile.in' in the
>      'tests/demo' subdirectory of the libtool distribution... note
>      especially that it was automatically generated from the
>      'Makefile.am' by Automake).

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

[Simon Marchi]

> It may be as easy as the following, plus start referring to libraries
> like ../libbacktrace/libbacktrace.la in CLIBS, plus the bits in
> configure.ac to search and SUBST LIBTOOL, etc.
> 
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index c528ee5aa80..d554e62b698 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -146,7 +146,7 @@ MAKEHTMLFLAGS =
>  # 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) --tag=CC --mode=link $(CXX) $(CXX_DIALECT)

I'd be fine with that, especially if it simplifies other spots so we
don't have to do different things based on enable_static.

Simon

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

[Mike Frysinger]

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

On 04 Nov 2022 12:04, Simon Marchi via Gdb-patches wrote:
> > It may be as easy as the following, plus start referring to libraries
> > like ../libbacktrace/libbacktrace.la in CLIBS, plus the bits in
> > configure.ac to search and SUBST LIBTOOL, etc.
> > 
> > diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> > index c528ee5aa80..d554e62b698 100644
> > --- a/gdb/Makefile.in
> > +++ b/gdb/Makefile.in
> > @@ -146,7 +146,7 @@ MAKEHTMLFLAGS =
> >  # 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) --tag=CC --mode=link $(CXX) $(CXX_DIALECT)
> 
> I'd be fine with that, especially if it simplifies other spots so we
> don't have to do different things based on enable_static.

looks like someone posted a patch back in June in
https://sourceware.org/PR29372
-mike

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