Skip to content

[clang][CGDebugInfo] Don't generate an implicit 'this' parameter if one was specified explicitly #100767

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

Michael137
Copy link
Member

Currently we would unconditionally add an implicit this parameter when creating an instance method type. However, when we have an explicit 'this', we shouldn't generate one. This patch only passes a valid ThisPtr type to getOrCreateInstanceMethodType if one wasn't explicitly specified. There's no way to get a pointer to a member function with an explicit this parameter (those are treated as regular function pointers instead). So there's no need to account for that case in CGDebugInfo::CreateType.

Fixes #99744

…ne was specified explicitly

Currently we would unconditionally add an implicit `this` parameter
when creating an instance method type. However, when we have an
explicit 'this', we shouldn't generate one. This patch only passes
a valid `ThisPtr` type to `getOrCreateInstanceMethodType` if one
wasn't explicitly specified. There's no way to get a pointer
to a member function with an explicit `this` parameter (those
are treated as regular function pointers instead). So there's
no need to account for that case in `CGDebugInfo::CreateType`.

Fixes llvm#99744
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. debuginfo labels Jul 26, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 26, 2024

@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-clang

Author: Michael Buch (Michael137)

Changes

Currently we would unconditionally add an implicit this parameter when creating an instance method type. However, when we have an explicit 'this', we shouldn't generate one. This patch only passes a valid ThisPtr type to getOrCreateInstanceMethodType if one wasn't explicitly specified. There's no way to get a pointer to a member function with an explicit this parameter (those are treated as regular function pointers instead). So there's no need to account for that case in CGDebugInfo::CreateType.

Fixes #99744


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

2 Files Affected:

  • (modified) clang/lib/CodeGen/CGDebugInfo.cpp (+30-22)
  • (added) clang/test/CodeGenCXX/debug-info-explicit-this.cpp (+16)
diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index 3d8a715b692de..ac31f5ce8a801 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -1942,7 +1942,12 @@ CGDebugInfo::getOrCreateMethodType(const CXXMethodDecl *Method,
   if (Method->isStatic())
     return cast_or_null<llvm::DISubroutineType>(
         getOrCreateType(QualType(Func, 0), Unit));
-  return getOrCreateInstanceMethodType(Method->getThisType(), Func, Unit);
+
+  QualType ThisType;
+  if (!Method->hasCXXExplicitFunctionObjectParameter())
+    ThisType = Method->getThisType();
+
+  return getOrCreateInstanceMethodType(ThisType, Func, Unit);
 }
 
 llvm::DISubroutineType *CGDebugInfo::getOrCreateInstanceMethodType(
@@ -1974,27 +1979,30 @@ llvm::DISubroutineType *CGDebugInfo::getOrCreateInstanceMethodType(
   Elts.push_back(Args[0]);
 
   // "this" pointer is always first argument.
-  const CXXRecordDecl *RD = ThisPtr->getPointeeCXXRecordDecl();
-  if (isa<ClassTemplateSpecializationDecl>(RD)) {
-    // Create pointer type directly in this case.
-    const PointerType *ThisPtrTy = cast<PointerType>(ThisPtr);
-    uint64_t Size = CGM.getContext().getTypeSize(ThisPtrTy);
-    auto Align = getTypeAlignIfRequired(ThisPtrTy, CGM.getContext());
-    llvm::DIType *PointeeType =
-        getOrCreateType(ThisPtrTy->getPointeeType(), Unit);
-    llvm::DIType *ThisPtrType =
-        DBuilder.createPointerType(PointeeType, Size, Align);
-    TypeCache[ThisPtr.getAsOpaquePtr()].reset(ThisPtrType);
-    // TODO: This and the artificial type below are misleading, the
-    // types aren't artificial the argument is, but the current
-    // metadata doesn't represent that.
-    ThisPtrType = DBuilder.createObjectPointerType(ThisPtrType);
-    Elts.push_back(ThisPtrType);
-  } else {
-    llvm::DIType *ThisPtrType = getOrCreateType(ThisPtr, Unit);
-    TypeCache[ThisPtr.getAsOpaquePtr()].reset(ThisPtrType);
-    ThisPtrType = DBuilder.createObjectPointerType(ThisPtrType);
-    Elts.push_back(ThisPtrType);
+  // ThisPtr may be null if the member function has an explicit 'this' parameter.
+  if (!ThisPtr.isNull()) {
+    const CXXRecordDecl *RD = ThisPtr->getPointeeCXXRecordDecl();
+    if (isa<ClassTemplateSpecializationDecl>(RD)) {
+      // Create pointer type directly in this case.
+      const PointerType *ThisPtrTy = cast<PointerType>(ThisPtr);
+      uint64_t Size = CGM.getContext().getTypeSize(ThisPtrTy);
+      auto Align = getTypeAlignIfRequired(ThisPtrTy, CGM.getContext());
+      llvm::DIType *PointeeType =
+          getOrCreateType(ThisPtrTy->getPointeeType(), Unit);
+      llvm::DIType *ThisPtrType =
+          DBuilder.createPointerType(PointeeType, Size, Align);
+      TypeCache[ThisPtr.getAsOpaquePtr()].reset(ThisPtrType);
+      // TODO: This and the artificial type below are misleading, the
+      // types aren't artificial the argument is, but the current
+      // metadata doesn't represent that.
+      ThisPtrType = DBuilder.createObjectPointerType(ThisPtrType);
+      Elts.push_back(ThisPtrType);
+    } else {
+      llvm::DIType *ThisPtrType = getOrCreateType(ThisPtr, Unit);
+      TypeCache[ThisPtr.getAsOpaquePtr()].reset(ThisPtrType);
+      ThisPtrType = DBuilder.createObjectPointerType(ThisPtrType);
+      Elts.push_back(ThisPtrType);
+    }
   }
 
   // Copy rest of the arguments.
diff --git a/clang/test/CodeGenCXX/debug-info-explicit-this.cpp b/clang/test/CodeGenCXX/debug-info-explicit-this.cpp
new file mode 100644
index 0000000000000..45ab2a0216ca7
--- /dev/null
+++ b/clang/test/CodeGenCXX/debug-info-explicit-this.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -emit-llvm -debug-info-kind=limited -std=c++2b %s -o - | FileCheck %s
+
+struct Foo {
+  void Bar(this Foo&& self) {}
+};
+
+void fn() {
+  Foo{}.Bar();
+}
+
+// CHECK: distinct !DISubprogram(name: "Bar", {{.*}}, type: ![[BAR_TYPE:[0-9]+]], {{.*}}, declaration: ![[BAR_DECL:[0-9]+]], {{.*}}
+// CHECK: ![[FOO:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "Foo"
+// CHECK: ![[BAR_DECL]] = !DISubprogram(name: "Bar", {{.*}}, type: ![[BAR_TYPE]], {{.*}},
+// CHECK: ![[BAR_TYPE]] = !DISubroutineType(types: ![[PARAMS:[0-9]+]])
+// CHECK: ![[PARAMS]] = !{null, ![[SELF:[0-9]+]]}
+// CHECK: ![[SELF]] = !DIDerivedType(tag: DW_TAG_rvalue_reference_type, baseType: ![[FOO]]

@llvmbot
Copy link
Member

llvmbot commented Jul 26, 2024

@llvm/pr-subscribers-clang-codegen

Author: Michael Buch (Michael137)

Changes

Currently we would unconditionally add an implicit this parameter when creating an instance method type. However, when we have an explicit 'this', we shouldn't generate one. This patch only passes a valid ThisPtr type to getOrCreateInstanceMethodType if one wasn't explicitly specified. There's no way to get a pointer to a member function with an explicit this parameter (those are treated as regular function pointers instead). So there's no need to account for that case in CGDebugInfo::CreateType.

Fixes #99744


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

2 Files Affected:

  • (modified) clang/lib/CodeGen/CGDebugInfo.cpp (+30-22)
  • (added) clang/test/CodeGenCXX/debug-info-explicit-this.cpp (+16)
diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index 3d8a715b692de..ac31f5ce8a801 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -1942,7 +1942,12 @@ CGDebugInfo::getOrCreateMethodType(const CXXMethodDecl *Method,
   if (Method->isStatic())
     return cast_or_null<llvm::DISubroutineType>(
         getOrCreateType(QualType(Func, 0), Unit));
-  return getOrCreateInstanceMethodType(Method->getThisType(), Func, Unit);
+
+  QualType ThisType;
+  if (!Method->hasCXXExplicitFunctionObjectParameter())
+    ThisType = Method->getThisType();
+
+  return getOrCreateInstanceMethodType(ThisType, Func, Unit);
 }
 
 llvm::DISubroutineType *CGDebugInfo::getOrCreateInstanceMethodType(
@@ -1974,27 +1979,30 @@ llvm::DISubroutineType *CGDebugInfo::getOrCreateInstanceMethodType(
   Elts.push_back(Args[0]);
 
   // "this" pointer is always first argument.
-  const CXXRecordDecl *RD = ThisPtr->getPointeeCXXRecordDecl();
-  if (isa<ClassTemplateSpecializationDecl>(RD)) {
-    // Create pointer type directly in this case.
-    const PointerType *ThisPtrTy = cast<PointerType>(ThisPtr);
-    uint64_t Size = CGM.getContext().getTypeSize(ThisPtrTy);
-    auto Align = getTypeAlignIfRequired(ThisPtrTy, CGM.getContext());
-    llvm::DIType *PointeeType =
-        getOrCreateType(ThisPtrTy->getPointeeType(), Unit);
-    llvm::DIType *ThisPtrType =
-        DBuilder.createPointerType(PointeeType, Size, Align);
-    TypeCache[ThisPtr.getAsOpaquePtr()].reset(ThisPtrType);
-    // TODO: This and the artificial type below are misleading, the
-    // types aren't artificial the argument is, but the current
-    // metadata doesn't represent that.
-    ThisPtrType = DBuilder.createObjectPointerType(ThisPtrType);
-    Elts.push_back(ThisPtrType);
-  } else {
-    llvm::DIType *ThisPtrType = getOrCreateType(ThisPtr, Unit);
-    TypeCache[ThisPtr.getAsOpaquePtr()].reset(ThisPtrType);
-    ThisPtrType = DBuilder.createObjectPointerType(ThisPtrType);
-    Elts.push_back(ThisPtrType);
+  // ThisPtr may be null if the member function has an explicit 'this' parameter.
+  if (!ThisPtr.isNull()) {
+    const CXXRecordDecl *RD = ThisPtr->getPointeeCXXRecordDecl();
+    if (isa<ClassTemplateSpecializationDecl>(RD)) {
+      // Create pointer type directly in this case.
+      const PointerType *ThisPtrTy = cast<PointerType>(ThisPtr);
+      uint64_t Size = CGM.getContext().getTypeSize(ThisPtrTy);
+      auto Align = getTypeAlignIfRequired(ThisPtrTy, CGM.getContext());
+      llvm::DIType *PointeeType =
+          getOrCreateType(ThisPtrTy->getPointeeType(), Unit);
+      llvm::DIType *ThisPtrType =
+          DBuilder.createPointerType(PointeeType, Size, Align);
+      TypeCache[ThisPtr.getAsOpaquePtr()].reset(ThisPtrType);
+      // TODO: This and the artificial type below are misleading, the
+      // types aren't artificial the argument is, but the current
+      // metadata doesn't represent that.
+      ThisPtrType = DBuilder.createObjectPointerType(ThisPtrType);
+      Elts.push_back(ThisPtrType);
+    } else {
+      llvm::DIType *ThisPtrType = getOrCreateType(ThisPtr, Unit);
+      TypeCache[ThisPtr.getAsOpaquePtr()].reset(ThisPtrType);
+      ThisPtrType = DBuilder.createObjectPointerType(ThisPtrType);
+      Elts.push_back(ThisPtrType);
+    }
   }
 
   // Copy rest of the arguments.
diff --git a/clang/test/CodeGenCXX/debug-info-explicit-this.cpp b/clang/test/CodeGenCXX/debug-info-explicit-this.cpp
new file mode 100644
index 0000000000000..45ab2a0216ca7
--- /dev/null
+++ b/clang/test/CodeGenCXX/debug-info-explicit-this.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -emit-llvm -debug-info-kind=limited -std=c++2b %s -o - | FileCheck %s
+
+struct Foo {
+  void Bar(this Foo&& self) {}
+};
+
+void fn() {
+  Foo{}.Bar();
+}
+
+// CHECK: distinct !DISubprogram(name: "Bar", {{.*}}, type: ![[BAR_TYPE:[0-9]+]], {{.*}}, declaration: ![[BAR_DECL:[0-9]+]], {{.*}}
+// CHECK: ![[FOO:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "Foo"
+// CHECK: ![[BAR_DECL]] = !DISubprogram(name: "Bar", {{.*}}, type: ![[BAR_TYPE]], {{.*}},
+// CHECK: ![[BAR_TYPE]] = !DISubroutineType(types: ![[PARAMS:[0-9]+]])
+// CHECK: ![[PARAMS]] = !{null, ![[SELF:[0-9]+]]}
+// CHECK: ![[SELF]] = !DIDerivedType(tag: DW_TAG_rvalue_reference_type, baseType: ![[FOO]]

Copy link

github-actions bot commented Jul 26, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Collaborator

@dwblaikie dwblaikie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Michael137 Michael137 merged commit 2db0c00 into llvm:main Jul 29, 2024
7 checks passed
@Michael137 Michael137 deleted the clang/debug-info/redundant-explicit-this-param branch July 29, 2024 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen IR generation bugs: mangling, exceptions, etc. clang Clang issues not falling into any other category debuginfo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extra implicit 'this' parameter entry when emitting a DW_TAG_subprogram with an explicit object parameter
3 participants