On Wed, 23 Aug 2023 at 04:09, Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
>
>
>
> On 8/22/23 3:07 PM, Jose E. Marchesi wrote:
> >
> >> On Wed, 9 Aug 2023 at 17:11, Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> wrote:
> >>>
> >>> [...]
> >>>
> >>> Known issues
> >>> ------------
> >>>
> >>> * Just asm volatile ("call bpf_throw" :::) does not emit DATASEC .ksyms
> >>> for bpf_throw, there needs to be explicit call in C for clang to emit
> >>> the DATASEC info in BTF, leading to errors during compilation.
> >>>
> >>
> >> Hi Yonghong, I'd like to ask you about this issue to figure out
> >> whether this is something worth fixing in clang or not.
> >> It pops up in programs which only use bpf_assert macros (which emit
> >> the call to bpf_throw using inline assembly) and not bpf_throw kfunc
> >> directly.
> >>
> >> I believe in case we emit a call bpf_throw instruction, the BPF
> >> backend code will not see any DWARF debug info for the respective
> >> symbol, so it will also not be able to convert it and emit anything to
> >> .BTF section in case no direct call without asm volatile is present.
> >> Therefore my guess is that this isn't something that can be fixed in
> >> clang/LLVM.
> >
> > Besides, please keep in mind that GCC doens't have an integrated
> > assembler, and therefore relying on clang's understanding on the
> > instructions in inline assembly is something to avoid.
> >
> >> There are also options like the one below to work around it.
> >> if ((volatile int){0}) bpf_throw();
> >> asm volatile ("call bpf_throw");
> >
> > I can confirm the above results in a BTF entry for bpf_throw with
> > bpf-unknown-none-gcc -gbtf.
>
> Kumar, you are correct.
> For clang, symbols inside 'asm volatile' statement or generally
> inside any asm code (e.g., kernel .s files) won't generate an entry
> in dwarf. The
> if ((volatile int){0}) bpf_throw();
> will force a dwarf, hence btf, entry.
>
> The unfortunately thing is the above code will generate redundant code
> like
> 0000000000000000 <foo>:
> 0: b7 01 00 00 00 00 00 00 r1 = 0x0
> 1: 63 1a fc ff 00 00 00 00 *(u32 *)(r10 - 0x4) = r1
> 2: 61 a1 fc ff 00 00 00 00 r1 = *(u32 *)(r10 - 0x4)
> 3: 15 01 01 00 00 00 00 00 if r1 == 0x0 goto +0x1 <LBB0_2>
> 4: 85 10 00 00 ff ff ff ff call -0x1
>
> 0000000000000028 <LBB0_2>:
> 5: 85 10 00 00 ff ff ff ff call -0x1
> 6: b7 00 00 00 00 00 00 00 r0 = 0x0
> 7: 95 00 00 00 00 00 00 00 exit
>
Yes, I am relying on the verifier to eliminate dead code later, but it
is obviously a hack.
> I am curious why in bpf_assert macro bpf_throw() kfunc cannot
> be used?
The reason was to force the compiler to emit a specific branch for the
assertion check without being influenced by compiler optimizations,
and tying comparison to the register holding a value being tested in
assertion. Secondly we also enforce that the first argument is in a
register and the second a constant, so as to apply the verifier bounds
gained after comparison op to the original register.
I am aware (though correct me if this is wrong) that the compiler can
select a different register for the input operand of the asm
constraint, but find_equal_id_scalars should still do correct
propagation. This is partly why I disabled usage of this macro for
variable widths < 64-bit, because then the compiler sometimes performs
shifts etc. for integer promotion implicitly, sometimes destroying
whatever information we gained through an assertion check. Depending
on the signedness of the variable, we emit the signed/unsigned
comparison.
I suppose we could switch to the ' if (!(LHS <op> RHS)) bpf_throw(); '
sequence in C, force volatile load for LHS and __builtin_constant_p
for RHS to get the same behavior. Emitting these redundant checks is
definitely a bit weird just to emit BTF.
