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

[Yonghong Song]

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@xxxxxxxxxx>
> > 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