[DebugInfo] Move constructor homing case in shouldOmitDefinition.

[akhuang]

Summary

For some reason the ctor homing case was before the template
specialization case, and could have returned false too early.
I moved the code out into a separate function to avoid this.

Also added a run line to the template specialization test. I guess
all the -debug-info-kind=limited tests should still pass with =constructor,
but it's probably unnecessary to test for all of those.

[dblaikie]

Thanks for testing/catching this (what sort of testing have you been doing that discovered this?)!

& yeah, the way shouldOmitDefinition is now written (with this fix) is right - that every check only gives an opportunity to return true, then falls through to the next case - never an early return false.

[akhuang]

unfortunately not any thorough testing :) I just happened to notice it the last time I looked at this code

[MaskRay]

Note that Harbormaster actually reported the check-clang-codegencxx issues B69369..

[akhuang]

In D86491#2235206, @MaskRay wrote:

Note that Harbormaster actually reported the check-clang-codegencxx issues B69369..

Definitely my bad; I thought I had run check-clang locally before committing, but maybe .. I didn't?

[akhuang]

just reopening to update the diff.

[akhuang]

Fix errors.

[dblaikie]

Looks good, thanks!

[akhuang]

ah sorry, this was relanded in b1009ee84fc0242bcebd07889306bf39d9b7170f.

clang/lib/CodeGen/CGDebugInfo.cpp

static bool hasExplicitMemberDefinition(CXXRecordDecl::method_iterator I,
   for (CXXMethodDecl *MD : llvm::make_range(I, End))
     if (FunctionDecl *Tmpl = MD->getInstantiatedFromMemberFunction())
       if (!Tmpl->isImplicit() && Tmpl->isThisDeclarationADefinition() &&
           !MD->getMemberSpecializationInfo()->isExplicitSpecialization())
         return true;
   return false;
 }
 
+static bool canUseCtorHoming(const CXXRecordDecl *RD) {
+  // Constructor homing can be used for classes that have at least one
+  // constructor and have no trivial or constexpr constructors.
+  // Skip this optimization if the class or any of its methods are marked
+  // dllimport.
+  if (RD->isLambda() || RD->hasConstexprNonCopyMoveConstructor() ||
+      isClassOrMethodDLLImport(RD))
+    return false;
+
+  if (RD->ctors().empty())
+    return false;
+
+  for (const auto *Ctor : RD->ctors())
+    if (Ctor->isTrivial() && !Ctor->isCopyOrMoveConstructor())
+      return false;
+
+  return true;
+}
+
 static bool shouldOmitDefinition(codegenoptions::DebugInfoKind DebugKind,
                                  bool DebugTypeExtRefs, const RecordDecl *RD,
                                  const LangOptions &LangOpts) {
   if (DebugTypeExtRefs && isDefinedInClangModule(RD->getDefinition()))
     return true;
 
   if (auto *ES = RD->getASTContext().getExternalSource())
     if (ES->hasExternalDefinitions(RD) == ExternalASTSource::EK_Always)
static bool shouldOmitDefinition(codegenoptions::DebugInfoKind DebugKind,
   // across DLL boundaries, so skip this optimization if the class or any of its
   // methods are marked dllimport. This isn't a complete solution, since objects
   // without any dllimport methods can be used in one DLL and constructed in
   // another, but it is the current behavior of LimitedDebugInfo.
   if (CXXDecl->hasDefinition() && CXXDecl->isDynamicClass() &&
       !isClassOrMethodDLLImport(CXXDecl))
     return true;
 
-  // In constructor debug mode, only emit debug info for a class when its
-  // constructor is emitted. Skip this optimization if the class or any of
-  // its methods are marked dllimport.
-  //
-  // This applies to classes that don't have any trivial constructors and have
-  // at least one constructor.
-  if (DebugKind == codegenoptions::DebugInfoConstructor &&
-      !CXXDecl->isLambda() && !CXXDecl->hasConstexprNonCopyMoveConstructor() &&
-      !isClassOrMethodDLLImport(CXXDecl)) {
-    if (CXXDecl->ctors().empty())
-      return false;
-    for (const auto *Ctor : CXXDecl->ctors())
-      if (Ctor->isTrivial() && !Ctor->isCopyOrMoveConstructor())
-        return false;
-    return true;
-  }
-
   TemplateSpecializationKind Spec = TSK_Undeclared;
   if (const auto *SD = dyn_cast<ClassTemplateSpecializationDecl>(RD))
     Spec = SD->getSpecializationKind();
 
   if (Spec == TSK_ExplicitInstantiationDeclaration &&
       hasExplicitMemberDefinition(CXXDecl->method_begin(),
                                   CXXDecl->method_end()))
     return true;
 
+  // In constructor homing mode, only emit complete debug info for a class
+  // when its constructor is emitted.
+  if ((DebugKind == codegenoptions::DebugInfoConstructor) &&
+      canUseCtorHoming(CXXDecl))
+    return true;
+
   return false;
 }
 
 void CGDebugInfo::completeRequiredType(const RecordDecl *RD) {
   if (shouldOmitDefinition(DebugKind, DebugTypeExtRefs, RD, CGM.getLangOpts()))
     return;
 
   QualType Ty = CGM.getContext().getRecordType(RD);

clang/test/CodeGenCXX/debug-info-template-explicit-specialization.cpp

 // RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -debug-info-kind=limited %s -o - | FileCheck %s
 
+// Make sure this still works with constructor homing.
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -debug-info-kind=constructor %s -o - | FileCheck %s
+
 // Run again with -gline-tables-only or -gline-directives-only and verify we don't crash.  We won't output
 // type info at all.
 // RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -debug-info-kind=line-tables-only %s -o - | FileCheck %s -check-prefix LINES-ONLY
 // RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -debug-info-kind=line-directives-only %s -o - | FileCheck %s -check-prefix LINES-ONLY
 
 // LINES-ONLY-NOT: !DICompositeType(tag: DW_TAG_structure_type
 
 // "h" is at the top because it's in the compile unit's retainedTypes: list.