diff mbox

[lto-plugin,build] Don't link libiberty.a into liblto-plugin.a

Message ID ydd1v5kci2a.fsf@manam.CeBiTec.Uni-Bielefeld.DE
State New
Headers show

Commit Message

Rainer Orth Dec. 14, 2010, 6:42 p.m. UTC
When trying a mainline bootstrap on Solaris 11/x86 with a debug version
of the Sun binutils, I ran into an assertion failure building the static
liblto-plugin.a:

$ ar cr .libs/liblto_plugin.a ../libiberty/pic/libiberty.a  lto-plugin.o
Assertion failed: !is_elf || (pad(lseek(fd, 0, SEEK_CUR), PADSZ) == 0), file ../common/file.c, line 726, function write_member_header
Abort

While the assertion failure won't show in a production version and the
condition has since been fixed to avoid it even in a debug version, it
seems clear that embedding an archive inside another archive doesn't
make any sense.

The following patch fixed the issue for me by avoiding to pass .a files
directly to libtool, but instead using the appropriate combination of -L
and -l.  It allowed my bootstraps to finish.

Going forward, this current mixture of libtool and non-libtool libraries
seems like a total mess to me.  Instead, it seems better to change
libiberty to use libtool itself.  This would certainly simplify the
Makefile.in (even if not going to automake at the same time), but this
is clearly not stage3 material and would need careful coordination with
other projects in src.

Any thoughts on the patch below and a possible libiberty conversion?

Thanks.

	Rainer


2010-12-11  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>

	* Makefile.am (liblto_plugin_la_DEPENDENCIES): Define.
	(liblto_plugin_la_LIBADD): Use -L../libiberty/pic -liberty.
	(liblto_plugin_la_LDFLAGS): Use -L../libiberty -liberty.
	* Makefile.in: Regenerate.

Comments

Cary Coutant Dec. 14, 2010, 9:37 p.m. UTC | #1
> 2010-12-11  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>
>
>        * Makefile.am (liblto_plugin_la_DEPENDENCIES): Define.
>        (liblto_plugin_la_LIBADD): Use -L../libiberty/pic -liberty.
>        (liblto_plugin_la_LDFLAGS): Use -L../libiberty -liberty.
>        * Makefile.in: Regenerate.

This is OK for lto-plugin. Thanks!

-cary
DJ Delorie Dec. 14, 2010, 9:44 p.m. UTC | #2
> > 2010-12-11  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>
> >
> >        * Makefile.am (liblto_plugin_la_DEPENDENCIES): Define.
> >        (liblto_plugin_la_LIBADD): Use -L../libiberty/pic -liberty.
> >        (liblto_plugin_la_LDFLAGS): Use -L../libiberty -liberty.
> >        * Makefile.in: Regenerate.
> 
> This is OK for lto-plugin. Thanks!

Note that not all platforms on which LTO can be built, will build a
PIC libiberty.
Cary Coutant Dec. 14, 2010, 9:55 p.m. UTC | #3
>> > 2010-12-11  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>
>> >
>> >        * Makefile.am (liblto_plugin_la_DEPENDENCIES): Define.
>> >        (liblto_plugin_la_LIBADD): Use -L../libiberty/pic -liberty.
>> >        (liblto_plugin_la_LDFLAGS): Use -L../libiberty -liberty.
>> >        * Makefile.in: Regenerate.
>>
>> This is OK for lto-plugin. Thanks!
>
> Note that not all platforms on which LTO can be built, will build a
> PIC libiberty.

If there is no PIC libiberty, Rainer's patch doesn't change anything,
as far as I can see. Am I misreading it? Or are you suggesting that
should be fixed as well?

-cary
DJ Delorie Dec. 14, 2010, 10:01 p.m. UTC | #4
Just want to make sure that possibility was considered, that's all.
Rainer Orth Dec. 15, 2010, 10:31 a.m. UTC | #5
DJ Delorie <dj@redhat.com> writes:

> Just want to make sure that possibility was considered, that's all.

As Cary said, no change here by my patch.  What do you think about
moving libiberty to libtool (and maybe automake) in the 4.7 timeframe?
That should avoid all of this mess.

	Rainer
Rainer Orth Dec. 15, 2010, 4:54 p.m. UTC | #6
Hi Dave,

>   Bad news I'm afraid.  It broke the plugin on Windows: it doesn't build a DLL
> any more, because ...
[...]
>   Do you actually have any use for the static version of the plugin?  If not,
> and you were just trying to avoid an assertion that won't happen any more, it
> would be helpful if you could revert your patch until we've had time to figure
> out a fuller fix.

I can do that, but I still can't see any use for a static plugin
anywhere (not knowing Windows at all, fortunately :-)

>   Ralf, does this mean we need to make a libiberty convenience library somehow?

That has been my suggestion all along, though nobody commented on that
so far.  Manually hacking around what libtool is designed to do doesn't
seem like the right way to handle this.

	Rainer
Dave Korn Dec. 15, 2010, 5:04 p.m. UTC | #7
On 14/12/2010 18:42, Rainer Orth wrote:

  Hi Rainer,

> When trying a mainline bootstrap on Solaris 11/x86 with a debug version
> of the Sun binutils, I ran into an assertion failure building the static
> liblto-plugin.a:
> 
> $ ar cr .libs/liblto_plugin.a ../libiberty/pic/libiberty.a  lto-plugin.o
> Assertion failed: !is_elf || (pad(lseek(fd, 0, SEEK_CUR), PADSZ) == 0), file ../common/file.c, line 726, function write_member_header
> Abort
> 
> While the assertion failure won't show in a production version and the
> condition has since been fixed to avoid it even in a debug version, it
> seems clear that embedding an archive inside another archive doesn't
> make any sense.
> 
> The following patch fixed the issue for me by avoiding to pass .a files
> directly to libtool, but instead using the appropriate combination of -L
> and -l.  It allowed my bootstraps to finish.

  Bad news I'm afraid.  It broke the plugin on Windows: it doesn't build a DLL
any more, because ...

> *** Warning: linker path does not have real file for library -liberty.
> *** I have the capability to make that library automatically link in when
> *** you link to this library.  But I can only do this if you have a
> *** shared version of the library, which you do not appear to have
> *** because I did check the linker path looking for a file starting
> *** with libiberty and none of the candidates passed a file format test
> *** using a file magic. Last file checked: /usr/lib/libiberty.a
> *** The inter-library dependencies that have been dropped here will be
> *** automatically added whenever a program is linked with this library
> *** or is declared to -dlopen it.
> 
> *** Since this library must not contain undefined symbols,
> *** because either the platform does not support them or
> *** it was explicitly requested with -no-undefined,
> *** libtool will only create a static version of it.

  Do you actually have any use for the static version of the plugin?  If not,
and you were just trying to avoid an assertion that won't happen any more, it
would be helpful if you could revert your patch until we've had time to figure
out a fuller fix.

  Ralf, does this mean we need to make a libiberty convenience library somehow?

    cheers,
      DaveK
Rainer Orth Dec. 15, 2010, 5:06 p.m. UTC | #8
Hi Dave,

>> I can do that, but I still can't see any use for a static plugin
>> anywhere (not knowing Windows at all, fortunately :-)
>
>   It's not a windows thing, it's just that libtool always builds both a static
> and a shared library.  There is in fact a potential use for it; libtool

I know, but you can disable that at configure time.  I don't know how to
make that the default for lto-plugin, but suppose it is doable.

> supports pseudo-dlopening on systems that don't actually have shared libraries
> by statically linking that archive into the program that wants to use it -
> i.e., ld in this case.  But I don't know any systems that support LTO but not
> shared libs.

Indeed, that would be very strange.

>> That has been my suggestion all along, though nobody commented on that
>> so far.  Manually hacking around what libtool is designed to do doesn't
>> seem like the right way to handle this.
>
>   Well, what I don't know is whether we have to fully libtoolize libiberty, or
> whether perhaps we can just get lto-plugin to build and link in a libtoolized
> convenience library, using the existing libiberty .a or .o files.

We'd have to build a convenience library anyway, just like libffi and
boehm-gc are used by libgcj.  I haven't yet looked into how to do this,
though.

	Rainer
Dave Korn Dec. 15, 2010, 5:26 p.m. UTC | #9
On 15/12/2010 16:54, Rainer Orth wrote:
> Hi Dave,
> 
>>   Bad news I'm afraid.  It broke the plugin on Windows: it doesn't build a DLL
>> any more, because ...
> [...]
>>   Do you actually have any use for the static version of the plugin?  If not,
>> and you were just trying to avoid an assertion that won't happen any more, it
>> would be helpful if you could revert your patch until we've had time to figure
>> out a fuller fix.
> 
> I can do that, but I still can't see any use for a static plugin
> anywhere (not knowing Windows at all, fortunately :-)

  It's not a windows thing, it's just that libtool always builds both a static
and a shared library.  There is in fact a potential use for it; libtool
supports pseudo-dlopening on systems that don't actually have shared libraries
by statically linking that archive into the program that wants to use it -
i.e., ld in this case.  But I don't know any systems that support LTO but not
shared libs.

>>   Ralf, does this mean we need to make a libiberty convenience library somehow?
> 
> That has been my suggestion all along, though nobody commented on that
> so far.  Manually hacking around what libtool is designed to do doesn't
> seem like the right way to handle this.

  Well, what I don't know is whether we have to fully libtoolize libiberty, or
whether perhaps we can just get lto-plugin to build and link in a libtoolized
convenience library, using the existing libiberty .a or .o files.

    cheers,
      DaveK
Rainer Orth Dec. 15, 2010, 5:44 p.m. UTC | #10
Hi Dave,

>   AC_DISABLE_STATIC, or pass "disable-static" as an argument to LT_INIT,
> according to the manual.  I'll try the attached (on top of reverting your
> patch); perhaps you'd like to try with your debug-build assembler and check

no need: I've done the reversion myself.

> that it also avoids the assert for you.

The assert was over-eager and has been fixed already.  My primary
motivation right now was to avoid creating a `recursive' archive that's
unusable/useless everywhere AFAICT.

	Rainer
Dave Korn Dec. 15, 2010, 5:58 p.m. UTC | #11
On 15/12/2010 17:06, Rainer Orth wrote:
> Hi Dave,
> 
>>> I can do that, but I still can't see any use for a static plugin
>>> anywhere (not knowing Windows at all, fortunately :-)
>>   It's not a windows thing, it's just that libtool always builds both a static
>> and a shared library.  There is in fact a potential use for it; libtool
> 
> I know, but you can disable that at configure time.  I don't know how to
> make that the default for lto-plugin, but suppose it is doable.

  AC_DISABLE_STATIC, or pass "disable-static" as an argument to LT_INIT,
according to the manual.  I'll try the attached (on top of reverting your
patch); perhaps you'd like to try with your debug-build assembler and check
that it also avoids the assert for you.

lto-plugin/ChangeLog:

	* configure.ac (AM_PROG_LIBTOOL): Replace deprecated name ...
	(LT_INIT): ... with new version, and disable building static library.


    cheers,
      DaveK
Ralf Wildenhues Dec. 15, 2010, 6:46 p.m. UTC | #12
* Rainer Orth wrote on Wed, Dec 15, 2010 at 06:06:28PM CET:
> >> I can do that, but I still can't see any use for a static plugin
> >> anywhere (not knowing Windows at all, fortunately :-)
> >
> >   It's not a windows thing, it's just that libtool always builds both a static
> > and a shared library.

You can influence that at configure time (AC_DISABLE_{STATIC,SHARED} or
LT_INIT options disable-{static,shared}), as well as in the Makefile
({AM,libfoo_la}_LDFLAGS += -static or
{AM,libfoo_la}_LIBTOOLFLAGS = --tag=disable-{static,shared}).

What you can't do, is build both a convenience library and a
to-be-installed library at the same time.

> >  There is in fact a potential use for it; libtool
> 
> I know, but you can disable that at configure time.  I don't know how to
> make that the default for lto-plugin, but suppose it is doable.
> 
> > supports pseudo-dlopening on systems that don't actually have shared libraries
> > by statically linking that archive into the program that wants to use it -
> > i.e., ld in this case.  But I don't know any systems that support LTO but not
> > shared libs.
> 
> Indeed, that would be very strange.

Right.  This part is not important for GCC.  Well, one could have
emulated plugins that way on static-only systems using libltdl, but
at least Joseph was arguing against using libltdl.  (I'm not trying
to dispute that one way or the other, now.)

> >> That has been my suggestion all along, though nobody commented on that
> >> so far.

I'm not sure whether it is possible to exactly map the intended
semantics on libtool usage.  Linking other in-tree libraries against a
non-convenience libiberty would trigger relinks at installation time of
several in-tree libraries and programs on several systems, which is
frowned upon in GCC and src projects (see also
http://gcc.gnu.org/PR46607).

Building both a convenience and a non-convenience libiberty is possible
(see e.g., zlib/Makefile.am).

But anyway such a move requires quite some testing effort on several
systems, so I really wouldn't like to see it done at this stage.

> >>  Manually hacking around what libtool is designed to do doesn't
> >> seem like the right way to handle this.
> >
> >   Well, what I don't know is whether we have to fully libtoolize libiberty, or
> > whether perhaps we can just get lto-plugin to build and link in a libtoolized
> > convenience library, using the existing libiberty .a or .o files.

It is certainly possible to just write a fake .la file for a convenience
libiberty/pic/libiberty.a.  You wouldn't even need libtool for that.
I'm not sure whether it will fix all your issues though.  The idea with
convenience libraries is that they are static, linked wholly into real
libraries, and normally into programs, and never installed.  This
doesn't quite match the intended semantics.

> We'd have to build a convenience library anyway, just like libffi and
> boehm-gc are used by libgcj.  I haven't yet looked into how to do this,
> though.

See above.

For fixing the original issue reported by Rainer in
http://gcc.gnu.org/ml/gcc-patches/2010-12/msg01116.html
using --tag=disable-static as above ought to be the easiest workaround
(untested) until libtool is fixed to not add a .a file to a .a file.

Cheers, and thanks,
Ralf
Joseph Myers Dec. 15, 2010, 6:54 p.m. UTC | #13
On Wed, 15 Dec 2010, Ralf Wildenhues wrote:

> It is certainly possible to just write a fake .la file for a convenience
> libiberty/pic/libiberty.a.  You wouldn't even need libtool for that.
> I'm not sure whether it will fix all your issues though.  The idea with
> convenience libraries is that they are static, linked wholly into real
> libraries, and normally into programs, and never installed.  This
> doesn't quite match the intended semantics.

libiberty shouldn't be installed by default.  (It is, but as far as I'm 
concerned that's a bug.  A configure option or makefile target to install 
it would be fine.)  Nor should it be built for the target (again, I 
consider it a bug that it is so built; no target code I know of in the GCC 
or src trees uses the target libiberty, and it includes 
GPL-with-no-exception code that is not generally appropriate for target 
libraries in GCC).  What's the mismatch?

(When linked into the LTO plugin, libiberty symbols, and any other plugin 
symbols that aren't actually part of the public interface to linker 
plugins, should be hidden rather than exported - that is, it should just 
export the "onload" symbol.  But this is just a matter of cleanliness and 
I don't know how hard it is to implement.)
Dave Korn Dec. 15, 2010, 7:49 p.m. UTC | #14
On 15/12/2010 18:46, Ralf Wildenhues wrote:

> You can influence that at configure time (AC_DISABLE_{STATIC,SHARED} or
> LT_INIT options disable-{static,shared}), as well as in the Makefile
> ({AM,libfoo_la}_LDFLAGS += -static or
> {AM,libfoo_la}_LIBTOOLFLAGS = --tag=disable-{static,shared}).

> For fixing the original issue reported by Rainer in
> http://gcc.gnu.org/ml/gcc-patches/2010-12/msg01116.html
> using --tag=disable-static as above ought to be the easiest workaround
> (untested) until libtool is fixed to not add a .a file to a .a file.

  Yeah, I prefer the tag too, because the AC_DISABLE_xxx and LT_INIT(xxx)
solutions get overridden if someone explicitly configures with --enable-static
at the top-level.  Testing this instead of the previous one:

lto-plugin/ChangeLog:

	* Makefile.am (AM_LIBTOOLFLAGS): Define.
	* Makefile.in: Regenerate.

    cheers,
      DaveK
Cary Coutant Dec. 15, 2010, 7:49 p.m. UTC | #15
>  Bad news I'm afraid.  It broke the plugin on Windows: it doesn't build a DLL
> any more, because ...
>
>> *** Warning: linker path does not have real file for library -liberty.
>> *** I have the capability to make that library automatically link in when
>> *** you link to this library.  But I can only do this if you have a
>> *** shared version of the library, which you do not appear to have
>> *** because I did check the linker path looking for a file starting
>> *** with libiberty and none of the candidates passed a file format test
>> *** using a file magic. Last file checked: /usr/lib/libiberty.a
>> *** The inter-library dependencies that have been dropped here will be
>> *** automatically added whenever a program is linked with this library
>> *** or is declared to -dlopen it.
>>
>> *** Since this library must not contain undefined symbols,
>> *** because either the platform does not support them or
>> *** it was explicitly requested with -no-undefined,
>> *** libtool will only create a static version of it.

I'm trying to understand the mechanics of how changing
"../libiberty/pic/libiberty.a" to "-L../libiberty/pic -liberty"
resulted in this error.

-cary
Dave Korn Dec. 15, 2010, 8:51 p.m. UTC | #16
On 15/12/2010 19:49, Cary Coutant wrote:
>>  Bad news I'm afraid.  It broke the plugin on Windows: it doesn't build a DLL
>> any more, because ...
>>
>>> *** Warning: linker path does not have real file for library -liberty.
>>> *** I have the capability to make that library automatically link in when
>>> *** you link to this library.  But I can only do this if you have a
>>> *** shared version of the library, which you do not appear to have
>>> *** because I did check the linker path looking for a file starting
>>> *** with libiberty and none of the candidates passed a file format test
>>> *** using a file magic. Last file checked: /usr/lib/libiberty.a
>>> *** The inter-library dependencies that have been dropped here will be
>>> *** automatically added whenever a program is linked with this library
>>> *** or is declared to -dlopen it.
>>>
>>> *** Since this library must not contain undefined symbols,
>>> *** because either the platform does not support them or
>>> *** it was explicitly requested with -no-undefined,
>>> *** libtool will only create a static version of it.
> 
> I'm trying to understand the mechanics of how changing
> "../libiberty/pic/libiberty.a" to "-L../libiberty/pic -liberty"
> resulted in this error.

  It's the loss of the "-Wc," that made the difference here.  That causes
libtool to just pass the option through, without doing any of the kind of
processing that it does on standard -l options.

  The reason libtool doesn't want to link static lib archives into a shared
lib it's building is because without a libtool control script, it has no idea
what interlibrary dependencies this may pull in.  On linux systems where we
have a /pic/libiberty.a, libtool gives this warning:

> *** Warning: Linking the shared library liblto_plugin.la against the
> *** static library ../libiberty/pic/libiberty.a is not portable!

... but it goes ahead and does it, because the ELF format doesn't have a
problem with potentially leaving undefined references in the final shared
library.  On a platform like Windows, where you can't have undefined
references in a DLL, libtool just refuses.  As it happens, we know that there
won't be any missing library dependencies in this case, because libiberty is
basically stand-alone and needs pretty much nothing except -lc.  So we smuggle
the archive past libtool by prefixing it with a -Wc, option.

    cheers,
      DaveK
Ralf Wildenhues Dec. 19, 2010, 11:44 a.m. UTC | #17
* Dave Korn wrote on Sun, Dec 19, 2010 at 12:49:59PM CET:
> On 15/12/2010 19:49, Dave Korn wrote:
> > lto-plugin/ChangeLog:
> > 
> > 	* Makefile.am (AM_LIBTOOLFLAGS): Define.
> > 	* Makefile.in: Regenerate.
> 
> 
>   Passed a bootstrap, install and test cycle with the desired results: no
> static library built.  OK?

OK.

Thanks,
Ralf
Dave Korn Dec. 19, 2010, 11:49 a.m. UTC | #18
On 15/12/2010 19:49, Dave Korn wrote:
> On 15/12/2010 18:46, Ralf Wildenhues wrote:
> 
>> You can influence that at configure time (AC_DISABLE_{STATIC,SHARED} or
>> LT_INIT options disable-{static,shared}), as well as in the Makefile
>> ({AM,libfoo_la}_LDFLAGS += -static or
>> {AM,libfoo_la}_LIBTOOLFLAGS = --tag=disable-{static,shared}).
> 
>> For fixing the original issue reported by Rainer in
>> http://gcc.gnu.org/ml/gcc-patches/2010-12/msg01116.html
>> using --tag=disable-static as above ought to be the easiest workaround
>> (untested) until libtool is fixed to not add a .a file to a .a file.
> 
>   Yeah, I prefer the tag too, because the AC_DISABLE_xxx and LT_INIT(xxx)
> solutions get overridden if someone explicitly configures with --enable-static
> at the top-level.  Testing this instead of the previous one:
> 
> lto-plugin/ChangeLog:
> 
> 	* Makefile.am (AM_LIBTOOLFLAGS): Define.
> 	* Makefile.in: Regenerate.


  Passed a bootstrap, install and test cycle with the desired results: no
static library built.  OK?

    cheers,
      DaveK
Dave Korn Dec. 20, 2010, 5:29 p.m. UTC | #19
On 19/12/2010 11:44, Ralf Wildenhues wrote:
> * Dave Korn wrote on Sun, Dec 19, 2010 at 12:49:59PM CET:
>> On 15/12/2010 19:49, Dave Korn wrote:
>>> lto-plugin/ChangeLog:
>>>
>>> 	* Makefile.am (AM_LIBTOOLFLAGS): Define.
>>> 	* Makefile.in: Regenerate.
>>
>>   Passed a bootstrap, install and test cycle with the desired results: no
>> static library built.  OK?
> 
> OK.

  Committed revision 168087, with the following slightly-expanded changelog:

2010-12-20  Dave Korn  <...

	* Makefile.am (AM_LIBTOOLFLAGS): Define, adding disable-static tag.
	* Makefile.in: Regenerate.


    cheers,
      DaveK
diff mbox

Patch

diff -r f6168cd5e1fe lto-plugin/Makefile.am
--- a/lto-plugin/Makefile.am	Fri Dec 10 19:55:31 2010 +0100
+++ b/lto-plugin/Makefile.am	Sat Dec 11 01:31:27 2010 +0100
@@ -13,11 +13,13 @@ 
 libexecsub_LTLIBRARIES = liblto_plugin.la
 
 liblto_plugin_la_SOURCES = lto-plugin.c
+liblto_plugin_la_DEPENDENCIES = \
+	$(if $(wildcard ../libiberty/pic/libiberty.a),../libiberty/pic/libiberty.a,)
 liblto_plugin_la_LIBADD = \
-	$(if $(wildcard ../libiberty/pic/libiberty.a),../libiberty/pic/libiberty.a,)
+	$(if $(wildcard ../libiberty/pic/libiberty.a),-L../libiberty/pic -liberty,)
 # Note that we intentionally override the bindir supplied by ACX_LT_HOST_FLAGS
 liblto_plugin_la_LDFLAGS = $(lt_host_flags) -bindir $(libexecsubdir) \
-	$(if $(wildcard ../libiberty/pic/libiberty.a),,-Wc,../libiberty/libiberty.a)
+	$(if $(wildcard ../libiberty/pic/libiberty.a),,-L../libiberty -liberty)
 
 all: copy_lto_plugin