Question regarding "Add testcases for tailcall hierarchy fixing"

Question regarding "Add testcases for tailcall hierarchy fixing"

[Cupertino Miranda]

Hi Leon,

In the following commit:

commit b83b936f3e9a3c63896852198a1814e90e68eef5
Author: Leon Hwang <hffilwlqm@gmail.com>

    selftests/bpf: Add testcases for tailcall hierarchy fixing

you created 2 tests files that contain the following inline assembly.

    asm volatile (""::"r+"(ret));

I presume the actual intent is to force the unused ret variable to
exist as a register.

When compiling that line in GCC it produces the following error:

progs/tailcall_bpf2bpf_hierarchy2.c: In function 'tailcall_bpf2bpf_hierarchy_2':
progs/tailcall_bpf2bpf_hierarchy2.c:66:9: error: input operand constraint contains '+'
   66 |         asm volatile (""::"r+"(ret));
      |         ^~~

After analysing the reasoning behind the error, the plausible solution
is to change the constraint to "+r" and move it from the input operands
list to output operands, i.e:

      asm volatile ("":"+r"(ret));

Can you please confirm that this change would be complient with the test
semantics ?

Regards,
Cupertino

Re: Question regarding "Add testcases for tailcall hierarchy fixing"

[Leon Hwang]

On 2/8/24 02:41, Cupertino Miranda wrote:
> 
> Hi Leon,
> 
> In the following commit:
> 
> commit b83b936f3e9a3c63896852198a1814e90e68eef5
> Author: Leon Hwang <hffilwlqm@gmail.com>
> 
>     selftests/bpf: Add testcases for tailcall hierarchy fixing
> 
> you created 2 tests files that contain the following inline assembly.
> 
>     asm volatile (""::"r+"(ret));
> 
> I presume the actual intent is to force the unused ret variable to
> exist as a register.
> 
> When compiling that line in GCC it produces the following error:
> 
> progs/tailcall_bpf2bpf_hierarchy2.c: In function 'tailcall_bpf2bpf_hierarchy_2':
> progs/tailcall_bpf2bpf_hierarchy2.c:66:9: error: input operand constraint contains '+'
>    66 |         asm volatile (""::"r+"(ret));
>       |         ^~~
> 
> After analysing the reasoning behind the error, the plausible solution
> is to change the constraint to "+r" and move it from the input operands
> list to output operands, i.e:
> 
>       asm volatile ("":"+r"(ret));
> 
> Can you please confirm that this change would be complient with the test
> semantics ?

Hi Cupertino,

The purpose of this "asm volatile" is to prevent that compiler optimizes
the return value of the function by returning 0 directly and maybe
eliminating bpf_tail_call_static().

Therefore, it's better to use "__sink()" defined in "bpf_misc.h":

/* make it look to compiler like value is read and written */
#define __sink(expr) asm volatile("" : "+g"(expr))

Here's the diff:

diff --git
a/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_hierarchy2.c
b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_hierarchy2.c
index 37604b0b97af..72fd0d577506 100644
--- a/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_hierarchy2.c
+++ b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_hierarchy2.c
@@ -58,12 +58,12 @@ __retval(33)
 SEC("tc")
 int tailcall_bpf2bpf_hierarchy_2(struct __sk_buff *skb)
 {
-       volatile int ret = 0;
+       int ret = 0;

        subprog_tail0(skb);
        subprog_tail1(skb);

-       asm volatile (""::"r+"(ret));
+       __sink(ret);
        return (count1 << 16) | count0;
 }

diff --git
a/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_hierarchy3.c
b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_hierarchy3.c
index 0cdbb781fcbc..a7fb91cb05b7 100644
--- a/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_hierarchy3.c
+++ b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_hierarchy3.c
@@ -51,11 +51,11 @@ __retval(33)
 SEC("tc")
 int tailcall_bpf2bpf_hierarchy_3(struct __sk_buff *skb)
 {
-       volatile int ret = 0;
+       int ret = 0;

        bpf_tail_call_static(skb, &jmp_table0, 0);

-       asm volatile (""::"r+"(ret));
+       __sink(ret);
        return ret;
 }

Thanks,
Leon

> 
> Regards,
> Cupertino