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

[Yuze Chi via Phabricator via cfe-commits]

chiyuze created this revision.
chiyuze added a reviewer: rnk.
Herald added a project: All.
chiyuze requested review of this revision.
Herald added a project: clang.

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++.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D153898

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/debug-info-extern-basic.cpp


Index: clang/test/CodeGen/debug-info-extern-basic.cpp
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-basic.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -x c++ -debug-info-kind=limited -triple bpf-linux-gnu 
-emit-llvm %s -o - | FileCheck %s
+
+namespace foo {
+
+template 
+struct S {
+  T x;
+};
+
+extern S 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",{{.*}}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]])
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -13773,7 +13773,7 @@
   }
 
   if (Context.getTargetInfo().allowDebugInfoForExternalRef() &&
-  !Var->isInvalidDecl() && !getLangOpts().CPlusPlus)
+  !Var->isInvalidDecl())
 ExternalDeclarations.push_back(Var);
 
   return;


Index: clang/test/CodeGen/debug-info-extern-basic.cpp
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-basic.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -x c++ -debug-info-kind=limited -triple bpf-linux-gnu -emit-llvm %s -o - | FileCheck %s
+
+namespace foo {
+
+template 
+struct S {
+  T x;
+};
+
+extern S 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",{{.*}}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]])
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -13773,7 +13773,7 @@
   }
 
   if (Context.getTargetInfo().allowDebugInfoForExternalRef() &&
-  !Var->isInvalidDecl() && !getLangOpts().CPlusPlus)
+  !Var->isInvalidDecl())
 ExternalDeclarations.push_back(Var);
 
   return;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yuze Chi via Phabricator via cfe-commits]

chiyuze added a comment.

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 SIZEVM 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%   151MiTOTAL

With patch:

  bloaty build/RelWithDebInfo/23bc4427ab49716ce2c24c81529d9c90953b3c54/bin/clang
  FILE SIZEVM 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%   151MiTOTAL

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/cakh8qbt4xqbupxefqpk5ayu1rr0-h-vcjzs_0bu-987gl4w...@mail.gmail.com/
 has more details regarding why we are trying to compile BPF from C++.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153898

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 , @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/cakh8qbt4xqbupxefqpk5ayu1rr0-h-vcjzs_0bu-987gl4w...@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