Page MenuHomePhabricator

BPF: emit debuginfo for Function of DeclRefExpr if requested
ClosedPublic

Authored by yonghong-song on Apr 15 2021, 8:20 AM.

Details

Summary

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(&do_work, &ctx, ...);
}

Basically bpf helper may have a callback function and the
callback function is defined in another file or in the kernel.
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.

For the following example,

extern int do_work(int);
long bpf_helper(void *callback_fn);
long prog() {
    return bpf_helper(&do_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)

The TargetInfo.allowDebugInfoForExternalVar is renamed to
TargetInfo.allowDebugInfoForExternalRef as now it guards
both extern variable and extern function debuginfo generation.

Diff Detail

Event Timeline

yonghong-song requested review of this revision.Apr 15 2021, 8:20 AM
yonghong-song created this revision.
ast accepted this revision.Apr 15 2021, 8:25 AM

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

This revision is now accepted and ready to land.Apr 15 2021, 8:25 AM
yonghong-song added a comment.EditedApr 15 2021, 8:28 AM

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

anakryiko accepted this revision.Apr 15 2021, 1:12 PM

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

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(&f1);
}

Does that produce the declaration of f1 too?

clang/lib/CodeGen/CGExpr.cpp
2839

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

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(&f1);                                                                                              
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", scope: !7, file: !1, line: 3, type: !10)
!12 = !DILocation(line: 3, column: 7, scope: !7)
!13 = !DILocation(line: 4, column: 10, scope: !7)
!14 = !DILocation(line: 4, column: 3, scope: !7)

The debuginfo for "f1" is not generated for case "sizeof(&f1);". I think it is okay in this case as in most cases, people wants debuginfo only if that pointer ptr appears in generated code.

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

yonghong-song added inline comments.Apr 15 2021, 3:49 PM
clang/lib/CodeGen/CGExpr.cpp
2839

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.

dblaikie added inline comments.Apr 15 2021, 3:49 PM
clang/lib/CodeGen/CGExpr.cpp
2839

Sounds good to me, thanks!

yonghong-song edited the summary of this revision. (Show Details)

Rename TargetInfo.allowDebugInfoForExternalVar to TargetInfo.allowDebugInfoForExternalRef.

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.

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

fix a clang-format warning

@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?

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)

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() { ... &foo ... &foo ... &foo ... }

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(&do_work2);
}

We will have a segfault

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

#4 0x00007f93e4fdfb20 __restore_rt sigaction.c:0:0
#5 0x0000000001506f70 llvm::Value::getMetadata(unsigned int) const (/home/yhs/work/llvm-project/llvm/build.cur/install/bin/clang-13+0x1506f70)
#6 0x00000000015074db llvm::Function::getSubprogram() const (/home/yhs/work/llvm-project/llvm/build.cur/install/bin/clang-13+0x15074db)
#7 0x00000000021ff5cf clang::CodeGen::CodeGenFunction::EmitDeclRefLValue(clang::DeclRefExpr const*) (/home/yhs/work/llvm-project/llvm/build.cur/install/bin
/clang-13+0x21ff5cf)
#8 0x00000000021fd866 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(&do_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.

Sorry, I know what is the segfault now after some debugging. It is because auto *Fn = dyn_cast<llvm::Function>(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.

yonghong-song updated this revision to Diff 338252.EditedApr 16 2021, 3:28 PM
  • isDefined() is not necessary, segfault is caused by a nullptr by casting a LV.getPointer(). Check nullptr instead.
  • add checking CGDebugInfo pointer. In my original patch but got lost in the revision.
yonghong-song added a comment.EditedApr 17 2021, 10:16 AM

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 ('&foo'):

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<llvm::Function>(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)&foo;
 }
 int foo() { return 0; }
 long test2() {
        return (long)&foo;
 }
 $ 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 '&foo' is a reference
to a declaration (llvm::Function type).
After the function definition, the emitted insn for '&foo' 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<llvm::Function>(LV.getPointer(*this));

in one of tests.

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

Sorry, I know what is the segfault now after some debugging. It is because auto *Fn = dyn_cast<llvm::Function>(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?

Sorry, I know what is the segfault now after some debugging. It is because auto *Fn = dyn_cast<llvm::Function>(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?

To answer one of your questions above, if there is a function definition before, dyn_cast<llvm::Function>(...) 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.

To answer one of your questions above, if there is a function definition before, dyn_cast<llvm::Function>(...) 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?)

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 '&func' 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<FunctionDecl>(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!

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

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

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

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

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

(ah, sorry, failed to submit comment)

clang/lib/CodeGen/CGExpr.cpp
2841–2842

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)

  • 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<llvm::Function>(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)
yonghong-song added inline comments.Apr 26 2021, 12:06 PM
clang/lib/CodeGen/CGExpr.cpp
2841–2842

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!

dblaikie added inline comments.Apr 26 2021, 1:22 PM
clang/lib/CodeGen/CGExpr.cpp
2841–2842

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?

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.

  • use stripPointerCasts() to remove the cast of DeclRefExpr pointer
dblaikie accepted this revision.Apr 26 2021, 2:27 PM

Looks good - thanks!

dblaikie added inline comments.Apr 26 2021, 2:30 PM
clang/lib/CodeGen/CGExpr.cpp
2842

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(...);
}

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

clang/lib/CodeGen/CGExpr.cpp
2842

done as suggested!