PR22978, TLS local-dynamic incorrectly linked on hppa-linux

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

PR22978, TLS local-dynamic incorrectly linked on hppa-linux

Alan Modra-3
We were emitting dynamic relocs on the second word of a TLS GD GOT
entry pair (the dtprel offset), without the addend necessary when no
symbol is present on the dynamic reloc.  Unfortunately the simple
solution of providing the proper addend doesn't work due to an hppa
glibc ld.so bug that ignores such addends.  So instead optimize the
relocs.  The dtprel offset is known at link time for locally defined
symbols (the only case where we'll end up with no symbol on a dynamic
reloc) so we can omit the dynamic reloc in that case.

Furthermore, we can omit a dynamic reloc on the first word of a TLS GD
GOT entry pair (the module id) if the symbol is local and we are
producing an executable.  Similarly, a tprel reloc on a TLS IE GOT
entry is not needed for local symbols in an executable.  So the
condition for TLS GOT relocs can become bfd_link_dll(info) rather than
bfd_link_pic(info) as needed for normal GOT relocs.

This all presumes hppa ld.so doesn't need to differentiate TLS GD GOT
pairs from TLS LD GOT pairs, which is currently true.

        PR 22978
        * elf32-hppa.c (got_relocs_needed): Add extra param to special
        case both dtprel and tprel relocs.
        (allocate_dynrelocs): Adjust conditions for got relocs.
        (elf32_hppa_relocate_section): Likewise for local sym got relocs.
        Emit dynamic relocs on TLS GOT entries for shared libraries,
        not when pic.  Omit dynamic reloc on dtprel entry when local,
        and on tprel entry when local and executable.

diff --git a/bfd/elf32-hppa.c b/bfd/elf32-hppa.c
index 44fb753..722452b 100644
--- a/bfd/elf32-hppa.c
+++ b/bfd/elf32-hppa.c
@@ -1894,18 +1894,19 @@ got_entries_needed (int tls_type)
 }
 
 /* Calculate size of relocs needed for symbol given its TLS_TYPE and
-   NEEDed GOT entries.  KNOWN says a TPREL offset can be calculated
-   at link time.  */
+   NEEDed GOT entries.  TPREL_KNOWN says a TPREL offset can be
+   calculated at link time.  DTPREL_KNOWN says the same for a DTPREL
+   offset.  */
 
 static inline unsigned int
-got_relocs_needed (int tls_type, unsigned int need, bfd_boolean known)
+got_relocs_needed (int tls_type, unsigned int need,
+   bfd_boolean dtprel_known, bfd_boolean tprel_known)
 {
   /* All the entries we allocated need relocs.
-     Except IE in executable with a local symbol.  We could also omit
-     the DTPOFF reloc on the second word of a GD entry under the same
-     condition as that for IE, but ld.so might want to differentiate
-     LD and GD entries at some stage.  */
-  if ((tls_type & GOT_TLS_IE) != 0 && known)
+     Except for GD and IE with local symbols.  */
+  if ((tls_type & GOT_TLS_GD) != 0 && dtprel_known)
+    need -= GOT_ENTRY_SIZE;
+  if ((tls_type & GOT_TLS_IE) != 0 && tprel_known)
     need -= GOT_ENTRY_SIZE;
   return need * sizeof (Elf32_External_Rela) / GOT_ENTRY_SIZE;
 }
@@ -1959,15 +1960,16 @@ allocate_dynrelocs (struct elf_link_hash_entry *eh, void *inf)
       need = got_entries_needed (hh->tls_type);
       sec->size += need;
       if (htab->etab.dynamic_sections_created
-  && (bfd_link_pic (info)
+  && (bfd_link_dll (info)
+      || (bfd_link_pic (info) && (hh->tls_type & GOT_NORMAL) != 0)
       || (eh->dynindx != -1
   && !SYMBOL_REFERENCES_LOCAL (info, eh)))
   && !UNDEFWEAK_NO_DYNAMIC_RELOC (info, eh))
  {
-  bfd_boolean tprel_known = (bfd_link_executable (info)
-     && SYMBOL_REFERENCES_LOCAL (info, eh));
+  bfd_boolean local = SYMBOL_REFERENCES_LOCAL (info, eh);
   htab->etab.srelgot->size
-    += got_relocs_needed (hh->tls_type, need, tprel_known);
+    += got_relocs_needed (hh->tls_type, need, local,
+  local && bfd_link_executable (info));
  }
     }
   else
@@ -2192,12 +2194,12 @@ elf32_hppa_size_dynamic_sections (bfd *output_bfd ATTRIBUTE_UNUSED,
       *local_got = sec->size;
       need = got_entries_needed (*local_tls_type);
       sec->size += need;
-      if (bfd_link_pic (info))
- {
-  bfd_boolean tprel_known = bfd_link_executable (info);
-  htab->etab.srelgot->size
-    += got_relocs_needed (*local_tls_type, need, tprel_known);
- }
+      if (bfd_link_dll (info)
+  || (bfd_link_pic (info)
+      && (*local_tls_type & GOT_NORMAL) != 0))
+ htab->etab.srelgot->size
+  += got_relocs_needed (*local_tls_type, need, TRUE,
+ bfd_link_executable (info));
     }
   else
     *local_got = (bfd_vma) -1;
@@ -4041,7 +4043,7 @@ elf32_hppa_relocate_section (bfd *output_bfd,
    GD GOT are necessary, we emit the GD first.  */
 
  if (indx != 0
-    || (bfd_link_pic (info)
+    || (bfd_link_dll (info)
  && (hh == NULL
     || !UNDEFWEAK_NO_DYNAMIC_RELOC (info, &hh->eh))))
   {
@@ -4065,6 +4067,20 @@ elf32_hppa_relocate_section (bfd *output_bfd,
  bfd_elf32_swap_reloca_out (output_bfd, &outrel, loc);
  htab->etab.srelgot->reloc_count++;
  loc += sizeof (Elf32_External_Rela);
+ bfd_put_32 (output_bfd, 0,
+    htab->etab.sgot->contents + cur_off);
+      }
+    else
+      /* If we are not emitting relocations for a
+ general dynamic reference, then we must be in a
+ static link or an executable link with the
+ symbol binding locally.  Mark it as belonging
+ to module 1, the executable.  */
+      bfd_put_32 (output_bfd, 1,
+  htab->etab.sgot->contents + cur_off);
+
+    if (indx != 0)
+      {
  outrel.r_info
   = ELF32_R_INFO (indx, R_PARISC_TLS_DTPOFF32);
  outrel.r_offset += 4;
@@ -4072,22 +4088,11 @@ elf32_hppa_relocate_section (bfd *output_bfd,
  htab->etab.srelgot->reloc_count++;
  loc += sizeof (Elf32_External_Rela);
  bfd_put_32 (output_bfd, 0,
-    htab->etab.sgot->contents + cur_off);
- bfd_put_32 (output_bfd, 0,
     htab->etab.sgot->contents + cur_off + 4);
       }
     else
-      {
- /* If we are not emitting relocations for a
-   general dynamic reference, then we must be in a
-   static link or an executable link with the
-   symbol binding locally.  Mark it as belonging
-   to module 1, the executable.  */
- bfd_put_32 (output_bfd, 1,
-    htab->etab.sgot->contents + cur_off);
- bfd_put_32 (output_bfd, relocation - dtpoff_base (info),
-    htab->etab.sgot->contents + cur_off + 4);
-      }
+      bfd_put_32 (output_bfd, relocation - dtpoff_base (info),
+  htab->etab.sgot->contents + cur_off + 4);
     cur_off += 8;
   }
 

--
Alan Modra
Australia Development Lab, IBM
Reply | Threaded
Open this post in threaded view
|

Re: PR22978, TLS local-dynamic incorrectly linked on hppa-linux

Carlos O'Donell-6
On 04/20/2018 08:49 AM, Alan Modra wrote:

> We were emitting dynamic relocs on the second word of a TLS GD GOT
> entry pair (the dtprel offset), without the addend necessary when no
> symbol is present on the dynamic reloc.  Unfortunately the simple
> solution of providing the proper addend doesn't work due to an hppa
> glibc ld.so bug that ignores such addends.  So instead optimize the
> relocs.  The dtprel offset is known at link time for locally defined
> symbols (the only case where we'll end up with no symbol on a dynamic
> reloc) so we can omit the dynamic reloc in that case.
>
> Furthermore, we can omit a dynamic reloc on the first word of a TLS GD
> GOT entry pair (the module id) if the symbol is local and we are
> producing an executable.  Similarly, a tprel reloc on a TLS IE GOT
> entry is not needed for local symbols in an executable.  So the
> condition for TLS GOT relocs can become bfd_link_dll(info) rather than
> bfd_link_pic(info) as needed for normal GOT relocs.
>
> This all presumes hppa ld.so doesn't need to differentiate TLS GD GOT
> pairs from TLS LD GOT pairs, which is currently true.
>
> PR 22978
> * elf32-hppa.c (got_relocs_needed): Add extra param to special
> case both dtprel and tprel relocs.
> (allocate_dynrelocs): Adjust conditions for got relocs.
> (elf32_hppa_relocate_section): Likewise for local sym got relocs.
> Emit dynamic relocs on TLS GOT entries for shared libraries,
> not when pic.  Omit dynamic reloc on dtprel entry when local,
> and on tprel entry when local and executable.

This looks reasonable, but if we could fix the ld.so bug that would also
be good!

How did you test this fix? Sometimes changes like this have surprises,
and I would test it but my box is currently inaccessible and needs some
cleanup.

I've included Dave on the TO, to see if he's interested in testing this
a little more widely.

--
Cheers,
Carlos.
Reply | Threaded
Open this post in threaded view
|

Re: PR22978, TLS local-dynamic incorrectly linked on hppa-linux

John David Anglin
On 2018-04-20 11:54 AM, Carlos O'Donell wrote:
> I've included Dave on the TO, to see if he's interested in testing this
> a little more widely.
I'll test in my next gcc build.

Dave

--
John David Anglin  [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: PR22978, TLS local-dynamic incorrectly linked on hppa-linux

Jeff Law
On 04/20/2018 10:02 AM, John David Anglin wrote:
> On 2018-04-20 11:54 AM, Carlos O'Donell wrote:
>> I've included Dave on the TO, to see if he's interested in testing this
>> a little more widely.
> I'll test in my next gcc build.
My tester will pick it up automatically over the weekend.

jeff
Reply | Threaded
Open this post in threaded view
|

Re: PR22978, TLS local-dynamic incorrectly linked on hppa-linux

Alan Modra-3
In reply to this post by Carlos O'Donell-6
On Fri, Apr 20, 2018 at 10:54:57AM -0500, Carlos O'Donell wrote:

> On 04/20/2018 08:49 AM, Alan Modra wrote:
> > We were emitting dynamic relocs on the second word of a TLS GD GOT
> > entry pair (the dtprel offset), without the addend necessary when no
> > symbol is present on the dynamic reloc.  Unfortunately the simple
> > solution of providing the proper addend doesn't work due to an hppa
> > glibc ld.so bug that ignores such addends.  So instead optimize the
> > relocs.  The dtprel offset is known at link time for locally defined
> > symbols (the only case where we'll end up with no symbol on a dynamic
> > reloc) so we can omit the dynamic reloc in that case.
> >
> > Furthermore, we can omit a dynamic reloc on the first word of a TLS GD
> > GOT entry pair (the module id) if the symbol is local and we are
> > producing an executable.  Similarly, a tprel reloc on a TLS IE GOT
> > entry is not needed for local symbols in an executable.  So the
> > condition for TLS GOT relocs can become bfd_link_dll(info) rather than
> > bfd_link_pic(info) as needed for normal GOT relocs.
> >
> > This all presumes hppa ld.so doesn't need to differentiate TLS GD GOT
> > pairs from TLS LD GOT pairs, which is currently true.
> >
> > PR 22978
> > * elf32-hppa.c (got_relocs_needed): Add extra param to special
> > case both dtprel and tprel relocs.
> > (allocate_dynrelocs): Adjust conditions for got relocs.
> > (elf32_hppa_relocate_section): Likewise for local sym got relocs.
> > Emit dynamic relocs on TLS GOT entries for shared libraries,
> > not when pic.  Omit dynamic reloc on dtprel entry when local,
> > and on tprel entry when local and executable.
>
> This looks reasonable, but if we could fix the ld.so bug that would also
> be good!
>
> How did you test this fix? Sometimes changes like this have surprises,
> and I would test it but my box is currently inaccessible and needs some
> cleanup.

I tested it along with the trivial glibc ld.so fix, on
phantom.parisc-linux.org with a binutils and glibc build plus
testsuite run.  There were some difficulties in testing, the machine
crashing a couple of times, and I forgot to increase the glibc
timeout..

Worse, on investigating a little more this morning, I think my newly
built and installed ld wasn't even used for the glibc build and
testsuite run.  "make check" doesn't always do what you think it
should be doing.  :-(

It seems that the system collect2 doesn't honour PATH.  For example:

amodra@phantom:~/src/glibc/elf$ which ld
/home/amodra/gnu/bin/ld
amodra@phantom:~/src/glibc/elf$ which hppa-linux-gnu-ld    
/home/amodra/gnu/bin/hppa-linux-gnu-ld
amodra@phantom:~/src/glibc/elf$ hppa-linux-gnu-gcc -nostdlib -nostartfiles -o /home/amodra/build/glibc32/elf/tst-tls2    -Wl,-z,combreloc -Wl,-z,relro -Wl,--hash-style=both /home/amodra/build/glibc32/csu/crt1.o /home/amodra/build/glibc32/csu/crti.o `hppa-linux-gnu-gcc  --print-file-name=crtbegin.o` /home/amodra/build/glibc32/elf/tst-tls2.o /home/amodra/build/glibc32/support/libsupport_nonshared.a  -Wl,-dynamic-linker=/home/amodra/gnu/hppa-linux-gnu/lib/ld.so.1 -Wl,-rpath-link=/home/amodra/build/glibc32:/home/amodra/build/glibc32/math:/home/amodra/build/glibc32/elf:/home/amodra/build/glibc32/dlfcn:/home/amodra/build/glibc32/nss:/home/amodra/build/glibc32/nis:/home/amodra/build/glibc32/rt:/home/amodra/build/glibc32/resolv:/home/amodra/build/glibc32/crypt:/home/amodra/build/glibc32/mathvec:/home/amodra/build/glibc32/support:/home/amodra/build/glibc32/nptl /home/amodra/build/glibc32/libc.so.6 /home/amodra/build/glibc32/libc_nonshared.a -Wl,--as-needed /home/amodra/build/glibc32/elf/ld.so -Wl,--no-as-needed -lgcc -Wl,--as-needed -lgcc_s  -Wl,--no-as-needed `hppa-linux-gnu-gcc  --print-file-name=crtend.o` /home/amodra/build/glibc32/csu/crtn.o -v -Wl,-v
Using built-in specs.
COLLECT_GCC=hppa-linux-gnu-gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/hppa-linux-gnu/7/lto-wrapper
Target: hppa-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Debian 7.3.0-16' --with-bugurl=file:///usr/share/doc/gcc-7/README.Bugs --enable-languages=c,ada,c++,d,fortran,objc,obj-c++ --prefix=/usr --with-gcc-major-version-only --with-as=/usr/bin/hppa-linux-gnu-as --with-ld=/usr/bin/hppa-linux-gnu-ld --program-suffix=-7 --program-prefix=hppa-linux-gnu- --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new --enable-gnu-unique-object --disable-libssp --disable-libitm --disable-libsanitizer --disable-libquadmath --disable-libquadmath-support --enable-plugin --with-system-zlib --enable-objc-gc=auto --enable-multiarch --disable-werror --disable-libstdcxx-pch --enable-checking=release --build=hppa-linux-gnu --host=hppa-linux-gnu --target=hppa-linux-gnu
Thread model: posix
gcc version 7.3.0 (Debian 7.3.0-16)
COMPILER_PATH=/usr/lib/gcc/hppa-linux-gnu/7/:/usr/lib/gcc/hppa-linux-gnu/7/:/usr/lib/gcc/hppa-linux-gnu/:/usr/lib/gcc/hppa-linux-gnu/7/:/usr/lib/gcc/hppa-linux-gnu/
LIBRARY_PATH=/usr/lib/gcc/hppa-linux-gnu/7/:/usr/lib/gcc/hppa-linux-gnu/7/../../../hppa-linux-gnu/:/usr/lib/gcc/hppa-linux-gnu/7/../../../:/lib/hppa-linux-gnu/:/lib/:/usr/lib/hppa-linux-gnu/:/usr/lib/
COLLECT_GCC_OPTIONS='-nostdlib' '-nostartfiles' '-o' '/home/amodra/build/glibc32/elf/tst-tls2' '-v'
 /usr/lib/gcc/hppa-linux-gnu/7/collect2 -plugin /usr/lib/gcc/hppa-linux-gnu/7/liblto_plugin.so -plugin-opt=/usr/lib/gcc/hppa-linux-gnu/7/lto-wrapper -plugin-opt=-fresolution=/tmp/cct34pSM.res --sysroot=/ --build-id --eh-frame-hdr -dynamic-linker /lib/ld.so.1 -o /home/amodra/build/glibc32/elf/tst-tls2 -L/usr/lib/gcc/hppa-linux-gnu/7 -L/usr/lib/gcc/hppa-linux-gnu/7/../../../hppa-linux-gnu -L/usr/lib/gcc/hppa-linux-gnu/7/../../.. -L/lib/hppa-linux-gnu -L/usr/lib/hppa-linux-gnu -z combreloc -z relro --hash-style=both /home/amodra/build/glibc32/csu/crt1.o /home/amodra/build/glibc32/csu/crti.o /usr/lib/gcc/hppa-linux-gnu/7/crtbegin.o /home/amodra/build/glibc32/elf/tst-tls2.o /home/amodra/build/glibc32/support/libsupport_nonshared.a -dynamic-linker=/home/amodra/gnu/hppa-linux-gnu/lib/ld.so.1 -rpath-link=/home/amodra/build/glibc32:/home/amodra/build/glibc32/math:/home/amodra/build/glibc32/elf:/home/amodra/build/glibc32/dlfcn:/home/amodra/build/glibc32/nss:/home/amodra/build/glibc32/nis:/home/amodra/build/glibc32/rt:/home/amodra/build/glibc32/resolv:/home/amodra/build/glibc32/crypt:/home/amodra/build/glibc32/mathvec:/home/amodra/build/glibc32/support:/home/amodra/build/glibc32/nptl /home/amodra/build/glibc32/libc.so.6 /home/amodra/build/glibc32/libc_nonshared.a --as-needed /home/amodra/build/glibc32/elf/ld.so --no-as-needed -lgcc --as-needed -lgcc_s --no-as-needed /usr/lib/gcc/hppa-linux-gnu/7/crtend.o /home/amodra/build/glibc32/csu/crtn.o -v
collect2 version 7.3.0
/usr/bin/hppa-linux-gnu-ld -plugin /usr/lib/gcc/hppa-linux-gnu/7/liblto_plugin.so -plugin-opt=/usr/lib/gcc/hppa-linux-gnu/7/lto-wrapper -plugin-opt=-fresolution=/tmp/cct34pSM.res --sysroot=/ --build-id --eh-frame-hdr -dynamic-linker /lib/ld.so.1 -o /home/amodra/build/glibc32/elf/tst-tls2 -L/usr/lib/gcc/hppa-linux-gnu/7 -L/usr/lib/gcc/hppa-linux-gnu/7/../../../hppa-linux-gnu -L/usr/lib/gcc/hppa-linux-gnu/7/../../.. -L/lib/hppa-linux-gnu -L/usr/lib/hppa-linux-gnu -z combreloc -z relro --hash-style=both /home/amodra/build/glibc32/csu/crt1.o /home/amodra/build/glibc32/csu/crti.o /usr/lib/gcc/hppa-linux-gnu/7/crtbegin.o /home/amodra/build/glibc32/elf/tst-tls2.o /home/amodra/build/glibc32/support/libsupport_nonshared.a -dynamic-linker=/home/amodra/gnu/hppa-linux-gnu/lib/ld.so.1 -rpath-link=/home/amodra/build/glibc32:/home/amodra/build/glibc32/math:/home/amodra/build/glibc32/elf:/home/amodra/build/glibc32/dlfcn:/home/amodra/build/glibc32/nss:/home/amodra/build/glibc32/nis:/home/amodra/build/glibc32/rt:/home/amodra/build/glibc32/resolv:/home/amodra/build/glibc32/crypt:/home/amodra/build/glibc32/mathvec:/home/amodra/build/glibc32/support:/home/amodra/build/glibc32/nptl /home/amodra/build/glibc32/libc.so.6 /home/amodra/build/glibc32/libc_nonshared.a --as-needed /home/amodra/build/glibc32/elf/ld.so --no-as-needed -lgcc --as-needed -lgcc_s --no-as-needed /usr/lib/gcc/hppa-linux-gnu/7/crtend.o /home/amodra/build/glibc32/csu/crtn.o -v
GNU ld (GNU Binutils for Debian) 2.30
COLLECT_GCC_OPTIONS='-nostdlib' '-nostartfiles' '-o' '/home/amodra/build/glibc32/elf/tst-tls2' '-v'

Oh yeah, configured with --with-ld=/usr/bin/hppa-linux-gnu-ld
Blah, so I'll need to build my own gcc to do proper testing on that
box.

> I've included Dave on the TO, to see if he's interested in testing this
> a little more widely.
>
> --
> Cheers,
> Carlos.

--
Alan Modra
Australia Development Lab, IBM