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

[chiyuze]

Summary

Debug info emission for extern variables in C++ was previously disabled
when the functionality was added in https://reviews.llvm.org/D71818 and
originally in https://reviews.llvm.org/D70696, because there was no use
case. We are enabling it now, as we start to deploy BPF programs
compiled from C++, leveraging C++ features like templates to reduce code
complexity. This patch is required so that we can still use kconfig in
such BPF programs compiled from C++.

[rnk]

This seems reasonable to me, but I want to get a second opinion from @dblaikie and @MaskRay for some debug info and BPF perspective.

[dblaikie]

Got some details on how much this costs? (eg: bloaty comparison for a clang bootstrap with/without this patch applied?)
Could you provide a quick summary of why this debug info is required? The definition should be provided in whatever translation unit defines the variable and you should be able to get debug info from there - if that translation unit is built without -g, then maybe this feature should be gated on -fstandalone-debug which is intended to express the intent to get debug info coverage when a translation unit may be the only one built with -g?

[chiyuze]

Regarding cost, only BPF target triggers this debug info generation for extern variables. There is no impact on x86 builds of clang:

Without patch:

bloaty build/RelWithDebInfo/83d47ba15a1229a21aaca8a8d6a33e0e90aabfd4/bin/clang
    FILE SIZE        VM SIZE    
 --------------  -------------- 
  74.7%  2.98Gi   0.0%       0    .debug_info
   9.2%   377Mi   0.0%       0    .debug_loclists
   5.2%   210Mi   0.0%       0    .debug_str
   4.3%   177Mi   0.0%       0    .debug_line
   2.0%  81.5Mi  53.7%  81.5Mi    .text
   1.7%  68.0Mi   0.0%       0    .debug_rnglists
   1.1%  43.2Mi  28.5%  43.2Mi    .rodata
   0.5%  21.4Mi   0.0%       0    .debug_abbrev
   0.5%  20.3Mi   0.0%       0    .strtab
   0.2%  8.79Mi   5.8%  8.79Mi    .eh_frame
   0.2%  6.73Mi   4.4%  6.73Mi    .rela.dyn
   0.1%  4.86Mi   0.0%       0    .symtab
   0.1%  4.09Mi   2.7%  4.09Mi    .dynstr
   0.1%  3.60Mi   2.4%  3.60Mi    .data.rel.ro
   0.1%  3.30Mi   0.0%       0    .debug_aranges
   0.0%  1.19Mi   0.8%  1.19Mi    .dynsym
   0.0%  1.09Mi   0.7%  1.09Mi    .eh_frame_hdr
   0.0%       0   0.4%   611Ki    .bss
   0.0%   449Ki   0.1%   127Ki    [25 Others]
   0.0%   393Ki   0.3%   393Ki    .gnu.hash
   0.0%   332Ki   0.2%   332Ki    .data
 100.0%  3.99Gi 100.0%   151Mi    TOTAL

With patch:

bloaty build/RelWithDebInfo/23bc4427ab49716ce2c24c81529d9c90953b3c54/bin/clang
    FILE SIZE        VM SIZE    
 --------------  -------------- 
  74.7%  2.98Gi   0.0%       0    .debug_info
   9.2%   377Mi   0.0%       0    .debug_loclists
   5.2%   210Mi   0.0%       0    .debug_str
   4.3%   177Mi   0.0%       0    .debug_line
   2.0%  81.5Mi  53.7%  81.5Mi    .text
   1.7%  68.0Mi   0.0%       0    .debug_rnglists
   1.1%  43.2Mi  28.5%  43.2Mi    .rodata
   0.5%  21.4Mi   0.0%       0    .debug_abbrev
   0.5%  20.3Mi   0.0%       0    .strtab
   0.2%  8.79Mi   5.8%  8.79Mi    .eh_frame
   0.2%  6.73Mi   4.4%  6.73Mi    .rela.dyn
   0.1%  4.86Mi   0.0%       0    .symtab
   0.1%  4.09Mi   2.7%  4.09Mi    .dynstr
   0.1%  3.60Mi   2.4%  3.60Mi    .data.rel.ro
   0.1%  3.30Mi   0.0%       0    .debug_aranges
   0.0%  1.19Mi   0.8%  1.19Mi    .dynsym
   0.0%  1.09Mi   0.7%  1.09Mi    .eh_frame_hdr
   0.0%       0   0.4%   611Ki    .bss
   0.0%   449Ki   0.1%   127Ki    [25 Others]
   0.0%   393Ki   0.3%   393Ki    .gnu.hash
   0.0%   332Ki   0.2%   332Ki    .data
 100.0%  3.99Gi 100.0%   151Mi    TOTAL

For BPF, this debug info is needed to generate proper BTF and use kconfig. This patch just enables it for C++, which makes it possible for us to leverage templates to deduplicate almost identical BPF code for IPv4 and IPv6.

https://reviews.llvm.org/D70696 has more details regarding why it is needed for BPF in general and https://lore.kernel.org/bpf/[e-mail]/ has more details regarding why we are trying to compile BPF from C++.

[MaskRay]

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)

# /tmp/Rel is a llvm-project build directory.
ninja -C /tmp/Rel clang lld llvm-{ar,nm,strings,objdump,objcopy,readelf,strip}

# In a Linux kernel tree,
PATH=/tmp/Rel/bin:$PATH make O=/tmp/linux/x86_64 LLVM=1 defconfig all -j 50

qemu-system-x86_64 ...

clang -O2 -emit-llvm -c bpf.c -o - | llc -march=bpf -filetype=obj -o bpf.o

Run XXX command to load bpf.o into some tools, say, tc
Run XXX command to inspect that the tool is happy with new Clang behavior

[MaskRay]

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?

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?

[dblaikie]

In D153898#4457263, @chiyuze wrote:

Regarding cost, only BPF target triggers this debug info generation for extern variables.

Ah, OK - if that's what you need for BPF and it doesn't affect anything else - have at.

(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?)?)

There is no impact on x86 builds of clang:

Oh, OK - no worries then.

[yonghong-song]

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/[e-mail]/

This will give people a sense why this is useful/better than other alternatives (like macro based approach).

[chiyuze]

Thank you for accepting this patch!

In 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, @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, @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, @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, @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/[e-mail]/

This will give people a sense why this is useful/better than other alternatives (like macro based approach).

Sure thing, will do!

clang/lib/Sema/SemaDecl.cpp

case VarDecl::DeclarationOnly:
         Var->setInvalidDecl();
       if (!Type->isDependentType() && !Var->isInvalidDecl() &&
           Var->getStorageClass() == SC_PrivateExtern) {
         Diag(Var->getLocation(), diag::warn_private_extern);
         Diag(Var->getLocation(), diag::note_private_extern);
       }
 
       if (Context.getTargetInfo().allowDebugInfoForExternalRef() &&
-          !Var->isInvalidDecl() && !getLangOpts().CPlusPlus)
+          !Var->isInvalidDecl())
         ExternalDeclarations.push_back(Var);
 
       return;
 
     case VarDecl::TentativeDefinition:
       // File scope. C99 6.9.2p2: A declaration of an identifier for an
       // object that has file scope without an initializer, and without a
       // storage-class specifier or with the storage-class specifier "static",

clang/test/CodeGen/debug-info-extern-basic.cpp

+// RUN: %clang_cc1 -x c++ -debug-info-kind=limited -triple bpf-linux-gnu -emit-llvm %s -o - | FileCheck %s
+
+namespace foo {
+
+template <typename T>
+struct S {
+  T x;
+};
+
+extern S<char> s;
+
+int test(void) {
+  return s.x;
+}
+
+}  // namespace foo
+
+// CHECK: distinct !DIGlobalVariable(name: "s", scope: ![[NAMESPACE:[0-9]+]],{{.*}} type: ![[STRUCT_TYPE:[0-9]+]], isLocal: false, isDefinition: false)
+// CHECK: ![[NAMESPACE]] = !DINamespace(name: "foo", scope: null)
+// CHECK: ![[STRUCT_TYPE]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "S<char>",{{.*}}size: 8, flags: DIFlagTypePassByValue, elements: ![[ELEMENT_TYPE:[0-9]+]], templateParams: ![[TEMPLATE_TYPE:[0-9]+]], identifier: "_ZTSN3foo1SIcEE")
+// CHECK: ![[ELEMENT_TYPE]] = !{![[ELEMENT_TYPE:[0-9]+]]}
+// CHECK: ![[ELEMENT_TYPE]] = !DIDerivedType(tag: DW_TAG_member, name: "x",{{.*}} baseType: ![[BASE_TYPE:[0-9]+]], size: 8)
+// CHECK: ![[BASE_TYPE]] = !DIBasicType(name: "char", size: 8, encoding: DW_ATE_signed_char)
+// CHECK: ![[TEMPLATE_TYPE]] = !{![[TEMPLATE_TYPE:[0-9]+]]}
+// CHECK: ![[TEMPLATE_TYPE]] = !DITemplateTypeParameter(name: "T", type: ![[BASE_TYPE]])