Re: bpf: handling non BPF register names in inline assembly with -target bpf

[Yonghong Song <yhs@fb.com>]

On 4/11/18 11:39 AM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Apr 11, 2018 at 09:37:46AM -0700, Yonghong Song escreveu:
>> Hi, Arnaldo,
>   
>> When I studied the bpf compilation issue with latest linus/net-next
>> kernel (https://patchwork.kernel.org/patch/10333829/), an alternative
>> approach I tried is to use __BPF__ macro.
> 
> You mean you used an alternative approach that does _not_ use the
> __BPF__ macro, right? I looked at the patch and yeah, looks sane as

Right.

> well, since the kernel build process already defines that
> CC_HAVE_ASM_GOTO, checking if gcc has that feature, etc.
> 
>> The following patch introduced "#ifndef __BPF__" in
>> arch/x86/include/asm/asm.h for some inline assembly related to x86
>> "esp" register name.
>   
> 
>> ==========
>>   commit ca26cffa4e4aaeb09bb9e308f95c7835cb149248
>> Author: Arnaldo Carvalho de Melo <acme@redhat.com>
>> Date:   Mon Dec 4 13:08:47 2017 -0300
>>
>>      x86/asm: Allow again using asm.h when building for the 'bpf'
>> clang target
>>
>>      Up to f5caf621ee35 ("x86/asm: Fix inline asm call constraints
>> for Clang")
>>      we were able to use x86 headers to build to the 'bpf' clang target, as
>>      done by the BPF code in tools/perf/.
>>   ...
>> diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
>> index 219faae..386a690 100644
>> --- a/arch/x86/include/asm/asm.h
>> +++ b/arch/x86/include/asm/asm.h
>> @@ -136,6 +136,7 @@
>>   #endif
>>
>>   #ifndef __ASSEMBLY__
>> +#ifndef __BPF__
>>   /*
>>    * This output constraint should be used for any inline asm which
>> has a "call"
>>    * instruction.  Otherwise the asm may be inserted before the frame
>> pointer
>> @@ -145,5 +146,6 @@
>>   register unsigned long current_stack_pointer asm(_ASM_SP);
>>   #define ASM_CALL_CONSTRAINT "+r" (current_stack_pointer)
>>   #endif
>> +#endif
>> ...
>> ==========
>>
>> I just landed a clang patch (clang 7.0.0 trunk)
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__reviews.llvm.org_rL329823&d=DwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=1D1Hmeg3QWPTsbr6rIHgNpk8Fiqk1jkAvrluQQsjk2Y&s=P5EaOqBvaqpDpx3di0Qm1W2fMKaooJJ7b6IkiGD6NU8&e=
>> which will permit bpf clang target to accept ANY register
>> names. In this case, the inline assembly will be accepted by clang
>> and will be thrown away since variable current_stack_pointer is
>> not used in bpf programs.
> 
> Ok, then that ifndef __BPF__ above will not be needed anymore, but only
> people with clang > that version will be able to build tools/perf/

Yes.

I have a suggestion, not sure whether it works for you or not.
The whole issue you needs to introduce this __BPF__ is because
you included ptrace.h with "clang -target bpf ...". Typically,
when people include ptrace.h, they use "clang <native_arch> ..."
and "llc -march=bpf ..." since ptrace.h and its dependent header
files added a lot of assembly codes and asm code related constructs or 
maybe other arch-spec #define as well, which bpf target cannot handle.

Look at test bpf-script-test-kbuild.c, I think you can drop
uapi/asm/ptrace.h from include file list. This way, you do not need
__BPF__ in x86/include/asm/asm.h. At the same time, you can
remove __BPF__ as well.

The clang compiler change I had is just a bonus. It intends to
help "clang -target bpf ..." users just in case their bpf program
header files contains some host-specific insn asm codes with host-arch
register names, for which I expect most users won't hit this.

>   
>> If the inline assembly is indeed for BPF program, later llc
>> AsmParser will do syntax and semantics checking again.
>>
>> With the above clang patch, the above "#ifndef __BPF__" can be removed.
>> You can decide when is the appropriate time to use latest clang compiler
>> and remove the above "#ifndef __BPF__".
> 
> So are you proposing that we have something similar to that
> CC_HAVE_ASM_GOTO check in the kernel main Makefile, to define something
> like CC_HAVE_ASM_REGS (or some better name), i.e. something like:
> 
> # check for 'asm(_ASM_SP)'
> ifeq ($(call shell-cached,$(CONFIG_SHELL) $(srctree)/scripts/cc-asm-regs.sh
> $(CC) $(KBUILD_CFLAGS)), y)
>    CC_HAVE_ASM_REGS := 1
>    KBUILD_CFLAGS += -DCC_HAVE_ASM_REGS
>    KBUILD_AFLAGS += -DCC_HAVE_ASM_REGS
> endif

No, I am not suggesting a kernel change like this.
If you can change bpf-script-test-kbuild.c file and get rid of __BPF__,
it will be good. Otherwise, we can leave __BPF__ there for a while
until later clang 7.0 becomes mainstream and then we can remove it.
I just want to you to be aware that there is a clang feature to solve
your problem.

Thanks!

Yonghong

> ?
> 
> - Arnaldo
>