Message ID | 20170902180801.27348-1-jrtc27@jrtc27.com |
---|---|
State | New |
Headers | show |
Series | Fix TLS relocations against local symbols on powerpc32, sparc32 and sparc64 | expand |
On Sat, Sep 02, 2017 at 07:08:01PM +0100, James Clarke wrote: > Normally, TLS relocations against local symbols are optimised by the linker to > be absolute. However, gold does not do this, and so it is possible to end up > with, for example, R_SPARC_TLS_DTPMOD64 referring to a local symbol. That is actually correct behaviour. The linker can't resolve DTPMOD relocs in a shared library at link time, since it doesn't know the tls module id that will be assigned at run time. So a dynamic DTPMOD reloc must be emitted against a local symbol or SHN_UNDEF (symbol index zero). There are other cases too where TLS relocs must be dynamic in a shared library. For example, TPREL relocs must be dynamic because the linker doesn't know where the TLS segment of the library will be laid out relative to the thread pointer. I guess what you're really saying is that gold doesn't make use of the fact that glibc ld.so handles symbol index zero in these cases, but instead emits a local dynamic symbol to use by the dynamic reloc. > Since > sym_map is left as null in elf_machine_rela for the special local symbol case, > the relocation handling thinks it has nothing to do, and so the module gets > left as 0. Yes, thanks for noticing! The only time we should have sym_map NULL is for an undefined weak symbol. The patch looks good to me.
Ping? On 3 Sep 2017, at 04:47, Alan Modra <amodra@gmail.com> wrote: > On Sat, Sep 02, 2017 at 07:08:01PM +0100, James Clarke wrote: >> Normally, TLS relocations against local symbols are optimised by the linker to >> be absolute. However, gold does not do this, and so it is possible to end up >> with, for example, R_SPARC_TLS_DTPMOD64 referring to a local symbol. > > That is actually correct behaviour. The linker can't resolve DTPMOD > relocs in a shared library at link time, since it doesn't know the > tls module id that will be assigned at run time. So a dynamic DTPMOD > reloc must be emitted against a local symbol or SHN_UNDEF (symbol > index zero). There are other cases too where TLS relocs must be > dynamic in a shared library. For example, TPREL relocs must be > dynamic because the linker doesn't know where the TLS segment of the > library will be laid out relative to the thread pointer. > > I guess what you're really saying is that gold doesn't make use of the > fact that glibc ld.so handles symbol index zero in these cases, but > instead emits a local dynamic symbol to use by the dynamic reloc. Yes, perhaps I should have been clearer in my original message. I never meant to imply that gold was emitting incorrect relocations (for once it wasn't its fault!). >> Since >> sym_map is left as null in elf_machine_rela for the special local symbol case, >> the relocation handling thinks it has nothing to do, and so the module gets >> left as 0. > > Yes, thanks for noticing! The only time we should have sym_map NULL > is for an undefined weak symbol. The patch looks good to me. Regards, James
James Clarke <jrtc27@jrtc27.com> writes: > Ping? > > On 3 Sep 2017, at 04:47, Alan Modra <amodra@gmail.com> wrote: >> >> Yes, thanks for noticing! The only time we should have sym_map NULL >> is for an undefined weak symbol. The patch looks good to me. As far as I understand, Alan already reviewed and approved this patch. However, it would be ideal if David Miller (SPARC maintainer, in Cc.) would review it too. I can push it after David's approval. Thanks!
On 18 Sep 2017, at 14:57, Tulio Magno Quites Machado Filho <tuliom@linux.vnet.ibm.com> wrote: > > James Clarke <jrtc27@jrtc27.com> writes: > >> Ping? >> >> On 3 Sep 2017, at 04:47, Alan Modra <amodra@gmail.com> wrote: >>> >>> Yes, thanks for noticing! The only time we should have sym_map NULL >>> is for an undefined weak symbol. The patch looks good to me. > > As far as I understand, Alan already reviewed and approved this patch. > > However, it would be ideal if David Miller (SPARC maintainer, in Cc.) would > review it too. > > I can push it after David's approval. David, ping? Thanks, James (https://sourceware.org/ml/libc-alpha/2017-09/msg00120.html for the actual patch in question to save you looking for it yourself.)
James Clarke <jrtc27@jrtc27.com> writes: > * sysdeps/powerpc/powerpc32/dl-machine.h (elf_machine_rela): > Assign sym_map to be map for local symbols, as TLS relocations > use sym_map to determine whether the symbol is defined and to > extract the TLS information. > * sysdeps/sparc/sparc32/dl-machine.h (elf_machine_rela): > Likewise. > * sysdeps/sparc/sparc64/dl-machine.h (elf_machine_rela): > Likewise. Pushed as 864458880721. Thanks!
diff --git a/sysdeps/powerpc/powerpc32/dl-machine.h b/sysdeps/powerpc/powerpc32/dl-machine.h index 1f8437ed9c..c19b3b7a16 100644 --- a/sysdeps/powerpc/powerpc32/dl-machine.h +++ b/sysdeps/powerpc/powerpc32/dl-machine.h @@ -310,7 +310,10 @@ elf_machine_rela (struct link_map *map, const Elf32_Rela *reloc, against local symbols. */ if (__builtin_expect (ELF32_ST_BIND (sym->st_info) == STB_LOCAL, 0) && sym->st_shndx != SHN_UNDEF) - value = map->l_addr; + { + sym_map = map; + value = map->l_addr; + } else { sym_map = RESOLVE_MAP (&sym, version, r_type); diff --git a/sysdeps/sparc/sparc32/dl-machine.h b/sysdeps/sparc/sparc32/dl-machine.h index 436e4e6cc3..debf67bd1b 100644 --- a/sysdeps/sparc/sparc32/dl-machine.h +++ b/sysdeps/sparc/sparc32/dl-machine.h @@ -376,6 +376,7 @@ elf_machine_rela (struct link_map *map, const Elf32_Rela *reloc, if (__builtin_expect (ELF32_ST_BIND (sym->st_info) == STB_LOCAL, 0) && sym->st_shndx != SHN_UNDEF) { + sym_map = map; value = map->l_addr; } else diff --git a/sysdeps/sparc/sparc64/dl-machine.h b/sysdeps/sparc/sparc64/dl-machine.h index c2871dca3a..e1ec7a532c 100644 --- a/sysdeps/sparc/sparc64/dl-machine.h +++ b/sysdeps/sparc/sparc64/dl-machine.h @@ -403,6 +403,7 @@ elf_machine_rela (struct link_map *map, const Elf64_Rela *reloc, if (__builtin_expect (ELF64_ST_BIND (sym->st_info) == STB_LOCAL, 0) && sym->st_shndx != SHN_UNDEF) { + sym_map = map; value = map->l_addr; } else