[PATCH] D153898: [DebugInfo] Enable debug info emission for extern variables in C++

[Yuze Chi via Phabricator via cfe-commits]

chiyuze added a comment.

Thank you for accepting this patch!

In D153898#4457432 <https://reviews.llvm.org/D153898#4457432>, @MaskRay wrote:

> I wonder whether you have ready-to-use instructions to test this for folks who are not familiar with Linux kernel/eBPF. (I happened to start to read eBPF one week ago, but I guess it would take some time for me to be more familiar with it, even if I contributed some stuff back in 2020)



In D153898#4457436 <https://reviews.llvm.org/D153898#4457436>, @MaskRay wrote:

>> This patch is required so that we can still use kconfig in such BPF programs compiled from C++.
>
> Assuming that you mean https://www.kernel.org/doc/html/next/kbuild/kconfig-language.html#kconfig-language , how is Kconfig (a DSL used in the build system)  relevant here?

The kconfig DSL allows extern variables to be filled in by libbpf before being loaded into the kernel. https://nakryiko.com/posts/bpf-core-reference-guide/#kconfig-extern-variables has more detailed explanations. In prod, we use this as a mechanism to flag-protect BPF features. For example, if we have `test.c` with the following content:

  // helpers
  #define __section(x) __attribute__((section(x)))
  #define __kconfig __attribute__((section(".kconfig")))
  #define __weak __attribute__((weak))
  // end of helpers
  
  extern const int CONFIG_ENABLE_FOO __kconfig __weak;
  __section("tc")
  int test(void* ctx) {
    if (CONFIG_ENABLE_FOO > 1000) {
      return 1;
    }
    return 0;
  }

libbpf will fill in 0 as CONFIG_ENABLE_FOO by default, and allow us to override it with other values using the kconfig DSL. However, without this patch, such BPF programs are only functional if compiled as C:

`clang -g -O2 -target bpf -c test.c -o test.c.o; sudo bpftool prog load test.c.o /sys/fs/bpf/test_c` returns OK.

If we compile the same source code as C++, it won't load:

`clang -g -O2 -target bpf -x c++ -c test.c -o test.cc.o; sudo bpftool prog load test.cc.o /sys/fs/bpf/test_cc` gives

  libbpf: failed to find BTF for extern 'CONFIG_ENABLE_FOO': -2
  Error: failed to open object file

The reason is that the generated object is missing BTF for the extern variable:

`bpftool btf dump file test.c.o` gives

  [1] PTR '(anon)' type_id=0
  [2] FUNC_PROTO '(anon)' ret_type_id=3 vlen=1
          'ctx' type_id=1
  [3] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
  [4] FUNC 'test' type_id=2 linkage=global
  [5] CONST '(anon)' type_id=3
  [6] VAR 'CONFIG_ENABLE_FOO' type_id=5, linkage=extern
  [7] DATASEC '.kconfig' size=0 vlen=1
          type_id=6 offset=0 size=4 (VAR 'CONFIG_ENABLE_FOO')

whereas `bpftool btf dump file test.cc.o` gives

  [1] PTR '(anon)' type_id=0
  [2] FUNC_PROTO '(anon)' ret_type_id=3 vlen=1
          'ctx' type_id=1
  [3] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
  [4] FUNC 'test' type_id=2 linkage=global



In D153898#4457436 <https://reviews.llvm.org/D153898#4457436>, @MaskRay wrote:

> Clang BPF supports `-mcpu=v[123]`. v1 is for a very old kernel. https://pchaigno.github.io/bpf/2021/10/20/ebpf-instruction-sets.html 
> I think there are some users so we cannot remove the support. Is it happy with the new behavior?

Yes. This patch does not change existing behaviors, including backend behaviors, upon which users already depend, since it only changes debug info for BPF compiled in C++. BPF & C++ is a combination that was never officially supported (and does not fully work).

In D153898#4457810 <https://reviews.llvm.org/D153898#4457810>, @dblaikie wrote:

> (though I'd still ask a bit about the issue of declaration V definition - is BPF just always "standalone" debug info generally? (is there a problem with the definition coming from another translation unit with debug info?)?)

I'm not aware of such problems, but my experience may be limited to the use cases owned by our team. AFAIK, clang always compiles BPF as individual translation units. "Linking" is implemented by libbpf (https://lwn.net/Articles/848997/), which is not used in BPF programs owned by our team today (those BPF programs predate libbpf's linking support). Our plan is to migrate existing use cases of BPF to C++ first, before trying to support more general use cases (e.g. libbpf linking). This patch is our first clang patch towards compiling BPF from C++; we'll send more if we found them necessary in the future :)

In D153898#4460475 <https://reviews.llvm.org/D153898#4460475>, @yonghong-song wrote:

> I am okay with this change. Once you used this patch and implemented the mechanism inside Google. Could you send a follow-up summary on what the implementation looks like in Google for the thread:
>
>   https://lore.kernel.org/bpf/CAKH8qBt4xqBUpXefqPk5AyU1Rr0-h-vCJzS_0Bu-987gL4wi4A@mail.gmail.com/
>
> This will give people a sense why this is useful/better than other alternatives (like macro based approach).

Sure thing, will do!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153898/new/

https://reviews.llvm.org/D153898