[Clang,debuginfo] added vtt parameter in destructor DISubroutineType

[mgschossmann]

Fixes issue #104765: When creating a virtual destructor with an artificial "vtt" argument, the type of "vtt" was previously missing in the DISubroutineType types array.

This commit fixes this behavior and adds a regression test.

[llvmbot]

@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-debuginfo

Author: None (mgschossmann)

Changes

Fixes issue #104765: When creating a virtual destructor with an artificial "vtt" argument, the type of "vtt" was previously missing in the DISubroutineType types array.

This commit fixes this behavior and adds a regression test.

---

Full diff: https://github.com/llvm/llvm-project/pull/130674.diff

3 Files Affected:

• (modified) clang/lib/CodeGen/CGDebugInfo.cpp (+16-3)
• (modified) clang/lib/CodeGen/CGDebugInfo.h (+6-1)
• (added) clang/test/CodeGenCXX/debug-info-vtt.cpp (+16)

diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index 0e6daa42ee7bf..ae1ce18ee1831 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -2003,8 +2003,17 @@ CGDebugInfo::getOrCreateMethodType(const CXXMethodDecl *Method,
   return getOrCreateInstanceMethodType(ThisType, Func, Unit);
 }
 
-llvm::DISubroutineType *CGDebugInfo::getOrCreateInstanceMethodType(
-    QualType ThisPtr, const FunctionProtoType *Func, llvm::DIFile *Unit) {
+llvm::DISubroutineType *CGDebugInfo::getOrCreateMethodTypeForDestructor(
+    const CXXMethodDecl *Method, llvm::DIFile *Unit, QualType FNType) {
+  const FunctionProtoType *Func = FNType->getAs<FunctionProtoType>();
+  // skip the first param since it is also this
+  return getOrCreateInstanceMethodType(Method->getThisType(), Func, Unit, true);
+}
+
+llvm::DISubroutineType *
+CGDebugInfo::getOrCreateInstanceMethodType(QualType ThisPtr,
+                                           const FunctionProtoType *Func,
+                                           llvm::DIFile *Unit, bool SkipFirst) {
   FunctionProtoType::ExtProtoInfo EPI = Func->getExtProtoInfo();
   Qualifiers &Qc = EPI.TypeQuals;
   Qc.removeConst();
@@ -2044,7 +2053,7 @@ llvm::DISubroutineType *CGDebugInfo::getOrCreateInstanceMethodType(
   }
 
   // Copy rest of the arguments.
-  for (unsigned i = 1, e = Args.size(); i != e; ++i)
+  for (unsigned i = (SkipFirst ? 2 : 1), e = Args.size(); i != e; ++i)
     Elts.push_back(Args[i]);
 
   // Attach FlagObjectPointer to the explicit "this" parameter.
@@ -4352,6 +4361,10 @@ llvm::DISubroutineType *CGDebugInfo::getOrCreateFunctionType(const Decl *D,
     // subprogram DIE will miss DW_AT_decl_file and DW_AT_decl_line fields.
     return DBuilder.createSubroutineType(DBuilder.getOrCreateTypeArray({}));
 
+  if (const auto *Method = dyn_cast<CXXDestructorDecl>(D)) {
+    return getOrCreateMethodTypeForDestructor(Method, F, FnType);
+  }
+
   if (const auto *Method = dyn_cast<CXXMethodDecl>(D))
     return getOrCreateMethodType(Method, F);
 
diff --git a/clang/lib/CodeGen/CGDebugInfo.h b/clang/lib/CodeGen/CGDebugInfo.h
index 38f73eca561b7..8d7591da0f518 100644
--- a/clang/lib/CodeGen/CGDebugInfo.h
+++ b/clang/lib/CodeGen/CGDebugInfo.h
@@ -249,9 +249,14 @@ class CGDebugInfo {
   /// to get a method type which includes \c this pointer.
   llvm::DISubroutineType *getOrCreateMethodType(const CXXMethodDecl *Method,
                                                 llvm::DIFile *F);
+
+  llvm::DISubroutineType *
+  getOrCreateMethodTypeForDestructor(const CXXMethodDecl *Method,
+                                     llvm::DIFile *F, QualType FNType);
+
   llvm::DISubroutineType *
   getOrCreateInstanceMethodType(QualType ThisPtr, const FunctionProtoType *Func,
-                                llvm::DIFile *Unit);
+                                llvm::DIFile *Unit, bool SkipFirst = false);
   llvm::DISubroutineType *
   getOrCreateFunctionType(const Decl *D, QualType FnType, llvm::DIFile *F);
   /// \return debug info descriptor for vtable.
diff --git a/clang/test/CodeGenCXX/debug-info-vtt.cpp b/clang/test/CodeGenCXX/debug-info-vtt.cpp
new file mode 100644
index 0000000000000..f888994d08d53
--- /dev/null
+++ b/clang/test/CodeGenCXX/debug-info-vtt.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang -g -c -emit-llvm %s -o - | llvm-dis | FileCheck %s
+
+struct B {
+    virtual ~B() {}
+};
+
+struct A : virtual B {
+};
+
+A a;
+
+// CHECK-DAG: distinct !DISubprogram(name: "~A", linkageName: "_ZN1AD2Ev", {{.*}}, type: ![[subroutinetype:[0-9]+]]
+// CHECK-DAG: ![[subroutinetype]] = !DISubroutineType(types: ![[types:[0-9]+]])
+// CHECK-DAG: [[types]] = !{null, !{{[0-9]+}}, !{{[0-9]+}}}
+
+

[github-actions]

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Discourse for more information.

[mgschossmann]

ping @rnk @dwblaikie @adrian-prantl

[rnk]

Please add a test for the MSVC ABI, since I don't think it has VTT parameters, and I'm not sure the code you added does the right thing.

[mgschossmann]

Thanks for your comment. Indeed, my code changes behavior when compiling for i686-pc-windows-msvc, which I did not expect.
However, there seems to be the same problem: There is an implicit parameter should_call_delete, whose DILocalVariable has arg: 2 even though there is no second argument in the function type's types array. Similarly to vtt, the patched code adds this parameter to the array.

Additionally, it changes the return type of the destructor to a void pointer. I am unsure, why this happens, since that does not happen on ItaniumABI. Does the MSVC ABI feature an implicit void return? Unfortunately, I am lacking knowledge of the MSVC ABI to be able to judge what the correct behavior should be.

Following are clang outputs for debug-info-vtt.cpp:
clang_outputs.zip

PS: I am unsure whether the bug is that the AST is not correctly updated in clang/lib/CodeGen/ItaniumCXXABI.cpp (and clang/lib/CodeGen/MicrosoftCXXABI.cpp) or that the AST is not correctly read in clang/lib/CodeGen/CGDebugInfo.cpp. In the former case, my patch feels like a workaround.

[rnk]

Thanks, I looked at the files, and I think the new debug info looks good, it adds a DISubroutineType int parameter.

However, I don't understand why the patch you've posted here has that effect. Do you have more local changes? Why does skipping more arguments in this loop create more subroutine parameter types? I'm struggling to understand, and I just assumed adding more testing would provide answers to these questions.

Please do create a test case out of the IR you shared, it does seem relevant.

[mgschossmann]

The key change of this pull request is that debug info for destructors is now taken from FNType instead of D->getType() / Method->getType(). Method->getType() does not include the implicit arguments, while FNType does. FNType however also includes the this argument. In order to prevent duplicate emission of this, the pull request introduces the SkipFirst argument in getOrCreateInstanceMethodType.

I now also added a test for MSVC, which checks for presence of the should_call_delete arg. It also enforces that the return value be void*. The new test seems to pass, however there seems to be an issue in the CI, so it is reported as failed.

[rnk]

Thanks for the comment!

[mgschossmann]

Thanks for your approval. As I do not have write permissions, can you please merge the PR for me?

[lexi-nadia]

I believe this commit introduced a crash. Here's a minimal repro:

class Foo {
public:
  virtual ~Foo();
};

class Bar : public Foo {
public:
  Bar() {}
};

with the following flags:

clang -cc1 -triple bpfel -emit-obj -debug-info-kind=constructor /tmp/thefile.cpp

[rnk]

Thanks!

It looks like the new IR after this change doesn't pass verification, and presumably this causes the crash later:

$ clang -cc1 t.cpp -triple bpfel  -debug-info-kind=constructor -emit-llvm  -o - 
Attribute after last parameter!
ptr @_ZN3FooD0Ev
fatal error: error in backend: Broken module found, compilation aborted!

If you disable the verifier you can see the generated IR, but you can't see the "attributes beyond the last parameter". I guess they don't exist, so they don't get printed:

$ clang -cc1 t.cpp -triple bpfel  -debug-info-kind=constructor -emit-llvm  -o - -disable-llvm-verifier
...
; Function Attrs: nounwind
declare !dbg !5 void @_ZN3FooD0Ev() unnamed_addr #136686 

Actual base destructor (D0) prototypes have argument attributes that look like this:

define dso_local void @_ZN3BarD0Ev(ptr noundef nonnull align 8 dereferenceable(8) %this) unnamed_addr #0 align 2 !dbg !27 {  

It's not clear what the BPF target is doing that's special that's forcing the compiler to emit this otherwise unused destructor declaration, that seems like the edge case here. Regardless, let's roll it back for now.

[rnk]

Hm, reverting this change doesn't fix the verifier error, the issue still repros for me with this change reverted. Are we sure this was bisected correctly? Maybe the verifier error was pre-existing, but after this change, now we crash when previously we silently generated IR that doesn't verify.

[lexi-nadia]

Maybe the verifier error was pre-existing, but after this change, now we crash when previously we silently generated IR that doesn't verify.

That would be my suspicion. I ran two different root cause analyses and they both identified this revision as causing the crash, but I didn't look at verification at all.

[rnk]

the issue still repros for me with this change reverted.

This was actually user error on my side, after messing around a bit I was able to reproduce the problem and confirm that the rollback worked, so I landed it.

[rnk]

The actual underlying issue appears to come from ae0d224 / #91310 , which added CodeGenModule::EmitExternalFunctionDeclaration, which doesn't handle C++ constructors/destructors correctly.

[rnk]

We should be able to reland after #137079 lands

[rnk]

I attempted to reapply this patch, but the BPF test which I added fails with this test for reasons I don't understand. @mgschossmann , feel free to take a look if you have time.