[PATCH v3] RISC-V: Fix IFUNC resolver cannot access gp pointer

[Palmer Dabbelt]

On Tue, 07 Jan 2025 09:00:10 PST (-0800), jrtc27@jrtc27.com wrote:
> On 7 Jan 2025, at 16:38, Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * Palmer Dabbelt:
>>
>>> On Mon, 06 Jan 2025 18:32:54 PST (-0800), cyy@cyyself.name wrote:
>>>> Ping again. It has already been 1 month after v3 patch was submitted.
>>>>
>>>> I think this patch is critical for RISC-V GCC Function multi-versioning
>>>> to work correctly, as GCC 15, which has this feature, is going to be
>>>> released.
>>>
>>> IIUC the conclusion here was to avoid global symbol resolution in
>>> IFUNCs and instead use the pre-resolved function hwprobe VDSO function
>>> pointer passed in via a register.  There was a long discussion on this
>>> during the hwprobe/IFUNC support patches, and IIRC one during the FMV
>>> patches -- though I remember accepting those and now I'm not sure why,
>>> so maybe I'm just forgetting something here?
>>
>> As far as I understand it, at least in some cases, the gp register needs
>> to be set up for the main program even for local static/hidden symbol
>> access.  Unfortunately, the ABI does not provide a completely reliable
>> way for the dynamic linker to discover the gp value the main program
>> requires, which leads to the code we have now.  I think we should have
>> something that is more declarative than running a piece of assembler
>> code at the right time (the existing code runs too late, after IFUNC
>> resolver), and which does not depend on dynamic symbol resolution.
>
> And, notably, source code that makes no use of global variables can
> still end up implicitly using them, e.g. for switch statements, which
> you could imagine being used for hwprobe-using code in the case of keys
> that have enum values, like misaligned scalar perf. Both GCC and Clang
> optimise to using a lookup table for switch statements over an enum
> with that many cases, which means a global access that could become
> GP-relative with the right (wrong?) linker and memory layout. See
> https://godbolt.org/z/KEbof5z7h for a minimal example of that, which is
> completely valid and sensible (modulo bugs from lack of testing) code
> that you can write today with hwprobe and that is broken if the lookup
> table is laid out by GNU ld to be close enough to __global_pointer$.

Here's the code, in case the godbolt short link goes away:

    typedef unsigned long size_t;
    #define NULL (void *)0
    
    struct riscv_hwprobe {
        signed long long int key;
        unsigned long long int value;
    };
    
    typedef int (*__riscv_hwprobe_t) (struct riscv_hwprobe *__pairs, size_t __pair_count,
    				  size_t __cpu_count, unsigned long int *__cpus,
    				  unsigned int __flags);
    
    #define RISCV_HWPROBE_KEY_MISALIGNED_SCALAR_PERF	9
    #define		RISCV_HWPROBE_MISALIGNED_SCALAR_UNKNOWN		0
    #define		RISCV_HWPROBE_MISALIGNED_SCALAR_EMULATED	1
    #define		RISCV_HWPROBE_MISALIGNED_SCALAR_SLOW		2
    #define		RISCV_HWPROBE_MISALIGNED_SCALAR_FAST		3
    #define		RISCV_HWPROBE_MISALIGNED_SCALAR_UNSUPPORTED	4
    
    #define __used __attribute__((used))
    #define __unused __attribute__((unused))
    
    #define	DEFINE_UIFUNC(qual, ret_type, name, args)			\
        static ret_type (*name##_resolver(unsigned long, __riscv_hwprobe_t,	\
    	unsigned long, unsigned long, unsigned long, unsigned long,	\
    	unsigned long, unsigned long))args __used;			\
        qual ret_type name args __attribute__((ifunc(#name "_resolver")));	\
        static ret_type (*name##_resolver(unsigned long hwcap __unused,	\
    	__riscv_hwprobe_t hwprobe __unused, unsigned long _arg3 __unused,	\
    	unsigned long _arg4 __unused, unsigned long _arg5 __unused,	\
    	unsigned long _arg6 __unused, unsigned long _arg7 __unused,	\
    	unsigned long _arg8 __unused))args
    
    void foo_unknown(void) {}
    void foo_emulated(void) {}
    void foo_slow(void) {}
    void foo_fast(void) {}
    void foo_unsupported(void) {}
    
    DEFINE_UIFUNC(, void, foo, (void))
    {
        struct riscv_hwprobe pair;
    
        pair.key = RISCV_HWPROBE_KEY_MISALIGNED_SCALAR_PERF;
        if (hwprobe(&pair, 1, 0, NULL, 0) != 0)
            goto unknown;
        
        switch (pair.value) {
        unknown:
        default:
        case RISCV_HWPROBE_MISALIGNED_SCALAR_UNKNOWN:
            return foo_unknown; 
        case RISCV_HWPROBE_MISALIGNED_SCALAR_EMULATED:
            return foo_emulated;
        case RISCV_HWPROBE_MISALIGNED_SCALAR_SLOW:
            return foo_slow;
        case RISCV_HWPROBE_MISALIGNED_SCALAR_FAST:
            return foo_fast;
        case RISCV_HWPROBE_MISALIGNED_SCALAR_UNSUPPORTED:
            return foo_unsupported;
        }
    }

At least in GCC the docs for `-fjump-table` say to turn it off if you're in
code that can't handle getting symbols references added, so if the rules are to
avoid resolving symbols in IFUNC resolvers then that's just user error (though
kind of a clunky one).  So I think GP is really just a red herring, here --
sure it's one way this can break, but we went through months of other reasons
this can break when we decided not to allow them.

That said, I think there's an even bigger issue here: if we're expecting users
to return function pointers from the IFUNC resolver, then how are they supposed
to get those function pointers pre-relocation?  Even if it was easy to get the
compiler to only use PC-relative references, those would still just be wrong.

So unless I'm just lost here, I think we've got something else wrong?

> Jess