[DebugInfo] Don't emit incorrect descriptions of thunk params (PR42627)

[vsk]

Summary

The this parameter of a thunk requires adjustment. Stop emitting an
incorrect dbg.declare pointing to the unadjusted pointer.

We could describe the adjusted value instead, but there may not be much
benefit in doing so as users tend not to debug thunks.

Robert O'Callahan reports that this matches gcc's behavior.

Fixes PR42627.

[vsk]

• Fix a thinko in the test updates.

[vsk]

Ping. @aprantl, any thoughts on this one?

[aprantl]

I agree, there seems not to be much value of generating debug info for the arguments of a thunk.

[dblaikie]

Should we describe thunks at all? (gcc 8.1 doesn't)

Interestingly gcc seems to include the thunk in the compile_unit address range, but doesn't have a subprogram for it...

[vsk]

In D65035#1615585, @dblaikie wrote:

Should we describe thunks at all? (gcc 8.1 doesn't)

Interestingly gcc seems to include the thunk in the compile_unit address range, but doesn't have a subprogram for it...

That's a great question. That could save a little space, but it could also impair tools that treat dsyms as the source of truth for symbol information, like dtrace. I'd be open to it if the size savings were appreciable.

[aprantl]

It's likely better for the tools to know where each function starts *and ends*.
More importantly though, if we don't generate debug info for the thunk, can we describe the function itself after it became inlined into the thunk?

[dblaikie]

In D65035#1615787, @aprantl wrote:

It's likely better for the tools to know where each function starts *and ends*.

attribute((nodebug)) breaks that invariant (& I think there's other functions we emit without debug info too - maybe some things to do with global ctors?)

More importantly though, if we don't generate debug info for the thunk, can we describe the function itself after it became inlined into the thunk?

Not really, no - and GCC doesn't describe the inlining either. A backtrace would demangle the name of the thunk & that's about all it could show there.

[aprantl]

In D65035#1615819, @dblaikie wrote:

In D65035#1615787, @aprantl wrote:

It's likely better for the tools to know where each function starts *and ends*.

attribute((nodebug)) breaks that invariant (& I think there's other functions we emit without debug info too - maybe some things to do with global ctors?)

Correct, and the tools need to fall back to heuristics in these cases. I don't know whether this causes any problems in practice.

More importantly though, if we don't generate debug info for the thunk, can we describe the function itself after it became inlined into the thunk?

Not really, no - and GCC doesn't describe the inlining either. A backtrace would demangle the name of the thunk & that's about all it could show there.

*That* sounds like a good reason to keep the debug info for the thunks around.

[dblaikie]

In D65035#1615833, @aprantl wrote:

In D65035#1615819, @dblaikie wrote:

In D65035#1615787, @aprantl wrote:

It's likely better for the tools to know where each function starts *and ends*.

attribute((nodebug)) breaks that invariant (& I think there's other functions we emit without debug info too - maybe some things to do with global ctors?)

Correct, and the tools need to fall back to heuristics in these cases. I don't know whether this causes any problems in practice.

More importantly though, if we don't generate debug info for the thunk, can we describe the function itself after it became inlined into the thunk?

Not really, no - and GCC doesn't describe the inlining either. A backtrace would demangle the name of the thunk & that's about all it could show there.

*That* sounds like a good reason to keep the debug info for the thunks around.

Not sure I follow why demangling the symbol would result in a worse user experience than the debug info? About the only thing in the debug info is the mangled name that's in the symbol table too.

[vsk]

In D65035#1615888, @dblaikie wrote:

In D65035#1615833, @aprantl wrote:

In D65035#1615819, @dblaikie wrote:

In D65035#1615787, @aprantl wrote:

It's likely better for the tools to know where each function starts *and ends*.

attribute((nodebug)) breaks that invariant (& I think there's other functions we emit without debug info too - maybe some things to do with global ctors?)

Correct, and the tools need to fall back to heuristics in these cases. I don't know whether this causes any problems in practice.

More importantly though, if we don't generate debug info for the thunk, can we describe the function itself after it became inlined into the thunk?

Not really, no - and GCC doesn't describe the inlining either. A backtrace would demangle the name of the thunk & that's about all it could show there.

*That* sounds like a good reason to keep the debug info for the thunks around.

Not sure I follow why demangling the symbol would result in a worse user experience than the debug info? About the only thing in the debug info is the mangled name that's in the symbol table too.

Oh, wouldn't the debugger lose the ability to synthesize an [inlined] frame for whatever is inlined into the thunk?

[dblaikie]

In D65035#1615906, @vsk wrote:

In D65035#1615888, @dblaikie wrote:

In D65035#1615833, @aprantl wrote:

In D65035#1615819, @dblaikie wrote:

In D65035#1615787, @aprantl wrote:

It's likely better for the tools to know where each function starts *and ends*.

attribute((nodebug)) breaks that invariant (& I think there's other functions we emit without debug info too - maybe some things to do with global ctors?)

Correct, and the tools need to fall back to heuristics in these cases. I don't know whether this causes any problems in practice.

More importantly though, if we don't generate debug info for the thunk, can we describe the function itself after it became inlined into the thunk?

Not really, no - and GCC doesn't describe the inlining either. A backtrace would demangle the name of the thunk & that's about all it could show there.

*That* sounds like a good reason to keep the debug info for the thunks around.

Not sure I follow why demangling the symbol would result in a worse user experience than the debug info? About the only thing in the debug info is the mangled name that's in the symbol table too.

Oh, wouldn't the debugger lose the ability to synthesize an [inlined] frame for whatever is inlined into the thunk?

True, true - you'd lose any variable descriptions etc. Fair point.

[aprantl]

In D65035#1615910, @dblaikie wrote:

In D65035#1615906, @vsk wrote:

In D65035#1615888, @dblaikie wrote:

In D65035#1615833, @aprantl wrote:

In D65035#1615819, @dblaikie wrote:

In D65035#1615787, @aprantl wrote:

It's likely better for the tools to know where each function starts *and ends*.

attribute((nodebug)) breaks that invariant (& I think there's other functions we emit without debug info too - maybe some things to do with global ctors?)

Correct, and the tools need to fall back to heuristics in these cases. I don't know whether this causes any problems in practice.

More importantly though, if we don't generate debug info for the thunk, can we describe the function itself after it became inlined into the thunk?

Not really, no - and GCC doesn't describe the inlining either. A backtrace would demangle the name of the thunk & that's about all it could show there.

*That* sounds like a good reason to keep the debug info for the thunks around.

Not sure I follow why demangling the symbol would result in a worse user experience than the debug info? About the only thing in the debug info is the mangled name that's in the symbol table too.

Oh, wouldn't the debugger lose the ability to synthesize an [inlined] frame for whatever is inlined into the thunk?

True, true - you'd lose any variable descriptions etc. Fair point.

That's what I meant. Sorry for not expressing that better!

clang/lib/CodeGen/CGDecl.cpp

void CodeGenFunction::EmitParmDecl(const VarDecl &D, ParamValue Arg,
   }
 
   // Store the initial value into the alloca.
   if (DoStore)
     EmitStoreOfScalar(ArgVal, lv, /* isInitialization */ true);
 
   setAddrOfLocalVar(&D, DeclPtr);
 
-  // Emit debug info for param declaration.
+  // Emit debug info for param declarations in non-thunk functions.
   if (CGDebugInfo *DI = getDebugInfo()) {
     if (CGM.getCodeGenOpts().getDebugInfo() >=
-        codegenoptions::LimitedDebugInfo) {
+            codegenoptions::LimitedDebugInfo &&
+        !CurFuncIsThunk) {
       DI->EmitDeclareOfArgVariable(&D, DeclPtr.getPointer(), ArgNo, Builder);
     }
   }
 
   if (D.hasAttr<AnnotateAttr>())
     EmitVarAnnotations(&D, DeclPtr.getPointer());
 
   // We can only check return value nullability if all arguments to the

clang/test/CodeGenCXX/thunks.cpp

 // RUN: %clang_cc1 %s -triple=x86_64-pc-linux-gnu -munwind-tables -emit-llvm -o - | FileCheck --check-prefix=CHECK --check-prefix=CHECK-NONOPT %s
+// RUN: %clang_cc1 %s -triple=x86_64-pc-linux-gnu -debug-info-kind=standalone -dwarf-version=5 -munwind-tables -emit-llvm -o - | FileCheck --check-prefix=CHECK --check-prefix=CHECK-NONOPT --check-prefix=CHECK-DBG %s
 // RUN: %clang_cc1 %s -triple=x86_64-pc-linux-gnu -munwind-tables -emit-llvm -o - -O1 -disable-llvm-passes | FileCheck --check-prefix=CHECK --check-prefix=CHECK-OPT %s
 
 namespace Test1 {
 
 // Check that we emit a non-virtual thunk for C::f.
 
 struct A {
   virtual void f();

 // CHECK-OPT-LABEL: define linkonce_odr void @_ZTv0_n24_N5Test51B1fEv
 
 // This is from Test10:
 // CHECK-OPT-LABEL: define linkonce_odr void @_ZN6Test101C3fooEv
 // CHECK-OPT-LABEL: define linkonce_odr void @_ZThn8_N6Test101C3fooEv
 
 // CHECK-NONOPT: attributes [[NUW]] = { noinline nounwind optnone uwtable{{.*}} }
 // CHECK-OPT: attributes [[NUW]] = { nounwind uwtable{{.*}} }
+
+// CHECK-DBG-NOT: DILocalVariable(name: "this"