[PATCH] RISC-V: Support GNU indirect functions.

[Fangrui Song]

On 2020-07-21, Nelson Chu wrote:
>Hi MaskRay,
>
>On Wed, Jul 15, 2020 at 12:44 PM Fangrui Song <i@maskray.me> wrote:
>> .text
>> .globl func
>> .type func, @gnu_indirect_function
>> func:
>>    ret
>>
>> .globl _start
>> _start:
>> .L:
>>    auipc a0, %pcrel_hi(func)
>>    addi a0, a0, %pcrel_lo(.L)
>>
>> % ld.lld -pie a.64.o -o a.64  # silent
>> % ld-new -pie a.64.o -o a.64
>> ....: warning: GNU indirect functions with DT_TEXTREL may result in a segfault at runtime; recompile with -fPIE
>
>The warning and redundant R_RISCV_NOP can be fixed by the following
>change, but haven't run all tests.
>
>diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
>index ca0dd11..73f44d8 100644
>--- a/bfd/elfnn-riscv.c
>+++ b/bfd/elfnn-riscv.c
>@@ -808,9 +808,10 @@ riscv_elf_check_relocs (bfd *abfd, struct
>bfd_link_info *info,
>
>          if ((bfd_link_pic (info)
>               && (sec->flags & SEC_ALLOC) != 0
>               && ((r != NULL && ! r->pc_relative)
>                   || (h != NULL
>-                      && (! info->symbolic
>+                      && (! (bfd_link_pie (info)
>+                             || info->symbolic)
>                           || h->root.type == bfd_link_hash_defweak
>                           || !h->def_regular))))
>              || (!bfd_link_pic (info)
>
>
>> This test checks a non-GOT-non-PLT relocation to a STT_GNU_IFUNC.
>> Such relocations can be used to take the address of the indirect function.
>> To make sure pointer equality holds (i.e. taking the address from another module will get the same address),
>> LLD will create a PLT (called "canonical PLT" among a few linker people),
>> change func's type to STT_FUNC, and redirect all references to the PLT.
>>
>>       4: 0000000000001270     0 FUNC    GLOBAL DEFAULT    7 func
>>
>> This patch does not change func's type. If you link with ld-new -E -pie a.64.o -o a.64,
>> func will be exported. Taking the address of func from another module will break pointer equality.
>>
>>      20: 00000000000003f0     0 IFUNC   GLOBAL DEFAULT    9 func
>
>Consider Vincent Chen's ifunc support in GLIBC,
>https://sourceware.org/pipermail/libc-alpha/2020-July/116059.html
>
>I'm not sure why we need to change the symbol type from STT_IFUNC to
>STT_FUNC, and then update the symbol value to the corresponding plt
>entry.  If other modules want to call `func`, it will use
>R_RISCV_JUMP_SLOT with the undef STT_FUNC `func`.  Glibc will know the
>`func` is actually an STT_IFUNC according to the executable (probably
>need --export-dynamic) or shared library which defines it, and then
>handle the case just like what R_RISCV_IRELATIVE does.  If we want to
>call the `ifunc`, and the `ifunc` is defined in the same
>executable/shared library, then linker will generate the
>R_RISCV_IRELATIVE for the call, it should be worked with or without
>changing the STT_IFUNC to STT_FUNC.  So are there any cases that would
>fail since we don't change the STT_IFUNC to STT_FUNC, and redirect it
>to the plt entry?

STT_FUNC is required for references within the module. If an ifunc is
referended by a local PC-relative relocation, the intention may be to
take the address. The address should be equal to other potentially
address taking relocation types. I don't think there is a way keeping
STT_IFUNC if no dynamic relocation is desired.

Compilers can produce a PC-relative relocation if it does not know the
target function is an ifunc (marked via inline asm, instead of
__attribute__((ifunc(...)))).

>I would be happy to do the changes, even if the GNU toolchain (gcc +
>binutils + glibc) seems OK for now.  But this wouldn't be the first
>time I was wrong, so please feel free to correct me.
>
>
>> Regarding tests, it'd be nice to check whether this example works
>> https://reviews.llvm.org/rG45acc35ac21323bafaf5d4367df10ebc4eed35f4
>>
>>    // gcc -fno-pie -no-pie a.c
>>    // gcc -fPIE -pie a.c
>>    #include <stdio.h>
>>    static void impl(void) { puts("meow"); }
>>    void thefunc(void) __attribute__((ifunc("resolver")));
>>    void *resolver(void) { return &impl; }
>>    int main(void) {
>>      thefunc();
>>      void (*theptr)(void) = &thefunc;
>>      theptr();
>>    }
>
>I assume both `thefunc` and `theptr` should print "meow".  If the
>result is expected, then our patches (Vincent Chen's glibc patch and
>my binutils patch) should work.

Great!