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 <a...@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

Reply via email to