[PATCH] D80242: [Clang] implement -fno-eliminate-unused-debug-types

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.


================
Comment at: clang/lib/CodeGen/CGDecl.cpp:103-118
+  case Decl::CXXRecord: // struct/union/class X; [C++]
+    if (CGDebugInfo *DI = getDebugInfo())
+      if (CGM.getCodeGenOpts().hasMaybeUnusedDebugInfo() &&
+          cast<CXXRecordDecl>(D).hasDefinition())
+        DI->completeUnusedClass(cast<CXXRecordDecl>(D));
+    return;
   case Decl::Record:    // struct/union/class X;
----------------
dblaikie wrote:
> All of these might be able to be collapsed together - using "cast<TagDecl>(D).hasDefinition()" ... to have a common implementation.
`TagDecl` doesn't have a method `hasDefinition`.  Only `CXXRecordDecl` does.


================
Comment at: clang/lib/CodeGen/CGDecl.cpp:116
+    if (CGDebugInfo *DI = getDebugInfo())
+      if (cast<EnumDecl>(&D)->getDefinition())
+        DI->EmitAndRetainType(getContext().getEnumType(cast<EnumDecl>(&D)));
----------------
dblaikie wrote:
> I think the right thing to call here (& the other places in this patch) is "hasDefinition" rather than "getDefinition" (this will avoid the definition being deserialized when it's not needed yet (then if we don't end up emitting the type because the flag hasn't been given, it won't be deserialized unnecessarily))
`TagDecl` doesn't have a method `hasDefinition`.  Only `CXXRecordDecl` does.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5385
+      if (getCodeGenOpts().hasMaybeUnusedDebugInfo() && CRD->hasDefinition())
+        DI->completeUnusedClass(*CRD);
+      else if (auto *ES = D->getASTContext().getExternalSource())
----------------
dblaikie wrote:
> I think this might not work as intended if the type trips over the other class deduplication optimizations (try manually testing with a type that has an external VTable?) - and perhaps this codepath should use the EmitAndRetain functionality.
completeUnusedClass eventually calls into CGDebugInfo::completeClass which makes use of the TypeCache.  What does it mean for a class to have "an external vtable?"  Can you give me an example?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80242/new/

https://reviews.llvm.org/D80242