Re: libbpf error: unknown register name 'r0' in asm

[Daniel Borkmann <daniel@iogearbox.net>]

On 10/9/20 9:33 PM, Yaniv Agman wrote:
> ‫בתאריך יום ו׳, 9 באוק׳ 2020 ב-22:08 מאת ‪Yonghong Song‬‏ <‪yhs@fb.com‬‏>:‬
>> On 10/9/20 11:59 AM, Andrii Nakryiko wrote:
>>> On Fri, Oct 9, 2020 at 11:41 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>>> On 10/9/20 8:35 PM, Andrii Nakryiko wrote:
>>>>> On Fri, Oct 9, 2020 at 11:21 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>>>>> On 10/9/20 8:09 PM, Yaniv Agman wrote:
>>>>>>> ‫בתאריך יום ו׳, 9 באוק׳ 2020 ב-20:39 מאת ‪Daniel Borkmann‬‏
>>>>>>> <‪daniel@iogearbox.net‬‏>:‬
>>>>>>>>
>>>>>>>> On 10/9/20 6:56 PM, Yaniv Agman wrote:
>>>>>>>>> ‫בתאריך יום ו׳, 9 באוק׳ 2020 ב-19:27 מאת ‪Daniel Borkmann‬‏
>>>>>>>>> <‪daniel@iogearbox.net‬‏>:‬
>>>>>>>>>>
>>>>>>>>>> [ Cc +Yonghong ]
>>>>>>>>>>
>>>>>>>>>> On 10/9/20 6:05 PM, Yaniv Agman wrote:
>>>>>>>>>>> Pulling the latest changes of libbpf and compiling my application with it,
>>>>>>>>>>> I see the following error:
>>>>>>>>>>>
>>>>>>>>>>> ../libbpf/src//root/usr/include/bpf/bpf_helpers.h:99:10: error:
>>>>>>>>>>> unknown register name 'r0' in asm
>>>>>>>>>>>                            : "r0", "r1", "r2", "r3", "r4", "r5");
>>>>>>>>>>>
>>>>>>>>>>> The commit which introduced this change is:
>>>>>>>>>>> 80c7838600d39891f274e2f7508b95a75e4227c1
>>>>>>>>>>>
>>>>>>>>>>> I'm not sure if I'm doing something wrong (missing include?), or this
>>>>>>>>>>> is a genuine error
>>>>>>>>>>
>>>>>>>>>> Seems like your clang/llvm version might be too old.
>>>>>>>>>
>>>>>>>>> I'm using clang 10.0.1
>>>>>>>>
>>>>>>>> Ah, okay, I see. Would this diff do the trick for you?
>>>>>>>
>>>>>>> Yes! Now it compiles without any problems!
>>>>>>
>>>>>> Great, thx, I'll cook proper fix and check with clang6 as Yonghong mentioned.
>>>>>
>>>>> Am I the only one confused here?... Yonghong said it should be
>>>>> supported as early as clang 6, Yaniv is using Clang 10 and is still
>>>>> getting this error. Let's figure out what's the problem before adding
>>>>> unnecessary checks.
>>>>>
>>>>> I think it's not the clang_major check that helped, rather __bpf__
>>>>> check. So please hold off on the fix, let's get to the bottom of this
>>>>> first.
>>>>
>>>> I don't see confusion here (maybe other than which minimal clang/llvm version
>>>> libbpf should support). If we do `#if __clang_major__ >= 6 && defined(__bpf__)`
>>>> for the final patch, then this means that user passed clang -target bpf and
>>>> the min supported version for inline assembly was there, otherwise we fall back
>>>> to bpf_tail_call. In Yaniv's case, he probably had native target with -emit-llvm
>>>> and then used llc invocation.
>>>
>>> The "-emit-llvm" was the part that we were missing and had to figure
>>> it out, before we could discuss the fix.
>>
>> Maybe Yaniv can confirm. I think the following properly happens.
>>      - clang10 -O2 -g -S -emit-llvm t.c  // This is native compilation
>> becasue some header files. Maybe some thing is guarded with x86 specific
>> config's which is not available to -target bpf. This is mostly for
>> tracing programs and Yanic mentions pt_regs which should be related
>> to tracing.
>>      - llc -march=bpf t.ll
> 
> Yes, like I said,  I do use --emit-llvm, and indeed have a tracing program
> 
>> So guarding the function with __bpf__ should be the one fixing this issue.
>>
>> guard with clang version >=6 should not hurt and may prevent
>> compilation failures if people use < 6 llvm with clang -target bpf.
>> I think most people should already use newer llvm, but who knows.

Yeah that was my thinking for those stuck for whatever reason on old LLVM.

>>>>>>>> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
>>>>>>>> index 2bdb7d6dbad2..31e356831fcf 100644
>>>>>>>> --- a/tools/lib/bpf/bpf_helpers.h
>>>>>>>> +++ b/tools/lib/bpf/bpf_helpers.h
>>>>>>>> @@ -72,6 +72,7 @@
>>>>>>>>       /*
>>>>>>>>        * Helper function to perform a tail call with a constant/immediate map slot.
>>>>>>>>        */
>>>>>>>> +#if __clang_major__ >= 10 && defined(__bpf__)
>>>>>>>>       static __always_inline void
>>>>>>>>       bpf_tail_call_static(void *ctx, const void *map, const __u32 slot)
>>>>>>>>       {
>>>>>>>> @@ -98,6 +99,9 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot)
>>>>>>>>                          :: [ctx]"r"(ctx), [map]"r"(map), [slot]"i"(slot)
>>>>>>>>                          : "r0", "r1", "r2", "r3", "r4", "r5");
>>>>>>>>       }
>>>>>>>> +#else
>>>>>>>> +# define bpf_tail_call_static  bpf_tail_call
>>>
>>> bpf_tail_call_static has very specific guarantees, so in cases where
>>> we can't use inline assembly to satisfy those guarantees, I think we
>>> should not just silently redefine bpf_tail_call_static as
>>> bpf_tail_call, rather make compilation fail if someone is attempting
>>> to use bpf_tail_call_static. _Static_assert could be used to provide a
>>> better error message here, probably.

Makes sense as well, I was mainly thinking if people include header files in
their project which are shared between tracing & non-tracing, so they compile
just fine, but I can see the point that wrt very specific guarantees, fully
agree. In that sense we should just have it defined with the clang + __bpf__
constraints mentioned earlier.

Thanks,
Daniel