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

CXXRecordDecl *getMostRecentDecl() {
     return cast<CXXRecordDecl>(
             static_cast<RecordDecl *>(this)->getMostRecentDecl());
   }
 
   const CXXRecordDecl *getMostRecentDecl() const {
     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.
     auto *DD = DefinitionData ? DefinitionData : dataPtr();
     return DD ? DD->Definition : nullptr;
   }
 
   bool hasDefinition() const { return DefinitionData || dataPtr(); }

include/clang/AST/DeclTemplate.h

void getNameForDiagnostic(raw_ostream &OS, const PrintingPolicy &Policy,
                             bool Qualified) const override;
 
   // FIXME: This is broken. CXXRecordDecl::getMostRecentDecl() returns a
   // different "most recent" declaration from this function for the same
   // declaration, because we don't override getMostRecentDeclImpl(). But
   // it's not clear that we should override that, because the most recent
   // declaration as a CXXRecordDecl sometimes is the injected-class-name.
   ClassTemplateSpecializationDecl *getMostRecentDecl() {
-    CXXRecordDecl *Recent = static_cast<CXXRecordDecl *>(
-                              this)->getMostRecentDecl();
-    while (!isa<ClassTemplateSpecializationDecl>(Recent)) {
-      // FIXME: Does injected class name need to be in the redeclarations chain?
-      assert(Recent->isInjectedClassName() && Recent->getPreviousDecl());
-      Recent = Recent->getPreviousDecl();
-    }
-    return cast<ClassTemplateSpecializationDecl>(Recent);
+    return cast<ClassTemplateSpecializationDecl>(
+        getMostRecentNonInjectedDecl());
   }
 
   /// Retrieve the template that this specialization specializes.
   ClassTemplateDecl *getSpecializedTemplate() const;
 
   /// Retrieve the template arguments of the class template
   /// specialization.
   const TemplateArgumentList &getTemplateArgs() const {

lib/AST/MicrosoftMangle.cpp

case TemplateArgument::Type: {
     QualType T = TA.getAsType();
     mangleType(T, SourceRange(), QMM_Escape);
     break;
   }
   case TemplateArgument::Declaration: {
     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);
         mangleFunctionEncoding(FD, /*ShouldMangle=*/true);
       }
     } else {
       mangle(ND, TA.getParamTypeForDecl()->isReferenceType() ? "$E?" : "$1?");
     }

lib/AST/Type.cpp

if (ClassTy->isDependentType())
       return false;
     const CXXRecordDecl *RD = ClassTy->getAsCXXRecordDecl();
     ASTContext &Context = RD->getASTContext();
     // Member pointers not in the MS ABI don't get special treatment.
     if (!Context.getTargetInfo().getCXXABI().isMicrosoft())
       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;
     return true;
   }
   case ObjCObject:
     return cast<ObjCObjectType>(CanonicalType)->getBaseType()
              ->isIncompleteType(Def);
if (const auto *CXXRD = dyn_cast<CXXRecordDecl>(RD)) {
         return DK_nontrivial_c_struct;
     }
   }
 
   return DK_none;
 }
 
 CXXRecordDecl *MemberPointerType::getMostRecentCXXRecordDecl() const {
-  return getClass()->getAsCXXRecordDecl()->getMostRecentDecl();
+  return getClass()->getAsCXXRecordDecl()->getMostRecentNonInjectedDecl();
 }

lib/CodeGen/MicrosoftCXXABI.cpp

llvm::Constant *MicrosoftCXXABI::EmitMemberPointer(const APValue &MP,
   return C;
 }
 
 llvm::Constant *
 MicrosoftCXXABI::EmitMemberFunctionPointer(const CXXMethodDecl *MD) {
   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;
   llvm::Constant *FirstField;
   const FunctionProtoType *FPT = MD->getType()->castAs<FunctionProtoType>();
   if (!MD->isVirtual()) {
     llvm::Type *Ty;
     // Check whether the function has a computable LLVM signature.

lib/Sema/SemaType.cpp

if (auto *Source = Context.getExternalSource()) {
     return isVisible(D);
   }
 
   return false;
 }
 
 /// 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;
 
     switch (S.MSPointerToMemberRepresentationMethod) {
     case LangOptions::PPTMK_BestCase:
       IM = RD->calculateInheritanceModel();
       break;
     case LangOptions::PPTMK_FullGeneralitySingleInheritance:

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 %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++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)();
+};
+// CHECK-DAG: %"struct.pr37399::Functor" = type { i8* }
+
+template <typename SomeType>
+class SimpleDerivedFunctor;
+template <typename SomeType>
+class SimpleDerivedFunctor : public Functor<SimpleDerivedFunctor<SomeType>> {};
+// CHECK-DAG: %"class.pr37399::SimpleDerivedFunctor" = type { %"struct.pr37399::Functor" }
+
+SimpleDerivedFunctor<void> SimpleFunctor;
+// CHECK-DAG: @"?SimpleFunctor@pr37399@@3V?$SimpleDerivedFunctor@X@1@A" = dso_local global %"class.pr37399::SimpleDerivedFunctor" zeroinitializer, align 4
+
+short Global = 0;
+template <typename SomeType>
+class DerivedFunctor;
+template <typename SomeType>
+class DerivedFunctor
+    : public Functor<DerivedFunctor<void>> {
+public:
+  void Foo() {
+    Global = 42;
+  }
+};
+
+class MultipleBase {
+public:
+  MultipleBase() : Value() {}
+  short Value;
+};
+// CHECK-DAG: %"class.pr37399::MultipleBase" = type { i16 }
+
+template <typename SomeType>
+class MultiplyDerivedFunctor;
+template <typename SomeType>
+class MultiplyDerivedFunctor
+    : public Functor<MultiplyDerivedFunctor<void>>,
+      public MultipleBase {
+public:
+  void Foo() {
+    MultipleBase::Value = 42*2;
+  }
+};
+
+class VirtualBase {
+public:
+  VirtualBase() : Value() {}
+  short Value;
+};
+// CHECK-DAG: %"class.pr37399::VirtualBase" = type { i16 }
+
+template <typename SomeType>
+class VirtBaseFunctor
+    : public Functor<SomeType>,
+      public virtual VirtualBase{};
+template <typename SomeType>
+class VirtuallyDerivedFunctor;
+template <typename SomeType>
+class VirtuallyDerivedFunctor
+    : public VirtBaseFunctor<VirtuallyDerivedFunctor<void>>,
+      public virtual VirtualBase {
+public:
+  void Foo() {
+    VirtualBase::Value = 42*3;
+  }
+};
+} // namespace pr37399
+
+pr37399::DerivedFunctor<int>           BFunctor;
+// CHECK-DAG: @"?BFunctor@@3V?$DerivedFunctor@H@pr37399@@A" = dso_local global %"[[BFUNCTOR:class.pr37399::DerivedFunctor(\.[0-9]+)?]]" zeroinitializer, align 8
+// CHECK-DAG: %"[[BFUNCTOR]]" = type { %"[[BFUNCTORBASE:struct.pr37399::Functor(\.[0-9]+)?]]" }
+// CHECK-DAG: %"[[BFUNCTORBASE]]" = type { { i8*, i32, i32, i32 } }
+pr37399::DerivedFunctor<void>          AFunctor;
+// CHECK-DAG: @"?AFunctor@@3V?$DerivedFunctor@X@pr37399@@A" = dso_local global %"[[AFUNCTOR:class.pr37399::DerivedFunctor(\.[0-9]+)?]]" zeroinitializer, align 8
+// CHECK-DAG: %"[[AFUNCTOR]]" = type { %"[[AFUNCTORBASE:struct.pr37399::Functor(\.[0-9]+)?]]" }
+// CHECK-DAG: %"[[AFUNCTORBASE]]" = type { { i8*, i32, i32, i32 } }
+
+pr37399::MultiplyDerivedFunctor<int>   DFunctor;
+// CHECK-DAG: @"?DFunctor@@3V?$MultiplyDerivedFunctor@H@pr37399@@A" = dso_local global %"[[DFUNCTOR:class.pr37399::MultiplyDerivedFunctor(\.[0-9]+)?]]" zeroinitializer, align 8
+// CHECK-DAG: %"[[DFUNCTOR]]" = type { %"[[DFUNCTORBASE:struct.pr37399::Functor(\.[0-9]+)?]]", %"class.pr37399::MultipleBase", [6 x i8] }
+// CHECK-DAG: %"[[DFUNCTORBASE]]" = type { { i8*, i32, i32, i32 } }
+pr37399::MultiplyDerivedFunctor<void>  CFunctor;
+// CHECK-DAG: @"?CFunctor@@3V?$MultiplyDerivedFunctor@X@pr37399@@A" = dso_local global %"[[CFUNCTOR:class.pr37399::MultiplyDerivedFunctor(\.[0-9]+)?]]" zeroinitializer, align 8
+// CHECK-DAG: %"[[CFUNCTOR]]" = type { %"[[CFUNCTORBASE:struct.pr37399::Functor(\.[0-9]+)?]]", %"class.pr37399::MultipleBase", [6 x i8] }
+// CHECK-DAG: %"[[CFUNCTORBASE]]" = type { { i8*, i32, i32, i32 } }
+
+pr37399::VirtuallyDerivedFunctor<int>  FFunctor;
+// CHECK-DAG: @"?FFunctor@@3V?$VirtuallyDerivedFunctor@H@pr37399@@A" = dso_local global %"[[FFUNCTOR:class.pr37399::VirtuallyDerivedFunctor(\.[0-9]+)?]]" zeroinitializer, align 8
+// CHECK-DAG: %"[[FFUNCTOR]]" = type { %"class.pr37399::VirtBaseFunctor.base", %"class.pr37399::VirtualBase" }
+pr37399::VirtuallyDerivedFunctor<void> EFunctor;
+// CHECK-DAG: @"?EFunctor@@3V?$VirtuallyDerivedFunctor@X@pr37399@@A" = dso_local global %"[[EFUNCTOR:class.pr37399::VirtuallyDerivedFunctor(\.[0-9]+)?]]" zeroinitializer, align 8
+// CHECK-DAG: %"[[EFUNCTOR]]" = type { %"class.pr37399::VirtBaseFunctor.base", %"class.pr37399::VirtualBase" }
+
+// CHECK-DAG: %"class.pr37399::VirtBaseFunctor.base" = type <{ %"[[VFUNCTORBASE:struct.pr37399::Functor(\.[0-9]+)?]]", i32*, [4 x i8] }>
+// CHECK-DAG: %"[[VFUNCTORBASE]]" = type { { i8*, i32, i32, i32 } }
+
+namespace pr37399 {
+void SingleInheritanceFnPtrCall() {
+  BFunctor.PtrToMemberFunction = &DerivedFunctor<void>::Foo;
+  (AFunctor.*(BFunctor.PtrToMemberFunction))();
+}
+void MultipleInheritanceFnPtrCall() {
+  DFunctor.PtrToMemberFunction = &MultiplyDerivedFunctor<void>::Foo;
+  Global = CFunctor.MultipleBase::Value;
+  (CFunctor.*(DFunctor.PtrToMemberFunction))();
+  Global = CFunctor.MultipleBase::Value;
+}
+void VirtualInheritanceFnPtrCall() {
+  FFunctor.PtrToMemberFunction = &VirtuallyDerivedFunctor<void>::Foo;
+  Global = EFunctor.VirtualBase::Value;
+  (EFunctor.*(FFunctor.PtrToMemberFunction))();
+  Global = EFunctor.VirtualBase::Value;
+}
+} // namespace pr37399
+
 struct PR26313_Y;
 typedef void (PR26313_Y::*PR26313_FUNC)();
 struct PR26313_X {
   PR26313_FUNC *ptr;
   PR26313_X();
 };
 PR26313_X::PR26313_X() {}
 void PR26313_f(PR26313_FUNC *p) { delete p; }