[RFC] RISC-V: Go PLT for CALL/JUMP/RVC_JUMP if `h->plt.offset' isn't -1
Checks
Commit Message
I got an request about the undefined behaviors, considering the following case,
$ cat test.c
void main ()
{
foo();
}
$ cat lib.h
void foo(void);
$ riscv64-unknown-linux-gnu-gcc test.c
riscv64-unknown-linux-gnu/bin/ld: /tmp/ccRO8fJl.o: in function `main':
test.c:(.text+0x8): undefined reference to `foo'
collect2: error: ld returned 1 exit status
$ riscv64-unknown-linux-gnu-gcc test.c -Wl,--unresolved-symbols=ignore-in-object-files
$ qemu-riscv64 a.out
Segmentation fault (core dumped)
Testing with x86 and aarch64, they won't get the segfault since they go plt
for the undefined foo symbol. So, after applying this patch, I can get the
following too,
$ qemu-riscv64 a.out
a.out: symbol lookup error: a.out: undefined symbol: foo
The change of this patch should only affect the call behavior, which refer
to an undefined (weak) symbol, when building an dynamic executable. I think
the pic/pie behavior won't be affected as usual. I am not sure if the change
will cause trouble or not for other projects, so please feels free to cc people
that you think they will be affected, thanks.
---
bfd/elfnn-riscv.c | 84 +++++++++++++++++++++++++----------------------
1 file changed, 44 insertions(+), 40 deletions(-)
Comments
On Sat, 08 Feb 2025 00:33:37 PST (-0800), Nelson Chu wrote:
> I got an request about the undefined behaviors, considering the following case,
>
> $ cat test.c
> void main ()
> {
> foo();
> }
> $ cat lib.h
> void foo(void);
> $ riscv64-unknown-linux-gnu-gcc test.c
> riscv64-unknown-linux-gnu/bin/ld: /tmp/ccRO8fJl.o: in function `main':
> test.c:(.text+0x8): undefined reference to `foo'
> collect2: error: ld returned 1 exit status
> $ riscv64-unknown-linux-gnu-gcc test.c -Wl,--unresolved-symbols=ignore-in-object-files
> $ qemu-riscv64 a.out
> Segmentation fault (core dumped)
>
> Testing with x86 and aarch64, they won't get the segfault since they go plt
> for the undefined foo symbol. So, after applying this patch, I can get the
> following too,
>
> $ qemu-riscv64 a.out
> a.out: symbol lookup error: a.out: undefined symbol: foo
>
> The change of this patch should only affect the call behavior, which refer
> to an undefined (weak) symbol, when building an dynamic executable. I think
> the pic/pie behavior won't be affected as usual. I am not sure if the change
> will cause trouble or not for other projects, so please feels free to cc people
> that you think they will be affected, thanks.
Thanks for doing this. For some more context, there's a handful of Go
plugins that seem to want `-Wl,--export-dynamic
-Wl,--unresolved-symbols=ignore-in-object-files` to result in
executables that have PLT-indirect calls to these undefined symbols,
which they'll then later LD_PRELOAD or dlopen() to resolve. Here's an
example
<https://github.com/NVIDIA/nvidia-container-toolkit/blob/b19f5d8f7d76f8ec05534aadfc0c3641bd281d55/internal/dxcore/dxcore.go#L20>.
IMO that's pretty far into the realm of undefined behavior, and from
poking around a bit it looks like that's the case even on x86 --
basically if my symbol gets linked before something has triggered a PLT
to be created, then I end up with a direct reference that isn't
dynamically resolvable.
It also looks like arm64 generates NOPs (rather than calls to absolute
0) on these undefined symbols, so it's possible some instances of these
are just crashing lazily. THere might be some context floating around
in 7cd2917227 ("aarch64: Return an error on conditional branch to an
undefined symbol"), it's a bit hard to follow so I'm not sure if that's
an intentional side-effect or just the easiest arbitrary thing to
generate for this flavor of undefined behavior.
So I'm kind of split on what we should do here: in general I like to
have undefined behavior crash eagerly, as otherwise we're just making
these issues harder to debug. That said, this has blown up internally
and making it crash lazily will make the fire go out, and it'd be really
nice to start a Monday morning with more fires going out than
starting...
Maybe there's some more context floating around in someone's brain about
this?
> ---
> bfd/elfnn-riscv.c | 84 +++++++++++++++++++++++++----------------------
> 1 file changed, 44 insertions(+), 40 deletions(-)
>
> diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
> index 57ced95fdb3..bca3a585f56 100644
> --- a/bfd/elfnn-riscv.c
> +++ b/bfd/elfnn-riscv.c
> @@ -2263,6 +2263,7 @@ riscv_elf_relocate_section (bfd *output_bfd,
> reloc_howto_type *howto = riscv_elf_rtype_to_howto (input_bfd, r_type);
> const char *msg = NULL;
> bool resolved_to_zero;
> + bool via_plt = false;
>
> if (howto == NULL)
> continue;
> @@ -2565,6 +2566,12 @@ riscv_elf_relocate_section (bfd *output_bfd,
> resolved_to_zero = (h != NULL
> && UNDEFWEAK_NO_DYNAMIC_RELOC (info, h));
>
> + /* Refer to the PLT entry. This check has to match the check in
> + _bfd_riscv_relax_section. */
> + via_plt = (htab->elf.splt != NULL
> + && h != NULL
> + && h->plt.offset != MINUS_ONE);
> +
> switch (r_type)
> {
> case R_RISCV_NONE:
> @@ -2776,8 +2783,7 @@ riscv_elf_relocate_section (bfd *output_bfd,
> case R_RISCV_CALL_PLT:
> /* Handle a call to an undefined weak function. This won't be
> relaxed, so we have to handle it here. */
> - if (h != NULL && h->root.type == bfd_link_hash_undefweak
> - && (!bfd_link_pic (info) || h->plt.offset == MINUS_ONE))
> + if (h->root.type == bfd_link_hash_undefweak && !via_plt)
> {
> /* We can use x0 as the base register. */
> bfd_vma insn = bfd_getl32 (contents + rel->r_offset + 4);
> @@ -2791,42 +2797,40 @@ riscv_elf_relocate_section (bfd *output_bfd,
>
> case R_RISCV_JAL:
> case R_RISCV_RVC_JUMP:
> - if (bfd_link_pic (info) && h != NULL)
> + if (via_plt)
> {
> - if (h->plt.offset != MINUS_ONE)
> - {
> - /* Refer to the PLT entry. This check has to match the
> - check in _bfd_riscv_relax_section. */
> - relocation = sec_addr (htab->elf.splt) + h->plt.offset;
> - unresolved_reloc = false;
> - }
> - else if (!SYMBOL_REFERENCES_LOCAL (info, h)
> - && (input_section->flags & SEC_ALLOC) != 0
> - && (input_section->flags & SEC_READONLY) != 0
> - && ELF_ST_VISIBILITY (h->other) == STV_DEFAULT)
> - {
> - /* PR 28509, when generating the shared object, these
> - referenced symbols may bind externally, which means
> - they will be exported to the dynamic symbol table,
> - and are preemptible by default. These symbols cannot
> - be referenced by the non-pic relocations, like
> - R_RISCV_JAL and R_RISCV_RVC_JUMP relocations.
> -
> - However, consider that linker may relax the R_RISCV_CALL
> - relocations to R_RISCV_JAL or R_RISCV_RVC_JUMP, if
> - these relocations are relocated to the plt entries,
> - then we won't report error for them.
> -
> - Perhaps we also need the similar checks for the
> - R_RISCV_BRANCH and R_RISCV_RVC_BRANCH relocations. */
> - msg = bfd_asprintf (_("%%X%%P: relocation %s against `%s'"
> - " which may bind externally"
> - " can not be used"
> - " when making a shared object;"
> - " recompile with -fPIC\n"),
> - howto->name, h->root.root.string);
> - r = bfd_reloc_notsupported;
> - }
> + relocation = sec_addr (htab->elf.splt) + h->plt.offset;
> + unresolved_reloc = false;
> + }
> + else if (bfd_link_pic (info)
> + && h != NULL
> + && h->plt.offset == MINUS_ONE
> + && !SYMBOL_REFERENCES_LOCAL (info, h)
> + && (input_section->flags & SEC_ALLOC) != 0
> + && (input_section->flags & SEC_READONLY) != 0
> + && ELF_ST_VISIBILITY (h->other) == STV_DEFAULT)
> + {
> + /* PR 28509, when generating the shared object, these
> + referenced symbols may bind externally, which means
> + they will be exported to the dynamic symbol table,
> + and are preemptible by default. These symbols cannot
> + be referenced by the non-pic relocations, like
> + R_RISCV_JAL and R_RISCV_RVC_JUMP relocations.
> +
> + However, consider that linker may relax the R_RISCV_CALL
> + relocations to R_RISCV_JAL or R_RISCV_RVC_JUMP, if
> + these relocations are relocated to the plt entries,
> + then we won't report error for them.
> +
> + Perhaps we also need the similar checks for the
> + R_RISCV_BRANCH and R_RISCV_RVC_BRANCH relocations. */
> + msg = bfd_asprintf (_("%%X%%P: relocation %s against `%s'"
> + " which may bind externally"
> + " can not be used"
> + " when making a shared object;"
> + " recompile with -fPIC\n"),
> + howto->name, h->root.root.string);
> + r = bfd_reloc_notsupported;
> }
> break;
>
> @@ -5365,9 +5369,9 @@ _bfd_riscv_relax_section (bfd *abfd, asection *sec,
> undefined_weak = true;
> }
>
> - /* This line has to match the check in riscv_elf_relocate_section
> - in the R_RISCV_CALL[_PLT] case. */
> - if (bfd_link_pic (info) && h->plt.offset != MINUS_ONE)
> + /* This line has to match the via_pltcheck in
> + riscv_elf_relocate_section in the R_RISCV_CALL[_PLT] case. */
> + if (h->plt.offset != MINUS_ONE)
> {
> sym_sec = htab->elf.splt;
> symval = h->plt.offset;
Looks like we could give it a try and see if it works and won't affect
current projects. I will commit it before this weekend if there are no
objections.
Thanks
Nelson
On Tue, Feb 11, 2025 at 1:53 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
> On Sat, 08 Feb 2025 00:33:37 PST (-0800), Nelson Chu wrote:
> > I got an request about the undefined behaviors, considering the
> following case,
> >
> > $ cat test.c
> > void main ()
> > {
> > foo();
> > }
> > $ cat lib.h
> > void foo(void);
> > $ riscv64-unknown-linux-gnu-gcc test.c
> > riscv64-unknown-linux-gnu/bin/ld: /tmp/ccRO8fJl.o: in function `main':
> > test.c:(.text+0x8): undefined reference to `foo'
> > collect2: error: ld returned 1 exit status
> > $ riscv64-unknown-linux-gnu-gcc test.c
> -Wl,--unresolved-symbols=ignore-in-object-files
> > $ qemu-riscv64 a.out
> > Segmentation fault (core dumped)
> >
> > Testing with x86 and aarch64, they won't get the segfault since they go
> plt
> > for the undefined foo symbol. So, after applying this patch, I can get
> the
> > following too,
> >
> > $ qemu-riscv64 a.out
> > a.out: symbol lookup error: a.out: undefined symbol: foo
> >
> > The change of this patch should only affect the call behavior, which
> refer
> > to an undefined (weak) symbol, when building an dynamic executable. I
> think
> > the pic/pie behavior won't be affected as usual. I am not sure if the
> change
> > will cause trouble or not for other projects, so please feels free to cc
> people
> > that you think they will be affected, thanks.
>
> Thanks for doing this. For some more context, there's a handful of Go
> plugins that seem to want `-Wl,--export-dynamic
> -Wl,--unresolved-symbols=ignore-in-object-files` to result in
> executables that have PLT-indirect calls to these undefined symbols,
> which they'll then later LD_PRELOAD or dlopen() to resolve. Here's an
> example
> <
> https://github.com/NVIDIA/nvidia-container-toolkit/blob/b19f5d8f7d76f8ec05534aadfc0c3641bd281d55/internal/dxcore/dxcore.go#L20
> >.
>
> IMO that's pretty far into the realm of undefined behavior, and from
> poking around a bit it looks like that's the case even on x86 --
> basically if my symbol gets linked before something has triggered a PLT
> to be created, then I end up with a direct reference that isn't
> dynamically resolvable.
>
> It also looks like arm64 generates NOPs (rather than calls to absolute
> 0) on these undefined symbols, so it's possible some instances of these
> are just crashing lazily. THere might be some context floating around
> in 7cd2917227 ("aarch64: Return an error on conditional branch to an
> undefined symbol"), it's a bit hard to follow so I'm not sure if that's
> an intentional side-effect or just the easiest arbitrary thing to
> generate for this flavor of undefined behavior.
>
> So I'm kind of split on what we should do here: in general I like to
> have undefined behavior crash eagerly, as otherwise we're just making
> these issues harder to debug. That said, this has blown up internally
> and making it crash lazily will make the fire go out, and it'd be really
> nice to start a Monday morning with more fires going out than
> starting...
>
> Maybe there's some more context floating around in someone's brain about
> this?
>
> > ---
> > bfd/elfnn-riscv.c | 84 +++++++++++++++++++++++++----------------------
> > 1 file changed, 44 insertions(+), 40 deletions(-)
> >
> > diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
> > index 57ced95fdb3..bca3a585f56 100644
> > --- a/bfd/elfnn-riscv.c
> > +++ b/bfd/elfnn-riscv.c
> > @@ -2263,6 +2263,7 @@ riscv_elf_relocate_section (bfd *output_bfd,
> > reloc_howto_type *howto = riscv_elf_rtype_to_howto (input_bfd,
> r_type);
> > const char *msg = NULL;
> > bool resolved_to_zero;
> > + bool via_plt = false;
> >
> > if (howto == NULL)
> > continue;
> > @@ -2565,6 +2566,12 @@ riscv_elf_relocate_section (bfd *output_bfd,
> > resolved_to_zero = (h != NULL
> > && UNDEFWEAK_NO_DYNAMIC_RELOC (info, h));
> >
> > + /* Refer to the PLT entry. This check has to match the check in
> > + _bfd_riscv_relax_section. */
> > + via_plt = (htab->elf.splt != NULL
> > + && h != NULL
> > + && h->plt.offset != MINUS_ONE);
> > +
> > switch (r_type)
> > {
> > case R_RISCV_NONE:
> > @@ -2776,8 +2783,7 @@ riscv_elf_relocate_section (bfd *output_bfd,
> > case R_RISCV_CALL_PLT:
> > /* Handle a call to an undefined weak function. This won't be
> > relaxed, so we have to handle it here. */
> > - if (h != NULL && h->root.type == bfd_link_hash_undefweak
> > - && (!bfd_link_pic (info) || h->plt.offset == MINUS_ONE))
> > + if (h->root.type == bfd_link_hash_undefweak && !via_plt)
> > {
> > /* We can use x0 as the base register. */
> > bfd_vma insn = bfd_getl32 (contents + rel->r_offset + 4);
> > @@ -2791,42 +2797,40 @@ riscv_elf_relocate_section (bfd *output_bfd,
> >
> > case R_RISCV_JAL:
> > case R_RISCV_RVC_JUMP:
> > - if (bfd_link_pic (info) && h != NULL)
> > + if (via_plt)
> > {
> > - if (h->plt.offset != MINUS_ONE)
> > - {
> > - /* Refer to the PLT entry. This check has to match the
> > - check in _bfd_riscv_relax_section. */
> > - relocation = sec_addr (htab->elf.splt) + h->plt.offset;
> > - unresolved_reloc = false;
> > - }
> > - else if (!SYMBOL_REFERENCES_LOCAL (info, h)
> > - && (input_section->flags & SEC_ALLOC) != 0
> > - && (input_section->flags & SEC_READONLY) != 0
> > - && ELF_ST_VISIBILITY (h->other) == STV_DEFAULT)
> > - {
> > - /* PR 28509, when generating the shared object, these
> > - referenced symbols may bind externally, which means
> > - they will be exported to the dynamic symbol table,
> > - and are preemptible by default. These symbols cannot
> > - be referenced by the non-pic relocations, like
> > - R_RISCV_JAL and R_RISCV_RVC_JUMP relocations.
> > -
> > - However, consider that linker may relax the
> R_RISCV_CALL
> > - relocations to R_RISCV_JAL or R_RISCV_RVC_JUMP, if
> > - these relocations are relocated to the plt entries,
> > - then we won't report error for them.
> > -
> > - Perhaps we also need the similar checks for the
> > - R_RISCV_BRANCH and R_RISCV_RVC_BRANCH relocations. */
> > - msg = bfd_asprintf (_("%%X%%P: relocation %s against
> `%s'"
> > - " which may bind externally"
> > - " can not be used"
> > - " when making a shared object;"
> > - " recompile with -fPIC\n"),
> > - howto->name, h->root.root.string);
> > - r = bfd_reloc_notsupported;
> > - }
> > + relocation = sec_addr (htab->elf.splt) + h->plt.offset;
> > + unresolved_reloc = false;
> > + }
> > + else if (bfd_link_pic (info)
> > + && h != NULL
> > + && h->plt.offset == MINUS_ONE
> > + && !SYMBOL_REFERENCES_LOCAL (info, h)
> > + && (input_section->flags & SEC_ALLOC) != 0
> > + && (input_section->flags & SEC_READONLY) != 0
> > + && ELF_ST_VISIBILITY (h->other) == STV_DEFAULT)
> > + {
> > + /* PR 28509, when generating the shared object, these
> > + referenced symbols may bind externally, which means
> > + they will be exported to the dynamic symbol table,
> > + and are preemptible by default. These symbols cannot
> > + be referenced by the non-pic relocations, like
> > + R_RISCV_JAL and R_RISCV_RVC_JUMP relocations.
> > +
> > + However, consider that linker may relax the R_RISCV_CALL
> > + relocations to R_RISCV_JAL or R_RISCV_RVC_JUMP, if
> > + these relocations are relocated to the plt entries,
> > + then we won't report error for them.
> > +
> > + Perhaps we also need the similar checks for the
> > + R_RISCV_BRANCH and R_RISCV_RVC_BRANCH relocations. */
> > + msg = bfd_asprintf (_("%%X%%P: relocation %s against `%s'"
> > + " which may bind externally"
> > + " can not be used"
> > + " when making a shared object;"
> > + " recompile with -fPIC\n"),
> > + howto->name, h->root.root.string);
> > + r = bfd_reloc_notsupported;
> > }
> > break;
> >
> > @@ -5365,9 +5369,9 @@ _bfd_riscv_relax_section (bfd *abfd, asection *sec,
> > undefined_weak = true;
> > }
> >
> > - /* This line has to match the check in riscv_elf_relocate_section
> > - in the R_RISCV_CALL[_PLT] case. */
> > - if (bfd_link_pic (info) && h->plt.offset != MINUS_ONE)
> > + /* This line has to match the via_pltcheck in
> > + riscv_elf_relocate_section in the R_RISCV_CALL[_PLT] case. */
> > + if (h->plt.offset != MINUS_ONE)
> > {
> > sym_sec = htab->elf.splt;
> > symval = h->plt.offset;
>
Committed, thanks.
Nelson
On Mon, Mar 3, 2025 at 11:51 AM Nelson Chu <nelson@rivosinc.com> wrote:
> Looks like we could give it a try and see if it works and won't affect
> current projects. I will commit it before this weekend if there are no
> objections.
>
> Thanks
> Nelson
>
> On Tue, Feb 11, 2025 at 1:53 AM Palmer Dabbelt <palmer@rivosinc.com>
> wrote:
>
>> On Sat, 08 Feb 2025 00:33:37 PST (-0800), Nelson Chu wrote:
>> > I got an request about the undefined behaviors, considering the
>> following case,
>> >
>> > $ cat test.c
>> > void main ()
>> > {
>> > foo();
>> > }
>> > $ cat lib.h
>> > void foo(void);
>> > $ riscv64-unknown-linux-gnu-gcc test.c
>> > riscv64-unknown-linux-gnu/bin/ld: /tmp/ccRO8fJl.o: in function `main':
>> > test.c:(.text+0x8): undefined reference to `foo'
>> > collect2: error: ld returned 1 exit status
>> > $ riscv64-unknown-linux-gnu-gcc test.c
>> -Wl,--unresolved-symbols=ignore-in-object-files
>> > $ qemu-riscv64 a.out
>> > Segmentation fault (core dumped)
>> >
>> > Testing with x86 and aarch64, they won't get the segfault since they go
>> plt
>> > for the undefined foo symbol. So, after applying this patch, I can get
>> the
>> > following too,
>> >
>> > $ qemu-riscv64 a.out
>> > a.out: symbol lookup error: a.out: undefined symbol: foo
>> >
>> > The change of this patch should only affect the call behavior, which
>> refer
>> > to an undefined (weak) symbol, when building an dynamic executable. I
>> think
>> > the pic/pie behavior won't be affected as usual. I am not sure if the
>> change
>> > will cause trouble or not for other projects, so please feels free to
>> cc people
>> > that you think they will be affected, thanks.
>>
>> Thanks for doing this. For some more context, there's a handful of Go
>> plugins that seem to want `-Wl,--export-dynamic
>> -Wl,--unresolved-symbols=ignore-in-object-files` to result in
>> executables that have PLT-indirect calls to these undefined symbols,
>> which they'll then later LD_PRELOAD or dlopen() to resolve. Here's an
>> example
>> <
>> https://github.com/NVIDIA/nvidia-container-toolkit/blob/b19f5d8f7d76f8ec05534aadfc0c3641bd281d55/internal/dxcore/dxcore.go#L20
>> >.
>>
>> IMO that's pretty far into the realm of undefined behavior, and from
>> poking around a bit it looks like that's the case even on x86 --
>> basically if my symbol gets linked before something has triggered a PLT
>> to be created, then I end up with a direct reference that isn't
>> dynamically resolvable.
>>
>> It also looks like arm64 generates NOPs (rather than calls to absolute
>> 0) on these undefined symbols, so it's possible some instances of these
>> are just crashing lazily. THere might be some context floating around
>> in 7cd2917227 ("aarch64: Return an error on conditional branch to an
>> undefined symbol"), it's a bit hard to follow so I'm not sure if that's
>> an intentional side-effect or just the easiest arbitrary thing to
>> generate for this flavor of undefined behavior.
>>
>> So I'm kind of split on what we should do here: in general I like to
>> have undefined behavior crash eagerly, as otherwise we're just making
>> these issues harder to debug. That said, this has blown up internally
>> and making it crash lazily will make the fire go out, and it'd be really
>> nice to start a Monday morning with more fires going out than
>> starting...
>>
>> Maybe there's some more context floating around in someone's brain about
>> this?
>>
>> > ---
>> > bfd/elfnn-riscv.c | 84 +++++++++++++++++++++++++----------------------
>> > 1 file changed, 44 insertions(+), 40 deletions(-)
>>
>
On Thu, Mar 6, 2025 at 7:05 PM Nelson Chu <nelson@rivosinc.com> wrote:
>
> Committed, thanks.
>
> Nelson
>
> On Mon, Mar 3, 2025 at 11:51 AM Nelson Chu <nelson@rivosinc.com> wrote:
>>
>> Looks like we could give it a try and see if it works and won't affect current projects. I will commit it before this weekend if there are no objections.
>>
>> Thanks
>> Nelson
>>
>> On Tue, Feb 11, 2025 at 1:53 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>>>
>>> On Sat, 08 Feb 2025 00:33:37 PST (-0800), Nelson Chu wrote:
>>> > I got an request about the undefined behaviors, considering the following case,
>>> >
>>> > $ cat test.c
>>> > void main ()
>>> > {
>>> > foo();
>>> > }
>>> > $ cat lib.h
>>> > void foo(void);
>>> > $ riscv64-unknown-linux-gnu-gcc test.c
>>> > riscv64-unknown-linux-gnu/bin/ld: /tmp/ccRO8fJl.o: in function `main':
>>> > test.c:(.text+0x8): undefined reference to `foo'
>>> > collect2: error: ld returned 1 exit status
>>> > $ riscv64-unknown-linux-gnu-gcc test.c -Wl,--unresolved-symbols=ignore-in-object-files
>>> > $ qemu-riscv64 a.out
>>> > Segmentation fault (core dumped)
>>> >
>>> > Testing with x86 and aarch64, they won't get the segfault since they go plt
>>> > for the undefined foo symbol. So, after applying this patch, I can get the
>>> > following too,
>>> >
>>> > $ qemu-riscv64 a.out
>>> > a.out: symbol lookup error: a.out: undefined symbol: foo
>>> >
>>> > The change of this patch should only affect the call behavior, which refer
>>> > to an undefined (weak) symbol, when building an dynamic executable. I think
>>> > the pic/pie behavior won't be affected as usual. I am not sure if the change
>>> > will cause trouble or not for other projects, so please feels free to cc people
>>> > that you think they will be affected, thanks.
>>>
>>> Thanks for doing this. For some more context, there's a handful of Go
>>> plugins that seem to want `-Wl,--export-dynamic
>>> -Wl,--unresolved-symbols=ignore-in-object-files` to result in
>>> executables that have PLT-indirect calls to these undefined symbols,
>>> which they'll then later LD_PRELOAD or dlopen() to resolve. Here's an
>>> example
>>> <https://github.com/NVIDIA/nvidia-container-toolkit/blob/b19f5d8f7d76f8ec05534aadfc0c3641bd281d55/internal/dxcore/dxcore.go#L20>.
>>>
>>> IMO that's pretty far into the realm of undefined behavior, and from
>>> poking around a bit it looks like that's the case even on x86 --
>>> basically if my symbol gets linked before something has triggered a PLT
>>> to be created, then I end up with a direct reference that isn't
>>> dynamically resolvable.
>>>
>>> It also looks like arm64 generates NOPs (rather than calls to absolute
>>> 0) on these undefined symbols, so it's possible some instances of these
>>> are just crashing lazily. THere might be some context floating around
>>> in 7cd2917227 ("aarch64: Return an error on conditional branch to an
>>> undefined symbol"), it's a bit hard to follow so I'm not sure if that's
>>> an intentional side-effect or just the easiest arbitrary thing to
>>> generate for this flavor of undefined behavior.
>>>
>>> So I'm kind of split on what we should do here: in general I like to
>>> have undefined behavior crash eagerly, as otherwise we're just making
>>> these issues harder to debug. That said, this has blown up internally
>>> and making it crash lazily will make the fire go out, and it'd be really
>>> nice to start a Monday morning with more fires going out than
>>> starting...
>>>
>>> Maybe there's some more context floating around in someone's brain about
>>> this?
>>>
>>> > ---
>>> > bfd/elfnn-riscv.c | 84 +++++++++++++++++++++++++----------------------
>>> > 1 file changed, 44 insertions(+), 40 deletions(-)
I am intrigued by the problem but I have trouble understanding the
description. What behavior does this patch change?
> riscv64-unknown-linux-gnu-gcc test.c -Wl,--unresolved-symbols=ignore-in-object-files
There is no -nostdlib, therefore I assume that this command creates a
dynamically linked executable (both -fpie -pie and -fno-pie -no-pie
are possible, depending on how users configure the GCC).
I've tested a few cases but do not observe a behavior difference (in
terms of whether `foo` is in .dynsym and whether there is a JUMP_SLOT
relocation).
ld-new --unresolved-symbols=ignore-in-object-files -o a a.o b.so
ld-new --unresolved-symbols=ignore-in-object-files -o a a.o
(lld has a quite simple model where undefined non-weak and undefined
weak symbols are handled in a unified way.
A symbol is preemptible if:
* -shared or at least one input file is DSO, and
* the symbol is undefined or exported (to .dynsym due to
--export-dynamic/--dynamic-list/referenced by DSO/etc), and
* other conditions that the symbol is preemptible
Then, a preemptible symbol might need a PLT and associated JUMP_SLOT
relocation.)
)
On Mon, Mar 10, 2025 at 10:04 AM Fangrui Song <i@maskray.me> wrote:
> I am intrigued by the problem but I have trouble understanding the
> description. What behavior does this patch change?
>
> > riscv64-unknown-linux-gnu-gcc test.c
> -Wl,--unresolved-symbols=ignore-in-object-files
>
> There is no -nostdlib, therefore I assume that this command creates a
> dynamically linked executable (both -fpie -pie and -fno-pie -no-pie
> are possible, depending on how users configure the GCC).
>
Oops, that's true. I think the original problem is for -no-pie.
> I've tested a few cases but do not observe a behavior difference (in
> terms of whether `foo` is in .dynsym and whether there is a JUMP_SLOT
> relocation).
>
> ld-new --unresolved-symbols=ignore-in-object-files -o a a.o b.so
> ld-new --unresolved-symbols=ignore-in-object-files -o a a.o
>
% cat a.s
.text
.weak c
.globl _start
_start:
call a
call b
call c
% cat b.s
.globl b
b:
nop
% riscv64-unknown-linux-gnu-as a.s -o a.o
% riscv64-unknown-linux-gnu-as b.s -o b.o
% riscv64-unknown-linux-gnu-ld -shared b.o -o b.so
% riscv64-unknown-linux-gnu-ld -unresolved-symbols=ignore-in-object-files
a.o b.o
% riscv64-unknown-linux-gnu-objdump -d a.out
...
Disassembly of section .text:
00000000000100b0 <_start>:
100b0: ffff0097 auipc ra,0xffff0
100b4: f50080e7 jalr -176(ra) # 0 <_start-0x100b0>
100b8: 00c000ef jal 100c4 <b>
100bc: 00000097 auipc ra,0x0
100c0: 000000e7 jalr zero # 0 <_start-0x100b0>
00000000000100c4 <b>:
100c4: 00000013 nop
% riscv64-unknown-linux-gnu-ld -unresolved-symbols=ignore-in-object-files
a.o b.so
% riscv64-unknown-linux-gnu-objdump -d a.out
...
Disassembly of section .plt:
...
0000000000010330 <b@plt>:
10330: 00002e17 auipc t3,0x2
10334: cd0e3e03 ld t3,-816(t3) # 12000 <b>
10338: 000e0367 jalr t1,t3
1033c: 00000013 nop
0000000000010340 <c@plt>:
10340: 00002e17 auipc t3,0x2
10344: cc8e3e03 ld t3,-824(t3) # 12008 <c>
10348: 000e0367 jalr t1,t3
1034c: 00000013 nop
0000000000010350 <a@plt>:
10350: 00002e17 auipc t3,0x2
10354: cc0e3e03 ld t3,-832(t3) # 12010 <a>
10358: 000e0367 jalr t1,t3
1035c: 00000013 nop
Disassembly of section .text:
0000000000010360 <_start>:
10360: ff1ff0ef jal 10350 <a@plt>
10364: fcdff0ef jal 10330 <b@plt>
10368: fd9ff0ef jal 10340 <c@plt>
But before this patch, all undefined weak and non-weak symbols will be
forced to zero,
% riscv64-unknown-linux-gnu-ld-old
-unresolved-symbols=ignore-in-object-files a.o b.so
% riscv64-unknown-linux-gnu-objdump -d a.out
...
Disassembly of section .plt:
...
0000000000010330 <b@plt>:
10330: 00002e17 auipc t3,0x2
10334: cd0e3e03 ld t3,-816(t3) # 12000 <b>
10338: 000e0367 jalr t1,t3
1033c: 00000013 nop
0000000000010340 <c@plt>:
10340: 00002e17 auipc t3,0x2
10344: cc8e3e03 ld t3,-824(t3) # 12008 <c>
10348: 000e0367 jalr t1,t3
1034c: 00000013 nop
0000000000010350 <a@plt>:
10350: 00002e17 auipc t3,0x2
10354: cc0e3e03 ld t3,-832(t3) # 12010 <a>
10358: 000e0367 jalr t1,t3
1035c: 00000013 nop
Disassembly of section .text:
0000000000010360 <_start>:
10360: ffff0097 auipc ra,0xffff0
10364: ca0080e7 jalr -864(ra) # 0
<_PROCEDURE_LINKAGE_TABLE_-0x10310>
10368: fc9ff0ef jal 10330 <b@plt>
1036c: 00000097 auipc ra,0x0
10370: 000000e7 jalr zero # 0
<_PROCEDURE_LINKAGE_TABLE_-0x10310>
(lld has a quite simple model where undefined non-weak and undefined
> weak symbols are handled in a unified way.
> A symbol is preemptible if:
>
> * -shared or at least one input file is DSO, and
> * the symbol is undefined or exported (to .dynsym due to
> --export-dynamic/--dynamic-list/referenced by DSO/etc), and
> * other conditions that the symbol is preemptible
>
> Then, a preemptible symbol might need a PLT and associated JUMP_SLOT
> relocation.)
>
Yeah I think now the behavior is more similar to lld. Well, except the
special handling for a call to undefined weak function, according to this
page,
https://github.com/riscv-non-isa/riscv-elf-psabi-doc/issues/126
I suddenly feel like we should also apply this special handling to the
calls which jump to an undefined non-weak function...
Thanks
Nelson
On Sun, Mar 9, 2025 at 7:59 PM Nelson Chu <nelson@rivosinc.com> wrote:
>
>
>
> On Mon, Mar 10, 2025 at 10:04 AM Fangrui Song <i@maskray.me> wrote:
>>
>> I am intrigued by the problem but I have trouble understanding the
>> description. What behavior does this patch change?
>>
>> > riscv64-unknown-linux-gnu-gcc test.c -Wl,--unresolved-symbols=ignore-in-object-files
>>
>> There is no -nostdlib, therefore I assume that this command creates a
>> dynamically linked executable (both -fpie -pie and -fno-pie -no-pie
>> are possible, depending on how users configure the GCC).
>
>
> Oops, that's true. I think the original problem is for -no-pie.
>
>>
>> I've tested a few cases but do not observe a behavior difference (in
>> terms of whether `foo` is in .dynsym and whether there is a JUMP_SLOT
>> relocation).
>>
>> ld-new --unresolved-symbols=ignore-in-object-files -o a a.o b.so
>> ld-new --unresolved-symbols=ignore-in-object-files -o a a.o
>
>
> % cat a.s
> .text
> .weak c
> .globl _start
> _start:
> call a
> call b
> call c
> % cat b.s
> .globl b
> b:
> nop
> % riscv64-unknown-linux-gnu-as a.s -o a.o
> % riscv64-unknown-linux-gnu-as b.s -o b.o
> % riscv64-unknown-linux-gnu-ld -shared b.o -o b.so
> % riscv64-unknown-linux-gnu-ld -unresolved-symbols=ignore-in-object-files a.o b.o
> % riscv64-unknown-linux-gnu-objdump -d a.out
> ...
> Disassembly of section .text:
>
> 00000000000100b0 <_start>:
> 100b0: ffff0097 auipc ra,0xffff0
> 100b4: f50080e7 jalr -176(ra) # 0 <_start-0x100b0>
> 100b8: 00c000ef jal 100c4 <b>
> 100bc: 00000097 auipc ra,0x0
> 100c0: 000000e7 jalr zero # 0 <_start-0x100b0>
>
> 00000000000100c4 <b>:
> 100c4: 00000013 nop
> % riscv64-unknown-linux-gnu-ld -unresolved-symbols=ignore-in-object-files a.o b.so
> % riscv64-unknown-linux-gnu-objdump -d a.out
> ...
> Disassembly of section .plt:
> ...
> 0000000000010330 <b@plt>:
> 10330: 00002e17 auipc t3,0x2
> 10334: cd0e3e03 ld t3,-816(t3) # 12000 <b>
> 10338: 000e0367 jalr t1,t3
> 1033c: 00000013 nop
>
> 0000000000010340 <c@plt>:
> 10340: 00002e17 auipc t3,0x2
> 10344: cc8e3e03 ld t3,-824(t3) # 12008 <c>
> 10348: 000e0367 jalr t1,t3
> 1034c: 00000013 nop
>
> 0000000000010350 <a@plt>:
> 10350: 00002e17 auipc t3,0x2
> 10354: cc0e3e03 ld t3,-832(t3) # 12010 <a>
> 10358: 000e0367 jalr t1,t3
> 1035c: 00000013 nop
>
> Disassembly of section .text:
>
> 0000000000010360 <_start>:
> 10360: ff1ff0ef jal 10350 <a@plt>
> 10364: fcdff0ef jal 10330 <b@plt>
> 10368: fd9ff0ef jal 10340 <c@plt>
>
> But before this patch, all undefined weak and non-weak symbols will be forced to zero,
>
> % riscv64-unknown-linux-gnu-ld-old -unresolved-symbols=ignore-in-object-files a.o b.so
> % riscv64-unknown-linux-gnu-objdump -d a.out
> ...
> Disassembly of section .plt:
> ...
> 0000000000010330 <b@plt>:
> 10330: 00002e17 auipc t3,0x2
> 10334: cd0e3e03 ld t3,-816(t3) # 12000 <b>
> 10338: 000e0367 jalr t1,t3
> 1033c: 00000013 nop
>
> 0000000000010340 <c@plt>:
> 10340: 00002e17 auipc t3,0x2
> 10344: cc8e3e03 ld t3,-824(t3) # 12008 <c>
> 10348: 000e0367 jalr t1,t3
> 1034c: 00000013 nop
>
> 0000000000010350 <a@plt>:
> 10350: 00002e17 auipc t3,0x2
> 10354: cc0e3e03 ld t3,-832(t3) # 12010 <a>
> 10358: 000e0367 jalr t1,t3
> 1035c: 00000013 nop
>
> Disassembly of section .text:
>
> 0000000000010360 <_start>:
> 10360: ffff0097 auipc ra,0xffff0
> 10364: ca0080e7 jalr -864(ra) # 0 <_PROCEDURE_LINKAGE_TABLE_-0x10310>
> 10368: fc9ff0ef jal 10330 <b@plt>
> 1036c: 00000097 auipc ra,0x0
> 10370: 000000e7 jalr zero # 0 <_PROCEDURE_LINKAGE_TABLE_-0x10310>
>
>> (lld has a quite simple model where undefined non-weak and undefined
>> weak symbols are handled in a unified way.
>> A symbol is preemptible if:
>>
>> * -shared or at least one input file is DSO, and
>> * the symbol is undefined or exported (to .dynsym due to
>> --export-dynamic/--dynamic-list/referenced by DSO/etc), and
>> * other conditions that the symbol is preemptible
>>
>> Then, a preemptible symbol might need a PLT and associated JUMP_SLOT
>> relocation.)
Thanks for the example! I see that the branch target is now more
meaningful after the patch.
>
> Yeah I think now the behavior is more similar to lld. Well, except the special handling for a call to undefined weak function, according to this page,
> https://github.com/riscv-non-isa/riscv-elf-psabi-doc/issues/126
> I suddenly feel like we should also apply this special handling to the calls which jump to an undefined non-weak function...
>
> Thanks
> Nelson
On Mon, Mar 10, 2025 at 11:33 AM Fangrui Song <i@maskray.me> wrote:
> >> (lld has a quite simple model where undefined non-weak and undefined
> >> weak symbols are handled in a unified way.
> >> A symbol is preemptible if:
> >>
> >> * -shared or at least one input file is DSO, and
> >> * the symbol is undefined or exported (to .dynsym due to
> >> --export-dynamic/--dynamic-list/referenced by DSO/etc), and
> >> * other conditions that the symbol is preemptible
> >>
> >> Then, a preemptible symbol might need a PLT and associated JUMP_SLOT
> >> relocation.)
>
> Thanks for the example! I see that the branch target is now more
> meaningful after the patch.
>
Thanks for the detailed description of lld's behavior for reference. The
behavior described above from you is the main purpose of this patch, so
that when "at least one input file is DSO but --no-pie", undefined weak and
non-weak symbols for call will also be preemptible and go to plt. Palmer
and I were having a hard time figuring out how to describe this problem in
short... Thanks for helping us out :-)
Nelson
@@ -2263,6 +2263,7 @@ riscv_elf_relocate_section (bfd *output_bfd,
reloc_howto_type *howto = riscv_elf_rtype_to_howto (input_bfd, r_type);
const char *msg = NULL;
bool resolved_to_zero;
+ bool via_plt = false;
if (howto == NULL)
continue;
@@ -2565,6 +2566,12 @@ riscv_elf_relocate_section (bfd *output_bfd,
resolved_to_zero = (h != NULL
&& UNDEFWEAK_NO_DYNAMIC_RELOC (info, h));
+ /* Refer to the PLT entry. This check has to match the check in
+ _bfd_riscv_relax_section. */
+ via_plt = (htab->elf.splt != NULL
+ && h != NULL
+ && h->plt.offset != MINUS_ONE);
+
switch (r_type)
{
case R_RISCV_NONE:
@@ -2776,8 +2783,7 @@ riscv_elf_relocate_section (bfd *output_bfd,
case R_RISCV_CALL_PLT:
/* Handle a call to an undefined weak function. This won't be
relaxed, so we have to handle it here. */
- if (h != NULL && h->root.type == bfd_link_hash_undefweak
- && (!bfd_link_pic (info) || h->plt.offset == MINUS_ONE))
+ if (h->root.type == bfd_link_hash_undefweak && !via_plt)
{
/* We can use x0 as the base register. */
bfd_vma insn = bfd_getl32 (contents + rel->r_offset + 4);
@@ -2791,42 +2797,40 @@ riscv_elf_relocate_section (bfd *output_bfd,
case R_RISCV_JAL:
case R_RISCV_RVC_JUMP:
- if (bfd_link_pic (info) && h != NULL)
+ if (via_plt)
{
- if (h->plt.offset != MINUS_ONE)
- {
- /* Refer to the PLT entry. This check has to match the
- check in _bfd_riscv_relax_section. */
- relocation = sec_addr (htab->elf.splt) + h->plt.offset;
- unresolved_reloc = false;
- }
- else if (!SYMBOL_REFERENCES_LOCAL (info, h)
- && (input_section->flags & SEC_ALLOC) != 0
- && (input_section->flags & SEC_READONLY) != 0
- && ELF_ST_VISIBILITY (h->other) == STV_DEFAULT)
- {
- /* PR 28509, when generating the shared object, these
- referenced symbols may bind externally, which means
- they will be exported to the dynamic symbol table,
- and are preemptible by default. These symbols cannot
- be referenced by the non-pic relocations, like
- R_RISCV_JAL and R_RISCV_RVC_JUMP relocations.
-
- However, consider that linker may relax the R_RISCV_CALL
- relocations to R_RISCV_JAL or R_RISCV_RVC_JUMP, if
- these relocations are relocated to the plt entries,
- then we won't report error for them.
-
- Perhaps we also need the similar checks for the
- R_RISCV_BRANCH and R_RISCV_RVC_BRANCH relocations. */
- msg = bfd_asprintf (_("%%X%%P: relocation %s against `%s'"
- " which may bind externally"
- " can not be used"
- " when making a shared object;"
- " recompile with -fPIC\n"),
- howto->name, h->root.root.string);
- r = bfd_reloc_notsupported;
- }
+ relocation = sec_addr (htab->elf.splt) + h->plt.offset;
+ unresolved_reloc = false;
+ }
+ else if (bfd_link_pic (info)
+ && h != NULL
+ && h->plt.offset == MINUS_ONE
+ && !SYMBOL_REFERENCES_LOCAL (info, h)
+ && (input_section->flags & SEC_ALLOC) != 0
+ && (input_section->flags & SEC_READONLY) != 0
+ && ELF_ST_VISIBILITY (h->other) == STV_DEFAULT)
+ {
+ /* PR 28509, when generating the shared object, these
+ referenced symbols may bind externally, which means
+ they will be exported to the dynamic symbol table,
+ and are preemptible by default. These symbols cannot
+ be referenced by the non-pic relocations, like
+ R_RISCV_JAL and R_RISCV_RVC_JUMP relocations.
+
+ However, consider that linker may relax the R_RISCV_CALL
+ relocations to R_RISCV_JAL or R_RISCV_RVC_JUMP, if
+ these relocations are relocated to the plt entries,
+ then we won't report error for them.
+
+ Perhaps we also need the similar checks for the
+ R_RISCV_BRANCH and R_RISCV_RVC_BRANCH relocations. */
+ msg = bfd_asprintf (_("%%X%%P: relocation %s against `%s'"
+ " which may bind externally"
+ " can not be used"
+ " when making a shared object;"
+ " recompile with -fPIC\n"),
+ howto->name, h->root.root.string);
+ r = bfd_reloc_notsupported;
}
break;
@@ -5365,9 +5369,9 @@ _bfd_riscv_relax_section (bfd *abfd, asection *sec,
undefined_weak = true;
}
- /* This line has to match the check in riscv_elf_relocate_section
- in the R_RISCV_CALL[_PLT] case. */
- if (bfd_link_pic (info) && h->plt.offset != MINUS_ONE)
+ /* This line has to match the via_pltcheck in
+ riscv_elf_relocate_section in the R_RISCV_CALL[_PLT] case. */
+ if (h->plt.offset != MINUS_ONE)
{
sym_sec = htab->elf.splt;
symval = h->plt.offset;