[PATCH] D79967: Fix debug info for NoDebug attr

Mon May 18 07:29:33 PDT 2020

yaxunl marked 4 inline comments as done.
yaxunl added a comment.

In D79967#2039153 <https://reviews.llvm.org/D79967#2039153>, @dblaikie wrote:

> Could you check the commit history for this feature and rope in some folks who added the function declaration work (it's for debug call sites) - maybe @vsk is the right person, or knows who is, to check this is the right fix for it/doesn't adversely affect the feature this code was added to implement.

Anders Carlsson introduced support of nodebug attribute by the following two commits:

https://github.com/llvm/llvm-project/commit/76187b4d689a6ce601f2055b3dad5be6a4ab1012
https://github.com/llvm/llvm-project/commit/63784f4e5e125b7a81c92c2cf176797ce67b2e6d

However, it seems he is not in Phabricator.

Based on documentation of nodebug attribute:

https://clang.llvm.org/docs/AttributeReference.html#nodebug

clang should not emit any debug information for functions with nodebug attr.

================
Comment at: clang/test/CodeGen/nodebug-attr.c:5-6
+
+// CHECK-NOT: define {{.*}}@foo{{.*}}!dbg
+// CHECK-LABEL: define {{.*}}@foo
+// CHECK-NOT: ret {{.*}}!dbg
----------------
dblaikie wrote:
> Does this test fail when the bug is present? I'd have thought not - my understanding was that CHECK-LABEL is found first, then CHECK-NOT is tested between labels/other checks, so it wouldn't find @foo.*!dbg anyway.
> 
> I think maybe it'd be better to tighten up the CHECK-LABEL to include "#0 {" at the end and a comment saying how that CHECK part ensures there's no !dbg attached to it.
You are right. The test is not good at detecting the bad IR. Will fix it by removing CHECK-NOT and tighten up CHECK-LABEL.

The test CodeGenCUDA/kernel-dbg-info.cu has similar issue and will also be fixed.

================
Comment at: clang/test/CodeGen/nodebug-attr.c:8-15
+__attribute__((nodebug)) void foo(int *a) {
+  *a = 1;
+}
+
+// CHECK-LABEL: define {{.*}}@bar{{.*}}!dbg
+void bar(int *a) {
+  foo(a);
----------------
dblaikie wrote:
> It looks like this test case currently crashes LLVM:
> 
> h$ clang++-tot test.cpp -g -c -O3 && llvm-dwarfdump-tot test.o
> PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.
> Stack dump:
> 0.      Program arguments: clang++-tot test.cpp -g -c -O3 
> 1.      <eof> parser at end of file
> 2.      Code generation
> 3.      Running pass 'Function Pass Manager' on module 'test.cpp'.
> 4.      Running pass 'Debug Variable Analysis' on function '@_Z3fooPi'
>  #0 0x0000000006b49927 llvm::sys::PrintStackTrace(llvm::raw_ostream&) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/Support/Unix/Signals.inc:564:11
>  #1 0x0000000006b49ac9 PrintStackTraceSignalHandler(void*) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/Support/Unix/Signals.inc:625:1
>  #2 0x0000000006b482db llvm::sys::RunSignalHandlers() /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/Support/Signals.cpp:67:5
>  #3 0x0000000006b4921e llvm::sys::CleanupOnSignal(unsigned long) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/Support/Unix/Signals.inc:362:1
>  #4 0x0000000006a7cae8 (anonymous namespace)::CrashRecoveryContextImpl::HandleCrash(int, unsigned long) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/Support/CrashRecoveryContext.cpp:77:20
>  #5 0x0000000006a7cd6e CrashRecoverySignalHandler(int) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/Support/CrashRecoveryContext.cpp:383:1
>  #6 0x00007fc197167520 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x13520)
>  #7 0x0000000005a309ac llvm::DICompileUnit::getEmissionKind() const /usr/local/google/home/blaikie/dev/llvm/src/llvm/include/llvm/IR/DebugInfoMetadata.h:1272:31
>  #8 0x0000000005a4a107 llvm::LexicalScopes::initialize(llvm::MachineFunction const&) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/CodeGen/LexicalScopes.cpp:53:70
>  #9 0x0000000005e128dd (anonymous namespace)::LDVImpl::computeIntervals() /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/CodeGen/LiveDebugVariables.cpp:972:17
> #10 0x0000000005e11284 (anonymous namespace)::LDVImpl::runOnMachineFunction(llvm::MachineFunction&) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/CodeGen/LiveDebugVariables.cpp:0:3
> #11 0x0000000005e10fe4 llvm::LiveDebugVariables::runOnMachineFunction(llvm::MachineFunction&) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/CodeGen/LiveDebugVariables.cpp:1014:3
> 
> 
> Is that crash something this patch is intended to address, or unrelated? I guess it's intended to address that problem - because it's a DISubprogram for 'foo' with no DICompileUnit attachment that causes the crash later on.
Yes this patch fixes the crash as a side effect. Basically what happens before this patch is:

When clang emits call of `foo`, it tries to emit DISubprogram for `foo` as declaration to provide debug info for the call site:

https://github.com/llvm/llvm-project/blob/57d8b8d6f0b91b380ca3b270b4439c338ed67f53/clang/lib/CodeGen/CGExpr.cpp#L5148 

Then control flow goes to CGDebugInfo::EmitFuncDeclForCallSite. 

https://github.com/llvm/llvm-project/blob/master/clang/lib/CodeGen/CGDebugInfo.cpp#L3877

As we can see for normal function with definitions, clang should have emitted DISubprogram, therefore clang just returns at line 3886. Also clang does not always emit debug info for function decl for call site. e.g. it does not do that for builtin functions, static and inline functions. IMHO it should not do that for functions with nodebug info either. However it does that.

Then control goes to 

https://github.com/llvm/llvm-project/blob/57d8b8d6f0b91b380ca3b270b4439c338ed67f53/clang/lib/CodeGen/CGDebugInfo.cpp#L3866

Here clang tries to emit DISubprogram for `foo` as a declaration instead of definition. It does not set llvm::DISubprogram::SPFlagDefinition, which causes a nullptr Unit in DISubprogram.

When control flow goes to

https://github.com/llvm/llvm-project/blob/5b0502dff5b6f510fbf823059faa042dd1523cef/llvm/lib/CodeGen/LexicalScopes.cpp#L53

Since getUnit() is nullptr, clang crashes.

The situation of a function with nodebug attr is similar to a builtin function. They are actually function definitions since clang emits instructions for them. On the other hand, there is no debug info for these instructions and there is no need to emit any debug info for the function as declarations. For builtin functions, clang does not emit DISubprogram for them as declarations for call sites, the same logic should apply to functions with nodebug attr.

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

https://reviews.llvm.org/D79967