Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BPF] Fix linking issues in static map initializers #91310

Merged
merged 1 commit into from
Jul 5, 2024

Conversation

mejedi
Copy link
Contributor

@mejedi mejedi commented May 7, 2024

When BPF object files are linked with bpftool, every symbol must be accompanied by BTF info. Ensure that extern functions referenced by global variable initializers are included in BTF.

The primary motivation is "static" initialization of PROG maps:

extern int elsewhere(struct xdp_md *);

struct {
  __uint(type, BPF_MAP_TYPE_PROG_ARRAY);
  __uint(max_entries, 1);
  __type(key, int);
  __type(value, int);
  __array(values, int (struct xdp_md *));
} prog_map SEC(".maps") = { .values = { elsewhere } };

BPF backend needs debug info to produce BTF. Debug info is not
normally generated for external variables and functions. Previously, it
was solved differently for variables (collecting variable declarations
in ExternalDeclarations vector) and functions (logic invoked during
codegen in CGExpr.cpp).

This patch generalises ExternalDefclarations to include both function
and variable declarations. This change ensures that function references
are not missed no matter the context. Previously external functions
referenced in constant expressions lacked debug info.

Copy link

github-actions bot commented May 7, 2024

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen labels May 7, 2024
@llvmbot
Copy link

llvmbot commented May 7, 2024

@llvm/pr-subscribers-clang-codegen

Author: Nick Zavaritsky (mejedi)

Changes

When BPF object files are linked with bpftool, every symbol must be accompanied by BTF info. Ensure that extern functions referenced by global variable initializers are included in BTF.

The primary motivation is "static" initialization of PROG maps:

extern int elsewhere(struct xdp_md *);

struct {
__uint(type, BPF_MAP_TYPE_PROG_ARRAY);
__uint(max_entries, 1);
__type(key, int);
__type(value, int);
__array(values, int (struct xdp_md *));
} prog_map SEC(".maps") = { .values = { elsewhere } };


Full diff: https://github.com/llvm/llvm-project/pull/91310.diff

5 Files Affected:

  • (modified) clang/lib/CodeGen/CGExprConstant.cpp (+16-2)
  • (added) clang/test/bpf-debug-info-extern-func.c (+9)
  • (modified) llvm/lib/Target/BPF/BTFDebug.cpp (+23)
  • (modified) llvm/lib/Target/BPF/BTFDebug.h (+4)
  • (added) llvm/test/CodeGen/BPF/BTF/extern-var-func2.ll (+54)
diff --git a/clang/lib/CodeGen/CGExprConstant.cpp b/clang/lib/CodeGen/CGExprConstant.cpp
index 94962091116af..945f2e222b23f 100644
--- a/clang/lib/CodeGen/CGExprConstant.cpp
+++ b/clang/lib/CodeGen/CGExprConstant.cpp
@@ -1950,8 +1950,22 @@ ConstantLValueEmitter::tryEmitBase(const APValue::LValueBase &base) {
     if (D->hasAttr<WeakRefAttr>())
       return CGM.GetWeakRefReference(D).getPointer();
 
-    if (auto FD = dyn_cast<FunctionDecl>(D))
-      return CGM.GetAddrOfFunction(FD);
+    if (auto FD = dyn_cast<FunctionDecl>(D)) {
+      auto *C = CGM.GetAddrOfFunction(FD);
+
+      // we don't normally emit debug info for extern fns referenced via
+      // variable initialisers; BPF needs it since it generates BTF from
+      // debug info and bpftool demands BTF for every symbol linked
+      if (CGM.getTarget().getTriple().isBPF() && FD->getStorageClass() == SC_Extern) {
+        if (CGDebugInfo *DI = CGM.getModuleDebugInfo()) {
+          auto *Fn = dyn_cast<llvm::Function>(C);
+          if (Fn && !Fn->getSubprogram())
+            DI->EmitFunctionDecl(FD, FD->getLocation(), D->getType(), Fn);
+        }
+      }
+
+      return C;
+    }
 
     if (auto VD = dyn_cast<VarDecl>(D)) {
       // We can never refer to a variable with local storage.
diff --git a/clang/test/bpf-debug-info-extern-func.c b/clang/test/bpf-debug-info-extern-func.c
new file mode 100644
index 0000000000000..da324630a314d
--- /dev/null
+++ b/clang/test/bpf-debug-info-extern-func.c
@@ -0,0 +1,9 @@
+// RUN: %clang -g -O2 -target bpf -S -emit-llvm %s -o - | FileCheck %s
+//
+// When linking BPF object files via bpftool, BTF info is required for
+// every symbol. BTF is generated from debug info. Ensure that debug info
+// is emitted for extern functions referenced via variable initializers.
+//
+// CHECK: !DISubprogram(name: "fn"
+extern void fn(void);
+void (*pfn) (void) = &fn;
diff --git a/llvm/lib/Target/BPF/BTFDebug.cpp b/llvm/lib/Target/BPF/BTFDebug.cpp
index b6d3b460005c9..7d64f2e04bc26 100644
--- a/llvm/lib/Target/BPF/BTFDebug.cpp
+++ b/llvm/lib/Target/BPF/BTFDebug.cpp
@@ -1494,6 +1494,29 @@ void BTFDebug::processGlobals(bool ProcessingMapDef) {
 
     DataSecEntries[std::string(SecName)]->addDataSecEntry(VarId,
         Asm->getSymbol(&Global), Size);
+
+    if (Global.hasInitializer())
+      processGlobalInitializer(Global.getInitializer());
+  }
+}
+
+/// Process global variable initializer in pursuit for function
+/// pointers. Add discovered (extern) functions to BTF. Some (extern)
+/// functions might have been missed otherwise. Every symbol needs BTF
+/// info when linking with bpftool. Primary use case: "static"
+/// initialization of BPF maps.
+///
+/// struct {
+///   __uint(type, BPF_MAP_TYPE_PROG_ARRAY);
+///   ...
+/// } prog_map SEC(".maps") = { .values = { extern_func } };
+///
+void BTFDebug::processGlobalInitializer(const Constant *C) {
+  if (auto *Fn = dyn_cast<Function>(C))
+    processFuncPrototypes(Fn);
+  if (auto *CA = dyn_cast<ConstantAggregate>(C)) {
+    for (unsigned I = 0, N = CA->getNumOperands(); I < N; ++I)
+      processGlobalInitializer(CA->getOperand(I));
   }
 }
 
diff --git a/llvm/lib/Target/BPF/BTFDebug.h b/llvm/lib/Target/BPF/BTFDebug.h
index 11a0c59ba6c90..02181d3013be9 100644
--- a/llvm/lib/Target/BPF/BTFDebug.h
+++ b/llvm/lib/Target/BPF/BTFDebug.h
@@ -352,6 +352,10 @@ class BTFDebug : public DebugHandlerBase {
   /// Generate types and variables for globals.
   void processGlobals(bool ProcessingMapDef);
 
+  /// Process global variable initializer in pursuit for function
+  /// pointers.
+  void processGlobalInitializer(const Constant *C);
+
   /// Generate types for function prototypes.
   void processFuncPrototypes(const Function *);
 
diff --git a/llvm/test/CodeGen/BPF/BTF/extern-var-func2.ll b/llvm/test/CodeGen/BPF/BTF/extern-var-func2.ll
new file mode 100644
index 0000000000000..700486d9f3515
--- /dev/null
+++ b/llvm/test/CodeGen/BPF/BTF/extern-var-func2.ll
@@ -0,0 +1,54 @@
+; RUN: llc -march=bpfel -filetype=asm -o - %s | FileCheck -check-prefixes=CHECK %s
+; RUN: llc -march=bpfeb -filetype=asm -o - %s | FileCheck -check-prefixes=CHECK %s
+;
+; Source code:
+;   extern int elsewhere(void);
+;   struct {
+;     void *values[];
+;   } prog_map = { .values = { elsewhere } };
+; Compilation flag:
+;   clang -target bpf -O2 -g -S -emit-llvm test.c
+; ModuleID = 'b.c'
+
+@prog_map = dso_local local_unnamed_addr global { [1 x ptr] } { [1 x ptr] [ptr @elsewhere] }, align 8, !dbg !0
+
+declare !dbg !17 dso_local i32 @elsewhere() #0
+
+; CHECK:             .long   0                               # BTF_KIND_FUNC_PROTO(id = 6)
+; CHECK-NEXT:        .long   218103808                       # 0xd000000
+; CHECK-NEXT:        .long   7
+; CHECK-NEXT:        .long   37                              # BTF_KIND_INT(id = 7)
+; CHECK-NEXT:        .long   16777216                        # 0x1000000
+; CHECK-NEXT:        .long   4
+; CHECK-NEXT:        .long   16777248                        # 0x1000020
+; CHECK-NEXT:        .long   41                              # BTF_KIND_FUNC(id = 8)
+; CHECK-NEXT:        .long   201326594                       # 0xc000002
+; CHECK-NEXT:        .long   6
+
+attributes #0 = { "frame-pointer"="all" "no-trapping-math"="true" "stack-protector-buffer-size"="8" }
+
+!llvm.dbg.cu = !{!2}
+!llvm.module.flags = !{!12, !13, !14, !15}
+!llvm.ident = !{!16}
+
+!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
+!1 = distinct !DIGlobalVariable(name: "prog_map", scope: !2, file: !3, line: 4, type: !5, isLocal: false, isDefinition: true)
+!2 = distinct !DICompileUnit(language: DW_LANG_C11, file: !3, producer: "clang version 19.0.0git (git@github.com:llvm/llvm-project.git 0390a6803608e3a5314315b73740c2d3f5a5723f)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, globals: !4, splitDebugInlining: false, nameTableKind: None)
+!3 = !DIFile(filename: "b.c", directory: "/home/nickz/llvm-project.git", checksumkind: CSK_MD5, checksum: "41cc17375f1261a0e072590833492553")
+!4 = !{!0}
+!5 = distinct !DICompositeType(tag: DW_TAG_structure_type, file: !3, line: 2, elements: !6)
+!6 = !{!7}
+!7 = !DIDerivedType(tag: DW_TAG_member, name: "values", scope: !5, file: !3, line: 3, baseType: !8)
+!8 = !DICompositeType(tag: DW_TAG_array_type, baseType: !9, elements: !10)
+!9 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: null, size: 64)
+!10 = !{!11}
+!11 = !DISubrange(count: -1)
+!12 = !{i32 7, !"Dwarf Version", i32 5}
+!13 = !{i32 2, !"Debug Info Version", i32 3}
+!14 = !{i32 1, !"wchar_size", i32 4}
+!15 = !{i32 7, !"frame-pointer", i32 2}
+!16 = !{!"clang version 19.0.0git (git@github.com:llvm/llvm-project.git 0390a6803608e3a5314315b73740c2d3f5a5723f)"}
+!17 = !DISubprogram(name: "elsewhere", scope: !3, file: !3, line: 1, type: !18, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized)
+!18 = !DISubroutineType(types: !19)
+!19 = !{!20}
+!20 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)

@llvmbot
Copy link

llvmbot commented May 7, 2024

@llvm/pr-subscribers-clang

Author: Nick Zavaritsky (mejedi)

Changes

When BPF object files are linked with bpftool, every symbol must be accompanied by BTF info. Ensure that extern functions referenced by global variable initializers are included in BTF.

The primary motivation is "static" initialization of PROG maps:

extern int elsewhere(struct xdp_md *);

struct {
__uint(type, BPF_MAP_TYPE_PROG_ARRAY);
__uint(max_entries, 1);
__type(key, int);
__type(value, int);
__array(values, int (struct xdp_md *));
} prog_map SEC(".maps") = { .values = { elsewhere } };


Full diff: https://github.com/llvm/llvm-project/pull/91310.diff

5 Files Affected:

  • (modified) clang/lib/CodeGen/CGExprConstant.cpp (+16-2)
  • (added) clang/test/bpf-debug-info-extern-func.c (+9)
  • (modified) llvm/lib/Target/BPF/BTFDebug.cpp (+23)
  • (modified) llvm/lib/Target/BPF/BTFDebug.h (+4)
  • (added) llvm/test/CodeGen/BPF/BTF/extern-var-func2.ll (+54)
diff --git a/clang/lib/CodeGen/CGExprConstant.cpp b/clang/lib/CodeGen/CGExprConstant.cpp
index 94962091116a..945f2e222b23 100644
--- a/clang/lib/CodeGen/CGExprConstant.cpp
+++ b/clang/lib/CodeGen/CGExprConstant.cpp
@@ -1950,8 +1950,22 @@ ConstantLValueEmitter::tryEmitBase(const APValue::LValueBase &base) {
     if (D->hasAttr<WeakRefAttr>())
       return CGM.GetWeakRefReference(D).getPointer();
 
-    if (auto FD = dyn_cast<FunctionDecl>(D))
-      return CGM.GetAddrOfFunction(FD);
+    if (auto FD = dyn_cast<FunctionDecl>(D)) {
+      auto *C = CGM.GetAddrOfFunction(FD);
+
+      // we don't normally emit debug info for extern fns referenced via
+      // variable initialisers; BPF needs it since it generates BTF from
+      // debug info and bpftool demands BTF for every symbol linked
+      if (CGM.getTarget().getTriple().isBPF() && FD->getStorageClass() == SC_Extern) {
+        if (CGDebugInfo *DI = CGM.getModuleDebugInfo()) {
+          auto *Fn = dyn_cast<llvm::Function>(C);
+          if (Fn && !Fn->getSubprogram())
+            DI->EmitFunctionDecl(FD, FD->getLocation(), D->getType(), Fn);
+        }
+      }
+
+      return C;
+    }
 
     if (auto VD = dyn_cast<VarDecl>(D)) {
       // We can never refer to a variable with local storage.
diff --git a/clang/test/bpf-debug-info-extern-func.c b/clang/test/bpf-debug-info-extern-func.c
new file mode 100644
index 000000000000..da324630a314
--- /dev/null
+++ b/clang/test/bpf-debug-info-extern-func.c
@@ -0,0 +1,9 @@
+// RUN: %clang -g -O2 -target bpf -S -emit-llvm %s -o - | FileCheck %s
+//
+// When linking BPF object files via bpftool, BTF info is required for
+// every symbol. BTF is generated from debug info. Ensure that debug info
+// is emitted for extern functions referenced via variable initializers.
+//
+// CHECK: !DISubprogram(name: "fn"
+extern void fn(void);
+void (*pfn) (void) = &fn;
diff --git a/llvm/lib/Target/BPF/BTFDebug.cpp b/llvm/lib/Target/BPF/BTFDebug.cpp
index b6d3b460005c..7d64f2e04bc2 100644
--- a/llvm/lib/Target/BPF/BTFDebug.cpp
+++ b/llvm/lib/Target/BPF/BTFDebug.cpp
@@ -1494,6 +1494,29 @@ void BTFDebug::processGlobals(bool ProcessingMapDef) {
 
     DataSecEntries[std::string(SecName)]->addDataSecEntry(VarId,
         Asm->getSymbol(&Global), Size);
+
+    if (Global.hasInitializer())
+      processGlobalInitializer(Global.getInitializer());
+  }
+}
+
+/// Process global variable initializer in pursuit for function
+/// pointers. Add discovered (extern) functions to BTF. Some (extern)
+/// functions might have been missed otherwise. Every symbol needs BTF
+/// info when linking with bpftool. Primary use case: "static"
+/// initialization of BPF maps.
+///
+/// struct {
+///   __uint(type, BPF_MAP_TYPE_PROG_ARRAY);
+///   ...
+/// } prog_map SEC(".maps") = { .values = { extern_func } };
+///
+void BTFDebug::processGlobalInitializer(const Constant *C) {
+  if (auto *Fn = dyn_cast<Function>(C))
+    processFuncPrototypes(Fn);
+  if (auto *CA = dyn_cast<ConstantAggregate>(C)) {
+    for (unsigned I = 0, N = CA->getNumOperands(); I < N; ++I)
+      processGlobalInitializer(CA->getOperand(I));
   }
 }
 
diff --git a/llvm/lib/Target/BPF/BTFDebug.h b/llvm/lib/Target/BPF/BTFDebug.h
index 11a0c59ba6c9..02181d3013be 100644
--- a/llvm/lib/Target/BPF/BTFDebug.h
+++ b/llvm/lib/Target/BPF/BTFDebug.h
@@ -352,6 +352,10 @@ class BTFDebug : public DebugHandlerBase {
   /// Generate types and variables for globals.
   void processGlobals(bool ProcessingMapDef);
 
+  /// Process global variable initializer in pursuit for function
+  /// pointers.
+  void processGlobalInitializer(const Constant *C);
+
   /// Generate types for function prototypes.
   void processFuncPrototypes(const Function *);
 
diff --git a/llvm/test/CodeGen/BPF/BTF/extern-var-func2.ll b/llvm/test/CodeGen/BPF/BTF/extern-var-func2.ll
new file mode 100644
index 000000000000..700486d9f351
--- /dev/null
+++ b/llvm/test/CodeGen/BPF/BTF/extern-var-func2.ll
@@ -0,0 +1,54 @@
+; RUN: llc -march=bpfel -filetype=asm -o - %s | FileCheck -check-prefixes=CHECK %s
+; RUN: llc -march=bpfeb -filetype=asm -o - %s | FileCheck -check-prefixes=CHECK %s
+;
+; Source code:
+;   extern int elsewhere(void);
+;   struct {
+;     void *values[];
+;   } prog_map = { .values = { elsewhere } };
+; Compilation flag:
+;   clang -target bpf -O2 -g -S -emit-llvm test.c
+; ModuleID = 'b.c'
+
+@prog_map = dso_local local_unnamed_addr global { [1 x ptr] } { [1 x ptr] [ptr @elsewhere] }, align 8, !dbg !0
+
+declare !dbg !17 dso_local i32 @elsewhere() #0
+
+; CHECK:             .long   0                               # BTF_KIND_FUNC_PROTO(id = 6)
+; CHECK-NEXT:        .long   218103808                       # 0xd000000
+; CHECK-NEXT:        .long   7
+; CHECK-NEXT:        .long   37                              # BTF_KIND_INT(id = 7)
+; CHECK-NEXT:        .long   16777216                        # 0x1000000
+; CHECK-NEXT:        .long   4
+; CHECK-NEXT:        .long   16777248                        # 0x1000020
+; CHECK-NEXT:        .long   41                              # BTF_KIND_FUNC(id = 8)
+; CHECK-NEXT:        .long   201326594                       # 0xc000002
+; CHECK-NEXT:        .long   6
+
+attributes #0 = { "frame-pointer"="all" "no-trapping-math"="true" "stack-protector-buffer-size"="8" }
+
+!llvm.dbg.cu = !{!2}
+!llvm.module.flags = !{!12, !13, !14, !15}
+!llvm.ident = !{!16}
+
+!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
+!1 = distinct !DIGlobalVariable(name: "prog_map", scope: !2, file: !3, line: 4, type: !5, isLocal: false, isDefinition: true)
+!2 = distinct !DICompileUnit(language: DW_LANG_C11, file: !3, producer: "clang version 19.0.0git (git@github.com:llvm/llvm-project.git 0390a6803608e3a5314315b73740c2d3f5a5723f)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, globals: !4, splitDebugInlining: false, nameTableKind: None)
+!3 = !DIFile(filename: "b.c", directory: "/home/nickz/llvm-project.git", checksumkind: CSK_MD5, checksum: "41cc17375f1261a0e072590833492553")
+!4 = !{!0}
+!5 = distinct !DICompositeType(tag: DW_TAG_structure_type, file: !3, line: 2, elements: !6)
+!6 = !{!7}
+!7 = !DIDerivedType(tag: DW_TAG_member, name: "values", scope: !5, file: !3, line: 3, baseType: !8)
+!8 = !DICompositeType(tag: DW_TAG_array_type, baseType: !9, elements: !10)
+!9 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: null, size: 64)
+!10 = !{!11}
+!11 = !DISubrange(count: -1)
+!12 = !{i32 7, !"Dwarf Version", i32 5}
+!13 = !{i32 2, !"Debug Info Version", i32 3}
+!14 = !{i32 1, !"wchar_size", i32 4}
+!15 = !{i32 7, !"frame-pointer", i32 2}
+!16 = !{!"clang version 19.0.0git (git@github.com:llvm/llvm-project.git 0390a6803608e3a5314315b73740c2d3f5a5723f)"}
+!17 = !DISubprogram(name: "elsewhere", scope: !3, file: !3, line: 1, type: !18, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized)
+!18 = !DISubroutineType(types: !19)
+!19 = !{!20}
+!20 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)

@mejedi mejedi force-pushed the bpf-fix-linking-static-map-init branch from 6fb034e to e518bdf Compare May 7, 2024 09:38
@mejedi
Copy link
Contributor Author

mejedi commented May 7, 2024

CC @yonghong-song

Copy link
Collaborator

@efriedma-quic efriedma-quic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible you could put this code into CompleteExternalDeclaration(), instead of writing it out in every place that can refer to the address of a function?

// we don't normally emit debug info for extern fns referenced via
// variable initialisers; BPF needs it since it generates BTF from
// debug info and bpftool demands BTF for every symbol linked
if (CGM.getTarget().getTriple().isBPF() && FD->getStorageClass() == SC_Extern) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an existing hook allowDebugInfoForExternalRef() for which targets want this.

You almost never want getStorageClass(); if you care about linkage, we have different methods for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@efriedma-quic Thank you for insightful comments! As a newbie, I need more guidance.

It is pretty much clear that allowDebugInfoForExternalRef is a preferred way to check whether we should emit additional debug info. It is currently only overriden by BPF target and is being used for exact same purpose we have here.

I need help getting up to speed irt. CompleteExternalDeclaration proposal. Having grepped through the code, I can see that BPF faced the same challenge with extern variables. At the moment, there's a vector of VariableDecls populated in SemaDecl.cpp and later used to attach debug info to variables.

A different mechanism is applied to discovering extern functions in need for debug info (prior to my patch, extern functions referenced through code were handled). This is accomplished by code in CGExpr.cpp. I am proposing a similar extension in CGExprConstant.cpp.

Would you like to have functions on ExternalDeclarations list instead?

What would be a good spot to get ahold of FunctionDecl to put it on ExternalDeclarations list?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code again, I guess the ultimate question is whether we want to emit debug info for all external functions/variables, or only functions/variables that are actually referenced.

If we want all functions/variables, we want to extend ExternalDeclarations to include them. Maybe put the code in Sema::ActOnFunctionDeclarator, or something like that.

If you just want referenced functions/variables, probably the code should be in GetAddrOfFunction() (and something similar for variables).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mejedi @efriedma-quic Yes, we only want referenced functions/variables. The support for external variable has been implemented here d77ae15. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@efriedma-quic @yonghong-song I ended up generalising ExternalDeclarations to include both VarDecls and FunctionDecls.
Since GetAddrOfFunction is invoked in so many different contexts, I found it difficult to emit debug info only for the subset of calls we care about.
Personally, I find it cleaner to collect external declarations on a list and handle them separately rather than plugging directly into codegen. Please let me know if I am missing something.
Concerning referenced/all debacle -

  • Sema::ActOnEndOfTranslationUnit checks whether a declaration is used (does it mean referenced?) before calling Consumer.CompleteExternalDeclaration;
  • plugging into codegen ensures that we notice any reference that made it into llvm bitcode BUT they could be removed by the optimiser later;
  • BPF backend ensures that only referenced entities make it to BTF.

It looks like both approaches are quite similar?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mejedi I think we do not need to worry about cases where external functions are in the code but removed by optimizer later as this should not that frequent. Your patch looks good to me, which follows what we did for VarDecl so adding FunctionDecl is a natural addition. BPF side of the change looks good to me too. @efriedma-quic could you take a look as well?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current approach seems fine.

Sema::ActOnEndOfTranslationUnit checks whether a declaration is used (does it mean referenced?)

"Used" in this context corresponds to odr-used in the C++ standard... which is stuff that Sema thinks the code generator might need to emit. In some cases we manage to skip actually emitting some declarations due to optimizations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @efriedma-quic if the clang believes the declaration might not be necessary due to optimizaiton, it is okay to remove it as in that case kernel/bpf does not need it either.

@mejedi
Copy link
Contributor Author

mejedi commented May 7, 2024

cc @anakryiko

Copy link

github-actions bot commented May 14, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@mejedi mejedi force-pushed the bpf-fix-linking-static-map-init branch from e518bdf to 440f62d Compare May 17, 2024 12:58
@mejedi mejedi requested a review from Endilll as a code owner May 17, 2024 12:58
@llvmbot llvmbot added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label May 17, 2024
@mejedi mejedi requested a review from efriedma-quic May 17, 2024 13:31
@mejedi mejedi force-pushed the bpf-fix-linking-static-map-init branch from 440f62d to 88f895c Compare May 17, 2024 19:30
Copy link
Contributor

@Endilll Endilll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sema.h changes look good to me.

@mejedi
Copy link
Contributor Author

mejedi commented May 27, 2024

Ping.

Copy link
Collaborator

@dwblaikie dwblaikie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some test coverage that shows that unreferenced variables/functions aren't included in the output?

@yonghong-song
Copy link
Contributor

Is there some test coverage that shows that unreferenced variables/functions aren't included in the output?

@mejedi Please add a test case for this.

@mejedi mejedi force-pushed the bpf-fix-linking-static-map-init branch from 88f895c to defe9d0 Compare June 10, 2024 18:12
@mejedi
Copy link
Contributor Author

mejedi commented Jun 10, 2024

@yonghong-song @dwblaikie

Is there some test coverage that shows that unreferenced variables/functions aren't included in the output?

Test added. https://github.com/llvm/llvm-project/pull/91310/files#diff-d262f0d82094d852c13fbf61a964211ad66d0143893737d58ef3a8df2504038a

@mejedi
Copy link
Contributor Author

mejedi commented Jun 17, 2024

ping

@dwblaikie
Copy link
Collaborator

happy with the test coverage, but I'm probably not the right person to review th ecode in more detail

@mejedi
Copy link
Contributor Author

mejedi commented Jun 30, 2024

@yonghong-song @efriedma-quic Any chance we could get it merged?

When BPF object files are linked with bpftool, every symbol must be
accompanied by BTF info. Ensure that extern functions referenced by
global variable initializers are included in BTF.

The primary motivation is "static" initialization of PROG maps:

  extern int elsewhere(struct xdp_md *);

  struct {
    __uint(type, BPF_MAP_TYPE_PROG_ARRAY);
    __uint(max_entries, 1);
    __type(key, int);
    __type(value, int);
    __array(values, int (struct xdp_md *));
  } prog_map SEC(".maps") = { .values = { elsewhere } };

BPF backend needs debug info to produce BTF. Debug info is not
normally generated for external variables and functions. Previously, it
was solved differently for variables (collecting variable declarations
in ExternalDeclarations vector) and functions (logic invoked during
codegen in CGExpr.cpp).

This patch generalises ExternalDefclarations to include both function
and variable declarations. This change ensures that function references
are not missed no matter the context. Previously external functions
referenced in constant expressions lacked debug info.
@mejedi mejedi force-pushed the bpf-fix-linking-static-map-init branch from defe9d0 to a9682a1 Compare July 5, 2024 11:30
@yonghong-song yonghong-song merged commit ae0d224 into llvm:main Jul 5, 2024
7 checks passed
Copy link

github-actions bot commented Jul 5, 2024

@mejedi Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested
by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as
the builds can include changes from many authors. It is not uncommon for your
change to be included in a build that fails due to someone else's changes, or
infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself.
This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

@mejedi mejedi deleted the bpf-fix-linking-static-map-init branch July 5, 2024 21:47
kbluck pushed a commit to kbluck/llvm-project that referenced this pull request Jul 6, 2024
When BPF object files are linked with bpftool, every symbol must be
accompanied by BTF info. Ensure that extern functions referenced by
global variable initializers are included in BTF.

The primary motivation is "static" initialization of PROG maps:

```c
extern int elsewhere(struct xdp_md *);

struct {
  __uint(type, BPF_MAP_TYPE_PROG_ARRAY);
  __uint(max_entries, 1);
  __type(key, int);
  __type(value, int);
  __array(values, int (struct xdp_md *));
} prog_map SEC(".maps") = { .values = { elsewhere } };
```

BPF backend needs debug info to produce BTF. Debug info is not
normally generated for external variables and functions. Previously, it
was solved differently for variables (collecting variable declarations
    in ExternalDeclarations vector) and functions (logic invoked during
    codegen in CGExpr.cpp).

This patch generalises ExternalDefclarations to include both function
and variable declarations. This change ensures that function references
    are not missed no matter the context. Previously external functions
    referenced in constant expressions lacked debug info.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants