בתאריך יום ד׳, 21 באוק׳ 2020 ב-20:18 מאת Daniel Borkmann
<daniel@xxxxxxxxxxxxx>:
>
> On 10/21/20 11:43 AM, Yaniv Agman wrote:
> > בתאריך יום ו׳, 9 באוק׳ 2020 ב-22:58 מאת Daniel Borkmann
> > <daniel@xxxxxxxxxxxxx>:
> >> On 10/9/20 9:33 PM, Yaniv Agman wrote:
> >>> בתאריך יום ו׳, 9 באוק׳ 2020 ב-22:08 מאת Yonghong Song <yhs@xxxxxx>:
> >>>> On 10/9/20 11:59 AM, Andrii Nakryiko wrote:
> >>>>> On Fri, Oct 9, 2020 at 11:41 AM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
> >>>>>> On 10/9/20 8:35 PM, Andrii Nakryiko wrote:
> >>>>>>> On Fri, Oct 9, 2020 at 11:21 AM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
> >>>>>>>> On 10/9/20 8:09 PM, Yaniv Agman wrote:
> >>>>>>>>> בתאריך יום ו׳, 9 באוק׳ 2020 ב-20:39 מאת Daniel Borkmann
> >>>>>>>>> <daniel@xxxxxxxxxxxxx>:
> >>>>>>>>>>
> >>>>>>>>>> On 10/9/20 6:56 PM, Yaniv Agman wrote:
> >>>>>>>>>>> בתאריך יום ו׳, 9 באוק׳ 2020 ב-19:27 מאת Daniel Borkmann
> >>>>>>>>>>> <daniel@xxxxxxxxxxxxx>:
> >>>>>>>>>>>>
> >>>>>>>>>>>> [ 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.
> >
> > Is this change going to happen?
> > I'm still having a compilation error when using master branch
>
> Yeah, I'll submit something tonight.
>
> Thanks,
> Daniel
Great, Thanks!
Yaniv
