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

[Tom Honermann via cfe-commits]

Author: Tom Honermann
Date: 2023-08-31T09:10:05-07:00
New Revision: 6658db5e501475bf288f6d9eb2a8ff21d5fee8b5

URL: https://github.com/llvm/llvm-project/commit/6658db5e501475bf288f6d9eb2a8ff21d5fee8b5
DIFF: https://github.com/llvm/llvm-project/commit/6658db5e501475bf288f6d9eb2a8ff21d5fee8b5.diff

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

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.

Reviewed By: aaron.ballman, rnk

Differential Revision: https://reviews.llvm.org/D158869

Added: 
    

Modified: 
    clang/docs/ReleaseNotes.rst
    clang/lib/Sema/SemaTemplate.cpp
    clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp

Removed: 
    


################################################################################
diff  --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index ded58a369c7a03..138730336f7426 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -292,6 +292,9 @@ 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
 ^^^^^^^^^^^^^^^^^

diff  --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index f45c4c463fa95f..808a8b000aff72 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -10110,6 +10110,17 @@ DeclResult Sema::ActOnExplicitInstantiation(
         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);
@@ -10226,11 +10237,6 @@ DeclResult Sema::ActOnExplicitInstantiation(
       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);

diff  --git a/clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp b/clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
index 04ce29c64d6e36..2ac1961465d8a8 100644
--- a/clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
+++ b/clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
@@ -1,5 +1,6 @@
 // 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
 
@@ -934,3 +935,32 @@ class A {
 void A::printd() { JSMethod<A, &A::printd>(); }
 // CHECK-LABEL: @"??$JSMethod at VA@PMFInTemplateArgument@@$1?printd at 12@AAEHH at 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
+}