Re: [PATCH] selftests/bpf: improve test coverage for kfunc call

[Vineet Gupta <vineet.gupta@linux.dev>]

On 3/11/26 11:03 AM, Hari Bathini wrote:
>
> On 11/03/26 9:32 pm, Alexei Starovoitov wrote:
>> On Wed, Mar 11, 2026 at 8:10 AM Hari Bathini <hbathini@linux.ibm.com> 
>> wrote:
>>>
>>>>
>>>>> +
>>>>> +       /* Check zero-extension */
>>>>> +       if (val != (unsigned long)a)
>>>>> +               return 1;
>>>>> +       /* Check no sign-extension */
>>>>> +       if (val < 0)
>>>>> +               return 2;
>>>>> +
>>>>> +       val = b;
>>>>> +       if (val != (unsigned long)b)
>>>>> +               return 3;
>>>>> +       if (val < 0)
>>>>> +               return 4;
>>>>> +
>>>>> +       val = c;
>>>>> +       if (val != (unsigned long)c)
>>>>> +               return 5;
>>>>> +       if (val < 0)
>>>>> +               return 6;
>>>>> +
>>>>> +       return 0;
>>>>> +}
>>>>
>>>> Overall this looks very useful.
>>>> I would expand with another test where a,b,c are s8,s16,s32.
>>>
>>> Slightly different approach but kfunc_call_test4/bpf_kfunc_call_test4
>>> cover signed arguments already?
>>
>> Ahh. Then may be tweak it to adopt similar fine grained
>> error reporting as your bpf_kfunc_call_test5()
>
> I Prefer this.
>
> Does the below change to bpf_kfunc_call_test4 look fine:
>
> diff --git a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c 
> b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
> index 48dcaf93bb9f..6237c2222633 100644
> --- a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
> @@ -760,8 +760,30 @@ __bpf_kfunc struct sock 
> *bpf_kfunc_call_test3(struct sock *sk)
>
>  __bpf_kfunc long noinline bpf_kfunc_call_test4(signed char a, short 
> b, int c, long d)

I might regret for bringing this up as it could be yet another ABI 
fiasco between gcc and llvm.

As per C standard, sign of unadorned char (i.e. w/o explicit signed or 
unsigned prefix) is ABI defined.
For gcc-bpf char is specified to be signed.
So test4 has s8, while new test5 has u8. Would it make sense to have an 
additional test without signed/unsigned annotation for char ?
This will flag any discrepancy between the two compilers.

Thx,
-Vineet

>  {
> +       /*
> +        * Make val as volatile to avoid compiler optimizations.
> +        * Verify that negative signed values remain negative after
> +        * sign-extension (JIT must sign-extend, not zero-extend).
> +        */
> +       volatile long val;
> +
> +       /* val will be positive, if JIT does zero-extension instead of 
> sign-extension */
> +       val = a;
> +       if (val >= 0)
> +               return 1;
> +
> +       val = b;
> +       if (val >= 0)
> +               return 2;
> +
> +       val = c;
> +       if (val >= 0)
> +               return 3;
> +
>         /* Provoke the compiler to assume that the caller has 
> sign-extended a,
>          * b and c on platforms where this is required (e.g. s390x).
> +        *
> +        * Original behavior: return sum for backward compatibility
>          */
>         return (long)a + (long)b + (long)c + d;
>  }
>
>
> - Hari