| Message ID | DCB1C42372B1674B8F912A294CCB775A71684631@BADAG02.ba.imgtec.org |
|---|---|
| State | New |
| Headers | show |
On Fri, 31 Jul 2015, Faraz Shahbazker wrote: > Hi, > > This is a follow up on IFUNC support for MIPS. Previous patch discussion on this list was: > https://sourceware.org/ml/libc-ports/2013-08/msg00007.html To what extent does the previous description apply to this patch? In particular, as regards the ABIs and other features supported. How has this patch been tested? > elf/ > * elf.h glibc does not have per-subdirectory ChangeLog files (except for libidn and localedata). So ChangeLog entries should contain the full paths to the files relative to the toplevel glibc source directory, elf/elf.h and sysdeps/mips/*.h. I'd expect an addition to sysdeps/unix/sysv/linux/mips/libc-abis for binary compatibility checks using EI_ABIVERSION to work properly (along with binutils setting EI_ABIVERSION to 4 when IFUNCs are used for MIPS).
Thanks for the patch. Faraz Shahbazker <Faraz.Shahbazker@imgtec.com> writes: > @@ -200,10 +201,13 @@ do { \ > if (__builtin_expect (map->l_addr == 0, 1)) \ > break; \ > \ > - /* got[0] is reserved. got[1] is also reserved for the dynamic object \ > - generated by gnu ld. Skip these reserved entries from \ > - relocation. */ \ > - i = (got[1] & ELF_MIPS_GNU_GOT1_MASK)? 2 : 1; \ > + if (__glibc_unlikely (map->l_info[DT_MIPS (GENERAL_GOTNO)] != NULL)) \ > + i = map->l_info[DT_MIPS (GENERAL_GOTNO)]->d_un.d_val; \ > + else \ > + /* got[0] is reserved. got[1] is also reserved for the dynamic \ > + object generated by gnu ld. Skip these reserved entries from \ > + relocation. */ \ > + i = (got[1] & ELF_MIPS_GNU_GOT1_MASK)? 2 : 1; \ > n = map->l_info[DT_MIPS (LOCAL_GOTNO)]->d_un.d_val; \ I'm not sure we should consider the new tag to be unlikely. If the intention is to use ifuncs in libc then it would eventually become pretty common. > + /* Symbol pre-empted, use value from GOT. > + 2nd part of condition is redundant, explicit for clarity. */ > + if (__glibc_unlikely (rmap->l_relocated) > + && __glibc_unlikely (rmap != map)) > + { > + const ElfW(Addr) *got > + = (const ElfW(Addr) *) D_PTR (rmap, l_info[DT_PLTGOT]); > + const ElfW(Word) local_gotno > + = (const ElfW(Word)) > + rmap->l_info[DT_MIPS (LOCAL_GOTNO)]->d_un.d_val; > + symidx = (sym > + - (ElfW(Sym) *) D_PTR(rmap, l_info[DT_SYMTAB])) > + / sizeof (sym); > + reloc_value = got[symidx + local_gotno - gotsym]; > + } > + /* Symbol pre-empted by not yet relocated, use best guess. */ > + else if (__glibc_unlikely (rmap != map)) > + reloc_value = sym->st_value + rmap->l_addr; > + /* Resolve IFUNC in this link unit. */ > + else if (__glibc_likely (ELFW(ST_TYPE) (sym->st_info) > + == STT_GNU_IFUNC)) > + reloc_value = elf_ifunc_invoke (sym->st_value + map->l_addr); > + else > + reloc_value += sym->st_value + map->l_addr; > + } > +#endif > else > { > #ifndef RTLD_BOOTSTRAP Does this mean that an STT_GNU_IFUNC can have two GOT entries, one in the new explicitly-relocated region and one in the normal global GOT? The way I'd imagined it working is that STT_GNU_IFUNC would be an exception to the usual ABI rule that every symbol involved in a relocation needs a GOT entry. Instead STT_GNU_IFUNCs would be kept below the DT_MIPS_GOTSYM threshold and the: if ((ElfW(Word))symidx < gotsym) { } block would then handle global symbols too. The IFUNC handling would be very similar to other targets. I suppose one downside of doing that for all global symbols is that we would then silently accept broken objects that older ld.sos wouldn't. If we really want to guard against that, we could raise a fatal error if a global symbol below gotsym is seen in a relocation and if DT_MIPS_GENERAL_GOTNO isn't set. The new behaviour would then be restricted to objects that use the new GOT layout. > @@ -795,8 +844,18 @@ elf_machine_got_rel (struct link_map *map, int lazy) > const struct r_found_version *version __attribute__ ((unused)) \ > = vernum ? &map->l_versions[vernum[sym_index] & 0x7fff] : NULL; \ > struct link_map *sym_map; \ > + ElfW(Addr) value = 0; \ > sym_map = RESOLVE_MAP (&ref, version, reloc); \ > - ref ? sym_map->l_addr + ref->st_value : 0; \ > + if (__glibc_likely(ref != NULL)) \ > + { \ > + value = sym_map->l_addr + ref->st_value; \ > + if (__glibc_unlikely (ELFW(ST_TYPE) (ref->st_info) \ > + == STT_GNU_IFUNC \ > + && sym_map != map \ > + && sym_map->l_relocated)) \ > + value = elf_ifunc_invoke (value); \ > + } \ > + value; \ > }) With that change, I think we should either: (a) make STT_GNU_IFUNCs in the global GOT be a fatal error or (b) call the resolver unconditionally, without the sym_map-based checks. We shouldn't resolve an IFUNC without calling the resolver. It's a question of whether it's more defensive to support IFUNCs in the ABI global GOT in the obvious way (b), in case we find a use for it later, or whether it's more defensive to rule it out until we know it has a use case (a). Anything that did want to put IFUNCs in the ABI global GOT would need to be sure that the resolver doesn't rely on unrelocated data. But in cases where that holds, traditional global entries might be (slightly) more efficient. Thanks, Richard
[Farez, sorry for the double message] Richard Sandiford <rdsandiford@googlemail.com> writes: > Faraz Shahbazker <Faraz.Shahbazker@imgtec.com> writes: >> @@ -795,8 +844,18 @@ elf_machine_got_rel (struct link_map *map, int lazy) >> const struct r_found_version *version __attribute__ ((unused)) \ >> = vernum ? &map->l_versions[vernum[sym_index] & 0x7fff] : NULL; \ >> struct link_map *sym_map; \ >> + ElfW(Addr) value = 0; \ >> sym_map = RESOLVE_MAP (&ref, version, reloc); \ >> - ref ? sym_map->l_addr + ref->st_value : 0; \ >> + if (__glibc_likely(ref != NULL)) \ >> + { \ >> + value = sym_map->l_addr + ref->st_value; \ >> + if (__glibc_unlikely (ELFW(ST_TYPE) (ref->st_info) \ >> + == STT_GNU_IFUNC \ >> + && sym_map != map \ >> + && sym_map->l_relocated)) \ >> + value = elf_ifunc_invoke (value); \ >> + } \ >> + value; \ >> }) > > With that change, I think we should either: > > (a) make STT_GNU_IFUNCs in the global GOT be a fatal error or > > (b) call the resolver unconditionally, without the sym_map-based checks. > We shouldn't resolve an IFUNC without calling the resolver. Sorry, I misread this. I thought it was checking the st_info of the original symbol rather than the resolved one. In that case I think we should do (b) -- remove the sym_map checks. Thanks, Richard
On 08/02/2015 07:03 AM, Richard Sandiford wrote: > I'm not sure we should consider the new tag to be unlikely. If the > intention is to use ifuncs in libc then it would eventually become > pretty common. Agreed. >> + /* Symbol pre-empted, use value from GOT. >> + 2nd part of condition is redundant, explicit for clarity. */ >> + if (__glibc_unlikely (rmap->l_relocated) >> + && __glibc_unlikely (rmap != map)) >> + { >> + const ElfW(Addr) *got >> + = (const ElfW(Addr) *) D_PTR (rmap, l_info[DT_PLTGOT]); >> + const ElfW(Word) local_gotno >> + = (const ElfW(Word)) >> + rmap->l_info[DT_MIPS (LOCAL_GOTNO)]->d_un.d_val; >> + symidx = (sym >> + - (ElfW(Sym) *) D_PTR(rmap, l_info[DT_SYMTAB])) >> + / sizeof (sym); >> + reloc_value = got[symidx + local_gotno - gotsym]; >> + } >> + /* Symbol pre-empted by not yet relocated, use best guess. */ >> + else if (__glibc_unlikely (rmap != map)) >> + reloc_value = sym->st_value + rmap->l_addr; >> + /* Resolve IFUNC in this link unit. */ >> + else if (__glibc_likely (ELFW(ST_TYPE) (sym->st_info) >> + == STT_GNU_IFUNC)) >> + reloc_value = elf_ifunc_invoke (sym->st_value + map->l_addr); >> + else >> + reloc_value += sym->st_value + map->l_addr; >> + } > > Does this mean that an STT_GNU_IFUNC can have two GOT entries, > one in the new explicitly-relocated region and one in the normal > global GOT? No. My intention is to keep global IFUNCs in normal global GOT only, so that the ABI method of mapping symbol index to GOT entry always works. The obvious anomaly is that the ABI global GOT now contains entries that are explicitly relocated. OTOH, by virtue of information present in the dynamic symbol table, these GOT entries are in no danger of being relocated twice. > The way I'd imagined it working is that STT_GNU_IFUNC > would be an exception to the usual ABI rule that every symbol involved > in a relocation needs a GOT entry. Instead STT_GNU_IFUNCs would be > kept below the DT_MIPS_GOTSYM threshold and the: > > if ((ElfW(Word))symidx < gotsym) > { > } > > block would then handle global symbols too. The IFUNC handling would be > very similar to other targets. We would still need to be able to map symbol index to GOT entry for global symbols, right? So if we have a set of global GOT entries within the new explicit GOT region for all global preemptible IFUNC symbols, we would need them sorted to be contiguous in the dynamic symbol table and add a new dynamic tag(DT_MIPS_GENERAL_GOTSYM) to mark the first index in this range. All instances of global GOT lookup would then be modified to check whether the symbol is IFUNC or normal. Is that what you are nudging me towards? >> @@ -795,8 +844,18 @@ elf_machine_got_rel (struct link_map *map, int lazy) >> const struct r_found_version *version __attribute__ ((unused)) \ >> = vernum ? &map->l_versions[vernum[sym_index] & 0x7fff] : NULL; \ >> struct link_map *sym_map; \ >> + ElfW(Addr) value = 0; \ >> sym_map = RESOLVE_MAP (&ref, version, reloc); \ >> - ref ? sym_map->l_addr + ref->st_value : 0; \ >> + if (__glibc_likely(ref != NULL)) \ >> + { \ >> + value = sym_map->l_addr + ref->st_value; \ >> + if (__glibc_unlikely (ELFW(ST_TYPE) (ref->st_info) \ >> + == STT_GNU_IFUNC \ >> + && sym_map != map \ >> + && sym_map->l_relocated)) \ >> + value = elf_ifunc_invoke (value); \ >> + } \ >> + value; \ >> }) > > With that change, I think we should either: > (b) call the resolver unconditionally, without the sym_map-based checks. > We shouldn't resolve an IFUNC without calling the resolver. > > It's a question of whether it's more defensive to support IFUNCs in the > ABI global GOT in the obvious way (b), in case we find a use for it later, > or whether it's more defensive to rule it out until we know it has > a use case (a). > > Anything that did want to put IFUNCs in the ABI global GOT would need > to be sure that the resolver doesn't rely on unrelocated data. But in > cases where that holds, traditional global entries might be (slightly) > more efficient. My understanding is: anything that wants to _invoke_ an IFUNC resolver needs to be sure that the resolver doesn't rely on unrelocated data. I'd rather have those sym_map checks in place and allow the program to at least load, with the unresolved IFUNC, without causing a crash. There is the possibility that an IFUNC which was not resolved at load time, does not get invoked at all during execution. Allowing a crash in ld.so due to the way the application is written makes me uncomfortable. I know the sym_map-check won't handle the most general case(mutual dependency, etc). If you deem it unnecessary, I will gladly defer to your judgement on this! I did earlier consider replacing the elf_ifunc_invoke() in this case with a GOT lookup: + if (__glibc_likely(ref != NULL)) \ + { \ + value = sym_map->l_addr + ref->st_value; \ + if (__glibc_unlikely (ELFW(ST_TYPE) (ref->st_info) \ + == STT_GNU_IFUNC \ + && sym_map != map \ + && sym_map->l_relocated)) \ + { \ + const ElfW(Addr) *got \ + = (const ElfW(Addr) *) D_PTR (sym_map, l_info[DT_PLTGOT]); \ + const ElfW(Word) local_gotno \ + = (const ElfW(Word)) \ + sym_map->l_info[DT_MIPS (LOCAL_GOTNO)]->d_un.d_val; \ + symidx = (ref - (ElfW(Sym) *) D_PTR(sym_map, l_info[DT_SYMTAB])) \ + / sizeof (ref); \ + reloc_value = got[symidx + local_gotno - gotsym]; \ + } \ + } \ + value; \ But couldn't make up my mind on which code would be more efficient. To be clear, ^that^ particular bit of code is only relevant for an undefined global symbol that resolves to an IFUNC in another ELF. IFUNCs in ABI global GOT are already excluded/deferred by the following hunk, as they are each assured to have an explicit R_MIPS_REL32 relocations. @@ -866,5 +928,5 @@ elf_machine_got_rel (struct link_map *map, int lazy) if (sym->st_other == 0) *got += map->l_addr; } - else + else if (ELFW(ST_TYPE) (sym->st_info) != STT_GNU_IFUNC) *got = RESOLVE_GOTSYM (sym, vernum, symidx, R_MIPS_32); Regards, Faraz Shahbazker
I fudged the text formatting on my mail client in the previous message. Hopefully, this version will read better: On 08/02/2015 07:03 AM, Richard Sandiford wrote: > I'm not sure we should consider the new tag to be unlikely. If the > intention is to use ifuncs in libc then it would eventually become > pretty common. Agreed. >>> + /* Symbol pre-empted, use value from GOT. >>> + 2nd part of condition is redundant, explicit for clarity. */ >>> + if (__glibc_unlikely (rmap->l_relocated) >>> + && __glibc_unlikely (rmap != map)) >>> + { >>> + const ElfW(Addr) *got >>> + = (const ElfW(Addr) *) D_PTR (rmap, l_info[DT_PLTGOT]); >>> + const ElfW(Word) local_gotno >>> + = (const ElfW(Word)) >>> + rmap->l_info[DT_MIPS (LOCAL_GOTNO)]->d_un.d_val; >>> + symidx = (sym >>> + - (ElfW(Sym) *) D_PTR(rmap, l_info[DT_SYMTAB])) >>> + / sizeof (sym); >>> + reloc_value = got[symidx + local_gotno - gotsym]; >>> + } >>> + /* Symbol pre-empted by not yet relocated, use best guess. */ >>> + else if (__glibc_unlikely (rmap != map)) >>> + reloc_value = sym->st_value + rmap->l_addr; >>> + /* Resolve IFUNC in this link unit. */ >>> + else if (__glibc_likely (ELFW(ST_TYPE) (sym->st_info) >>> + == STT_GNU_IFUNC)) >>> + reloc_value = elf_ifunc_invoke (sym->st_value + map->l_addr); >>> + else >>> + reloc_value += sym->st_value + map->l_addr; >>> + } > > Does this mean that an STT_GNU_IFUNC can have two GOT entries, > one in the new explicitly-relocated region and one in the normal > global GOT? No. My intention is to keep global IFUNCs in normal global GOT only, so that the ABI method of mapping symbol index to GOT entry always works. The obvious anomaly is that the ABI global GOT now contains entries that are explicitly relocated. OTOH, by virtue of information present in the dynamic symbol table, these GOT entries are in no danger of being relocated twice. > The way I'd imagined it working is that STT_GNU_IFUNC > would be an exception to the usual ABI rule that every symbol involved > in a relocation needs a GOT entry. Instead STT_GNU_IFUNCs would be > kept below the DT_MIPS_GOTSYM threshold and the: > > if ((ElfW(Word))symidx < gotsym) > { > } > > block would then handle global symbols too. The IFUNC handling would be > very similar to other targets. We would still need to be able to map symbol index to GOT entry for global symbols, right? So if we have a set of global GOT entries within the new explicit GOT region for all global preemptible IFUNC symbols, we would need them sorted to be contiguous in the dynamic symbol table and add a new dynamic tag(DT_MIPS_GENERAL_GOTSYM) to mark the first index in this range. All instances of global GOT lookup would then be modified to check whether the symbol is IFUNC or normal. Is that what you are nudging me towards? >>> @@ -795,8 +844,18 @@ elf_machine_got_rel (struct link_map *map, int lazy) >>> const struct r_found_version *version __attribute__ ((unused)) \ >>> = vernum ? &map->l_versions[vernum[sym_index] & 0x7fff] : NULL; \ >>> struct link_map *sym_map; \ >>> + ElfW(Addr) value = 0; \ >>> sym_map = RESOLVE_MAP (&ref, version, reloc); \ >>> - ref ? sym_map->l_addr + ref->st_value : 0; \ >>> + if (__glibc_likely(ref != NULL)) \ >>> + { \ >>> + value = sym_map->l_addr + ref->st_value; \ >>> + if (__glibc_unlikely (ELFW(ST_TYPE) (ref->st_info) \ >>> + == STT_GNU_IFUNC \ >>> + && sym_map != map \ >>> + && sym_map->l_relocated)) \ >>> + value = elf_ifunc_invoke (value); \ >>> + } \ >>> + value; \ >>> }) > > With that change, I think we should either: > (b) call the resolver unconditionally, without the sym_map-based checks. > We shouldn't resolve an IFUNC without calling the resolver. > > It's a question of whether it's more defensive to support IFUNCs in the > ABI global GOT in the obvious way (b), in case we find a use for it later, > or whether it's more defensive to rule it out until we know it has > a use case (a). > > Anything that did want to put IFUNCs in the ABI global GOT would need > to be sure that the resolver doesn't rely on unrelocated data. But in > cases where that holds, traditional global entries might be (slightly) > more efficient. My understanding is: anything that wants to _invoke_ an IFUNC resolver needs to be sure that the resolver doesn't rely on unrelocated data. I'd rather have those sym_map checks in place and allow the program to at least load, with the unresolved IFUNC, without causing a crash. There is the possibility that an IFUNC which was not resolved at load time, does not get invoked at all during execution. Allowing a crash in ld.so due to the way the application is written makes me uncomfortable. I know the sym_map-check won't handle the most general case(mutual dependency, etc). If you deem it unnecessary, I will gladly defer to your judgement on this! I did earlier consider replacing the elf_ifunc_invoke() in this case with a GOT lookup: + if (__glibc_likely(ref != NULL)) \ + { \ + value = sym_map->l_addr + ref->st_value; \ + if (__glibc_unlikely (ELFW(ST_TYPE) (ref->st_info) \ + == STT_GNU_IFUNC \ + && sym_map != map \ + && sym_map->l_relocated)) \ + { \ + const ElfW(Addr) *got \ + = (const ElfW(Addr) *) D_PTR (sym_map, l_info[DT_PLTGOT]); \ + const ElfW(Word) local_gotno \ + = (const ElfW(Word)) \ + sym_map->l_info[DT_MIPS (LOCAL_GOTNO)]->d_un.d_val; \ + symidx = (ref - (ElfW(Sym) *) D_PTR(sym_map, l_info[DT_SYMTAB])) \ + / sizeof (ref); \ + reloc_value = got[symidx + local_gotno - gotsym]; \ + } \ + } \ + value; \ But couldn't make up my mind on which code would be more efficient. To be clear, ^that^ particular bit of code is only relevant for an undefined global symbol that resolves to an IFUNC in another ELF. IFUNCs in ABI global GOT are already excluded/deferred by the following hunk, as they are each assured to have an explicit R_MIPS_REL32 relocations. @@ -866,5 +928,5 @@ elf_machine_got_rel (struct link_map *map, int lazy) if (sym->st_other == 0) *got += map->l_addr; } - else + else if (ELFW(ST_TYPE) (sym->st_info) != STT_GNU_IFUNC) *got = RESOLVE_GOTSYM (sym, vernum, symidx, R_MIPS_32); Regards, Faraz Shahbazker
On 07/31/2015 08:30 AM, Joseph Myers wrote: > On Fri, 31 Jul 2015, Faraz Shahbazker wrote: >> This is a follow up on IFUNC support for MIPS. Previous patch discussion on >> this list was: https://sourceware.org/ml/libc-ports/2013-08/msg00007.html > > To what extent does the previous description apply to this patch? In > particular, as regards the ABIs and other features supported. How has > this patch been tested?I The previous description mostly doesn't apply, specially for shared libraries and PIC. Mechanisms for non-PIC are roughly similar. Support for local IFUNC symbols and non-default visibility are new additions. The story behind what changed and why is spread across various threads on the binutils mailing list. This is definitely not an 'incremental' update. The intention as always is to support all ABI variants. I don't see ABI-specific changes as far as glibc is concerned. I have tested with o32/n32/n64/mips16/micromips (though the last 2 are due for rechecking due to recent tweaks). I have entirely skipped vxworks. I am working off an internal run-time testsuite - transitioning whatever can be checked statically, to be included in the ld testsuite in binutils. Not quite sure what to do with my functional tests - if someone could point me to where such tests belong ... > glibc does not have per-subdirectory ChangeLog files (except for libidn > and localedata). So ChangeLog entries should contain the full paths to > the files relative to the toplevel glibc source directory, elf/elf.h and > sysdeps/mips/*.h. Got it. > I'd expect an addition to sysdeps/unix/sysv/linux/mips/libc-abis for > binary compatibility checks using EI_ABIVERSION to work properly (along > with binutils setting EI_ABIVERSION to 4 when IFUNCs are used for MIPS). Got it. Thanks for looking at this. Regards, Faraz Shahbazker
[Sorry for mistyping your name in the previous message :-(] Faraz Shahbazker <faraz.shahbazker@imgtec.com> writes: >>>> + /* Symbol pre-empted, use value from GOT. >>>> + 2nd part of condition is redundant, explicit for clarity. */ >>>> + if (__glibc_unlikely (rmap->l_relocated) >>>> + && __glibc_unlikely (rmap != map)) >>>> + { >>>> + const ElfW(Addr) *got >>>> + = (const ElfW(Addr) *) D_PTR (rmap, l_info[DT_PLTGOT]); >>>> + const ElfW(Word) local_gotno >>>> + = (const ElfW(Word)) >>>> + rmap->l_info[DT_MIPS (LOCAL_GOTNO)]->d_un.d_val; >>>> + symidx = (sym >>>> + - (ElfW(Sym) *) D_PTR(rmap, l_info[DT_SYMTAB])) >>>> + / sizeof (sym); >>>> + reloc_value = got[symidx + local_gotno - gotsym]; >>>> + } >>>> + /* Symbol pre-empted by not yet relocated, use best guess. */ >>>> + else if (__glibc_unlikely (rmap != map)) >>>> + reloc_value = sym->st_value + rmap->l_addr; >>>> + /* Resolve IFUNC in this link unit. */ >>>> + else if (__glibc_likely (ELFW(ST_TYPE) (sym->st_info) >>>> + == STT_GNU_IFUNC)) >>>> + reloc_value = elf_ifunc_invoke (sym->st_value + map->l_addr); >>>> + else >>>> + reloc_value += sym->st_value + map->l_addr; >>>> + } >> >> Does this mean that an STT_GNU_IFUNC can have two GOT entries, >> one in the new explicitly-relocated region and one in the normal >> global GOT? > > No. My intention is to keep global IFUNCs in normal global GOT only, so that > the ABI method of mapping symbol index to GOT entry always works. The obvious > anomaly is that the ABI global GOT now contains entries that are explicitly > relocated. OTOH, by virtue of information present in the dynamic symbol table, > these GOT entries are in no danger of being relocated twice. I don't really see the benefit of that though. It's possible for this particular case because the specialness of the GOT entry is encoded in the symbol type. But the idea of the new GOT region was that we could put any new GOT entry types there, even if the relocation is against a normal function or data symbol (local or global). We wouldn't have STT to guide us then. Even though it's possible to avoid putting global symbols in the new region here, you seem to be jumping through some hoops to make it work. And the "best guess" worries me :-) If we just treat the new GOT region as an ordinary relocated region and allow it to refer to global symbols, we never have to guess. It's just a case of applying the relocation in the same way as for any other part of the image. There's already precendent for allowing global symbols not to be in the directly-mapped GOT (for TLS), so it doesn't seem like that big a leap. >> The way I'd imagined it working is that STT_GNU_IFUNC >> would be an exception to the usual ABI rule that every symbol involved >> in a relocation needs a GOT entry. Instead STT_GNU_IFUNCs would be >> kept below the DT_MIPS_GOTSYM threshold and the: >> >> if ((ElfW(Word))symidx < gotsym) >> { >> } >> >> block would then handle global symbols too. The IFUNC handling would be >> very similar to other targets. > > We would still need to be able to map symbol index to GOT entry for global > symbols, right? No, I don't think so. We just handle relocations against unmapped symbols in the same way as other targets do. I.e. we use RESOLVE_MAP rather than the directly-mapped GOT cache. > So if we have a set of global GOT entries within the new explicit GOT > region for all global preemptible IFUNC symbols, we would need them > sorted to be contiguous in the dynamic symbol table and add a new > dynamic tag(DT_MIPS_GENERAL_GOTSYM) to mark the first index in this > range. All instances of global GOT lookup would then be modified to > check whether the symbol is IFUNC or normal. Is that what you are > nudging me towards? I'd like to avoid another directly-mapped region at all costs :-) The ABI GOT scheme, including direct mapping, works very well for the situation it was designed for. But I don't think it extends naturally to ifuncs (or to TLS). Normal relocations seem like a better fit. >>>> @@ -795,8 +844,18 @@ elf_machine_got_rel (struct link_map *map, int lazy) >>>> const struct r_found_version *version __attribute__ ((unused)) \ >>>> = vernum ? &map->l_versions[vernum[sym_index] & 0x7fff] : NULL; \ >>>> struct link_map *sym_map; \ >>>> + ElfW(Addr) value = 0; \ >>>> sym_map = RESOLVE_MAP (&ref, version, reloc); \ >>>> - ref ? sym_map->l_addr + ref->st_value : 0; \ >>>> + if (__glibc_likely(ref != NULL)) \ >>>> + { \ >>>> + value = sym_map->l_addr + ref->st_value; \ >>>> + if (__glibc_unlikely (ELFW(ST_TYPE) (ref->st_info) \ >>>> + == STT_GNU_IFUNC \ >>>> + && sym_map != map \ >>>> + && sym_map->l_relocated)) \ >>>> + value = elf_ifunc_invoke (value); \ >>>> + } \ >>>> + value; \ >>>> }) >> >> With that change, I think we should either: >> (b) call the resolver unconditionally, without the sym_map-based checks. >> We shouldn't resolve an IFUNC without calling the resolver. >> >> It's a question of whether it's more defensive to support IFUNCs in the >> ABI global GOT in the obvious way (b), in case we find a use for it later, >> or whether it's more defensive to rule it out until we know it has >> a use case (a). >> >> Anything that did want to put IFUNCs in the ABI global GOT would need >> to be sure that the resolver doesn't rely on unrelocated data. But in >> cases where that holds, traditional global entries might be (slightly) >> more efficient. > > My understanding is: anything that wants to _invoke_ an IFUNC resolver needs > to be sure that the resolver doesn't rely on unrelocated data. I'd rather have > those sym_map checks in place and allow the program to at least load, with the > unresolved IFUNC, without causing a crash. There is the possibility that an > IFUNC which was not resolved at load time, does not get invoked at all during > execution. Allowing a crash in ld.so due to the way the application is written > makes me uncomfortable. I know the sym_map-check won't handle the most general > case(mutual dependency, etc). If you deem it unnecessary, I will gladly defer > to your judgement on this! Heh. I have no authority here. :-) But IMO this is no different from DSOs having to be careful about DT_INIT order. Yes, it's bad if they get it wrong, but we shouldn't second-guess the user. At least a segfault at start-up is easier to debug than a symbol having the wrong value. That was the point of the example I sent on the binutils list. The problem isn't specific to MIPS and other targets call the resolver regardless. I think we should be consistent here. Thanks, Richard
diff --git a/elf/elf.h b/elf/elf.h index fbadda4..cfcf72c 100644 --- a/elf/elf.h +++ b/elf/elf.h @@ -1653,8 +1653,9 @@ typedef struct #define R_MIPS_GLOB_DAT 51 #define R_MIPS_COPY 126 #define R_MIPS_JUMP_SLOT 127 +#define R_MIPS_IRELATIVE 128 /* Keep this the last entry. */ -#define R_MIPS_NUM 128 +#define R_MIPS_NUM 129 /* Legal values for p_type field of Elf32_Phdr. */ @@ -1731,7 +1732,8 @@ typedef struct in a PIE as it stores a relative offset from the address of the tag rather than an absolute address. */ #define DT_MIPS_RLD_MAP_REL 0x70000035 -#define DT_MIPS_NUM 0x36 +#define DT_MIPS_GENERAL_GOTNO 0x70000036 /* Number of relocated GOT entries */ +#define DT_MIPS_NUM 0x37 /* Legal values for DT_MIPS_FLAGS Elf32_Dyn entry. */ diff --git a/sysdeps/mips/dl-irel.h b/sysdeps/mips/dl-irel.h new file mode 100644 index 0000000..7e1fdc4 --- /dev/null +++ b/sysdeps/mips/dl-irel.h @@ -0,0 +1,63 @@ +/* Machine-dependent ELF indirect relocation inline functions. + MIPS version. + Copyright (C) 2015 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library. If not, see + <http://www.gnu.org/licenses/>. */ + +#ifndef _DL_IREL_H +#define _DL_IREL_H + +#include <stdio.h> +#include <unistd.h> +#include <sgidefs.h> +#include <link.h> +#include <elf.h> +#include <ldsodefs.h> + +#define ELF_MACHINE_IREL 1 + +static inline ElfW(Addr) +__attribute ((always_inline)) +elf_ifunc_invoke (ElfW(Addr) addr) +{ + /* Print some debugging info if wanted. */ + if (__builtin_expect (GLRO(dl_debug_mask) & DL_DEBUG_SYMBOLS, 0)) + { + ElfW(Addr) t_addr = + ((ElfW(Addr) (*) (unsigned long int)) (addr)) (GLRO(dl_hwcap)); + GLRO(dl_debug_printf) ("In elf_ifunc_invoke(0x%lx), return(0x%lx)\n", + (unsigned long int)addr, + (unsigned long int)t_addr); + } + + return ((ElfW(Addr) (*) (unsigned long int)) (addr)) (GLRO(dl_hwcap)); +} + +/* Allow either R_MIPS_RELATIVE or the nop R_MIPS_NONE. */ +static inline void +__attribute ((always_inline)) +elf_irel (const ElfW(Rel) *reloc) +{ + ElfW(Addr) *const reloc_addr = (void *) reloc->r_offset; + const unsigned long int r_type = ELFW(R_TYPE) (reloc->r_info); + + if (__builtin_expect (r_type == R_MIPS_IRELATIVE, 1)) + *reloc_addr = elf_ifunc_invoke (*reloc_addr); + else if (r_type) + __libc_fatal ("unexpected reloc type in static binary"); +} + +#endif /* dl-irel.h */ diff --git a/sysdeps/mips/dl-machine.h b/sysdeps/mips/dl-machine.h index 8738564..e7e0dc4 100644 --- a/sysdeps/mips/dl-machine.h +++ b/sysdeps/mips/dl-machine.h @@ -33,6 +33,7 @@ #include <sysdep.h> #include <sys/asm.h> #include <dl-tls.h> +#include <dl-irel.h> /* The offset of gp from GOT might be system-dependent. It's set by ld. The same value is also */ @@ -200,10 +201,13 @@ do { \ if (__builtin_expect (map->l_addr == 0, 1)) \ break; \ \ - /* got[0] is reserved. got[1] is also reserved for the dynamic object \ - generated by gnu ld. Skip these reserved entries from \ - relocation. */ \ - i = (got[1] & ELF_MIPS_GNU_GOT1_MASK)? 2 : 1; \ + if (__glibc_unlikely (map->l_info[DT_MIPS (GENERAL_GOTNO)] != NULL)) \ + i = map->l_info[DT_MIPS (GENERAL_GOTNO)]->d_un.d_val; \ + else \ + /* got[0] is reserved. got[1] is also reserved for the dynamic \ + object generated by gnu ld. Skip these reserved entries from \ + relocation. */ \ + i = (got[1] & ELF_MIPS_GNU_GOT1_MASK)? 2 : 1; \ n = map->l_info[DT_MIPS (LOCAL_GOTNO)]->d_un.d_val; \ \ /* Add the run-time displacement to all local got entries. */ \ @@ -493,7 +497,8 @@ auto inline void __attribute__ ((always_inline)) elf_machine_reloc (struct link_map *map, ElfW(Addr) r_info, const ElfW(Sym) *sym, const struct r_found_version *version, - void *reloc_addr, ElfW(Addr) r_addend, int inplace_p) + void *reloc_addr, ElfW(Addr) r_addend, int inplace_p, + int skip_ifunc) { const unsigned long int r_type = ELFW(R_TYPE) (r_info); ElfW(Addr) *addr_field = (ElfW(Addr) *) reloc_addr; @@ -599,6 +604,41 @@ elf_machine_reloc (struct link_map *map, ElfW(Addr) r_info, #endif reloc_value += sym->st_value + map->l_addr; } +#ifndef RTLD_BOOTSTRAP + /* Resolve IFUNC symbols with pre-emption. */ + else if (sym + && __glibc_unlikely (ELFW(ST_TYPE) (sym->st_info) + == STT_GNU_IFUNC) + && !skip_ifunc) + { + struct link_map *rmap = RESOLVE_MAP (&sym, version, r_type); + + /* Symbol pre-empted, use value from GOT. + 2nd part of condition is redundant, explicit for clarity. */ + if (__glibc_unlikely (rmap->l_relocated) + && __glibc_unlikely (rmap != map)) + { + const ElfW(Addr) *got + = (const ElfW(Addr) *) D_PTR (rmap, l_info[DT_PLTGOT]); + const ElfW(Word) local_gotno + = (const ElfW(Word)) + rmap->l_info[DT_MIPS (LOCAL_GOTNO)]->d_un.d_val; + symidx = (sym + - (ElfW(Sym) *) D_PTR(rmap, l_info[DT_SYMTAB])) + / sizeof (sym); + reloc_value = got[symidx + local_gotno - gotsym]; + } + /* Symbol pre-empted by not yet relocated, use best guess. */ + else if (__glibc_unlikely (rmap != map)) + reloc_value = sym->st_value + rmap->l_addr; + /* Resolve IFUNC in this link unit. */ + else if (__glibc_likely (ELFW(ST_TYPE) (sym->st_info) + == STT_GNU_IFUNC)) + reloc_value = elf_ifunc_invoke (sym->st_value + map->l_addr); + else + reloc_value += sym->st_value + map->l_addr; + } +#endif else { #ifndef RTLD_BOOTSTRAP @@ -698,6 +738,14 @@ elf_machine_reloc (struct link_map *map, ElfW(Addr) r_info, break; } + case R_MIPS_IRELATIVE: + /* The resolver routine is the symbol referenced by this relocation. + To get the address of the function to use at runtime, the resolver + routine is called and its return value is the address of the target + functon which is final relocation value. */ + *addr_field = elf_ifunc_invoke (map->l_addr + *addr_field); + break; + #if _MIPS_SIM == _ABI64 case R_MIPS_64: /* For full compliance with the ELF64 ABI, one must precede the @@ -727,7 +775,8 @@ elf_machine_rel (struct link_map *map, const ElfW(Rel) *reloc, const ElfW(Sym) *sym, const struct r_found_version *version, void *const reloc_addr, int skip_ifunc) { - elf_machine_reloc (map, reloc->r_info, sym, version, reloc_addr, 0, 1); + elf_machine_reloc (map, reloc->r_info, sym, version, reloc_addr, 0, 1, + skip_ifunc); } auto inline void @@ -768,7 +817,7 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc, void *const reloc_addr, int skip_ifunc) { elf_machine_reloc (map, reloc->r_info, sym, version, reloc_addr, - reloc->r_addend, 0); + reloc->r_addend, 0, skip_ifunc); } auto inline void @@ -795,8 +844,18 @@ elf_machine_got_rel (struct link_map *map, int lazy) const struct r_found_version *version __attribute__ ((unused)) \ = vernum ? &map->l_versions[vernum[sym_index] & 0x7fff] : NULL; \ struct link_map *sym_map; \ + ElfW(Addr) value = 0; \ sym_map = RESOLVE_MAP (&ref, version, reloc); \ - ref ? sym_map->l_addr + ref->st_value : 0; \ + if (__glibc_likely(ref != NULL)) \ + { \ + value = sym_map->l_addr + ref->st_value; \ + if (__glibc_unlikely (ELFW(ST_TYPE) (ref->st_info) \ + == STT_GNU_IFUNC \ + && sym_map != map \ + && sym_map->l_relocated)) \ + value = elf_ifunc_invoke (value); \ + } \ + value; \ }) if (map->l_info[VERSYMIDX (DT_VERSYM)] != NULL) @@ -810,9 +869,12 @@ elf_machine_got_rel (struct link_map *map, int lazy) /* The dynamic linker's local got entries have already been relocated. */ if (map != &GL(dl_rtld_map)) { - /* got[0] is reserved. got[1] is also reserved for the dynamic object - generated by gnu ld. Skip these reserved entries from relocation. */ - i = (got[1] & ELF_MIPS_GNU_GOT1_MASK)? 2 : 1; + if (__glibc_unlikely(map->l_info[DT_MIPS (GENERAL_GOTNO)] != NULL)) + i = map->l_info[DT_MIPS (GENERAL_GOTNO)]->d_un.d_val; + else + /* got[0] is reserved. got[1] is also reserved for the dynamic object + generated by gnu ld. Skip these reserved entries from relocation. */ + i = (got[1] & ELF_MIPS_GNU_GOT1_MASK)? 2 : 1; /* Add the run-time displacement to all local got entries if needed. */ @@ -866,7 +928,7 @@ elf_machine_got_rel (struct link_map *map, int lazy) if (sym->st_other == 0) *got += map->l_addr; } - else + else if (ELFW(ST_TYPE) (sym->st_info) != STT_GNU_IFUNC) *got = RESOLVE_GOTSYM (sym, vernum, symidx, R_MIPS_32); ++got; diff --git a/sysdeps/mips/dl-trampoline.c b/sysdeps/mips/dl-trampoline.c index 25b1709..d5f8891 100644 --- a/sysdeps/mips/dl-trampoline.c +++ b/sysdeps/mips/dl-trampoline.c @@ -193,6 +193,10 @@ __dl_runtime_resolve (ElfW(Word) sym_index, /* Currently value contains the base load address of the object that defines sym. Now add in the symbol offset. */ value = (sym ? sym_map->l_addr + sym->st_value : 0); + if (sym != NULL + && __builtin_expect (ELFW(ST_TYPE) (sym->st_info) + == STT_GNU_IFUNC, 0)) + value = elf_ifunc_invoke (DL_FIXUP_VALUE_ADDR (value)); } else /* We already found the symbol. The module (and therefore its load