Message ID | 529461B1.8080200@linaro.org |
---|---|
State | Accepted |
Headers | show |
On 26 November 2013 08:54, Will Newton <will.newton@linaro.org> wrote: > - if (info->shared && SYMBOL_REFERENCES_LOCAL (info, h)) > + if (h->def_regular > + && h->type == STT_GNU_IFUNC) > + { > + if (info->shared) > + { > + /* Generate R_AARCH64_GLOB_DAT. */ > + goto do_glob_dat; > + } > + else > + { > + asection *plt; > + > + if (!h->pointer_equality_needed) > + abort (); Is abort() the best option here, can't we spit out a diagnostic and return? /Marcus
On 26/11/13 10:34, Marcus Shawcroft wrote: > On 26 November 2013 08:54, Will Newton <will.newton@linaro.org> wrote: > >> - if (info->shared && SYMBOL_REFERENCES_LOCAL (info, h)) >> + if (h->def_regular >> + && h->type == STT_GNU_IFUNC) >> + { >> + if (info->shared) >> + { >> + /* Generate R_AARCH64_GLOB_DAT. */ >> + goto do_glob_dat; >> + } >> + else >> + { >> + asection *plt; >> + >> + if (!h->pointer_equality_needed) >> + abort (); > > Is abort() the best option here, can't we spit out a diagnostic and return? > > /Marcus > In general, abort is OK here if it's due to internal inconsistencies in the linker. It's not OK if it's due to inconsistencies in the input object files. R.
On 26 November 2013 10:34, Marcus Shawcroft <marcus.shawcroft@gmail.com> wrote: > On 26 November 2013 08:54, Will Newton <will.newton@linaro.org> wrote: > >> - if (info->shared && SYMBOL_REFERENCES_LOCAL (info, h)) >> + if (h->def_regular >> + && h->type == STT_GNU_IFUNC) >> + { >> + if (info->shared) >> + { >> + /* Generate R_AARCH64_GLOB_DAT. */ >> + goto do_glob_dat; >> + } >> + else >> + { >> + asection *plt; >> + >> + if (!h->pointer_equality_needed) >> + abort (); > > Is abort() the best option here, can't we spit out a diagnostic and return? I believe it is really an assert as _bfd_elf_allocate_ifunc_dyn_relocs ensures this condition does not occur. The code is copied from i386/x86_64 for better or worse - I am never quite sure whether it is better to be the same or different in these cases. ;-)
On 11/26/13 08:54, Will Newton wrote: > The code for handling GOT references to ifunc symbols in static links > was missing. > > bfd/ChangeLog: > > 2013-11-25 Will Newton<will.newton@linaro.org> > > * elfnn-aarch64.c (elfNN_aarch64_finish_dynamic_symbol): > Handle STT_GNU_IFUNC symbols correctly in static links. > > 2013-11-25 Will Newton<will.newton@linaro.org> > > * ld-aarch64/aarch64-elf.exp: Add ifunc-22. > * ld-aarch64/ifunc-22.d: New file. > * ld-aarch64/ifunc-22.s: Likewise. > --- > bfd/elfnn-aarch64.c | 30 +++++++++++++++++++++++++++++- > ld/testsuite/ld-aarch64/aarch64-elf.exp | 1 + > ld/testsuite/ld-aarch64/ifunc-22.d | 11 +++++++++++ > ld/testsuite/ld-aarch64/ifunc-22.s | 14 ++++++++++++++ > 4 files changed, 55 insertions(+), 1 deletion(-) > create mode 100644 ld/testsuite/ld-aarch64/ifunc-22.d > create mode 100644 ld/testsuite/ld-aarch64/ifunc-22.s > > OK for trunk and binutils_2_24-branch? > > diff --git a/bfd/elfnn-aarch64.c b/bfd/elfnn-aarch64.c > index 7cce6f4..1467f5d 100644 > --- a/bfd/elfnn-aarch64.c > +++ b/bfd/elfnn-aarch64.c > @@ -6824,7 +6824,34 @@ elfNN_aarch64_finish_dynamic_symbol (bfd *output_bfd, > + htab->root.sgot->output_offset > + (h->got.offset& ~(bfd_vma) 1)); > > - if (info->shared&& SYMBOL_REFERENCES_LOCAL (info, h)) > + if (h->def_regular > + && h->type == STT_GNU_IFUNC) > + { > + if (info->shared) > + { > + /* Generate R_AARCH64_GLOB_DAT. */ > + goto do_glob_dat; > + } Can the control flow be optimized so that the outer if condition also checks !info->shared? I wonder whether the goto statement be avoided. + if (h->def_regular + && h->type == STT_GNU_IFUNC + && !info->shared ) > + else > + { > + asection *plt; > + > + if (!h->pointer_equality_needed) > + abort (); > + > + /* For non-shared object, we can't use .got.plt, which > + contains the real function addres if we need pointer addres/address > + equality. We load the GOT entry with the PLT entry. */ > + plt = htab->root.splt ? htab->root.splt : htab->root.iplt; > + bfd_put_NN (output_bfd, (plt->output_section->vma > + + plt->output_offset > + + h->plt.offset), > + htab->root.sgot->contents > + + (h->got.offset& ~(bfd_vma) 1)); > + return TRUE; > + } > + } > + else if (info->shared&& SYMBOL_REFERENCES_LOCAL (info, h)) > { > if (!h->def_regular) > return FALSE; > @@ -6838,6 +6865,7 @@ elfNN_aarch64_finish_dynamic_symbol (bfd *output_bfd, > else > { > BFD_ASSERT ((h->got.offset& 1) == 0); > +do_glob_dat: > bfd_put_NN (output_bfd, (bfd_vma) 0, > htab->root.sgot->contents + h->got.offset); > rela.r_info = ELFNN_R_INFO (h->dynindx, AARCH64_R (GLOB_DAT)); Is do_glob_dat placed deliberately after the assertion? Thanks, Yufeng > diff --git a/ld/testsuite/ld-aarch64/aarch64-elf.exp b/ld/testsuite/ld-aarch64/aarch64-elf.exp > index a6b3ea2..692bf34 100644 > --- a/ld/testsuite/ld-aarch64/aarch64-elf.exp > +++ b/ld/testsuite/ld-aarch64/aarch64-elf.exp > @@ -156,3 +156,4 @@ run_dump_test "ifunc-19a" > run_dump_test "ifunc-19b" > run_dump_test "ifunc-20" > run_dump_test "ifunc-21" > +run_dump_test "ifunc-22" > diff --git a/ld/testsuite/ld-aarch64/ifunc-22.d b/ld/testsuite/ld-aarch64/ifunc-22.d > new file mode 100644 > index 0000000..f28b039 > --- /dev/null > +++ b/ld/testsuite/ld-aarch64/ifunc-22.d > @@ -0,0 +1,11 @@ > +#source: ifunc-22.s > +#objdump: -s -j .got > +#ld: -static > +#target: aarch64*-*-* > + > +# Ensure GOT is populated correctly in static link > + > +.*: file format elf64-(little|big)aarch64 > + > +Contents of section \.got: > + 4100f0 00000000 00000000 d0004000 00000000 ..........@..... > diff --git a/ld/testsuite/ld-aarch64/ifunc-22.s b/ld/testsuite/ld-aarch64/ifunc-22.s > new file mode 100644 > index 0000000..69a87bb > --- /dev/null > +++ b/ld/testsuite/ld-aarch64/ifunc-22.s > @@ -0,0 +1,14 @@ > + .text > + .type ifunc, @gnu_indirect_function > + .global ifunc > +ifunc: > + ret > + .size ifunc, .-ifunc > + .type _start, @function > + .globl _start > +_start: > + adrp x0, :got:ifunc > + ldr x0, [x0, #:got_lo12:ifunc] > + .size _start, .-_start > + .data > + .xword ifunc > -- 1.8.1.4 >
On 26 November 2013 11:06, Yufeng Zhang <Yufeng.Zhang@arm.com> wrote: > On 11/26/13 08:54, Will Newton wrote: >> >> The code for handling GOT references to ifunc symbols in static links >> was missing. >> >> bfd/ChangeLog: >> >> 2013-11-25 Will Newton<will.newton@linaro.org> >> >> * elfnn-aarch64.c (elfNN_aarch64_finish_dynamic_symbol): >> Handle STT_GNU_IFUNC symbols correctly in static links. >> >> 2013-11-25 Will Newton<will.newton@linaro.org> >> >> * ld-aarch64/aarch64-elf.exp: Add ifunc-22. >> * ld-aarch64/ifunc-22.d: New file. >> * ld-aarch64/ifunc-22.s: Likewise. >> --- >> bfd/elfnn-aarch64.c | 30 >> +++++++++++++++++++++++++++++- >> ld/testsuite/ld-aarch64/aarch64-elf.exp | 1 + >> ld/testsuite/ld-aarch64/ifunc-22.d | 11 +++++++++++ >> ld/testsuite/ld-aarch64/ifunc-22.s | 14 ++++++++++++++ >> 4 files changed, 55 insertions(+), 1 deletion(-) >> create mode 100644 ld/testsuite/ld-aarch64/ifunc-22.d >> create mode 100644 ld/testsuite/ld-aarch64/ifunc-22.s >> >> OK for trunk and binutils_2_24-branch? >> >> diff --git a/bfd/elfnn-aarch64.c b/bfd/elfnn-aarch64.c >> index 7cce6f4..1467f5d 100644 >> --- a/bfd/elfnn-aarch64.c >> +++ b/bfd/elfnn-aarch64.c >> @@ -6824,7 +6824,34 @@ elfNN_aarch64_finish_dynamic_symbol (bfd >> *output_bfd, >> + htab->root.sgot->output_offset >> + (h->got.offset& ~(bfd_vma) 1)); >> >> - if (info->shared&& SYMBOL_REFERENCES_LOCAL (info, h)) >> >> + if (h->def_regular >> + && h->type == STT_GNU_IFUNC) >> + { >> + if (info->shared) >> + { >> + /* Generate R_AARCH64_GLOB_DAT. */ >> + goto do_glob_dat; >> + } > > > Can the control flow be optimized so that the outer if condition also checks > !info->shared? I wonder whether the goto statement be avoided. It would involve altering the else if condition somewhat so I am inclined to leave as is. I am aware that the bfd copy and paste model of code reuse is rather unsatisfactory, but sometimes I think it is better to have the same bugs as everyone else rather than different ones! And I also I have dreams that one day some kindly bfd hacker will come along and pull some of this stuff out into common code and at that point the similarity between ports will make that job easier. > + if (h->def_regular > + && h->type == STT_GNU_IFUNC > + && !info->shared ) > > > >> + else >> + { >> + asection *plt; >> + >> + if (!h->pointer_equality_needed) >> + abort (); >> + >> + /* For non-shared object, we can't use .got.plt, which >> + contains the real function addres if we need pointer > > > addres/address Thanks, fixed. >> + equality. We load the GOT entry with the PLT entry. */ >> + plt = htab->root.splt ? htab->root.splt : htab->root.iplt; >> + bfd_put_NN (output_bfd, (plt->output_section->vma >> + + plt->output_offset >> + + h->plt.offset), >> + htab->root.sgot->contents >> + + (h->got.offset& ~(bfd_vma) 1)); >> >> + return TRUE; >> + } >> + } >> + else if (info->shared&& SYMBOL_REFERENCES_LOCAL (info, h)) >> >> { >> if (!h->def_regular) >> return FALSE; >> @@ -6838,6 +6865,7 @@ elfNN_aarch64_finish_dynamic_symbol (bfd >> *output_bfd, >> else >> { >> BFD_ASSERT ((h->got.offset& 1) == 0); >> >> +do_glob_dat: >> bfd_put_NN (output_bfd, (bfd_vma) 0, >> htab->root.sgot->contents + h->got.offset); >> rela.r_info = ELFNN_R_INFO (h->dynindx, AARCH64_R (GLOB_DAT)); > > > Is do_glob_dat placed deliberately after the assertion? I don't know. But I share your concern about this code, I'll move the label up above the assert as I don't see a case where the assert could fail but the code below remain valid.
On 11/26/13 11:16, Will Newton wrote: > On 26 November 2013 11:06, Yufeng Zhang<Yufeng.Zhang@arm.com> wrote: >> On 11/26/13 08:54, Will Newton wrote: >>> >>> The code for handling GOT references to ifunc symbols in static links >>> was missing. >>> >>> bfd/ChangeLog: >>> >>> 2013-11-25 Will Newton<will.newton@linaro.org> >>> >>> * elfnn-aarch64.c (elfNN_aarch64_finish_dynamic_symbol): >>> Handle STT_GNU_IFUNC symbols correctly in static links. >>> >>> 2013-11-25 Will Newton<will.newton@linaro.org> >>> >>> * ld-aarch64/aarch64-elf.exp: Add ifunc-22. >>> * ld-aarch64/ifunc-22.d: New file. >>> * ld-aarch64/ifunc-22.s: Likewise. >>> --- >>> bfd/elfnn-aarch64.c | 30 >>> +++++++++++++++++++++++++++++- >>> ld/testsuite/ld-aarch64/aarch64-elf.exp | 1 + >>> ld/testsuite/ld-aarch64/ifunc-22.d | 11 +++++++++++ >>> ld/testsuite/ld-aarch64/ifunc-22.s | 14 ++++++++++++++ >>> 4 files changed, 55 insertions(+), 1 deletion(-) >>> create mode 100644 ld/testsuite/ld-aarch64/ifunc-22.d >>> create mode 100644 ld/testsuite/ld-aarch64/ifunc-22.s >>> >>> OK for trunk and binutils_2_24-branch? >>> >>> diff --git a/bfd/elfnn-aarch64.c b/bfd/elfnn-aarch64.c >>> index 7cce6f4..1467f5d 100644 >>> --- a/bfd/elfnn-aarch64.c >>> +++ b/bfd/elfnn-aarch64.c >>> @@ -6824,7 +6824,34 @@ elfNN_aarch64_finish_dynamic_symbol (bfd >>> *output_bfd, >>> + htab->root.sgot->output_offset >>> + (h->got.offset& ~(bfd_vma) 1)); >>> >>> - if (info->shared&& SYMBOL_REFERENCES_LOCAL (info, h)) >>> >>> + if (h->def_regular >>> +&& h->type == STT_GNU_IFUNC) >>> + { >>> + if (info->shared) >>> + { >>> + /* Generate R_AARCH64_GLOB_DAT. */ >>> + goto do_glob_dat; >>> + } >> >> >> Can the control flow be optimized so that the outer if condition also checks >> !info->shared? I wonder whether the goto statement be avoided. > > It would involve altering the else if condition somewhat so I am > inclined to leave as is. > > I am aware that the bfd copy and paste model of code reuse is rather > unsatisfactory, but sometimes I think it is better to have the same > bugs as everyone else rather than different ones! Now I realized where the code is from. Not ideal but I see your point. > And I also I have > dreams that one day some kindly bfd hacker will come along and pull > some of this stuff out into common code and at that point the > similarity between ports will make that job easier. I have a similar dream ;) Yufeng > >> + if (h->def_regular >> +&& h->type == STT_GNU_IFUNC >> +&& !info->shared ) >> >> >> >>> + else >>> + { >>> + asection *plt; >>> + >>> + if (!h->pointer_equality_needed) >>> + abort (); >>> + >>> + /* For non-shared object, we can't use .got.plt, which >>> + contains the real function addres if we need pointer >> >> >> addres/address > > Thanks, fixed. > >>> + equality. We load the GOT entry with the PLT entry. */ >>> + plt = htab->root.splt ? htab->root.splt : htab->root.iplt; >>> + bfd_put_NN (output_bfd, (plt->output_section->vma >>> + + plt->output_offset >>> + + h->plt.offset), >>> + htab->root.sgot->contents >>> + + (h->got.offset& ~(bfd_vma) 1)); >>> >>> + return TRUE; >>> + } >>> + } >>> + else if (info->shared&& SYMBOL_REFERENCES_LOCAL (info, h)) >>> >>> { >>> if (!h->def_regular) >>> return FALSE; >>> @@ -6838,6 +6865,7 @@ elfNN_aarch64_finish_dynamic_symbol (bfd >>> *output_bfd, >>> else >>> { >>> BFD_ASSERT ((h->got.offset& 1) == 0); >>> >>> +do_glob_dat: >>> bfd_put_NN (output_bfd, (bfd_vma) 0, >>> htab->root.sgot->contents + h->got.offset); >>> rela.r_info = ELFNN_R_INFO (h->dynindx, AARCH64_R (GLOB_DAT)); >> >> >> Is do_glob_dat placed deliberately after the assertion? > > I don't know. But I share your concern about this code, I'll move the > label up above the assert as I don't see a case where the assert could > fail but the code below remain valid. >
diff --git a/bfd/elfnn-aarch64.c b/bfd/elfnn-aarch64.c index 7cce6f4..1467f5d 100644 --- a/bfd/elfnn-aarch64.c +++ b/bfd/elfnn-aarch64.c @@ -6824,7 +6824,34 @@ elfNN_aarch64_finish_dynamic_symbol (bfd *output_bfd, + htab->root.sgot->output_offset + (h->got.offset & ~(bfd_vma) 1)); - if (info->shared && SYMBOL_REFERENCES_LOCAL (info, h)) + if (h->def_regular + && h->type == STT_GNU_IFUNC) + { + if (info->shared) + { + /* Generate R_AARCH64_GLOB_DAT. */ + goto do_glob_dat; + } + else + { + asection *plt; + + if (!h->pointer_equality_needed) + abort (); + + /* For non-shared object, we can't use .got.plt, which + contains the real function addres if we need pointer + equality. We load the GOT entry with the PLT entry. */ + plt = htab->root.splt ? htab->root.splt : htab->root.iplt; + bfd_put_NN (output_bfd, (plt->output_section->vma + + plt->output_offset + + h->plt.offset), + htab->root.sgot->contents + + (h->got.offset & ~(bfd_vma) 1)); + return TRUE; + } + } + else if (info->shared && SYMBOL_REFERENCES_LOCAL (info, h)) { if (!h->def_regular) return FALSE; @@ -6838,6 +6865,7 @@ elfNN_aarch64_finish_dynamic_symbol (bfd *output_bfd, else { BFD_ASSERT ((h->got.offset & 1) == 0); +do_glob_dat: bfd_put_NN (output_bfd, (bfd_vma) 0, htab->root.sgot->contents + h->got.offset); rela.r_info = ELFNN_R_INFO (h->dynindx, AARCH64_R (GLOB_DAT)); diff --git a/ld/testsuite/ld-aarch64/aarch64-elf.exp b/ld/testsuite/ld-aarch64/aarch64-elf.exp index a6b3ea2..692bf34 100644 --- a/ld/testsuite/ld-aarch64/aarch64-elf.exp +++ b/ld/testsuite/ld-aarch64/aarch64-elf.exp @@ -156,3 +156,4 @@ run_dump_test "ifunc-19a" run_dump_test "ifunc-19b" run_dump_test "ifunc-20" run_dump_test "ifunc-21" +run_dump_test "ifunc-22" diff --git a/ld/testsuite/ld-aarch64/ifunc-22.d b/ld/testsuite/ld-aarch64/ifunc-22.d new file mode 100644 index 0000000..f28b039 --- /dev/null +++ b/ld/testsuite/ld-aarch64/ifunc-22.d @@ -0,0 +1,11 @@ +#source: ifunc-22.s +#objdump: -s -j .got +#ld: -static +#target: aarch64*-*-* + +# Ensure GOT is populated correctly in static link + +.*: file format elf64-(little|big)aarch64 + +Contents of section \.got: + 4100f0 00000000 00000000 d0004000 00000000 ..........@..... diff --git a/ld/testsuite/ld-aarch64/ifunc-22.s b/ld/testsuite/ld-aarch64/ifunc-22.s new file mode 100644 index 0000000..69a87bb --- /dev/null +++ b/ld/testsuite/ld-aarch64/ifunc-22.s @@ -0,0 +1,14 @@ + .text + .type ifunc, @gnu_indirect_function + .global ifunc +ifunc: + ret + .size ifunc, .-ifunc + .type _start, @function + .globl _start +_start: + adrp x0, :got:ifunc + ldr x0, [x0, #:got_lo12:ifunc] + .size _start, .-_start + .data + .xword ifunc