diff mbox

PATCH] PR target/65612: Multiversioning doesn't work with DSO nor PIE

Message ID 5550825F.1050300@arm.com
State New
Headers show

Commit Message

Szabolcs Nagy May 11, 2015, 10:20 a.m. UTC 
On 09/05/15 19:57, Szabolcs Nagy wrote:
> * H.J. Lu <hjl.tools@gmail.com> [2015-05-09 10:41:41 -0700]:
>> There are
>>
>>      4: 0000000000002b70   806 FUNC    GLOBAL DEFAULT   12
>> __cpu_indicator_init@GCC_4.8.0
>>     38: 00000000002153d0    16 OBJECT  GLOBAL DEFAULT   25 __cpu_model@GCC_4.8.0
>>
>> and
>>
>> 000000000215000  0000000400000001 R_X86_64_64
>> 0000000000002b70 __cpu_indicator_init@GCC_4.8.0 + 0
>> 0000000000215220  0000002600000006 R_X86_64_GLOB_DAT
>> 00000000002153d0 __cpu_model@GCC_4.8.0 + 0
>>
>> in libgcc_s.so.1.  Musl ld.so must be fixed to handle it.
>>

Rich is looking at how to do this non-intrusively, but
it seems non-trivial (some users of musl prefer not to
resolve such versioned symbols).

> 
> i think it might be enough to add __cpu_indicator_init_local
> as an alias to __cpu_indicator_init in libgcc.a and then use
> the *_local symbol from the ifunc resolver, that way no new
> dependency is added to libgcc_s.so handling.

i tried this approach and it seems to work: passes all
multiversioning tests on x86_64.

i think it's no worse than the symver approach.

is it ok to change the current fix to this?


libgcc/Changelog:

2015-05-11  Szabolcs Nagy  <szabolcs.nagy@arm.com>

	* config/i386/cpuinfo.c (__cpu_indicator_init_local): Add.
	(__cpu_indicator_init@GCC_4.8.0, __cpu_model@GCC_4.8.0): Remove.

	* config/i386/t-linux (HOST_LIBGCC2_CFLAGS): Remove -DUSE_ELF_SYMVER.

gcc/Changelog:

2015-05-11  Szabolcs Nagy  <szabolcs.nagy@arm.com>

	* config/i386/i386.c (ix86_expand_builtin): Make __builtin_cpu_init
	call __cpu_indicator_init_local instead of __cpu_indicator_init.

Comments

Jakub Jelinek May 11, 2015, 10:31 a.m. UTC | #1 
On Mon, May 11, 2015 at 11:20:15AM +0100, Szabolcs Nagy wrote:
> 
> 
> On 09/05/15 19:57, Szabolcs Nagy wrote:
> > * H.J. Lu <hjl.tools@gmail.com> [2015-05-09 10:41:41 -0700]:
> >> There are
> >>
> >>      4: 0000000000002b70   806 FUNC    GLOBAL DEFAULT   12
> >> __cpu_indicator_init@GCC_4.8.0
> >>     38: 00000000002153d0    16 OBJECT  GLOBAL DEFAULT   25 __cpu_model@GCC_4.8.0
> >>
> >> and
> >>
> >> 000000000215000  0000000400000001 R_X86_64_64
> >> 0000000000002b70 __cpu_indicator_init@GCC_4.8.0 + 0
> >> 0000000000215220  0000002600000006 R_X86_64_GLOB_DAT
> >> 00000000002153d0 __cpu_model@GCC_4.8.0 + 0
> >>
> >> in libgcc_s.so.1.  Musl ld.so must be fixed to handle it.
> >>
> 
> Rich is looking at how to do this non-intrusively, but
> it seems non-trivial (some users of musl prefer not to
> resolve such versioned symbols).
> 
> > 
> > i think it might be enough to add __cpu_indicator_init_local
> > as an alias to __cpu_indicator_init in libgcc.a and then use
> > the *_local symbol from the ifunc resolver, that way no new
> > dependency is added to libgcc_s.so handling.
> 
> i tried this approach and it seems to work: passes all
> multiversioning tests on x86_64.
> 
> i think it's no worse than the symver approach.
> 
> is it ok to change the current fix to this?

No.  Instead of piling hacks like this just fix it in musl.
libgcc certainly isn't the only library that uses @ symbol versions,
e.g. libstdc++ does as well, as well as many other shared libraries.

	Jakub
Szabolcs Nagy May 11, 2015, 12:39 p.m. UTC | #2 
On 11/05/15 11:31, Jakub Jelinek wrote:
> On Mon, May 11, 2015 at 11:20:15AM +0100, Szabolcs Nagy wrote:
>>> i think it might be enough to add __cpu_indicator_init_local
>>> as an alias to __cpu_indicator_init in libgcc.a and then use
>>> the *_local symbol from the ifunc resolver, that way no new
>>> dependency is added to libgcc_s.so handling.
>>
>> i tried this approach and it seems to work: passes all
>> multiversioning tests on x86_64.
>>
>> i think it's no worse than the symver approach.
>>
>> is it ok to change the current fix to this?
> 
> No.  Instead of piling hacks like this just fix it in musl.
> libgcc certainly isn't the only library that uses @ symbol versions,
> e.g. libstdc++ does as well, as well as many other shared libraries.
> 

can you explain how using a standard elf feature is a hack,
but the current symver asm directive is not?

fyi, musl loader loads libstdc++ just fine because it has no
relocations for symbols which only has sym@version definition
(libgcc_s.so.1 has because __cpu_indicator_init is a ctor).

musl may end up supporting @version but that's an independent
quest.

(the one big pile of hacks here is multiversioning: each function
with mv has its own ifunc resolver repeating the same logic, then
the >1.5K cpuinfo.o is static linked into every single dso that
uses mv and the separate __cpu_model structs all have to be
initialized.. including the unused one in libgcc_s.so.1 because
of the ctor.. adding more startup overhead.

i'd gladly propose a patch to remove this feature if getting rid
of piling hacks has priority.. the current design seems problematic
to me for other archs that may need to call libc functions to do
the dispatch anyway.)
Jakub Jelinek May 11, 2015, 1:05 p.m. UTC | #3 
On Mon, May 11, 2015 at 01:39:17PM +0100, Szabolcs Nagy wrote:
> fyi, musl loader loads libstdc++ just fine because it has no

But will it load any shared library that uses any of the 26 (if I count well
on x86_64) @ symbols from libstdc++.so.6?
readelf -Ws /lib64/libstdc++.so.6 | grep '@' | grep -v 'UND\|@@'
For libstdc++ that is primarily C++ apps and shared libraries
compiled/linked with GCC 4.0.0.

> musl may end up supporting @version but that's an independent
> quest.

It is not independent.  If musl claims to support symbol versioning, it
should support it properly, if not, then supposedly gcc configured for musl
can't be compatible with gcc configured for other linux C libraries, and
should force symbol versioning off.

	Jakub
Rich Felker May 11, 2015, 2:11 p.m. UTC | #4 
On Mon, May 11, 2015 at 12:31:51PM +0200, Jakub Jelinek wrote:
> On Mon, May 11, 2015 at 11:20:15AM +0100, Szabolcs Nagy wrote:
> > 
> > 
> > On 09/05/15 19:57, Szabolcs Nagy wrote:
> > > * H.J. Lu <hjl.tools@gmail.com> [2015-05-09 10:41:41 -0700]:
> > >> There are
> > >>
> > >>      4: 0000000000002b70   806 FUNC    GLOBAL DEFAULT   12
> > >> __cpu_indicator_init@GCC_4.8.0
> > >>     38: 00000000002153d0    16 OBJECT  GLOBAL DEFAULT   25 __cpu_model@GCC_4.8.0
> > >>
> > >> and
> > >>
> > >> 000000000215000  0000000400000001 R_X86_64_64
> > >> 0000000000002b70 __cpu_indicator_init@GCC_4.8.0 + 0
> > >> 0000000000215220  0000002600000006 R_X86_64_GLOB_DAT
> > >> 00000000002153d0 __cpu_model@GCC_4.8.0 + 0
> > >>
> > >> in libgcc_s.so.1.  Musl ld.so must be fixed to handle it.
> > >>
> > 
> > Rich is looking at how to do this non-intrusively, but
> > it seems non-trivial (some users of musl prefer not to
> > resolve such versioned symbols).
> > 
> > > 
> > > i think it might be enough to add __cpu_indicator_init_local
> > > as an alias to __cpu_indicator_init in libgcc.a and then use
> > > the *_local symbol from the ifunc resolver, that way no new
> > > dependency is added to libgcc_s.so handling.
> > 
> > i tried this approach and it seems to work: passes all
> > multiversioning tests on x86_64.
> > 
> > i think it's no worse than the symver approach.
> > 
> > is it ok to change the current fix to this?
> 
> No.  Instead of piling hacks like this just fix it in musl.

I wouldn't call it piling hacks; it's an improvement as far as I can
tell since it remove symbolic relocations and replaces them with
relative ones.

> libgcc certainly isn't the only library that uses @ symbol versions,
> e.g. libstdc++ does as well, as well as many other shared libraries.

We haven't encountered such issues there.

Rich
Szabolcs Nagy May 11, 2015, 3:30 p.m. UTC | #5 
On 11/05/15 14:05, Jakub Jelinek wrote:
> On Mon, May 11, 2015 at 01:39:17PM +0100, Szabolcs Nagy wrote:
>> fyi, musl loader loads libstdc++ just fine because it has no
> 
> But will it load any shared library that uses any of the 26 (if I count well
> on x86_64) @ symbols from libstdc++.so.6?

i looked, but all of those symbols have @@ variant too
so at least the libraries would load with musl.

>> musl may end up supporting @version but that's an independent
>> quest.
> 
> It is not independent.  If musl claims to support symbol versioning, it
> should support it properly, if not, then supposedly gcc configured for musl
> can't be compatible with gcc configured for other linux C libraries, and
> should force symbol versioning off.

ok, but the current solution does not make that easy:
configuring gcc with

  --disable-gnu-indirect-function --disable-symvers

has no effect on libgcc_s.so.1 on linux.

(i can try to create a patch that removes the new
-DUSE_ELF_SYMVER from the libgcc cflags for musl, but that
seems a worse solution than the weak alias one).

(note that previously a simple spec file was enough to use an
existing gcc on linux to build things against musl...
this didnt work for c++ code that used libstdc++ headers, but
now it also fails for any build using -lgcc_s).
diff mbox

Patch

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 7bd9ff3..7327cf3 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -38398,10 +38398,10 @@  ix86_expand_builtin (tree exp, rtx target, rtx subtarget,
     {
     case IX86_BUILTIN_CPU_INIT:
       {
-	/* Make it call __cpu_indicator_init in libgcc. */
+	/* Make it call __cpu_indicator_init_local in libgcc.a. */
 	tree call_expr, fndecl, type;
         type = build_function_type_list (integer_type_node, NULL_TREE); 
-	fndecl = build_fn_decl ("__cpu_indicator_init", type);
+	fndecl = build_fn_decl ("__cpu_indicator_init_local", type);
 	call_expr = build_call_expr (fndecl, 0); 
 	return expand_expr (call_expr, target, mode, EXPAND_NORMAL);
       }
diff --git a/libgcc/config/i386/cpuinfo.c b/libgcc/config/i386/cpuinfo.c
index f6f91dd..86b6d21 100644
--- a/libgcc/config/i386/cpuinfo.c
+++ b/libgcc/config/i386/cpuinfo.c
@@ -425,7 +425,7 @@  __cpu_indicator_init (void)
   return 0;
 }
 
-#if defined SHARED && defined USE_ELF_SYMVER
-__asm__ (".symver __cpu_indicator_init, __cpu_indicator_init@GCC_4.8.0");
-__asm__ (".symver __cpu_model, __cpu_model@GCC_4.8.0");
+#ifndef SHARED
+int __cpu_indicator_init_local (void)
+  __attribute__ ((weak, alias ("__cpu_indicator_init")));
 #endif
diff --git a/libgcc/config/i386/t-linux b/libgcc/config/i386/t-linux
index 11bb46e..4f47f7b 100644
--- a/libgcc/config/i386/t-linux
+++ b/libgcc/config/i386/t-linux
@@ -3,4 +3,4 @@ 
 # t-slibgcc-elf-ver and t-linux
 SHLIB_MAPFILES = libgcc-std.ver $(srcdir)/config/i386/libgcc-glibc.ver
 
-HOST_LIBGCC2_CFLAGS += -mlong-double-80 -DUSE_ELF_SYMVER
+HOST_LIBGCC2_CFLAGS += -mlong-double-80