[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!

cfe/trunk/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

cfe/trunk/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();
 };
 
 struct B {
   virtual void f();
 };
 
 struct C : A, B {
   virtual void c();
   
   virtual void f();
 };
 
 // CHECK-LABEL: define void @_ZThn8_N5Test11C1fEv(
+// CHECK-DBG-NOT: dbg.declare
+// CHECK: ret void
 void C::f() { }
 
 }
 
 namespace Test2 {
 
 // Check that we emit a thunk for B::f since it's overriding a virtual base.
 
 struct A {
   virtual void f();
 };
 
 struct B : virtual A {
   virtual void b();
   virtual void f();
 };
 
 // CHECK-LABEL: define void @_ZTv0_n24_N5Test21B1fEv(
+// CHECK-DBG-NOT: dbg.declare
+// CHECK: ret void
 void B::f() { }
 
 }
 
 namespace Test3 {
 
 // Check that we emit a covariant thunk for B::f.
 

 
 struct __attribute__((visibility("protected"))) C : A, B {
   virtual void c();
   
   virtual void f();
 };
 
 // CHECK-LABEL: define protected void @_ZThn8_N5Test41C1fEv(
+// CHECK-DBG-NOT: dbg.declare
+// CHECK: ret void
 void C::f() { }
 
 }
 
 // Check that the thunk gets internal linkage.
 namespace Test4B {
   struct A {
     virtual void f();
namespace Test6 {
 
   struct Thunks : Base1, Base2 {
     long l;
 
     virtual X f();
   };
 
   // CHECK-LABEL: define void @_ZThn16_N5Test66Thunks1fEv
+  // CHECK-DBG-NOT: dbg.declare
   // CHECK-NOT: memcpy
   // CHECK: {{call void @_ZN5Test66Thunks1fEv.*sret}}
   // CHECK: ret void
   X Thunks::f() { return X(); }
 }
 
 namespace Test7 {
   // PR7188
class D : public B,
     void foo() {}
     void bar() {}
     void baz(X, X&, _Complex float, Small, Small&, Large);
   };
 
   void D::baz(X, X&, _Complex float, Small, Small&, Large) { }
 
   // CHECK-LABEL: define void @_ZThn8_N5Test71D3bazENS_1XERS1_CfNS_5SmallERS4_NS_5LargeE(
+  // CHECK-DBG-NOT: dbg.declare
   // CHECK-NOT: memcpy
   // CHECK: ret void
   void testD() { D d; }
 }
 
 namespace Test8 {
   struct NonPOD { ~NonPOD(); int x, y, z; };
   struct A { virtual void foo(); };
   struct B { virtual void bar(NonPOD); };
   struct C : A, B { virtual void bar(NonPOD); static void helper(NonPOD); };
 
   // CHECK: define void @_ZN5Test81C6helperENS_6NonPODE([[NONPODTYPE:%.*]]*
   void C::helper(NonPOD var) {}
 
   // CHECK-LABEL: define void @_ZThn8_N5Test81C3barENS_6NonPODE(
+  // CHECK-DBG-NOT: dbg.declare
   // CHECK-NOT: load [[NONPODTYPE]], [[NONPODTYPE]]*
   // CHECK-NOT: memcpy
   // CHECK: ret void
   void C::bar(NonPOD var) {}
 }
 
 // PR7241: Emitting thunks for a method shouldn't require the vtable for
 // that class to be emitted.
namespace Test11 {
   C* C::f() { return 0; }
 
   //  C::f itself.
   // CHECK: define {{.*}} @_ZN6Test111C1fEv(
 
   //  The this-adjustment and return-adjustment thunk required when
   //  C::f appears in a vtable where A is at a nonzero offset from C.
   // CHECK: define {{.*}} @_ZTcv0_n24_v0_n32_N6Test111C1fEv(
+  // CHECK-DBG-NOT: dbg.declare
+  // CHECK: ret
 
   //  The return-adjustment thunk required when C::f appears in a vtable
   //  where A is at a zero offset from C.
   // CHECK: define {{.*}} @_ZTch0_v0_n32_N6Test111C1fEv(
+  // CHECK-DBG-NOT: dbg.declare
+  // CHECK: ret
 }
 
 // Varargs thunk test.
 namespace Test12 {
   struct A {
     virtual A* f(int x, ...);
   };
   struct B {
     virtual B* f(int x, ...);
   };
   struct C : A, B {
     virtual void c();
     virtual C* f(int x, ...);
   };
   C* C::f(int x, ...) { return this; }
 
   // C::f
   // CHECK: define {{.*}} @_ZN6Test121C1fEiz
 
   // Varargs thunk; check that both the this and covariant adjustments
   // are generated.
   // CHECK: define {{.*}} @_ZTchn8_h8_N6Test121C1fEiz
+  // CHECK-DBG-NOT: dbg.declare
   // CHECK: getelementptr inbounds i8, i8* {{.*}}, i64 -8
   // CHECK: getelementptr inbounds i8, i8* {{.*}}, i64 8
 }
 
 // PR13832
 namespace Test13 {
   struct B1 {
     virtual B1 &foo1();
   };
   struct Pad1 {
     virtual ~Pad1();
   };
   struct Proxy1 : Pad1, B1 {
     virtual ~Proxy1();
   };
   struct D : virtual Proxy1 {
     virtual ~D();
     virtual D &foo1();
   };
   D& D::foo1() {
     return *this;
   }
   // CHECK: define {{.*}} @_ZTcvn8_n32_v8_n24_N6Test131D4foo1Ev
+  // CHECK-DBG-NOT: dbg.declare
   // CHECK: getelementptr inbounds i8, i8* {{.*}}, i64 -8
   // CHECK: getelementptr inbounds i8, i8* {{.*}}, i64 -32
   // CHECK: getelementptr inbounds i8, i8* {{.*}}, i64 -24
   // CHECK: getelementptr inbounds i8, i8* {{.*}}, i64 8
   // CHECK: ret %"struct.Test13::D"*
 }
 
 namespace Test14 {
   class A {
     virtual void f();
   };
   class B {
     virtual void f();
   };
   class C : public A, public B  {
     virtual void f();
   };
   void C::f() {
   }
   // CHECK: define void @_ZThn8_N6Test141C1fEv({{.*}}) unnamed_addr [[NUW:#[0-9]+]]
+  // CHECK-DBG-NOT: dbg.declare
+  // CHECK: ret void
 }
 
 // Varargs non-covariant thunk test.
 // PR18098
 namespace Test15 {
   struct A {
     virtual ~A();
   };

 struct C : public A, public B {
   void foo() {}
 };
 struct D : public C {
   ~D();
 };
 D::~D() {}
 // CHECK: define linkonce_odr void @_ZThn8_N6Test161C3fooEv({{.*}}) {{.*}} comdat
+// CHECK-DBG-NOT: dbg.declare
+// CHECK: ret void
 }
 
 /**** The following has to go at the end of the file ****/
 
 // checking without opt
 // CHECK-NONOPT-LABEL: define internal void @_ZThn8_N6Test4B12_GLOBAL__N_11C1fEv(
 // CHECK-NONOPT-NOT: comdat