Fix null MSInheritanceAttr deref in CXXRecordDecl::getMSInheritanceModel()

[adr26]

Summary

Ensure latest MPT decl has a MSInheritanceAttr when instantiating templates, to avoid null MSInheritanceAttr deref in CXXRecordDecl::getMSInheritanceModel().

See PR#37399 for repo / details.

[rnk]

Is it possible to fix this in assignInheritanceModel instead? I'd imagine we'd get the most recent decl. If that's not the issue, maybe you're fixing the bug in the right spot, but we need to find out where other class template attributes are moved from instantiation to real declaration.

lib/Sema/SemaTemplate.cpp

7079 ↗

(On Diff #146016)

This seems like an overly-generic name for what it does, which currently is specific to class template instantiations.

[adr26]

Update to change MSInheritanceAttr to always be attached to the latest non-injected class name decl, as suggested by Reid.

[rnk]

As far as workarounds go, this looks great! Please add a test case, I think you had one in the initial patch.

Should I go ahead and commit this?

[adr26]

Hi Reid,

I've added testcases matching the issues I hit in microsoft-abi-member-pointers.cpp (no change to the rest of the fix). If you are happy with this, please feel free to push.

I checked with MSVC, and as per the added testcases, it appears to always lock in the inheritance model as virutal when there's a member funtion pointer in a dependant base, irrespective of whether the most-derived class ends up using single/multiple/virtual inheritance. In fact if, for example, you annotate DerivedFunctor (in the test I've added) using an MS inheritance keyword - i.e. as "class __single_inheritance DerviceFunctor" - then MSVC complains that it's already fixed the representation to be virtual:

C:\Temp\clang\clang>cl C:\workspaces\llvm\tools\clang\test\CodeGenCXX\microsoft-abi-member-pointers.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.14.26428.1 for x86
Copyright (C) Microsoft Corporation.  All rights reserved.

microsoft-abi-member-pointers.cpp
C:\workspaces\llvm\tools\clang\test\CodeGenCXX\microsoft-abi-member-pointers.cpp(81): error C2286: pointers to members of 'pr37399::DerivedFunctor<void>' representation is already set to virtual_inheritance - declaration ignored
C:\workspaces\llvm\tools\clang\test\CodeGenCXX\microsoft-abi-member-pointers.cpp(27): note: see declaration of 'pr37399::DerivedFunctor<void>'

as such, these tests cover the cases I was hitting and appear to have behavioural parity with MSVC (as is the intention!).

Thanks, Andrew R

[rnk]

Thanks for the patch, I committed it as r333680 with a minor modification.

[adr26]

That's a nice change to avoid duplication in ClassTemplateSpecializationDecl::getMostRecentDecl(). Thanks for your help getting this out the door!

include/clang/AST/DeclCXX.h

     return const_cast<CXXRecordDecl*>(this)->getMostRecentDecl();
   }
 
+  CXXRecordDecl *getMostRecentNonInjectedDecl() {
+    CXXRecordDecl *Recent =
+        static_cast<CXXRecordDecl *>(this)->getMostRecentDecl();
+    while (Recent->isInjectedClassName()) {
+      // FIXME: Does injected class name need to be in the redeclarations chain?
+      assert(Recent->getPreviousDecl());
+      Recent = Recent->getPreviousDecl();
+    }
+    return Recent;
+  }
+
+  const CXXRecordDecl *getMostRecentNonInjectedDecl() const {
+    return const_cast<CXXRecordDecl*>(this)->getMostRecentNonInjectedDecl();
+  }
+
   CXXRecordDecl *getDefinition() const {
     // We only need an update if we don't already know which
     // declaration is the definition.

lib/AST/MicrosoftMangle.cpp

     const NamedDecl *ND = TA.getAsDecl();
     if (isa<FieldDecl>(ND) || isa<IndirectFieldDecl>(ND)) {
       mangleMemberDataPointer(
-          cast<CXXRecordDecl>(ND->getDeclContext())->getMostRecentDecl(),
+          cast<CXXRecordDecl>(ND->getDeclContext())->getMostRecentNonInjectedDecl(),
           cast<ValueDecl>(ND));
     } else if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(ND)) {
       const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(FD);
       if (MD && MD->isInstance()) {
-        mangleMemberFunctionPointer(MD->getParent()->getMostRecentDecl(), MD);
+        mangleMemberFunctionPointer(MD->getParent()->getMostRecentNonInjectedDecl(), MD);
       } else {
         Out << "$1?";
         mangleName(FD);

lib/AST/Type.cpp

       return false;
     // The inheritance attribute might only be present on the most recent
     // CXXRecordDecl, use that one.
-    RD = RD->getMostRecentDecl();
+    RD = RD->getMostRecentNonInjectedDecl();
     // Nothing interesting to do if the inheritance attribute is already set.
     if (RD->hasAttr<MSInheritanceAttr>())
       return false;

 }
 
 CXXRecordDecl *MemberPointerType::getMostRecentCXXRecordDecl() const {
-  return getClass()->getAsCXXRecordDecl()->getMostRecentDecl();
+  return getClass()->getAsCXXRecordDecl()->getMostRecentNonInjectedDecl();
 }

lib/CodeGen/MicrosoftCXXABI.cpp

   assert(MD->isInstance() && "Member function must not be static!");
 
   CharUnits NonVirtualBaseAdjustment = CharUnits::Zero();
-  const CXXRecordDecl *RD = MD->getParent()->getMostRecentDecl();
+  const CXXRecordDecl *RD = MD->getParent()->getMostRecentNonInjectedDecl();
   CodeGenTypes &Types = CGM.getTypes();
 
   unsigned VBTableIndex = 0;

lib/Sema/SemaType.cpp

 
 /// Locks in the inheritance model for the given class and all of its bases.
 static void assignInheritanceModel(Sema &S, CXXRecordDecl *RD) {
-  RD = RD->getMostRecentDecl();
+  RD = RD->getMostRecentNonInjectedDecl();
   if (!RD->hasAttr<MSInheritanceAttr>()) {
     MSInheritanceAttr::Spelling IM;