[clang] Fix timing of propagation of MSInheritanceAttr for template instantiation declarations.

[tahonermann]

Summary

This change addresses three issues:

1. A failure to propagate a MSInheritanceAttr prior to it being required by an explicit class template instantiation definition.
2. The same MSInheritanceAttr attribute being attached to the same ClassTemplateSpecializationDecl twice.
3. MSInheritanceAttr attributes were not being cloned nor marked as inherited when added to new template instantiation declarations.

Sema::ActOnExplicitInstantiation() is responsible for the construction of ClassTemplateSpecializationDecl nodes for explicit template instantiation declarations and definitions. When invoked when a prior declaration node corresponding to an implicit instantiation exists, the prior declaration node is repurposed to represent the explicit instantiation declaration or definition. When no previous declaration node exists or when the previous node corresponds to an explicit declaration, a new node is allocated. Previously, in either case, the function attempted to propagate any existing MSInheritanceAttr attribute from the previous node, but did so regardless of whether the previous node was reused (in which case the repurposed previous node would gain a second attachment of the attribute; the second issue listed above) or a new node was created. In the latter case, the attribute was not propagated before it was required to be present when compiling for C++17 or later (the first issue listed above). The absent attribute resulted in an assertion failure that occurred during instantiation of the specialization definition when attempting to complete the definition in order to determine its alignment so as to resolve a lookup for a deallocation function for a virtual destructor. This change addresses both issues by propagating the attribute closer in time to when a new ClassTemplateSpecializationDecl node is created and only when such a node is newly created.

[tahonermann]

Adding Erich as attributes owner, and Reid and Zequan for MS ABI. This patch slightly modifies the changes made via D94646 and committed as rG4fffbc150cca1638051b8ad2a20f4b8240df0869 for GH48031.

clang/lib/Sema/SemaTemplate.cpp

10117

I am concerned that moving the call to Consumer.AssignInheritanceModel() to immediately after the creation of the node, but before it is populated might be problematic. Previously, this call was still made before the node was completely constructed (e.g., before setTemplateSpecializationKind() is called for it). It is difficult to tell if any consumers might be dependent on more of the definition being present.

[erichkeane]

clang/lib/Sema/SemaTemplate.cpp

10117

SO I think we still need to do this off of the definition, right? Else if PrevDecl ends up being a declaration (followed later by a definition that has the attribute), we'll end up missing it? Typically attributes are 'added' by later decls, so only the 'latest one' (though attributes can't be added after definition IIRC) has 'them all'.

[tahonermann]

clang/lib/Sema/SemaTemplate.cpp

10117

This handles the situation where a new node is created for either an explicit template instantiation declaration or definition; that matches prior behavior. The goal is to ensure each node, regardless of the reason for its creation, inherits the attribute from the previous declaration; that ensures that any explicitly declared attributes are checked for consistency (see Sema::mergeMSInheritanceAttr()).

Note that an explicit class template specialization is not allowed to follow an explicit template instantiation declaration or definition. https://godbolt.org/z/cbvaac717.

[rnk]

clang/lib/Sema/SemaTemplate.cpp

10115–10116

hasAttr() is O(n) in the attribute list size, so I would recommend this pattern:

if (PrevDecl) {
  if (const auto *A = PrevDecl->getAttr<MSInheritanceAttr>()) {
    Specialization->addAttr(A);
    ...

10117

I think it is safe to call AssignInheritanceModel earlier. We mostly just use it to invalidate some clang AST -> LLVM IR type translation cache.

clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp

958

My expectation here is that we assign the unknown inheritance model to a<int>, is that right? Can you add a static_assert to that effect, or add some CHECK lines for the structure, maybe make an alloca of type void (a<int>::*var)() and check for the allocated type (it should be a struct with a pointer with lots of i32s)?

[tahonermann]

clang/lib/Sema/SemaTemplate.cpp

10115–10116

Sounds good, I'll make that change.

10117

Great, thank you for confirming that.

clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp

958

Yes, when a pointer to member is formed for an incomplete class, in the absence of a #pragma pointers_to_members directive, use of inheritance keywords like __single_inheritance, or use of the /vmb or /vmg options (or equivalents), the "full_generality" / "virtual_inheritance" model is selected. I did verify that manually.

I can add a static_assert so long as it follows the explicit template instantiation definition. If it is placed earlier, then code related to obtaining a complete type is run and that ends up avoiding the assertion failure. See https://godbolt.org/z/qzTTfdfY1. This might indicate there is a bug elsewhere; I find it surprising that the early static_assert has that effect.

[tahonermann]

Address prior review feedback.

This update also modifies the propagation of the MSInheritanceAttr attribute for explicit template instantiations to clone the attribute and to set it as inherited. Previously, the same MSInheritanceAttr object was being attached to each declaration. @erichkeane, it would be great if you could confirm that this is the correct behavior.

[rnk]

lgtm

clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp

958

Got it, that makes sense. It's really just anything to document the requirement that we expect to get the most general member pointer representation from this code (notwithstanding global member pointer settings).

[aaron.ballman]

LGTM but please add a release note

[tahonermann]

Added a release note.

clang/docs/ReleaseNotes.rst

 X86 Support
 ^^^^^^^^^^^
 
 Arm and AArch64 Support
 ^^^^^^^^^^^^^^^^^^^^^^^
 
 Windows Support
 ^^^^^^^^^^^^^^^
+- Fixed an assertion failure that occurred due to a failure to propagate
+  ``MSInheritanceAttr`` attributes to class template instantiations created
+  for explicit template instantiation declarations.
 
 LoongArch Support
 ^^^^^^^^^^^^^^^^^
 
 RISC-V Support
 ^^^^^^^^^^^^^^
 - Unaligned memory accesses can be toggled by ``-m[no-]unaligned-access`` or the
   aliases ``-m[no-]strict-align``.

clang/lib/Sema/SemaTemplate.cpp

DeclResult Sema::ActOnExplicitInstantiation(
   if (!Specialization) {
     // Create a new class template specialization declaration node for
     // this explicit specialization.
     Specialization = ClassTemplateSpecializationDecl::Create(
         Context, Kind, ClassTemplate->getDeclContext(), KWLoc, TemplateNameLoc,
         ClassTemplate, CanonicalConverted, PrevDecl);
     SetNestedNameSpecifier(*this, Specialization, SS);
 
+    // A MSInheritanceAttr attached to the previous declaration must be
+    // propagated to the new node prior to instantiation.
+    if (PrevDecl) {
+      if (const auto *A = PrevDecl->getAttr<MSInheritanceAttr>()) {
+        auto *Clone = A->clone(getASTContext());
+        Clone->setInherited(true);
+        Specialization->addAttr(Clone);
+        Consumer.AssignInheritanceModel(Specialization);
+      }
+    }
+
     if (!HasNoEffect && !PrevDecl) {
       // Insert the new specialization.
       ClassTemplate->AddSpecialization(Specialization, InsertPos);
     }
   }
 
   // Build the fully-sugared type for this explicit instantiation as
   // the user wrote in the explicit instantiation itself. This means
if (Def) {
     // In MinGW mode, export the template instantiation if the declaration
     // was marked dllexport.
     if (PrevDecl_TSK == TSK_ExplicitInstantiationDeclaration &&
         Context.getTargetInfo().getTriple().isWindowsGNUEnvironment() &&
         PrevDecl->hasAttr<DLLExportAttr>()) {
       dllExportImportClassTemplateSpecialization(*this, Def);
     }
 
-    if (Def->hasAttr<MSInheritanceAttr>()) {
-      Specialization->addAttr(Def->getAttr<MSInheritanceAttr>());
-      Consumer.AssignInheritanceModel(Specialization);
-    }
-
     // Set the template specialization kind. Make sure it is set before
     // instantiating the members which will trigger ASTConsumer callbacks.
     Specialization->setTemplateSpecializationKind(TSK);
     InstantiateClassTemplateSpecializationMembers(TemplateNameLoc, Def, TSK);
   } else {
 
     // Set the template specialization kind.
     Specialization->setTemplateSpecializationKind(TSK);

clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp

 // RUN: %clang_cc1 -std=c++11 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - -triple=i386-pc-win32 -fms-extensions | FileCheck -allow-deprecated-dag-overlap %s
 // RUN: %clang_cc1 -std=c++11 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - -triple=x86_64-pc-win32 -fms-extensions | FileCheck %s -check-prefix=X64
+// RUN: %clang_cc1 -std=c++17 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - -triple=x86_64-pc-win32 -fms-extensions | FileCheck %s -check-prefix=X64
 // RUN: %clang_cc1 -std=c++11 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - -triple=i386-pc-win32 -DINCOMPLETE_VIRTUAL -fms-extensions -verify
 // RUN: %clang_cc1 -std=c++11 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - -triple=i386-pc-win32 -DINCOMPLETE_VIRTUAL -DMEMFUN -fms-extensions -verify
 
 namespace pr37399 {
 template <typename T>
 struct Functor {
   void (T::*PtrToMemberFunction)();
 };

 void JSMethod();
 class A {
   int printd(int);
   void printd();
 };
 void A::printd() { JSMethod<A, &A::printd>(); }
 // CHECK-LABEL: @"??$JSMethod@VA@PMFInTemplateArgument@@$1?printd@12@AAEHH@Z@PMFInTemplateArgument@@YAXXZ"(
 }
+
+namespace MissingMSInheritanceAttr {
+// This is a regression test for an assertion failure that occurred when
+// compiling for C++17. The issue concerned a failure to propagate a
+// MSInheritanceAttr attribute for the explicit template instantiation
+// definition prior to it being required to complete the specialization
+// definition in order to determine its alignment so as to resolve a
+// lookup for a deallocation function for the virtual destructor.
+template <typename>
+class a;
+struct b {
+  typedef void (a<int>::*f)();
+  f d;
+};
+template <typename>
+class a {
+  virtual ~a();
+  b e;
+};
+extern template class a<int>;
+template class a<int>;
+#ifdef _WIN64
+static_assert(sizeof(b::d) == 24, "");
+static_assert(sizeof(void (a<int>::*)()) == 24, "");
+#else
+static_assert(sizeof(b::d) == 16, "");
+static_assert(sizeof(void (a<int>::*)()) == 16, "");
+#endif
+}