[PATCH] Move DONT_USE_BOOTSTRAP_MAP defininition to dl-machine.h

[PATCH] Move DONT_USE_BOOTSTRAP_MAP defininition to dl-machine.h

[Jakub Jelinek]

Hi!

__attribute__((visibility("hidden"))) does not guarantee no .got
references on all arches (well, only on the few where there are suitable
relocations etc.).
Current glibc e.g. segfaults on Alpha, certainly will not work on SPARC
until GOTOFF like relocs are added, etc.
IMHO it is best to move the definition to sysdep header as this
is something that can be known on a per-arch basis.

2002-08-23  Jakub Jelinek  <jakub@redhat.com>

	* elf/rtld.c (DONT_USE_BOOTSTRAP_MAP): Don't define here.
	Rename to RTLD_DONT_USE_BOOTSTRAP_MAP.
	* sysdeps/i386/dl-machine.h (RTLD_DONT_USE_BOOTSTRAP_MAP): Define.

--- libc/elf/rtld.c.jj	Thu Aug 22 06:21:50 2002
+++ libc/elf/rtld.c	Thu Aug 22 18:47:34 2002
@@ -125,15 +125,14 @@ TLS_INIT_HELPER
 
 /* Before ld.so is relocated we must not access variables which need
    relocations.  This means variables which are exported.  Variables
-   declared as static are fine.  If we can mark a variable hidden this
-   is fine, too.  The latter is impotant here.  We can avoid setting
-   up a temporary link map for ld.so if we can mark _rtld_global as
-   hidden.  */
-#ifdef HAVE_HIDDEN
-# define DONT_USE_BOOTSTRAP_MAP	1
-#endif
+   declared as static are fine.  If we can mark a variable hidden and
+   the accesses won't go through .got, it is fine, too.
+   The latter is impotant here.  We can avoid setting up a temporary
+   link map for ld.so if we can mark _rtld_global as hidden.
+   Define RTLD_DONT_USE_BOOTSTRAP_MAP in dl-machine.h if this is the
+   case.  */
 
-#ifdef DONT_USE_BOOTSTRAP_MAP
+#ifdef RTLD_DONT_USE_BOOTSTRAP_MAP
 static ElfW(Addr) _dl_start_final (void *arg);
 #else
 static ElfW(Addr) _dl_start_final (void *arg,
@@ -156,12 +155,12 @@ RTLD_START
 #endif
 
 /* This is the second half of _dl_start (below).  It can be inlined safely
-   under DONT_USE_BOOTSTRAP_MAP, where it is careful not to make any GOT
-   references.  When the tools don't permit us to avoid using a GOT entry
-   for _dl_rtld_global (no attribute_hidden support), we must make sure
-   this function is not inlined (see below).  */
+   under RTLD_DONT_USE_BOOTSTRAP_MAP, where it is careful not to make any
+   GOT references.  When the tools don't permit us to avoid using a GOT
+   entry for _dl_rtld_global (no attribute_hidden support), we must make
+   sure this function is not inlined (see below).  */
 
-#ifdef DONT_USE_BOOTSTRAP_MAP
+#ifdef RTLD_DONT_USE_BOOTSTRAP_MAP
 static inline ElfW(Addr) __attribute__ ((always_inline))
 _dl_start_final (void *arg)
 #else
@@ -184,7 +183,7 @@ _dl_start_final (void *arg, struct link_
     }
 
   /* Transfer data about ourselves to the permanent link_map structure.  */
-#ifndef DONT_USE_BOOTSTRAP_MAP
+#ifndef RTLD_DONT_USE_BOOTSTRAP_MAP
   GL(dl_rtld_map).l_addr = bootstrap_map_p->l_addr;
   GL(dl_rtld_map).l_ld = bootstrap_map_p->l_ld;
   memcpy (GL(dl_rtld_map).l_info, bootstrap_map_p->l_info,
@@ -196,7 +195,7 @@ _dl_start_final (void *arg, struct link_
   GL(dl_rtld_map).l_map_start = (ElfW(Addr)) _begin;
   GL(dl_rtld_map).l_map_end = (ElfW(Addr)) _end;
   /* Copy the TLS related data if necessary.  */
-#if USE_TLS && !defined DONT_USE_BOOTSTRAP_MAP
+#if USE_TLS && !defined RTLD_DONT_USE_BOOTSTRAP_MAP
 # ifdef HAVE___THREAD
   assert (bootstrap_map_p->l_tls_modid != 0);
 # else
@@ -247,7 +246,7 @@ _dl_start_final (void *arg, struct link_
 static ElfW(Addr) __attribute_used__ internal_function
 _dl_start (void *arg)
 {
-#ifdef DONT_USE_BOOTSTRAP_MAP
+#ifdef RTLD_DONT_USE_BOOTSTRAP_MAP
 # define bootstrap_map GL(dl_rtld_map)
 #else
   struct link_map bootstrap_map;
@@ -279,7 +278,7 @@ _dl_start (void *arg)
      know it is available.  We do not have to clear the memory if we
      do not have to use the temporary bootstrap_map.  Global variables
      are initialized to zero by default.  */
-#ifndef DONT_USE_BOOTSTRAP_MAP
+#ifndef RTLD_DONT_USE_BOOTSTRAP_MAP
 # ifdef HAVE_BUILTIN_MEMSET
   __builtin_memset (bootstrap_map.l_info, '\0', sizeof (bootstrap_map.l_info));
 # else
@@ -298,7 +297,7 @@ _dl_start (void *arg)
   elf_get_dynamic_info (&bootstrap_map);
 
 #if USE_TLS
-# if !defined HAVE___THREAD && !defined DONT_USE_BOOTSTRAP_MAP
+# if !defined HAVE___THREAD && !defined RTLD_DONT_USE_BOOTSTRAP_MAP
   /* Signal that we have not found TLS data so far.  */
   bootstrap_map.l_tls_modid = 0;
 # endif
@@ -426,7 +425,7 @@ _dl_start (void *arg)
      function, that way the compiler cannot put accesses to the GOT
      before ELF_DYNAMIC_RELOCATE.  */
   {
-#ifdef DONT_USE_BOOTSTRAP_MAP
+#ifdef RTLD_DONT_USE_BOOTSTRAP_MAP
     ElfW(Addr) entry = _dl_start_final (arg);
 #else
     ElfW(Addr) entry = _dl_start_final (arg, &bootstrap_map);
--- libc/sysdeps/i386/dl-machine.h.jj	Fri Aug  9 04:02:54 2002
+++ libc/sysdeps/i386/dl-machine.h	Thu Aug 22 18:48:06 2002
@@ -196,6 +196,16 @@ _dl_runtime_profile:\n\
    where the dynamic linker should not map anything.  */
 #define ELF_MACHINE_USER_ADDRESS_MASK	0xf8000000UL
 
+/* Before ld.so is relocated we must not access variables which need
+   relocations.  This means variables which are exported.  Variables
+   declared as static are fine.  If we can mark a variable hidden this
+   is fine, too.  The latter is impotant here.  We can avoid setting
+   up a temporary link map for ld.so if we can mark _rtld_global as
+   hidden.  */
+#ifdef HAVE_VISIBILITY_ATTRIBUTE
+# define RTLD_DONT_USE_BOOTSTRAP_MAP	1
+#endif
+
 /* Initial entry point code for the dynamic linker.
    The C function `_dl_start' is the real entry point;
    its return value is the user program's entry point.  */

	Jakub

Re: [PATCH] Move DONT_USE_BOOTSTRAP_MAP defininition to dl-machine.h

[Roland McGrath]

I just experimented with making a static variable in rtld.c and redefining
GL using that locally, with strong_alias to define _rtld_global.  It seems
to work.  Can platforms like Alpha and SPARC avoid the relocs for local
symbols?

Re: [PATCH] Move DONT_USE_BOOTSTRAP_MAP defininition to dl-machine.h

[Jakub Jelinek]

On Thu, Aug 22, 2002 at 08:07:26PM -0400, Roland McGrath wrote:
> I just experimented with making a static variable in rtld.c and redefining
> GL using that locally, with strong_alias to define _rtld_global.  It seems
> to work.  Can platforms like Alpha and SPARC avoid the relocs for local
> symbols?

SPARC, unless GOTOFF like relocs are added cannot, it still has to use a
R_SPARC_RELATIVE reloc and thus we must not reference it
before ld.so relocation. For Alpha, I'll look when I wake up.

Note that i386-linux glibc build segfaults in ld.so ATM too (while
i686-linux works), both using gcc 3.2. In that case it seems like
%ebx was not set up as early as needed before _dl_start first references it.

	Jakub

Re: [PATCH] Move DONT_USE_BOOTSTRAP_MAP defininition to dl-machine.h

[Jakub Jelinek]

On Thu, Aug 22, 2002 at 08:07:26PM -0400, Roland McGrath wrote:
> I just experimented with making a static variable in rtld.c and redefining
> GL using that locally, with strong_alias to define _rtld_global.  It seems
> to work.  Can platforms like Alpha and SPARC avoid the relocs for local
> symbols?

Ok, Alpha can do it, just tested:
static int i;
int j __attribute__((visibility("hidden")));
int k[100] __attribute__((visibility("hidden"), section(".sdata")));
int foo (void)
{
  return i;
}
int bar (void)
{
  return j;
}
int baz (void)
{
  return k[0];
}

on CVS head, unfortunately it doesn't do this in visibility
backport for 3.2 (yet, I'll have a look at this next weak).
On SPARC, it is not possible until binutils/gcc/glibc are
changed for the proposed new relocs.
IA-64 is ok with both 3.2 backport and 3.3 mainline.

So it IMHO looks like we need configury test for this (which will be
unfortunately arch dependend, and have a default for not tested architecture
(use bootstrap map)).

	Jakub

Re: [PATCH] Move DONT_USE_BOOTSTRAP_MAP defininition to dl-machine.h

[Roland McGrath]

> Note that i386-linux glibc build segfaults in ld.so ATM too (while
> i686-linux works), both using gcc 3.2. In that case it seems like
> %ebx was not set up as early as needed before _dl_start first references it.

I cannot reproduce this with my gcc (3.2-0.2 maybe?) using -O1 -march=i386
but did with -O3 -march=i386.  I think it is just happenstance that this
did not bite before (if it did not) and is not related to the
DONT_USE_BOOTSTRAP_MAP changes per se.  I think it bites i386 because
HP_TIMING_AVAIL is not defined and so the `start_time' reference in the
beginning of _dl_start never happens.  Without that reference, the compiler
doesn't think it needs to have set up the GOT register by the time the
elf_machine_load_address code presumes it's there.  I added a hack to the
asm in elf_machine_load_address to fool the compiler into thinking there is
a global variable reference, and that fixed it so it doesn't crash any more.

However, I am concerned that the presumptions about %ebx in dl-machine.h
(elf_machine_load_address and elf_machine_dynamic) may bite us again at
some point.  If _dl_start makes no calls through the PLT, the compiler no
longer necessarily uses %ebx for the GOT register.  If we added more
attribute_hidden or hidden_proto decls for _dl_* as we probably should,
there would in fact be no PLT calls.  

I have been experimenting a little just now and I think we can arrange for
the compiler to emit this code for us and avoid the register assumption.
For elf_machine_dynamic I think a plain reference to _DYNAMIC should be
fine, right?  _DYNAMIC@GOT just produces 0.  For elf_machine_load_address,
I have gotten the compiler to reproduce the same pair of instructions
(actually, when scheduling for i686 it decides that three are better than
two) by using a pair of decls:

extern char foobar __attribute__ ((visibility ("hidden")));
extern char foobar_x asm ("foobar");

int foo (void) { return &foobar - &foobar_x; }

Re: [PATCH] Move DONT_USE_BOOTSTRAP_MAP defininition to dl-machine.h

[Roland McGrath]

I'm now trying these definitions and they seem to be fine.  This requires a
compiler with working visibility support to make sure you get a GOTOFF
reloc.  But given that, it is about as kosher as this code could get,
giving the compiler a free hand to choose registers.  GCC 3.2 under -O3
produces very tight code for this, using a single load from the GOT for
both calls, and scheduling other insns inside the sequence to it's little
optimizing heart's content as it could not with the asm.

    /* Return the link-time address of _DYNAMIC.  We can access the GOT, which
       has not been relocated yet.  Since _DYNAMIC is not an exported symbol,
       a GOT reference will produce an R_386_RELATIVE reloc with an addend
       (initial value) of the link-time address we are looking for.  */
    static inline Elf32_Addr __attribute__ ((unused))
    elf_machine_dynamic (void)
    {
      return (Elf32_Addr) &_DYNAMIC;
    }


    /* Return the run-time load address of the shared object.  */
    static inline Elf32_Addr __attribute__ ((unused))
    elf_machine_load_address (void)
    {
      /* Compute the difference between the runtime address of a symbol as seen
	 by a GOTOFF reference, and the link-time address found in the
	 unrelocated GOT entry for an unexported symbol.  Any unexported symbol
	 would do, but using _DYNAMIC reuses the same GOT entry produced by
	 `elf_machine_dynamic', and in fact the loaded value can be CSE'd.  */
      extern Elf32_Dyn bygotoff[] asm ("_DYNAMIC") attribute_hidden;
      return (Elf32_Addr) &bygotoff - (Elf32_Addr) &_DYNAMIC;
    }

It seems safe to leave the old code as it was for compilers without working
visibility support.  Such compilers won't ever know they can PLT-free calls
and so they will always use %ebx (with my recent fix).  For compilers with
visibility support, it could become unsafe to do anything but this new code
below if we were to the more visibility decls we should add to allow the
freeest possible optimization (i.e. not necessarily using %ebx for the GOT).

The optimal thing would be to get rid of the normal (local) GOT entry this
creates for _DYNAMIC (corresponding to the one for _dl_start in the old
code) and have some way to refer to the special unrelocated 0'th GOT entry
that is already there for it.  Wait!  I got it!

   static inline Elf32_Addr __attribute__ ((unused))
   elf_machine_dynamic (void)
   {
     extern Elf32_Addr _GLOBAL_OFFSET_TABLE_[] attribute_hidden;
     return _GLOBAL_OFFSET_TABLE_[0];
   }

   static inline Elf32_Addr __attribute__ ((unused))
   elf_machine_load_address (void)
   {
     extern Elf32_Dyn bygotoff[] asm ("_DYNAMIC") attribute_hidden;
     return (Elf32_Addr) &bygotoff - elf_machine_dynamic ();
   }

That seems to work, and the useless R_386_RELATIVE reloc is gone.

Re: [PATCH] Move DONT_USE_BOOTSTRAP_MAP defininition to dl-machine.h

[Andreas Jaeger]

Jakub Jelinek <jakub@redhat.com> writes:

> On Thu, Aug 22, 2002 at 08:07:26PM -0400, Roland McGrath wrote:
>> I just experimented with making a static variable in rtld.c and redefining
>> GL using that locally, with strong_alias to define _rtld_global.  It seems
>> to work.  Can platforms like Alpha and SPARC avoid the relocs for local
>> symbols?
>
> Ok, Alpha can do it, just tested:
> static int i;
> int j __attribute__((visibility("hidden")));
> int k[100] __attribute__((visibility("hidden"), section(".sdata")));
> int foo (void)
> {
>   return i;
> }
> int bar (void)
> {
>   return j;
> }
> int baz (void)
> {
>   return k[0];
> }
>
> on CVS head, unfortunately it doesn't do this in visibility
> backport for 3.2 (yet, I'll have a look at this next weak).
> On SPARC, it is not possible until binutils/gcc/glibc are
> changed for the proposed new relocs.
> IA-64 is ok with both 3.2 backport and 3.3 mainline.
>
> So it IMHO looks like we need configury test for this (which will be
> unfortunately arch dependend, and have a default for not tested architecture
> (use bootstrap map)).

What should I test on x86-64?

Btw. elf/ld.so crashes also on x86-64 with current CVS.  Is this the
same problem?

Andreas
-- 
 Andreas Jaeger
  SuSE Labs aj@suse.de
   private aj@arthur.inka.de
    http://www.suse.de/~aj

Re: [PATCH] Move DONT_USE_BOOTSTRAP_MAP defininition to dl-machine.h

[Roland McGrath]

> What should I test on x86-64?
> 
> Btw. elf/ld.so crashes also on x86-64 with current CVS.  Is this the
> same problem?

I don't have an x86-64 to test, so I can only guess.  It looks like
GOTPCREL is the equivalent of GOTOFF for x86-64.  You should test that an
access to an attribute_hidden variable uses foo@GOTPCREL(%rip) instead of 
using a GOT entry.

Re: [PATCH] Move DONT_USE_BOOTSTRAP_MAP defininition to dl-machine.h

[Ulrich Drepper]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jakub Jelinek wrote:

> __attribute__((visibility("hidden"))) does not guarantee no .got
> references on all arches (well, only on the few where there are suitable
> relocations etc.).

I've checked in a patch for this.  Machine-dependent configure files 
have to define PI_STATIC_AND_HIDDEN (PI = position-independent).  If 
this symbol is defined and HAVE_HIDDEN is defined and 
HAVE_VISIBILITY_ATTRIBUTE is defined we use the new and improved way of 
handling startup in ld.so.

For x86 we unconditionally define PI_STATIC_AND_HIDDEN (note, that any 
user of the macro has to make sure that the symbol visibility is 
suppoerted as well).  For other architectures defining this symbol might 
require running tests.

- -- 
- ---------------.                          ,-.   1325 Chesapeake Terrace
Ulrich Drepper  \    ,-------------------'   \  Sunnyvale, CA 94089 USA
Red Hat          `--' drepper at redhat.com   `------------------------
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQE9aJ892ijCOnn/RHQRAl70AKCuiAPU01MF9pdSyy0TPD7yG1BpygCgwDxI
qB5X/o8IlbkmWUwDqmucx50=
=ESKk
-----END PGP SIGNATURE-----