[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Yonghong Song via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa2a3ca8d9796: BPF: emit debuginfo for Function of 
DeclRefExpr if requested (authored by yonghong-song).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

Files:
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/Targets/BPF.h
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/debug-info-extern-callback.c


Index: clang/test/CodeGen/debug-info-extern-callback.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-callback.c
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -x c -debug-info-kind=limited -triple bpf-linux-gnu 
-emit-llvm %s -o - | FileCheck %s
+
+extern int do_work1(int);
+long bpf_helper1(void *callback_fn);
+long prog1() {
+  return bpf_helper1(_work1);
+}
+
+extern int do_work2();
+long prog2_1() {
+  return (long)_work2;
+}
+int do_work2() { return 0; }
+long prog2_2() {
+  return (long)_work2;
+}
+
+// CHECK: declare !dbg ![[FUNC1:[0-9]+]] i32 @do_work1
+// CHECK: define dso_local i32 @do_work2() #{{[0-9]+}} !dbg ![[FUNC2:[0-9]+]]
+
+// CHECK: ![[FUNC1]] = !DISubprogram(name: "do_work1"
+// CHECK: ![[FUNC2]] = distinct !DISubprogram(name: "do_work2"
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -12688,7 +12688,7 @@
 Diag(Var->getLocation(), diag::note_private_extern);
   }
 
-  if (Context.getTargetInfo().allowDebugInfoForExternalVar() &&
+  if (Context.getTargetInfo().allowDebugInfoForExternalRef() &&
   !Var->isInvalidDecl() && !getLangOpts().CPlusPlus)
 ExternalDeclarations.push_back(Var);
 
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -2832,8 +2832,21 @@
 return LV;
   }
 
-  if (const auto *FD = dyn_cast(ND))
-return EmitFunctionDeclLValue(*this, E, FD);
+  if (const auto *FD = dyn_cast(ND)) {
+LValue LV = EmitFunctionDeclLValue(*this, E, FD);
+
+// Emit debuginfo for the function declaration if the target wants to.
+if (getContext().getTargetInfo().allowDebugInfoForExternalRef()) {
+  if (CGDebugInfo *DI = CGM.getModuleDebugInfo()) {
+auto *Fn =
+cast(LV.getPointer(*this)->stripPointerCasts());
+if (!Fn->getSubprogram())
+  DI->EmitFunctionDecl(FD, FD->getLocation(), T, Fn);
+  }
+}
+
+return LV;
+  }
 
   // FIXME: While we're emitting a binding from an enclosing scope, all other
   // DeclRefExprs we see should be implicitly treated as if they also refer to
Index: clang/lib/Basic/Targets/BPF.h
===
--- clang/lib/Basic/Targets/BPF.h
+++ clang/lib/Basic/Targets/BPF.h
@@ -76,7 +76,7 @@
 return None;
   }
 
-  bool allowDebugInfoForExternalVar() const override { return true; }
+  bool allowDebugInfoForExternalRef() const override { return true; }
 
   CallingConvCheckResult checkCallingConvention(CallingConv CC) const override 
{
 switch (CC) {
Index: clang/include/clang/Basic/TargetInfo.h
===
--- clang/include/clang/Basic/TargetInfo.h
+++ clang/include/clang/Basic/TargetInfo.h
@@ -1538,8 +1538,8 @@
 
   virtual void setAuxTarget(const TargetInfo *Aux) {}
 
-  /// Whether target allows debuginfo types for decl only variables.
-  virtual bool allowDebugInfoForExternalVar() const { return false; }
+  /// Whether target allows debuginfo types for decl only variables/functions.
+  virtual bool allowDebugInfoForExternalRef() const { return false; }
 
 protected:
   /// Copy type and layout related info.


Index: clang/test/CodeGen/debug-info-extern-callback.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-callback.c
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -x c -debug-info-kind=limited -triple bpf-linux-gnu -emit-llvm %s -o - | FileCheck %s
+
+extern int do_work1(int);
+long bpf_helper1(void *callback_fn);
+long prog1() {
+  return bpf_helper1(_work1);
+}
+
+extern int do_work2();
+long prog2_1() {
+  return (long)_work2;
+}
+int do_work2() { return 0; }
+long prog2_2() {
+  return (long)_work2;
+}
+
+// CHECK: declare !dbg ![[FUNC1:[0-9]+]] i32 @do_work1
+// CHECK: define dso_local i32 @do_work2() #{{[0-9]+}} !dbg ![[FUNC2:[0-9]+]]
+
+// CHECK: ![[FUNC1]] = !DISubprogram(name: "do_work1"
+// CHECK: ![[FUNC2]] = distinct !DISubprogram(name: "do_work2"
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -12688,7 +12688,7 @@
 

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added inline comments.



Comment at: clang/lib/CodeGen/CGExpr.cpp:2844
+  auto *Fn =
+  dyn_cast(LV.getPointer(*this)->stripPointerCasts());
+  if (DI && !Fn->getSubprogram())

dblaikie wrote:
> Oh, please change this to cast, rather than dyn_cast before committing. 
> (since the Fn is unconditionally dereferenced on the next line (well, 
> conditional on DI, but that's not relevant to this)
> 
> Could also move the "if (DI)" further out, like this:
> ```
> if (CGDebugInfo *DI = ...) {
>   auto *Fn = cast...
>   if (!Fn->getSubprogram())
> DI->EmitFunctionDecl(...);
> }
> ```
done as suggested!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

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

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song updated this revision to Diff 340675.
yonghong-song added a comment.

formatting to mode "DI = CGM.getModuleDebugInfo()" and use cast<...> instead of 
dyn_cast<...> since there should be no possibility of null ptr.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

Files:
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/Targets/BPF.h
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/debug-info-extern-callback.c


Index: clang/test/CodeGen/debug-info-extern-callback.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-callback.c
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -x c -debug-info-kind=limited -triple bpf-linux-gnu 
-emit-llvm %s -o - | FileCheck %s
+
+extern int do_work1(int);
+long bpf_helper1(void *callback_fn);
+long prog1() {
+  return bpf_helper1(_work1);
+}
+
+extern int do_work2();
+long prog2_1() {
+  return (long)_work2;
+}
+int do_work2() { return 0; }
+long prog2_2() {
+  return (long)_work2;
+}
+
+// CHECK: declare !dbg ![[FUNC1:[0-9]+]] i32 @do_work1
+// CHECK: define dso_local i32 @do_work2() #{{[0-9]+}} !dbg ![[FUNC2:[0-9]+]]
+
+// CHECK: ![[FUNC1]] = !DISubprogram(name: "do_work1"
+// CHECK: ![[FUNC2]] = distinct !DISubprogram(name: "do_work2"
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -12667,7 +12667,7 @@
 Diag(Var->getLocation(), diag::note_private_extern);
   }
 
-  if (Context.getTargetInfo().allowDebugInfoForExternalVar() &&
+  if (Context.getTargetInfo().allowDebugInfoForExternalRef() &&
   !Var->isInvalidDecl() && !getLangOpts().CPlusPlus)
 ExternalDeclarations.push_back(Var);
 
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -2834,8 +2834,21 @@
 return LV;
   }
 
-  if (const auto *FD = dyn_cast(ND))
-return EmitFunctionDeclLValue(*this, E, FD);
+  if (const auto *FD = dyn_cast(ND)) {
+LValue LV = EmitFunctionDeclLValue(*this, E, FD);
+
+// Emit debuginfo for the function declaration if the target wants to.
+if (getContext().getTargetInfo().allowDebugInfoForExternalRef()) {
+  if (CGDebugInfo *DI = CGM.getModuleDebugInfo()) {
+auto *Fn =
+cast(LV.getPointer(*this)->stripPointerCasts());
+if (!Fn->getSubprogram())
+  DI->EmitFunctionDecl(FD, FD->getLocation(), T, Fn);
+  }
+}
+
+return LV;
+  }
 
   // FIXME: While we're emitting a binding from an enclosing scope, all other
   // DeclRefExprs we see should be implicitly treated as if they also refer to
Index: clang/lib/Basic/Targets/BPF.h
===
--- clang/lib/Basic/Targets/BPF.h
+++ clang/lib/Basic/Targets/BPF.h
@@ -76,7 +76,7 @@
 return None;
   }
 
-  bool allowDebugInfoForExternalVar() const override { return true; }
+  bool allowDebugInfoForExternalRef() const override { return true; }
 
   CallingConvCheckResult checkCallingConvention(CallingConv CC) const override 
{
 switch (CC) {
Index: clang/include/clang/Basic/TargetInfo.h
===
--- clang/include/clang/Basic/TargetInfo.h
+++ clang/include/clang/Basic/TargetInfo.h
@@ -1538,8 +1538,8 @@
 
   virtual void setAuxTarget(const TargetInfo *Aux) {}
 
-  /// Whether target allows debuginfo types for decl only variables.
-  virtual bool allowDebugInfoForExternalVar() const { return false; }
+  /// Whether target allows debuginfo types for decl only variables/functions.
+  virtual bool allowDebugInfoForExternalRef() const { return false; }
 
 protected:
   /// Copy type and layout related info.


Index: clang/test/CodeGen/debug-info-extern-callback.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-callback.c
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -x c -debug-info-kind=limited -triple bpf-linux-gnu -emit-llvm %s -o - | FileCheck %s
+
+extern int do_work1(int);
+long bpf_helper1(void *callback_fn);
+long prog1() {
+  return bpf_helper1(_work1);
+}
+
+extern int do_work2();
+long prog2_1() {
+  return (long)_work2;
+}
+int do_work2() { return 0; }
+long prog2_2() {
+  return (long)_work2;
+}
+
+// CHECK: declare !dbg ![[FUNC1:[0-9]+]] i32 @do_work1
+// CHECK: define dso_local i32 @do_work2() #{{[0-9]+}} !dbg ![[FUNC2:[0-9]+]]
+
+// CHECK: ![[FUNC1]] = !DISubprogram(name: "do_work1"
+// CHECK: ![[FUNC2]] = distinct !DISubprogram(name: "do_work2"
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ 

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/lib/CodeGen/CGExpr.cpp:2844
+  auto *Fn =
+  dyn_cast(LV.getPointer(*this)->stripPointerCasts());
+  if (DI && !Fn->getSubprogram())

Oh, please change this to cast, rather than dyn_cast before committing. (since 
the Fn is unconditionally dereferenced on the next line (well, conditional on 
DI, but that's not relevant to this)

Could also move the "if (DI)" further out, like this:
```
if (CGDebugInfo *DI = ...) {
  auto *Fn = cast...
  if (!Fn->getSubprogram())
DI->EmitFunctionDecl(...);
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

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

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[David Blaikie via Phabricator via cfe-commits]

dblaikie accepted this revision.
dblaikie added a comment.

Looks good - thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

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

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song updated this revision to Diff 340650.
yonghong-song added a comment.

- use stripPointerCasts() to remove the cast of DeclRefExpr pointer


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

Files:
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/Targets/BPF.h
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/debug-info-extern-callback.c


Index: clang/test/CodeGen/debug-info-extern-callback.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-callback.c
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -x c -debug-info-kind=limited -triple bpf-linux-gnu 
-emit-llvm %s -o - | FileCheck %s
+
+extern int do_work1(int);
+long bpf_helper1(void *callback_fn);
+long prog1() {
+  return bpf_helper1(_work1);
+}
+
+extern int do_work2();
+long prog2_1() {
+  return (long)_work2;
+}
+int do_work2() { return 0; }
+long prog2_2() {
+  return (long)_work2;
+}
+
+// CHECK: declare !dbg ![[FUNC1:[0-9]+]] i32 @do_work1
+// CHECK: define dso_local i32 @do_work2() #{{[0-9]+}} !dbg ![[FUNC2:[0-9]+]]
+
+// CHECK: ![[FUNC1]] = !DISubprogram(name: "do_work1"
+// CHECK: ![[FUNC2]] = distinct !DISubprogram(name: "do_work2"
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -12667,7 +12667,7 @@
 Diag(Var->getLocation(), diag::note_private_extern);
   }
 
-  if (Context.getTargetInfo().allowDebugInfoForExternalVar() &&
+  if (Context.getTargetInfo().allowDebugInfoForExternalRef() &&
   !Var->isInvalidDecl() && !getLangOpts().CPlusPlus)
 ExternalDeclarations.push_back(Var);
 
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -2834,8 +2834,20 @@
 return LV;
   }
 
-  if (const auto *FD = dyn_cast(ND))
-return EmitFunctionDeclLValue(*this, E, FD);
+  if (const auto *FD = dyn_cast(ND)) {
+LValue LV = EmitFunctionDeclLValue(*this, E, FD);
+
+// Emit debuginfo for the function declaration if the target wants to.
+if (getContext().getTargetInfo().allowDebugInfoForExternalRef()) {
+  CGDebugInfo *DI = CGM.getModuleDebugInfo();
+  auto *Fn =
+  dyn_cast(LV.getPointer(*this)->stripPointerCasts());
+  if (DI && !Fn->getSubprogram())
+DI->EmitFunctionDecl(FD, FD->getLocation(), T, Fn);
+}
+
+return LV;
+  }
 
   // FIXME: While we're emitting a binding from an enclosing scope, all other
   // DeclRefExprs we see should be implicitly treated as if they also refer to
Index: clang/lib/Basic/Targets/BPF.h
===
--- clang/lib/Basic/Targets/BPF.h
+++ clang/lib/Basic/Targets/BPF.h
@@ -76,7 +76,7 @@
 return None;
   }
 
-  bool allowDebugInfoForExternalVar() const override { return true; }
+  bool allowDebugInfoForExternalRef() const override { return true; }
 
   CallingConvCheckResult checkCallingConvention(CallingConv CC) const override 
{
 switch (CC) {
Index: clang/include/clang/Basic/TargetInfo.h
===
--- clang/include/clang/Basic/TargetInfo.h
+++ clang/include/clang/Basic/TargetInfo.h
@@ -1538,8 +1538,8 @@
 
   virtual void setAuxTarget(const TargetInfo *Aux) {}
 
-  /// Whether target allows debuginfo types for decl only variables.
-  virtual bool allowDebugInfoForExternalVar() const { return false; }
+  /// Whether target allows debuginfo types for decl only variables/functions.
+  virtual bool allowDebugInfoForExternalRef() const { return false; }
 
 protected:
   /// Copy type and layout related info.


Index: clang/test/CodeGen/debug-info-extern-callback.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-callback.c
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -x c -debug-info-kind=limited -triple bpf-linux-gnu -emit-llvm %s -o - | FileCheck %s
+
+extern int do_work1(int);
+long bpf_helper1(void *callback_fn);
+long prog1() {
+  return bpf_helper1(_work1);
+}
+
+extern int do_work2();
+long prog2_1() {
+  return (long)_work2;
+}
+int do_work2() { return 0; }
+long prog2_2() {
+  return (long)_work2;
+}
+
+// CHECK: declare !dbg ![[FUNC1:[0-9]+]] i32 @do_work1
+// CHECK: define dso_local i32 @do_work2() #{{[0-9]+}} !dbg ![[FUNC2:[0-9]+]]
+
+// CHECK: ![[FUNC1]] = !DISubprogram(name: "do_work1"
+// CHECK: ![[FUNC2]] = distinct !DISubprogram(name: "do_work2"
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -12667,7 +12667,7 @@
 Diag(Var->getLocation(), diag::note_private_extern);
   }
 
-  

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

Looks like stripPointerCasts() is enough to make it work (no null Function 
pointer any more). This is consistent with

  llvm::Constant *CodeGenModule::GetOrCreateLLVMFunction(
  StringRef MangledName, llvm::Type *Ty, GlobalDecl GD, bool ForVTable,
  bool DontDefer, bool IsThunk, llvm::AttributeList ExtraAttrs,
  ForDefinition_t IsForDefinition) {
const Decl *D = GD.getDecl();
  ...
// Make sure the result is of the requested type.
if (!IsIncompleteFunction) {
  assert(F->getFunctionType() == Ty);
  return F;
} 
  
llvm::Type *PTy = llvm::PointerType::getUnqual(Ty);
return llvm::ConstantExpr::getBitCast(F, PTy);
  }

It either returns a Function, or a Function-Casting-To-ConstantExpr.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

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

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/lib/CodeGen/CGExpr.cpp:2843-2844
+  CGDebugInfo *DI = CGM.getModuleDebugInfo();
+  auto *Fn = dyn_cast(LV.getPointer(*this));
+  if (DI && Fn && !Fn->getSubprogram())
+DI->EmitFunctionDecl(FD, FD->getLocation(), T, Fn);

yonghong-song wrote:
> dblaikie wrote:
> > It looks like there isn't any test coverage for the case where Fn is null 
> > here (I added an assertion that Fn is non-null and it didn't fire when 
> > running check-clang) - please add some to this patch. (I'd like to then 
> > look at those cases to better understand when they come up and whether 
> > there's a different/better way to phrase this code to handle those cases)
> Sorry. It is my fault. Tested with a slight different test case than this one 
> in my actual test setup. Now the new test should reflect null Fn. Thanks!
Thanks for the updated test case!

I think for generality, either this code shouldn't use the `LV` value - instead 
calling `GetOrCreateLLVMFunction` (though that's probably not great - since 
that would involve things like going through ConvertType and other shenanigans 
in `GetAddrOfFunction` - I guess if pieces of `GetAddrToFunction`, 
`EmitFunctionDeclPointer`, and `EmitFunctionDeclPointer` were split up maybe it 
could be done - or changet he return value to include the raw llvm::Function 
result in addition to the LValue (& keep the existing function name returning 
just the LValue for other callers)) - or doing more work to unwrap the Constant 
- maybe it would be enough to use "stripPointerCasts" & that'd get you 
something you can cast to llvm::Function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

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

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added inline comments.



Comment at: clang/lib/CodeGen/CGExpr.cpp:2843-2844
+  CGDebugInfo *DI = CGM.getModuleDebugInfo();
+  auto *Fn = dyn_cast(LV.getPointer(*this));
+  if (DI && Fn && !Fn->getSubprogram())
+DI->EmitFunctionDecl(FD, FD->getLocation(), T, Fn);

dblaikie wrote:
> It looks like there isn't any test coverage for the case where Fn is null 
> here (I added an assertion that Fn is non-null and it didn't fire when 
> running check-clang) - please add some to this patch. (I'd like to then look 
> at those cases to better understand when they come up and whether there's a 
> different/better way to phrase this code to handle those cases)
Sorry. It is my fault. Tested with a slight different test case than this one 
in my actual test setup. Now the new test should reflect null Fn. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

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

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song updated this revision to Diff 340606.
yonghong-song added a comment.

- somehow previous test case didn't trigger nullptr for Fn. Replaced it with a 
working test case.

With the following change,

  diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
  index a784aade88da..fc86c1a72056 100644
  --- a/clang/lib/CodeGen/CGExpr.cpp
  +++ b/clang/lib/CodeGen/CGExpr.cpp
  @@ -2841,6 +2841,7 @@ LValue CodeGenFunction::EmitDeclRefLValue(const 
DeclRefExpr *E) {
   if (getContext().getTargetInfo().allowDebugInfoForExternalRef()) {
 CGDebugInfo *DI = CGM.getModuleDebugInfo();
 auto *Fn = dyn_cast(LV.getPointer(*this));
  +  fprintf(stderr, "Fn = %p\n", (void *)Fn);
 if (DI && Fn && !Fn->getSubprogram())
   DI->EmitFunctionDecl(FD, FD->getLocation(), T, Fn);
   }

I have the following for the added test case,

  $ clang -cc1 -x c -debug-info-kind=limited -triple bpf-linux-gnu -emit-llvm 
debug-info-extern-callback.c
  Fn = 0x7afcf28
  Fn = 0x7afce98
  Fn = (nil)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

Files:
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/Targets/BPF.h
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/debug-info-extern-callback.c


Index: clang/test/CodeGen/debug-info-extern-callback.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-callback.c
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -x c -debug-info-kind=limited -triple bpf-linux-gnu 
-emit-llvm %s -o - | FileCheck %s
+
+extern int do_work1(int);
+long bpf_helper1(void *callback_fn);
+long prog1() {
+  return bpf_helper1(_work1);
+}
+
+extern int do_work2();
+long prog2_1() {
+  return (long)_work2;
+}
+int do_work2() { return 0; }
+long prog2_2() {
+  return (long)_work2;
+}
+
+// CHECK: declare !dbg ![[FUNC1:[0-9]+]] i32 @do_work1
+// CHECK: define dso_local i32 @do_work2() #{{[0-9]+}} !dbg ![[FUNC2:[0-9]+]]
+
+// CHECK: ![[FUNC1]] = !DISubprogram(name: "do_work1"
+// CHECK: ![[FUNC2]] = distinct !DISubprogram(name: "do_work2"
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -12667,7 +12667,7 @@
 Diag(Var->getLocation(), diag::note_private_extern);
   }
 
-  if (Context.getTargetInfo().allowDebugInfoForExternalVar() &&
+  if (Context.getTargetInfo().allowDebugInfoForExternalRef() &&
   !Var->isInvalidDecl() && !getLangOpts().CPlusPlus)
 ExternalDeclarations.push_back(Var);
 
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -2834,8 +2834,19 @@
 return LV;
   }
 
-  if (const auto *FD = dyn_cast(ND))
-return EmitFunctionDeclLValue(*this, E, FD);
+  if (const auto *FD = dyn_cast(ND)) {
+LValue LV = EmitFunctionDeclLValue(*this, E, FD);
+
+// Emit debuginfo for the function declaration if the target wants to.
+if (getContext().getTargetInfo().allowDebugInfoForExternalRef()) {
+  CGDebugInfo *DI = CGM.getModuleDebugInfo();
+  auto *Fn = dyn_cast(LV.getPointer(*this));
+  if (DI && Fn && !Fn->getSubprogram())
+DI->EmitFunctionDecl(FD, FD->getLocation(), T, Fn);
+}
+
+return LV;
+  }
 
   // FIXME: While we're emitting a binding from an enclosing scope, all other
   // DeclRefExprs we see should be implicitly treated as if they also refer to
Index: clang/lib/Basic/Targets/BPF.h
===
--- clang/lib/Basic/Targets/BPF.h
+++ clang/lib/Basic/Targets/BPF.h
@@ -76,7 +76,7 @@
 return None;
   }
 
-  bool allowDebugInfoForExternalVar() const override { return true; }
+  bool allowDebugInfoForExternalRef() const override { return true; }
 
   CallingConvCheckResult checkCallingConvention(CallingConv CC) const override 
{
 switch (CC) {
Index: clang/include/clang/Basic/TargetInfo.h
===
--- clang/include/clang/Basic/TargetInfo.h
+++ clang/include/clang/Basic/TargetInfo.h
@@ -1538,8 +1538,8 @@
 
   virtual void setAuxTarget(const TargetInfo *Aux) {}
 
-  /// Whether target allows debuginfo types for decl only variables.
-  virtual bool allowDebugInfoForExternalVar() const { return false; }
+  /// Whether target allows debuginfo types for decl only variables/functions.
+  virtual bool allowDebugInfoForExternalRef() const { return false; }
 
 protected:
   /// Copy type and layout related info.


Index: clang/test/CodeGen/debug-info-extern-callback.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-callback.c
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -x c 

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

(ah, sorry, failed to submit comment)




Comment at: clang/lib/CodeGen/CGExpr.cpp:2843-2844
+  CGDebugInfo *DI = CGM.getModuleDebugInfo();
+  auto *Fn = dyn_cast(LV.getPointer(*this));
+  if (DI && Fn && !Fn->getSubprogram())
+DI->EmitFunctionDecl(FD, FD->getLocation(), T, Fn);

It looks like there isn't any test coverage for the case where Fn is null here 
(I added an assertion that Fn is non-null and it didn't fire when running 
check-clang) - please add some to this patch. (I'd like to then look at those 
cases to better understand when they come up and whether there's a 
different/better way to phrase this code to handle those cases)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

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

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

@dblaikie ping. Did you get a chance of looking at it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

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

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D100567#2702394 , @yonghong-song 
wrote:

> ping @dblaikie could you take a look of my new investigation?

Yep, it's on my list to look at.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

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

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

ping @dblaikie could you take a look of my new investigation?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

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

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

@dblaikie I did some investigation, found the root cause and provided an 
alternative solution. Could you check whether you are okay with either approach?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

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

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

Did some further investigation, the following is the callchain:

  EmitFunctionDeclLValue
 EmitFunctionDeclPointer
 GetAddrOfFunction
 GetOrCreateLLVMFunction

The GetOrCreateLLVMFunction has the following comments:

  /// GetOrCreateLLVMFunction - If the specified mangled name is not in the
  /// module, create and return an llvm Function with the specified type. If 
there
  /// is something in the module with the specified name, return it potentially
  /// bitcasted to the right type.
  ///
  /// If D is non-null, it specifies a decl that correspond to this.  This is 
used
  /// to set the attributes on the function when it is first created.
  llvm::Constant *CodeGenModule::GetOrCreateLLVMFunction(
  StringRef MangledName, llvm::Type *Ty, GlobalDecl GD, bool ForVTable,
  bool DontDefer, bool IsThunk, llvm::AttributeList ExtraAttrs,
  ForDefinition_t IsForDefinition) {
const Decl *D = GD.getDecl();
  ...

Basically, GetOrCreateLLVMFunction may return a Function or a Const
depending on whether there is an actual definition.

I still like to generate DebugInfo in the current place. The main reason
is this is the place where '' happens and it will ensure the
generated debuginfo will be useful for BPF. We could add some
comments here to explain why sometimes Fn could be nullptr.

Alternatively, we could have the following change,

  diff --git a/clang/lib/CodeGen/CodeGenModule.cpp 
b/clang/lib/CodeGen/CodeGenModule.cpp
  index f719f009ea99..2a6ce6468a8c 100644
  --- a/clang/lib/CodeGen/CodeGenModule.cpp
  +++ b/clang/lib/CodeGen/CodeGenModule.cpp
  @@ -3589,6 +3589,15 @@ llvm::Constant *CodeGenModule::GetOrCreateLLVMFunction(
 // Make sure the result is of the requested type.
 if (!IsIncompleteFunction) {
   assert(F->getFunctionType() == Ty);
  +
  +// Emit debuginfo for the function declaration if the target wants to.
  +if (Context.getTargetInfo().allowDebugInfoForExternalRef()) {
  +  CGDebugInfo *DI = getModuleDebugInfo();
  +  const FunctionDecl *FD = cast_or_null(D);
  +  if (DI && FD && !FD->isDefined())
  +DI->EmitFunctionDecl(FD, FD->getLocation(), FD->getType(), F);
  +}
  +
   return F;
 }

We can emit the debuginfo inside function GetOrCreateLLVMFunction.
We don't need to do dedup (like checking Fn->getSubprogram()) and
F is known to be non-null. The only drawback (maybe only to me) is
that this debuginfo generation is a little further from the use case.
But if you think this is better, I am fine too. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

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

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D100567#2697415 , @yonghong-song 
wrote:

> To answer one of your questions above, if there is a function definition 
> before, dyn_cast(...) will return nullptr. I tried starting 
> from "Value" class and found dyn_cast to llvm::ConstantExpr will result in a 
> non-null object, but I did not trace down further with subclasses of 
> llvm::ConstantExpr.

Might be worth understanding what's going on here before committing this patch, 
as it might not be the right direction.

(perhaps the right thing to do is to look at the AST expression rather than 
whatever IR entity is created for it?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

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

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

To answer one of your questions above, if there is a function definition 
before, dyn_cast(...) will return nullptr. I tried starting 
from "Value" class and found dyn_cast to llvm::ConstantExpr will result in a 
non-null object, but I did not trace down further with subclasses of 
llvm::ConstantExpr.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

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

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

In D100567#2697412 , @dblaikie wrote:

> In D100567#2696095 , @yonghong-song 
> wrote:
>
>> Sorry, I know what is the segfault now after some debugging. It is because 
>> `auto *Fn = dyn_cast(LV.getPointer(*this));` is a NULL 
>> pointer after there is a definition before this.
>
> Hmm, not sure I follow - is `LV.getPointer(*this)` returning null, or is the 
> dyn_cast returning null because the thing isn't an llvm::Function? If it's 
> not a function, what is it?

The patched code is `LV.getPointer(*this)->dump();`, so `LV.getPointer(*this)` 
is not return NULL. It returns a valid object. If the function already has a 
definition, it returns a llvm:ConstantExpr,
and if you call the `LV.getPointer(*this)->dump()`, and it prints `i32 (...)* 
bitcast (i32 ()* @foo to i32 (...)*)`. I didn't try subclasses of 
"llvm:ConstantExpr". I think you may have a better idea
what the real object could be?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

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

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D100567#2696095 , @yonghong-song 
wrote:

> Sorry, I know what is the segfault now after some debugging. It is because 
> `auto *Fn = dyn_cast(LV.getPointer(*this));` is a NULL 
> pointer after there is a definition before this.

Hmm, not sure I follow - is `LV.getPointer(*this)` returning null, or is the 
dyn_cast returning null because the thing isn't an llvm::Function? If it's not 
a function, what is it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

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

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

I did some debugging on why DeclRefExpr evaluated not to be a Function pointer. 
Here is what I got.
I made the following clang change to dump out the pointer to the emited value 
(''):

diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index a784aade88da..80d19c7bcdf7 100644

- a/clang/lib/CodeGen/CGExpr.cpp

+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -2840,6 +2840,7 @@ LValue CodeGenFunction::EmitDeclRefLValue(const 
DeclRefExpr *E) {

  // Emit debuginfo for the function declaration if the target wants to.
  if (getContext().getTargetInfo().allowDebugInfoForExternalRef()) {
CGDebugInfo *DI = CGM.getModuleDebugInfo();

+  LV.getPointer(*this)->dump();

  auto *Fn = dyn_cast(LV.getPointer(*this));
  if (DI && Fn && !Fn->getSubprogram())
DI->EmitFunctionDecl(FD, FD->getLocation(), T, Fn);

The C code and the compiler run:

$ cat t1.c
 extern int foo() __attribute__((section("abc")));
 long test() {

  return (long)

}
 int foo() { return 0; }
 long test2() {

  return (long)

}
 $ clang -target bpf -O2 -S -g -emit-llvm t1.c
 declare dso_local i32 @foo(...) #0 section "abc"

i32 (...)* bitcast (i32 ()* @foo to i32 (...)*)

You can see, if there is no definition, the emitted insn for '' is a 
reference
to a declaration (llvm::Function type).
After the function definition, the emitted insn for '' is a reference to
some kind of a function definition (I checked it is a llvm:ConstantExpr).
That is the reason why I hit a NULL pointer for

  auto *Fn = dyn_cast(LV.getPointer(*this));

in one of tests.

@dblaikie the code is ready to be reviewed again. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

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

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song updated this revision to Diff 338323.
yonghong-song added a comment.

- add checking CGDebugInfo pointer. In my original patch but got lost in the 
revision.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

Files:
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/Targets/BPF.h
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/debug-info-extern-callback.c

Index: clang/test/CodeGen/debug-info-extern-callback.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-callback.c
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -x c -debug-info-kind=limited -triple bpf-linux-gnu -emit-llvm %s -o - | FileCheck %s
+
+extern int do_work1(int);
+long bpf_helper1(void *callback_fn);
+long prog1() {
+  return bpf_helper1(_work1);
+}
+
+extern int do_work2(int);
+long bpf_helper2(void *callback_fn);
+int do_work2(int arg) {
+  return arg;
+}
+long prog2() {
+  return bpf_helper2(_work2);
+}
+
+extern int do_work3(int);
+long bpf_helper3(void *callback_fn);
+long prog3() {
+  return bpf_helper3(_work3);
+}
+int do_work3(int arg) {
+  return arg;
+}
+
+// CHECK: declare !dbg ![[FUNC1:[0-9]+]] i32 @do_work1(i32)
+// CHECK: define dso_local i32 @do_work2(i32 %arg) #{{[0-9]+}} !dbg ![[FUNC2:[0-9]+]]
+// CHECK: define dso_local i32 @do_work3(i32 %arg) #{{[0-9]+}} !dbg ![[FUNC3:[0-9]+]]
+
+// CHECK: ![[FUNC1]] = !DISubprogram(name: "do_work1"
+// CHECK: ![[FUNC2]] = distinct !DISubprogram(name: "do_work2"
+// CHECK: ![[FUNC3]] = distinct !DISubprogram(name: "do_work3"
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -12667,7 +12667,7 @@
 Diag(Var->getLocation(), diag::note_private_extern);
   }
 
-  if (Context.getTargetInfo().allowDebugInfoForExternalVar() &&
+  if (Context.getTargetInfo().allowDebugInfoForExternalRef() &&
   !Var->isInvalidDecl() && !getLangOpts().CPlusPlus)
 ExternalDeclarations.push_back(Var);
 
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -2834,8 +2834,19 @@
 return LV;
   }
 
-  if (const auto *FD = dyn_cast(ND))
-return EmitFunctionDeclLValue(*this, E, FD);
+  if (const auto *FD = dyn_cast(ND)) {
+LValue LV = EmitFunctionDeclLValue(*this, E, FD);
+
+// Emit debuginfo for the function declaration if the target wants to.
+if (getContext().getTargetInfo().allowDebugInfoForExternalRef()) {
+  CGDebugInfo *DI = CGM.getModuleDebugInfo();
+  auto *Fn = dyn_cast(LV.getPointer(*this));
+  if (DI && Fn && !Fn->getSubprogram())
+DI->EmitFunctionDecl(FD, FD->getLocation(), T, Fn);
+}
+
+return LV;
+  }
 
   // FIXME: While we're emitting a binding from an enclosing scope, all other
   // DeclRefExprs we see should be implicitly treated as if they also refer to
Index: clang/lib/Basic/Targets/BPF.h
===
--- clang/lib/Basic/Targets/BPF.h
+++ clang/lib/Basic/Targets/BPF.h
@@ -76,7 +76,7 @@
 return None;
   }
 
-  bool allowDebugInfoForExternalVar() const override { return true; }
+  bool allowDebugInfoForExternalRef() const override { return true; }
 
   CallingConvCheckResult checkCallingConvention(CallingConv CC) const override {
 switch (CC) {
Index: clang/include/clang/Basic/TargetInfo.h
===
--- clang/include/clang/Basic/TargetInfo.h
+++ clang/include/clang/Basic/TargetInfo.h
@@ -1538,8 +1538,8 @@
 
   virtual void setAuxTarget(const TargetInfo *Aux) {}
 
-  /// Whether target allows debuginfo types for decl only variables.
-  virtual bool allowDebugInfoForExternalVar() const { return false; }
+  /// Whether target allows debuginfo types for decl only variables/functions.
+  virtual bool allowDebugInfoForExternalRef() const { return false; }
 
 protected:
   /// Copy type and layout related info.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song updated this revision to Diff 338252.
yonghong-song added a comment.

- isDefined() is not necessary, segfault is caused by a nullptr by causing a 
LV.getPointer(). Check nullptr instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

Files:
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/Targets/BPF.h
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/debug-info-extern-callback.c

Index: clang/test/CodeGen/debug-info-extern-callback.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-callback.c
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -x c -debug-info-kind=limited -triple bpf-linux-gnu -emit-llvm %s -o - | FileCheck %s
+
+extern int do_work1(int);
+long bpf_helper1(void *callback_fn);
+long prog1() {
+  return bpf_helper1(_work1);
+}
+
+extern int do_work2(int);
+long bpf_helper2(void *callback_fn);
+int do_work2(int arg) {
+  return arg;
+}
+long prog2() {
+  return bpf_helper2(_work2);
+}
+
+extern int do_work3(int);
+long bpf_helper3(void *callback_fn);
+long prog3() {
+  return bpf_helper3(_work3);
+}
+int do_work3(int arg) {
+  return arg;
+}
+
+// CHECK: declare !dbg ![[FUNC1:[0-9]+]] i32 @do_work1(i32)
+// CHECK: define dso_local i32 @do_work2(i32 %arg) #{{[0-9]+}} !dbg ![[FUNC2:[0-9]+]]
+// CHECK: define dso_local i32 @do_work3(i32 %arg) #{{[0-9]+}} !dbg ![[FUNC3:[0-9]+]]
+
+// CHECK: ![[FUNC1]] = !DISubprogram(name: "do_work1"
+// CHECK: ![[FUNC2]] = distinct !DISubprogram(name: "do_work2"
+// CHECK: ![[FUNC3]] = distinct !DISubprogram(name: "do_work3"
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -12667,7 +12667,7 @@
 Diag(Var->getLocation(), diag::note_private_extern);
   }
 
-  if (Context.getTargetInfo().allowDebugInfoForExternalVar() &&
+  if (Context.getTargetInfo().allowDebugInfoForExternalRef() &&
   !Var->isInvalidDecl() && !getLangOpts().CPlusPlus)
 ExternalDeclarations.push_back(Var);
 
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -2834,8 +2834,20 @@
 return LV;
   }
 
-  if (const auto *FD = dyn_cast(ND))
-return EmitFunctionDeclLValue(*this, E, FD);
+  if (const auto *FD = dyn_cast(ND)) {
+LValue LV = EmitFunctionDeclLValue(*this, E, FD);
+
+// Emit debuginfo for the function declaration if the target wants to.
+if (getContext().getTargetInfo().allowDebugInfoForExternalRef()) {
+  auto *Fn = dyn_cast(LV.getPointer(*this));
+  if (Fn && !Fn->getSubprogram()) {
+CGM.getModuleDebugInfo()->EmitFunctionDecl(FD, FD->getLocation(), T,
+   Fn);
+  }
+}
+
+return LV;
+  }
 
   // FIXME: While we're emitting a binding from an enclosing scope, all other
   // DeclRefExprs we see should be implicitly treated as if they also refer to
Index: clang/lib/Basic/Targets/BPF.h
===
--- clang/lib/Basic/Targets/BPF.h
+++ clang/lib/Basic/Targets/BPF.h
@@ -76,7 +76,7 @@
 return None;
   }
 
-  bool allowDebugInfoForExternalVar() const override { return true; }
+  bool allowDebugInfoForExternalRef() const override { return true; }
 
   CallingConvCheckResult checkCallingConvention(CallingConv CC) const override {
 switch (CC) {
Index: clang/include/clang/Basic/TargetInfo.h
===
--- clang/include/clang/Basic/TargetInfo.h
+++ clang/include/clang/Basic/TargetInfo.h
@@ -1538,8 +1538,8 @@
 
   virtual void setAuxTarget(const TargetInfo *Aux) {}
 
-  /// Whether target allows debuginfo types for decl only variables.
-  virtual bool allowDebugInfoForExternalVar() const { return false; }
+  /// Whether target allows debuginfo types for decl only variables/functions.
+  virtual bool allowDebugInfoForExternalRef() const { return false; }
 
 protected:
   /// Copy type and layout related info.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

Sorry, I know what is the segfault now after some debugging. It is because 
`auto *Fn = dyn_cast(LV.getPointer(*this));` is a NULL pointer 
after there is a definition before this. Will pose another patch but with test 
case remains to cover mix of declaration, pointer reference and definition.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

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

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

In D100567#2695827 , @dblaikie wrote:

> Generally looks OK, but could you explain more about the problems 
> with/motivation for the declaration/definition issue? (probably worth noting 
> in the patch description maybe comment in code and test)

First about testing Fn->getSubprogram(), the reason is to avoid repeatedly 
attempt to generate debuginfo, e.g.,

  extern int foo(int);
  long test() { ...  ...  ...  ... }

Without checking it will try to generate the debuginfo three times although 
later it is deduplicated, but still a waste.

But Fn->getSubprogram() alone may cause issues if the debuginfo has been 
generated for a definition,

  extern int do_work2(int);
  long bpf_helper2(void *callback_fn);
  int do_work2(int arg) {
return arg;
  }
  long prog2() {
return bpf_helper2(_work2);
  }

We will have a segfault

  #3 0x01bf2138 CrashRecoverySignalHandler(int) 
CrashRecoveryContext.cpp:0:0
 

#4 0x7f93e4fdfb20 __restore_rt sigaction.c:0:0  
   
 #5 0x01506f70 llvm::Value::getMetadata(unsigned int) const 
(/home/yhs/work/llvm-project/llvm/build.cur/install/bin/clang-13+0x1506f70) 

 #6 0x015074db llvm::Function::getSubprogram() const 
(/home/yhs/work/llvm-project/llvm/build.cur/install/bin/clang-13+0x15074db) 
   
 #7 0x021ff5cf 
clang::CodeGen::CodeGenFunction::EmitDeclRefLValue(clang::DeclRefExpr const*) 
(/home/yhs/work/llvm-project/llvm/build.cur/install/bin
/clang-13+0x21ff5cf)

 #8 0x021fd866 clang::CodeGen::CodeGenFunction::EmitLValue(clang::Expr 
const*) (/home/yhs/work/llvm-project/llvm/build.cur/install/bin/clang-13+0x21
fd866)

So we need to stop calling Fn->getSubprogram() if FD->isDefined() is true. 
Hence I added FD->isDefined().

In summary, the new code

  if (!FD->isDefined() && !Fn->getSubprogram()) {
CGM.getModuleDebugInfo()->EmitFunctionDecl(FD, FD->getLocation(), T,
   Fn);
  }

is an optimization to avoid repeatedly generating the debuginfo. If 
FD->isDefined(), stop,
otherwise, if Fn->getSubprogram() which means having generated, stop. 
Otherwise, generate one.

Note that my previous code without " if (!FD->isDefined() && 
!Fn->getSubprogram())" totally
working fine, I just feel a little bit waste.

I didn't debug why we have error for this test and you may have a better idea 
from the above
stack trace.

  extern int do_work2(int);
  long bpf_helper2(void *callback_fn);
  int do_work2(int arg) {
return arg;
  }
  long prog2() {
return bpf_helper2(_work2);
  }

Apparently, do_work2 already generated a subprogram debuginfo when inside 
prog2() another attempt tried to check whether the subprogram has been 
generated or not.
I have added a test for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

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

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Generally looks OK, but could you explain more about the problems 
with/motivation for the declaration/definition issue? (probably worth noting in 
the patch description maybe comment in code and test)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

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

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

@dblaikie I renamed the variable, added a few guards to ensure safety and avoid 
unnecessary attempt to generate duplicated debuginfo, and enhanced the test 
case to cover mix of declaration, accessing extern func address and definition 
of the function. Could you take a look?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

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

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song updated this revision to Diff 338202.
yonghong-song added a comment.

fix a clang-format warning


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

Files:
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/Targets/BPF.h
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/debug-info-extern-callback.c

Index: clang/test/CodeGen/debug-info-extern-callback.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-callback.c
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -x c -debug-info-kind=limited -triple bpf-linux-gnu -emit-llvm %s -o - | FileCheck %s
+
+extern int do_work1(int);
+long bpf_helper1(void *callback_fn);
+long prog1() {
+  return bpf_helper1(_work1);
+}
+
+extern int do_work2(int);
+long bpf_helper2(void *callback_fn);
+int do_work2(int arg) {
+  return arg;
+}
+long prog2() {
+  return bpf_helper2(_work2);
+}
+
+extern int do_work3(int);
+long bpf_helper3(void *callback_fn);
+long prog3() {
+  return bpf_helper3(_work3);
+}
+int do_work3(int arg) {
+  return arg;
+}
+
+// CHECK: declare !dbg ![[FUNC1:[0-9]+]] i32 @do_work1(i32)
+// CHECK: define dso_local i32 @do_work2(i32 %arg) #{{[0-9]+}} !dbg ![[FUNC2:[0-9]+]]
+// CHECK: define dso_local i32 @do_work3(i32 %arg) #{{[0-9]+}} !dbg ![[FUNC3:[0-9]+]]
+
+// CHECK: ![[FUNC1]] = !DISubprogram(name: "do_work1"
+// CHECK: ![[FUNC2]] = distinct !DISubprogram(name: "do_work2"
+// CHECK: ![[FUNC3]] = distinct !DISubprogram(name: "do_work3"
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -12667,7 +12667,7 @@
 Diag(Var->getLocation(), diag::note_private_extern);
   }
 
-  if (Context.getTargetInfo().allowDebugInfoForExternalVar() &&
+  if (Context.getTargetInfo().allowDebugInfoForExternalRef() &&
   !Var->isInvalidDecl() && !getLangOpts().CPlusPlus)
 ExternalDeclarations.push_back(Var);
 
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -2834,8 +2834,20 @@
 return LV;
   }
 
-  if (const auto *FD = dyn_cast(ND))
-return EmitFunctionDeclLValue(*this, E, FD);
+  if (const auto *FD = dyn_cast(ND)) {
+LValue LV = EmitFunctionDeclLValue(*this, E, FD);
+
+// Emit debuginfo for the function declaration if the target wants to.
+if (getContext().getTargetInfo().allowDebugInfoForExternalRef()) {
+  auto *Fn = dyn_cast(LV.getPointer(*this));
+  if (!FD->isDefined() && !Fn->getSubprogram()) {
+CGM.getModuleDebugInfo()->EmitFunctionDecl(FD, FD->getLocation(), T,
+   Fn);
+  }
+}
+
+return LV;
+  }
 
   // FIXME: While we're emitting a binding from an enclosing scope, all other
   // DeclRefExprs we see should be implicitly treated as if they also refer to
Index: clang/lib/Basic/Targets/BPF.h
===
--- clang/lib/Basic/Targets/BPF.h
+++ clang/lib/Basic/Targets/BPF.h
@@ -76,7 +76,7 @@
 return None;
   }
 
-  bool allowDebugInfoForExternalVar() const override { return true; }
+  bool allowDebugInfoForExternalRef() const override { return true; }
 
   CallingConvCheckResult checkCallingConvention(CallingConv CC) const override {
 switch (CC) {
Index: clang/include/clang/Basic/TargetInfo.h
===
--- clang/include/clang/Basic/TargetInfo.h
+++ clang/include/clang/Basic/TargetInfo.h
@@ -1538,8 +1538,8 @@
 
   virtual void setAuxTarget(const TargetInfo *Aux) {}
 
-  /// Whether target allows debuginfo types for decl only variables.
-  virtual bool allowDebugInfoForExternalVar() const { return false; }
+  /// Whether target allows debuginfo types for decl only variables/functions.
+  virtual bool allowDebugInfoForExternalRef() const { return false; }
 
 protected:
   /// Copy type and layout related info.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song updated this revision to Diff 338002.
yonghong-song added a comment.

- checked both !FD->isDefined() and !Fn->getSubprogram() before emitting 
debuginfo for the declaration
- added additional tests to mix DeclRefExpr and actual function definition.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

Files:
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/Targets/BPF.h
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/debug-info-extern-callback.c

Index: clang/test/CodeGen/debug-info-extern-callback.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-callback.c
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -x c -debug-info-kind=limited -triple bpf-linux-gnu -emit-llvm %s -o - | FileCheck %s
+
+extern int do_work1(int);
+long bpf_helper1(void *callback_fn);
+long prog1() {
+  return bpf_helper1(_work1);
+}
+
+extern int do_work2(int);
+long bpf_helper2(void *callback_fn);
+int do_work2(int arg) {
+  return arg;
+}
+long prog2() {
+  return bpf_helper2(_work2);
+}
+
+extern int do_work3(int);
+long bpf_helper3(void *callback_fn);
+long prog3() {
+  return bpf_helper3(_work3);
+}
+int do_work3(int arg) {
+  return arg;
+}
+
+// CHECK: declare !dbg ![[FUNC1:[0-9]+]] i32 @do_work1(i32)
+// CHECK: define dso_local i32 @do_work2(i32 %arg) #{{[0-9]+}} !dbg ![[FUNC2:[0-9]+]]
+// CHECK: define dso_local i32 @do_work3(i32 %arg) #{{[0-9]+}} !dbg ![[FUNC3:[0-9]+]]
+
+// CHECK: ![[FUNC1]] = !DISubprogram(name: "do_work1"
+// CHECK: ![[FUNC2]] = distinct !DISubprogram(name: "do_work2"
+// CHECK: ![[FUNC3]] = distinct !DISubprogram(name: "do_work3"
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -12667,7 +12667,7 @@
 Diag(Var->getLocation(), diag::note_private_extern);
   }
 
-  if (Context.getTargetInfo().allowDebugInfoForExternalVar() &&
+  if (Context.getTargetInfo().allowDebugInfoForExternalRef() &&
   !Var->isInvalidDecl() && !getLangOpts().CPlusPlus)
 ExternalDeclarations.push_back(Var);
 
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -2834,8 +2834,20 @@
 return LV;
   }
 
-  if (const auto *FD = dyn_cast(ND))
-return EmitFunctionDeclLValue(*this, E, FD);
+  if (const auto *FD = dyn_cast(ND)) {
+LValue LV = EmitFunctionDeclLValue(*this, E, FD);
+
+// Emit debuginfo for the function declaration if the target wants to.
+if (getContext().getTargetInfo().allowDebugInfoForExternalRef()) {
+  auto *Fn = dyn_cast(LV.getPointer(*this));
+  if (!FD->isDefined() && !Fn->getSubprogram()) {
+CGM.getModuleDebugInfo()->EmitFunctionDecl(FD, FD->getLocation(),
+   T, Fn);
+  }
+}
+
+return LV;
+  }
 
   // FIXME: While we're emitting a binding from an enclosing scope, all other
   // DeclRefExprs we see should be implicitly treated as if they also refer to
Index: clang/lib/Basic/Targets/BPF.h
===
--- clang/lib/Basic/Targets/BPF.h
+++ clang/lib/Basic/Targets/BPF.h
@@ -76,7 +76,7 @@
 return None;
   }
 
-  bool allowDebugInfoForExternalVar() const override { return true; }
+  bool allowDebugInfoForExternalRef() const override { return true; }
 
   CallingConvCheckResult checkCallingConvention(CallingConv CC) const override {
 switch (CC) {
Index: clang/include/clang/Basic/TargetInfo.h
===
--- clang/include/clang/Basic/TargetInfo.h
+++ clang/include/clang/Basic/TargetInfo.h
@@ -1538,8 +1538,8 @@
 
   virtual void setAuxTarget(const TargetInfo *Aux) {}
 
-  /// Whether target allows debuginfo types for decl only variables.
-  virtual bool allowDebugInfoForExternalVar() const { return false; }
+  /// Whether target allows debuginfo types for decl only variables/functions.
+  virtual bool allowDebugInfoForExternalRef() const { return false; }
 
 protected:
   /// Copy type and layout related info.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song updated this revision to Diff 337993.
yonghong-song added a comment.

- check Fn->getSubprogram() before emit debuginfo. This is consistent with 
EmitFuncDeclForCallSite() and is better than checking FD->isDefined() as we may 
hit multiple DeclRefExpr and all of them will go through emitting debuginfo.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

Files:
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/Targets/BPF.h
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/debug-info-extern-callback.c


Index: clang/test/CodeGen/debug-info-extern-callback.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-callback.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -x c -debug-info-kind=limited -triple bpf-linux-gnu 
-emit-llvm %s -o - | FileCheck %s
+
+extern int do_work(int);
+long bpf_helper(void *callback_fn);
+long prog() {
+   return bpf_helper(_work);
+}
+
+// CHECK: declare !dbg ![[FUNC:[0-9]+]] i32 @do_work(i32)
+// CHECK: ![[FUNC]] = !DISubprogram(name: "do_work"
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -12667,7 +12667,7 @@
 Diag(Var->getLocation(), diag::note_private_extern);
   }
 
-  if (Context.getTargetInfo().allowDebugInfoForExternalVar() &&
+  if (Context.getTargetInfo().allowDebugInfoForExternalRef() &&
   !Var->isInvalidDecl() && !getLangOpts().CPlusPlus)
 ExternalDeclarations.push_back(Var);
 
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -2834,8 +2834,20 @@
 return LV;
   }
 
-  if (const auto *FD = dyn_cast(ND))
-return EmitFunctionDeclLValue(*this, E, FD);
+  if (const auto *FD = dyn_cast(ND)) {
+LValue LV = EmitFunctionDeclLValue(*this, E, FD);
+
+// Emit debuginfo for the function declaration if the target wants to.
+if (getContext().getTargetInfo().allowDebugInfoForExternalRef()) {
+  auto *Fn = dyn_cast(LV.getPointer(*this));
+  if (!Fn->getSubprogram()) {
+CGM.getModuleDebugInfo()->EmitFunctionDecl(FD, FD->getLocation(),
+   T, Fn);
+  }
+}
+
+return LV;
+  }
 
   // FIXME: While we're emitting a binding from an enclosing scope, all other
   // DeclRefExprs we see should be implicitly treated as if they also refer to
Index: clang/lib/Basic/Targets/BPF.h
===
--- clang/lib/Basic/Targets/BPF.h
+++ clang/lib/Basic/Targets/BPF.h
@@ -76,7 +76,7 @@
 return None;
   }
 
-  bool allowDebugInfoForExternalVar() const override { return true; }
+  bool allowDebugInfoForExternalRef() const override { return true; }
 
   CallingConvCheckResult checkCallingConvention(CallingConv CC) const override 
{
 switch (CC) {
Index: clang/include/clang/Basic/TargetInfo.h
===
--- clang/include/clang/Basic/TargetInfo.h
+++ clang/include/clang/Basic/TargetInfo.h
@@ -1538,8 +1538,8 @@
 
   virtual void setAuxTarget(const TargetInfo *Aux) {}
 
-  /// Whether target allows debuginfo types for decl only variables.
-  virtual bool allowDebugInfoForExternalVar() const { return false; }
+  /// Whether target allows debuginfo types for decl only variables/functions.
+  virtual bool allowDebugInfoForExternalRef() const { return false; }
 
 protected:
   /// Copy type and layout related info.


Index: clang/test/CodeGen/debug-info-extern-callback.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-callback.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -x c -debug-info-kind=limited -triple bpf-linux-gnu -emit-llvm %s -o - | FileCheck %s
+
+extern int do_work(int);
+long bpf_helper(void *callback_fn);
+long prog() {
+	return bpf_helper(_work);
+}
+
+// CHECK: declare !dbg ![[FUNC:[0-9]+]] i32 @do_work(i32)
+// CHECK: ![[FUNC]] = !DISubprogram(name: "do_work"
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -12667,7 +12667,7 @@
 Diag(Var->getLocation(), diag::note_private_extern);
   }
 
-  if (Context.getTargetInfo().allowDebugInfoForExternalVar() &&
+  if (Context.getTargetInfo().allowDebugInfoForExternalRef() &&
   !Var->isInvalidDecl() && !getLangOpts().CPlusPlus)
 ExternalDeclarations.push_back(Var);
 
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song updated this revision to Diff 337988.
yonghong-song added a comment.

check FD->isDefined() as well before emit debuginfo for the declaration. It is 
okay to emit a declaration subprogram and later refined to be with definition. 
But it is not okay to refine a definition to a declaration. So add check here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

Files:
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/Targets/BPF.h
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/debug-info-extern-callback.c


Index: clang/test/CodeGen/debug-info-extern-callback.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-callback.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -x c -debug-info-kind=limited -triple bpf-linux-gnu 
-emit-llvm %s -o - | FileCheck %s
+
+extern int do_work(int);
+long bpf_helper(void *callback_fn);
+long prog() {
+   return bpf_helper(_work);
+}
+
+// CHECK: declare !dbg ![[FUNC:[0-9]+]] i32 @do_work(i32)
+// CHECK: ![[FUNC]] = !DISubprogram(name: "do_work"
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -12667,7 +12667,7 @@
 Diag(Var->getLocation(), diag::note_private_extern);
   }
 
-  if (Context.getTargetInfo().allowDebugInfoForExternalVar() &&
+  if (Context.getTargetInfo().allowDebugInfoForExternalRef() &&
   !Var->isInvalidDecl() && !getLangOpts().CPlusPlus)
 ExternalDeclarations.push_back(Var);
 
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -2834,8 +2834,20 @@
 return LV;
   }
 
-  if (const auto *FD = dyn_cast(ND))
-return EmitFunctionDeclLValue(*this, E, FD);
+  if (const auto *FD = dyn_cast(ND)) {
+LValue LV = EmitFunctionDeclLValue(*this, E, FD);
+
+// Emit debuginfo for the function declaration if the target wants to.
+if (!FD->isDefined() &&
+getContext().getTargetInfo().allowDebugInfoForExternalRef()) {
+  CGDebugInfo *DI = CGM.getModuleDebugInfo();
+  auto *Fn = dyn_cast(LV.getPointer(*this));
+  if (DI && Fn)
+DI->EmitFunctionDecl(FD, FD->getLocation(), T, Fn);
+}
+
+return LV;
+  }
 
   // FIXME: While we're emitting a binding from an enclosing scope, all other
   // DeclRefExprs we see should be implicitly treated as if they also refer to
Index: clang/lib/Basic/Targets/BPF.h
===
--- clang/lib/Basic/Targets/BPF.h
+++ clang/lib/Basic/Targets/BPF.h
@@ -76,7 +76,7 @@
 return None;
   }
 
-  bool allowDebugInfoForExternalVar() const override { return true; }
+  bool allowDebugInfoForExternalRef() const override { return true; }
 
   CallingConvCheckResult checkCallingConvention(CallingConv CC) const override 
{
 switch (CC) {
Index: clang/include/clang/Basic/TargetInfo.h
===
--- clang/include/clang/Basic/TargetInfo.h
+++ clang/include/clang/Basic/TargetInfo.h
@@ -1538,8 +1538,8 @@
 
   virtual void setAuxTarget(const TargetInfo *Aux) {}
 
-  /// Whether target allows debuginfo types for decl only variables.
-  virtual bool allowDebugInfoForExternalVar() const { return false; }
+  /// Whether target allows debuginfo types for decl only variables/functions.
+  virtual bool allowDebugInfoForExternalRef() const { return false; }
 
 protected:
   /// Copy type and layout related info.


Index: clang/test/CodeGen/debug-info-extern-callback.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-callback.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -x c -debug-info-kind=limited -triple bpf-linux-gnu -emit-llvm %s -o - | FileCheck %s
+
+extern int do_work(int);
+long bpf_helper(void *callback_fn);
+long prog() {
+	return bpf_helper(_work);
+}
+
+// CHECK: declare !dbg ![[FUNC:[0-9]+]] i32 @do_work(i32)
+// CHECK: ![[FUNC]] = !DISubprogram(name: "do_work"
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -12667,7 +12667,7 @@
 Diag(Var->getLocation(), diag::note_private_extern);
   }
 
-  if (Context.getTargetInfo().allowDebugInfoForExternalVar() &&
+  if (Context.getTargetInfo().allowDebugInfoForExternalRef() &&
   !Var->isInvalidDecl() && !getLangOpts().CPlusPlus)
 ExternalDeclarations.push_back(Var);
 
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -2834,8 

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song updated this revision to Diff 337948.
yonghong-song edited the summary of this revision.
yonghong-song added a comment.

Rename TargetInfo.allowDebugInfoForExternalVar to 
TargetInfo.allowDebugInfoForExternalRef.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

Files:
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/Targets/BPF.h
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/debug-info-extern-callback.c


Index: clang/test/CodeGen/debug-info-extern-callback.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-callback.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -x c -debug-info-kind=limited -triple bpf-linux-gnu 
-emit-llvm %s -o - | FileCheck %s
+
+extern int do_work(int);
+long bpf_helper(void *callback_fn);
+long prog() {
+   return bpf_helper(_work);
+}
+
+// CHECK: declare !dbg ![[FUNC:[0-9]+]] i32 @do_work(i32)
+// CHECK: ![[FUNC]] = !DISubprogram(name: "do_work"
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -12667,7 +12667,7 @@
 Diag(Var->getLocation(), diag::note_private_extern);
   }
 
-  if (Context.getTargetInfo().allowDebugInfoForExternalVar() &&
+  if (Context.getTargetInfo().allowDebugInfoForExternalRef() &&
   !Var->isInvalidDecl() && !getLangOpts().CPlusPlus)
 ExternalDeclarations.push_back(Var);
 
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -2833,8 +2833,19 @@
 return LV;
   }
 
-  if (const auto *FD = dyn_cast(ND))
-return EmitFunctionDeclLValue(*this, E, FD);
+  if (const auto *FD = dyn_cast(ND)) {
+LValue LV = EmitFunctionDeclLValue(*this, E, FD);
+
+// Emit debuginfo for the function declaration if the target wants to.
+if (getContext().getTargetInfo().allowDebugInfoForExternalRef()) {
+  CGDebugInfo *DI = CGM.getModuleDebugInfo();
+  auto *Fn = dyn_cast(LV.getPointer(*this));
+  if (DI && Fn)
+DI->EmitFunctionDecl(FD, FD->getLocation(), T, Fn);
+}
+
+return LV;
+  }
 
   // FIXME: While we're emitting a binding from an enclosing scope, all other
   // DeclRefExprs we see should be implicitly treated as if they also refer to
Index: clang/lib/Basic/Targets/BPF.h
===
--- clang/lib/Basic/Targets/BPF.h
+++ clang/lib/Basic/Targets/BPF.h
@@ -76,7 +76,7 @@
 return None;
   }
 
-  bool allowDebugInfoForExternalVar() const override { return true; }
+  bool allowDebugInfoForExternalRef() const override { return true; }
 
   CallingConvCheckResult checkCallingConvention(CallingConv CC) const override 
{
 switch (CC) {
Index: clang/include/clang/Basic/TargetInfo.h
===
--- clang/include/clang/Basic/TargetInfo.h
+++ clang/include/clang/Basic/TargetInfo.h
@@ -1538,8 +1538,8 @@
 
   virtual void setAuxTarget(const TargetInfo *Aux) {}
 
-  /// Whether target allows debuginfo types for decl only variables.
-  virtual bool allowDebugInfoForExternalVar() const { return false; }
+  /// Whether target allows debuginfo types for decl only variables/functions.
+  virtual bool allowDebugInfoForExternalRef() const { return false; }
 
 protected:
   /// Copy type and layout related info.


Index: clang/test/CodeGen/debug-info-extern-callback.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-callback.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -x c -debug-info-kind=limited -triple bpf-linux-gnu -emit-llvm %s -o - | FileCheck %s
+
+extern int do_work(int);
+long bpf_helper(void *callback_fn);
+long prog() {
+	return bpf_helper(_work);
+}
+
+// CHECK: declare !dbg ![[FUNC:[0-9]+]] i32 @do_work(i32)
+// CHECK: ![[FUNC]] = !DISubprogram(name: "do_work"
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -12667,7 +12667,7 @@
 Diag(Var->getLocation(), diag::note_private_extern);
   }
 
-  if (Context.getTargetInfo().allowDebugInfoForExternalVar() &&
+  if (Context.getTargetInfo().allowDebugInfoForExternalRef() &&
   !Var->isInvalidDecl() && !getLangOpts().CPlusPlus)
 ExternalDeclarations.push_back(Var);
 
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -2833,8 +2833,19 @@
 return LV;
   }
 
-  if (const auto *FD = dyn_cast(ND))
-return EmitFunctionDeclLValue(*this, E, FD);
+  

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/lib/CodeGen/CGExpr.cpp:2840
+// Emit debuginfo for the function declaration if the target wants to.
+if (getContext().getTargetInfo().allowDebugInfoForExternalVar()) {
+  CGDebugInfo *DI = CGM.getModuleDebugInfo();

yonghong-song wrote:
> dblaikie wrote:
> > Seems like this should be renamed given it's being used for things other 
> > than external variables?
> I noticed this but didn't change it as I thought the current name is *sort 
> of* okay. But you are right. It is not precise. Will change to 
> "allowDebugInfoForExternalRef()" and resubmit. Let me know if you have better 
> function name suggestion.
Sounds good to me, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

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

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added inline comments.



Comment at: clang/lib/CodeGen/CGExpr.cpp:2840
+// Emit debuginfo for the function declaration if the target wants to.
+if (getContext().getTargetInfo().allowDebugInfoForExternalVar()) {
+  CGDebugInfo *DI = CGM.getModuleDebugInfo();

dblaikie wrote:
> Seems like this should be renamed given it's being used for things other than 
> external variables?
I noticed this but didn't change it as I thought the current name is *sort of* 
okay. But you are right. It is not precise. Will change to 
"allowDebugInfoForExternalRef()" and resubmit. Let me know if you have better 
function name suggestion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

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

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

ah, right, because this is powered by seeing the DeclRefExpr only in code 
that's codegen'd - fair enough. Thanks for checking!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

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

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

For the first example, actually clang is smart enough to remove all dead code, 
so nothing generated.

[yhs@devbig003.ftw2 ~/tmp/ext_func_var]$ cat t1.c
extern void f1();
void f2(void *);
inline void f3() {

  f2(f1);

}
[yhs@devbig003.ftw2 ~/tmp/ext_func_var]$ clang -target bpf -g -S -emit-llvm t1.c
[yhs@devbig003.ftw2 ~/tmp/ext_func_var]$ cat t1.ll
; ModuleID = 't1.c'
source_filename = "t1.c"
target datalayout = "e-m:e-p:64:64-i64:64-i128:128-n32:64-S128"
target triple = "bpf"

!llvm.dbg.cu = !{!0}
!llvm.module.flags = !{!3, !4, !5}
!llvm.ident = !{!6}

!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang 
version 13.0.0 (https://github.com/llvm/llvm-project.git 
68275c77c92b89fafbacc31b4f40303bb9e0c9a7)", isOptimized: false, runtimeVersion: 
0, emissionKind: FullDebug, enums: !2, splitDebugInlining: false, 
nameTableKind: None)
!1 = !DIFile(filename: "t1.c", directory: "/home/yhs/tmp/ext_func_var")
!2 = !{}
!3 = !{i32 7, !"Dwarf Version", i32 4}
!4 = !{i32 2, !"Debug Info Version", i32 3}
!5 = !{i32 1, !"wchar_size", i32 4}
!6 = !{!"clang version 13.0.0 (https://github.com/llvm/llvm-project.git 
68275c77c92b89fafbacc31b4f40303bb9e0c9a7)"}
[yhs@devbig003.ftw2 ~/tmp/ext_func_var]$

For the second example,

[yhs@devbig003.ftw2 ~/tmp/ext_func_var]$ cat t2.c   

void f1();  

int main() {

  int x = sizeof();  

  return x; 


}   

[yhs@devbig003.ftw2 ~/tmp/ext_func_var]$ clang -target bpf -g -S -emit-llvm 
t2.c
[yhs@devbig003.ftw2 ~/tmp/ext_func_var]$ cat t2.ll  

; ModuleID = 't2.c' 

source_filename = "t2.c"

target datalayout = "e-m:e-p:64:64-i64:64-i128:128-n32:64-S128" 

target triple = "bpf"

  

; Function Attrs: noinline nounwind optnone 

define dso_local i32 @main() #0 !dbg !7 {   

entry:

  %retval = alloca i32, align 4 

  %x = alloca i32, align 4  

  store i32 0, i32* %retval, align 4

  call void @llvm.dbg.declare(metadata i32* %x, metadata !11, metadata 
!DIExpression()), !dbg !12   
  store i32 8, i32* %x, align 4, !dbg !12   

  %0 = load i32, i32* %x, align 4, !dbg !13 

  ret i32 %0, !dbg !14  


}

; Function Attrs: nofree nosync nounwind readnone speculatable willreturn
declare void @llvm.dbg.declare(metadata, metadata, metadata) #1

attributes #0 = { noinline nounwind optnone "frame-pointer"="all" 
"min-legal-vector-width"="0" "no-trapping-math"="t
rue" "stack-protector-buffer-size"="8" }
attributes #1 = { nofree nosync nounwind readnone speculatable willreturn }

!llvm.dbg.cu = !{!0}
!llvm.module.flags = !{!3, !4, !5}
!llvm.ident = !{!6}

!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang 
version 13.0.0 (https://github.com/ll
vm/llvm-project.git 68275c77c92b89fafbacc31b4f40303bb9e0c9a7)", isOptimized: 
false, runtimeVersion: 0, emissionKind:
 FullDebug, enums: !2, splitDebugInlining: false, nameTableKind: None)
!1 = !DIFile(filename: "t2.c", directory: "/home/yhs/tmp/ext_func_var")
!2 = !{}
!3 = !{i32 7, !"Dwarf Version", i32 4}
!4 = !{i32 2, !"Debug Info Version", i32 3}
!5 = !{i32 1, !"wchar_size", i32 4}
!6 = !{!"clang version 13.0.0 (https://github.com/llvm/llvm-project.git 
68275c77c92b89fafbacc31b4f40303bb9e0c9a7)"}
!7 = distinct !DISubprogram(name: "main", scope: !1, file: !1, line: 2, type: 
!8, scopeLine: 2, spFlags: DISPFlagDef
inition, unit: !0, retainedNodes: !2)
!8 = !DISubroutineType(types: !9)
!9 = !{!10}
!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
!11 = !DILocalVariable(name: "x", 

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

What happens for this program:

  extern void f1();
  void f2(void *);
  inline void f3() {
f2(f1);
  }
  ...

Even when `f3` is never called, I'm guessing your change will cause `f1` to be 
emitted?

Also something like this:

  void f1();
  int main() {
int x = sizeof();
  }

Does that produce the declaration of `f1` too?




Comment at: clang/lib/CodeGen/CGExpr.cpp:2840
+// Emit debuginfo for the function declaration if the target wants to.
+if (getContext().getTargetInfo().allowDebugInfoForExternalVar()) {
+  CGDebugInfo *DI = CGM.getModuleDebugInfo();

Seems like this should be renamed given it's being used for things other than 
external variables?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

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

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

Yes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

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

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Andrii Nakryiko via Phabricator via cfe-commits]

anakryiko accepted this revision.
anakryiko added a comment.

Nice, thanks! This will work for externs with explicit section name (.ksym) and 
with no section name (externs for static linking), right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

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

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

The corresponding clang patch is here: https://reviews.llvm.org/D100567


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

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

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Alexei Starovoitov via Phabricator via cfe-commits]

ast accepted this revision.
ast added a comment.
This revision is now accepted and ready to land.

Makes sense to me. Only BPF target will notice this difference.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100567

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

[PATCH] D100567: BPF: emit debuginfo for Function of DeclRefExpr if requested

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song created this revision.
yonghong-song added a reviewer: dblaikie.
yonghong-song added projects: debug-info, clang.
yonghong-song requested review of this revision.
Herald added a subscriber: cfe-commits.

Commit e3d8ee35e4ad 
 ("reland 
"[DebugInfo] Support to emit debugInfo
for extern variables"") added support to emit debugInfo for
extern variables if requested by the target. Currently, only
BPF target enables this feature by default.

As BPF ecosystem grows, callback function started to get
support, e.g., recently bpf_for_each_map_elem() is introduced
(https://lwn.net/Articles/846504/) with a callback function as an
argument. In the future we may have something like below as
a demonstration of use case :

  extern int do_work(int);
  long bpf_helper(void *callback_fn, void *callback_ctx, ...);
  long prog_main() {
  struct { ... } ctx = { ... };
  return bpf_helper(_work, , ...);
  }

Basically bpf helper may have a callback function and the
callback function is defined in another file. In this case,
we would like to know the debuginfo types for do_work(), so
the verifier can proper verify the safety of bpf_helper() call.
The do_work() might be a function from another file or from
the kernel.

For the following example,

  extern int do_work(int);
  long bpf_helper(void *callback_fn);
  long prog() {
  return bpf_helper(_work);
  }

Currently, there is no debuginfo generated for extern function do_work().
In the IR, we have,

  ...
  define dso_local i64 @prog() local_unnamed_addr #0 !dbg !7 {
  entry:
%call = tail call i64 @bpf_helper(i8* bitcast (i32 (i32)* @do_work to i8*)) 
#2, !dbg !11
ret i64 %call, !dbg !12
  }
  ...
  declare dso_local i32 @do_work(i32) #1
  ...

This patch added support for the above callback function use case, and
the generated IR looks like below:

  ...
  declare !dbg !17 dso_local i32 @do_work(i32) #1
  ...
  !17 = !DISubprogram(name: "do_work", scope: !1, file: !1, line: 1, type: !18, 
flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
  !18 = !DISubroutineType(types: !19)
  !19 = !{!20, !20}
  !20 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D100567

Files:
  clang/lib/CodeGen/CGExpr.cpp
  clang/test/CodeGen/debug-info-extern-callback.c


Index: clang/test/CodeGen/debug-info-extern-callback.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-callback.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -x c -debug-info-kind=limited -triple bpf-linux-gnu 
-emit-llvm %s -o - | FileCheck %s
+
+extern int do_work(int);
+long bpf_helper(void *callback_fn);
+long prog() {
+   return bpf_helper(_work);
+}
+
+// CHECK: declare !dbg ![[FUNC:[0-9]+]] i32 @do_work(i32)
+// CHECK: ![[FUNC]] = !DISubprogram(name: "do_work"
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -2833,8 +2833,19 @@
 return LV;
   }
 
-  if (const auto *FD = dyn_cast(ND))
-return EmitFunctionDeclLValue(*this, E, FD);
+  if (const auto *FD = dyn_cast(ND)) {
+LValue LV = EmitFunctionDeclLValue(*this, E, FD);
+
+// Emit debuginfo for the function declaration if the target wants to.
+if (getContext().getTargetInfo().allowDebugInfoForExternalVar()) {
+  CGDebugInfo *DI = CGM.getModuleDebugInfo();
+  auto *Fn = dyn_cast(LV.getPointer(*this));
+  if (DI && Fn)
+DI->EmitFunctionDecl(FD, FD->getLocation(), T, Fn);
+}
+
+return LV;
+  }
 
   // FIXME: While we're emitting a binding from an enclosing scope, all other
   // DeclRefExprs we see should be implicitly treated as if they also refer to


Index: clang/test/CodeGen/debug-info-extern-callback.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-callback.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -x c -debug-info-kind=limited -triple bpf-linux-gnu -emit-llvm %s -o - | FileCheck %s
+
+extern int do_work(int);
+long bpf_helper(void *callback_fn);
+long prog() {
+	return bpf_helper(_work);
+}
+
+// CHECK: declare !dbg ![[FUNC:[0-9]+]] i32 @do_work(i32)
+// CHECK: ![[FUNC]] = !DISubprogram(name: "do_work"
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -2833,8 +2833,19 @@
 return LV;
   }
 
-  if (const auto *FD = dyn_cast(ND))
-return EmitFunctionDeclLValue(*this, E, FD);
+  if (const auto *FD = dyn_cast(ND)) {
+LValue LV = EmitFunctionDeclLValue(*this, E, FD);
+
+// Emit debuginfo for the function declaration if the target wants to.
+if (getContext().getTargetInfo().allowDebugInfoForExternalVar()) {
+