[NFC] Updating Debug Info generation for 'this'

[joaosaffran]

This is PR is updating the debug info generation for this. This is required to fix the generation of debug information for HLSL RWBuffer type. This was required from another PR: https://github.com/llvm/llvm-project/pull/119041/files

[llvmbot]

@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-clang-codegen

Author: None (joaosaffran)

Changes

This is PR is updating the debug info generation for this. This is required to fix the generation of debug information for HLSL RWBuffer type. This was required from another PR: https://github.com/llvm/llvm-project/pull/119041/files

---

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

1 Files Affected:

• (modified) clang/lib/CodeGen/CGDebugInfo.cpp (+4-22)

diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index 60f32f76109e9a..f81ca727e1154d 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -2021,28 +2021,10 @@ llvm::DISubroutineType *CGDebugInfo::getOrCreateInstanceMethodType(
   // 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);
-    }
+    llvm::DIType *ThisPtrType = getOrCreateType(ThisPtr, Unit);
+    TypeCache[ThisPtr.getAsOpaquePtr()].reset(ThisPtrType);
+    ThisPtrType = DBuilder.createObjectPointerType(ThisPtrType);
+    Elts.push_back(ThisPtrType);
   }
 
   // Copy rest of the arguments.

[dwblaikie]

any chance you could A/B test this on a bootstrap of clang, for instance? To help validate that this really is NFC/dead code?

[llvm-beanz]

any chance you could A/B test this on a bootstrap of clang, for instance? To help validate that this really is NFC/dead code?

I did a little more archeology, and I'm actually struggling to find any point in the history where this change was required. The difference between the two sides of the if is really that if the declaration is a specialization declaration, it creates the pointed-to object then explicitly queries for the size and alignment before creating the pointer type.

On the other side of the branch it calls getOrCreateType with the pointer type, which eventually calls into CGDebugInfo::CreatePointerLikeType, which as far back as I've looked (443f677), has always included alignment and size.

Here's a simplified diff of the two sides of the branch with comments and whitespace differences removed to make it more clear:

--- /Users/cbieneman/dev/scratch/a	2024-12-12 09:29:27
+++ /Users/cbieneman/dev/scratch/b	2024-12-12 09:29:32
@@ -1,10 +1,4 @@
-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);
+llvm::DIType *ThisPtrType = getOrCreateType(ThisPtr, Unit);
 TypeCache[ThisPtr.getAsOpaquePtr()].reset(ThisPtrType);
 ThisPtrType = DBuilder.createObjectPointerType(ThisPtrType);
 Elts.push_back(ThisPtrType);

Is there something I'm missing that explicitly setting the alignment handles? I feel like if getOrCreateType didn't properly handle the type's alignment we would be in a world of different hurt.

[joaosaffran]

FYI: been working on the bootstrap build as well, those have been a bit slow on my setup

[Michael137]

Don't see a problem with this but could we elaborate on the motivation for this? Looks like this is required for #119041? Why is that?

The original change this is based on is: microsoft/DirectXShaderCompiler#6296? Which makes it sounds like it does affect the generated debug-info. I'm confused as to what's NFC and what's not

[joaosaffran]

@Michael137, currently we have this issue open: #118523, after some investigation, we figured that such was the same issue we have faced here microsoft/DirectXShaderCompiler#6296, the fix we did there is the same one being added here. The issue is being caused when emitting debug info for HLSL RWBuffer type.

@llvm-beanz and @bogner have more context than I do with this issue. Can any of you give some more detailed explanation, please?

[bogner]

Don't see a problem with this but could we elaborate on the motivation for this? Looks like this is required for #119041? Why is that?

The original change this is based on is: microsoft/DirectXShaderCompiler#6296? Which makes it sounds like it does affect the generated debug-info. I'm confused as to what's NFC and what's not

This PR is entirely NFC. #119041 additionally has a functional change - the relevant part of which is the addition of an overload:

llvm::DIType *CGDebugInfo::CreateType(const HLSLAttributedResourceType *Ty,
                                      llvm::DIFile *U) {
  return getOrCreateType(Ty->getWrappedType(), U);
}

The reason this change is needed for that PR, is that this new overload needs to be called whether or not we're dealing with a function specialization, but since the code as is special-cases specializations we don't ever call the overloaded CreateType.

[joaosaffran]

@llvm-beanz @dwblaikie I've built a bootstrap version of clang with and without this change, the binary output was identical for both builds (check them with diff), please let me know if there is something else you might need me testing