[PATCH] MS ABI: Improve selection of an inheritance model
Aaron Ballman
aaron at aaronballman.com
Wed Jan 15 05:55:02 PST 2014
Some attribute-related nits below, but aside from those, LGTM (though
I would get secondary confirmation).
> Index: include/clang/AST/DeclCXX.h
> ===================================================================
> --- include/clang/AST/DeclCXX.h
> +++ include/clang/AST/DeclCXX.h
> @@ -261,9 +261,7 @@
> /// The inheritance model to use for member pointers of a given CXXRecordDecl.
> enum MSInheritanceModel {
> MSIM_Single,
> - MSIM_SinglePolymorphic,
> MSIM_Multiple,
> - MSIM_MultiplePolymorphic,
> MSIM_Virtual,
> MSIM_Unspecified
> };
> @@ -1609,6 +1607,7 @@
>
> /// \brief Returns the inheritance model used for this record.
> MSInheritanceModel getMSInheritanceModel() const;
> + void setMSInheritanceModel();
>
> /// \brief Determine whether this lambda expression was known to be dependent
> /// at the time it was created, even if its context does not appear to be
> Index: include/clang/Basic/Attr.td
> ===================================================================
> --- include/clang/Basic/Attr.td
> +++ include/clang/Basic/Attr.td
> @@ -1354,7 +1354,12 @@
> Accessor<"IsUnspecified", [Keyword<"">]>];
> // This index is based off the Spellings list and corresponds to the empty
> // keyword "spelling."
> - let AdditionalMembers = [{static const int UnspecifiedSpellingIndex = 3;}];
> + let AdditionalMembers = [{
> + static const unsigned SingleSpellingIndex = 0;
> + static const unsigned MultipleSpellingIndex = 1;
> + static const unsigned VirtualSpellingIndex = 2;
> + static const unsigned UnspecifiedSpellingIndex = 3;
I'd rather to go great length to avoid exposing these further...
let AdditionalMembers = [{
// Note: these correspond to the spelling list index for a reason,
so keep them in sync.
enum Type {
Single,
Multiple,
Virtual,
Unspecified
};
static MSInheritanceAttr *Create(SourceRange R, ASTContext &C, Type T) {
return ::new (C) MSInheritanceAttr(R, C, T);
}
}];
This keeps all of the crazy and fragile in one spot. Once you land
that, I can change the usage in SemaType.cpp over to the new style,
and remove the old spelling list constant.
> + }];
> }
>
> def Unaligned : IgnoredAttr {
> Index: lib/AST/MicrosoftCXXABI.cpp
> ===================================================================
> --- lib/AST/MicrosoftCXXABI.cpp
> +++ lib/AST/MicrosoftCXXABI.cpp
> @@ -92,7 +92,8 @@
> return false;
> }
>
> -static MSInheritanceModel MSInheritanceAttrToModel(MSInheritanceAttr *Attr) {
> +static MSInheritanceModel
> +MSInheritanceAttrToModel(const MSInheritanceAttr *Attr) {
> if (Attr->IsSingle())
> return MSIM_Single;
> else if (Attr->IsMultiple())
> @@ -104,16 +105,44 @@
> return MSIM_Unspecified;
> }
>
> +static unsigned MSInheritanceModelToSpellingKind(MSInheritanceModel MSIM) {
> + switch (MSIM) {
> + case MSIM_Single:
> + return MSInheritanceAttr::SingleSpellingIndex;
> + case MSIM_Multiple:
> + return MSInheritanceAttr::MultipleSpellingIndex;
> + case MSIM_Virtual:
> + return MSInheritanceAttr::VirtualSpellingIndex;
> + case MSIM_Unspecified:
> + return MSInheritanceAttr::UnspecifiedSpellingIndex;
> + }
> +}
> +
> +
> +static MSInheritanceModel calculateInheritanceModel(const CXXRecordDecl *RD) {
> + if (!RD->hasDefinition())
> + return MSIM_Unspecified;
> + if (RD->getNumVBases() > 0)
> + return MSIM_Virtual;
> + if (usesMultipleInheritanceModel(RD))
> + return MSIM_Multiple;
> + return MSIM_Single;
> +}
> +
> MSInheritanceModel CXXRecordDecl::getMSInheritanceModel() const {
> - if (MSInheritanceAttr *IA = this->getAttr<MSInheritanceAttr>())
> + if (MSInheritanceAttr *IA = getAttr<MSInheritanceAttr>())
> return MSInheritanceAttrToModel(IA);
> - // If there was no explicit attribute, the record must be defined already, and
> - // we can figure out the inheritance model from its other properties.
> - if (this->getNumVBases() > 0)
> - return MSIM_Virtual;
> - if (usesMultipleInheritanceModel(this))
> - return this->isPolymorphic() ? MSIM_MultiplePolymorphic : MSIM_Multiple;
> - return this->isPolymorphic() ? MSIM_SinglePolymorphic : MSIM_Single;
> +
> + return calculateInheritanceModel(this);
> +}
> +
> +void CXXRecordDecl::setMSInheritanceModel() {
> + if (hasAttr<MSInheritanceAttr>())
> + return;
> +
> + MSInheritanceModel MSIM = calculateInheritanceModel(this);
> + addAttr(::new (getASTContext()) MSInheritanceAttr(
> + SourceRange(), getASTContext(), MSInheritanceModelToSpellingKind(MSIM)));
This should be using the source range for the record decl (better
error reporting if it ever comes up).
> }
>
> // Returns the number of pointer and integer slots used to represent a member
> @@ -148,7 +177,7 @@
> getMSMemberPointerSlots(const MemberPointerType *MPT) {
> const CXXRecordDecl *RD = MPT->getClass()->getAsCXXRecordDecl();
> MSInheritanceModel Inheritance = RD->getMSInheritanceModel();
> - unsigned Ptrs;
> + unsigned Ptrs = 0;
> unsigned Ints = 0;
> if (MPT->isMemberFunctionPointer()) {
> // Member function pointers are a struct of a function pointer followed by a
> @@ -160,22 +189,18 @@
> switch (Inheritance) {
> case MSIM_Unspecified: ++Ints; // VBTableOffset
> case MSIM_Virtual: ++Ints; // VirtualBaseAdjustmentOffset
> - case MSIM_MultiplePolymorphic:
> case MSIM_Multiple: ++Ints; // NonVirtualBaseAdjustment
> - case MSIM_SinglePolymorphic:
> case MSIM_Single: break; // Nothing
> }
> } else {
> // Data pointers are an aggregate of ints. The first int is an offset
> // followed by vbtable-related offsets.
> - Ptrs = 0;
> + Ints = 1; // We always have a field offset.
> switch (Inheritance) {
> case MSIM_Unspecified: ++Ints; // VBTableOffset
> case MSIM_Virtual: ++Ints; // VirtualBaseAdjustmentOffset
> - case MSIM_MultiplePolymorphic:
> - case MSIM_Multiple: // Nothing
> - case MSIM_SinglePolymorphic:
> - case MSIM_Single: ++Ints; // Field offset
> + case MSIM_Multiple:
> + case MSIM_Single: break; // Nothing
> }
> }
> return std::make_pair(Ptrs, Ints);
> Index: lib/CodeGen/MicrosoftCXXABI.cpp
> ===================================================================
> --- lib/CodeGen/MicrosoftCXXABI.cpp
> +++ lib/CodeGen/MicrosoftCXXABI.cpp
> @@ -1328,8 +1328,8 @@
>
> static bool hasOnlyOneField(bool IsMemberFunction,
> MSInheritanceModel Inheritance) {
> - return Inheritance <= MSIM_SinglePolymorphic ||
> - (!IsMemberFunction && Inheritance <= MSIM_MultiplePolymorphic);
> + return Inheritance <= MSIM_Single ||
> + (!IsMemberFunction && Inheritance <= MSIM_Multiple);
> }
>
> // Only member pointers to functions need a this adjustment, since it can be
> @@ -1349,8 +1349,10 @@
> // use zero for null. If there are multiple fields, we can use zero even if it
> // is a valid field offset because null-ness testing will check the other
> // fields.
> -static bool nullFieldOffsetIsZero(MSInheritanceModel Inheritance) {
> - return Inheritance != MSIM_Multiple && Inheritance != MSIM_Single;
> +static bool nullFieldOffsetIsZero(const CXXRecordDecl *RD) {
> + if (RD->getMSInheritanceModel() >= MSIM_Virtual)
> + return true;
> + return RD->isPolymorphic();
> }
>
> bool MicrosoftCXXABI::isZeroInitializable(const MemberPointerType *MPT) {
> @@ -1365,7 +1367,7 @@
> const CXXRecordDecl *RD = MPT->getClass()->getAsCXXRecordDecl();
> MSInheritanceModel Inheritance = RD->getMSInheritanceModel();
> return (!hasVirtualBaseAdjustmentField(Inheritance) &&
> - nullFieldOffsetIsZero(Inheritance));
> + nullFieldOffsetIsZero(RD));
> }
>
> llvm::Type *
> @@ -1401,7 +1403,7 @@
> // FunctionPointerOrVirtualThunk
> fields.push_back(llvm::Constant::getNullValue(CGM.VoidPtrTy));
> } else {
> - if (nullFieldOffsetIsZero(Inheritance))
> + if (nullFieldOffsetIsZero(RD))
> fields.push_back(getZeroInt()); // FieldOffset
> else
> fields.push_back(getAllOnesInt()); // FieldOffset
> @@ -1817,15 +1819,14 @@
> const MemberPointerType *SrcTy =
> E->getSubExpr()->getType()->castAs<MemberPointerType>();
> const MemberPointerType *DstTy = E->getType()->castAs<MemberPointerType>();
> - MSInheritanceModel SrcInheritance = getInheritanceFromMemptr(SrcTy);
> - MSInheritanceModel DstInheritance = getInheritanceFromMemptr(DstTy);
> bool IsFunc = SrcTy->isMemberFunctionPointer();
>
> // If the classes use the same null representation, reinterpret_cast is a nop.
> bool IsReinterpret = E->getCastKind() == CK_ReinterpretMemberPointer;
> - if (IsReinterpret && (IsFunc ||
> - nullFieldOffsetIsZero(SrcInheritance) ==
> - nullFieldOffsetIsZero(DstInheritance)))
> + if (IsReinterpret &&
> + (IsFunc ||
> + nullFieldOffsetIsZero(SrcTy->getClass()->getAsCXXRecordDecl()) ==
> + nullFieldOffsetIsZero(DstTy->getClass()->getAsCXXRecordDecl())))
> return Src;
>
> CGBuilderTy &Builder = CGF.Builder;
> @@ -1854,6 +1855,7 @@
> llvm::Value *NonVirtualBaseAdjustment = 0;
> llvm::Value *VirtualBaseAdjustmentOffset = 0;
> llvm::Value *VBPtrOffset = 0;
> + MSInheritanceModel SrcInheritance = getInheritanceFromMemptr(SrcTy);
> if (!hasOnlyOneField(IsFunc, SrcInheritance)) {
> // We need to extract values.
> unsigned I = 0;
> @@ -1884,6 +1886,7 @@
> // FIXME PR15713: Support conversions through virtually derived classes.
>
> // Recompose dst from the null struct and the adjusted fields from src.
> + MSInheritanceModel DstInheritance = getInheritanceFromMemptr(DstTy);
> llvm::Value *Dst;
> if (hasOnlyOneField(IsFunc, DstInheritance)) {
> Dst = FirstField;
> Index: lib/Sema/SemaType.cpp
> ===================================================================
> --- lib/Sema/SemaType.cpp
> +++ lib/Sema/SemaType.cpp
> @@ -1751,35 +1751,6 @@
> return QualType();
> }
>
> - // C++ allows the class type in a member pointer to be an incomplete type.
> - // In the Microsoft ABI, the size of the member pointer can vary
> - // according to the class type, which means that we really need a
> - // complete type if possible, which means we need to instantiate templates.
> - //
> - // If template instantiation fails or the type is just incomplete, we have to
> - // add an extra slot to the member pointer. Yes, this does cause problems
> - // when passing pointers between TUs that disagree about the size.
> - if (Context.getTargetInfo().getCXXABI().isMicrosoft()) {
> - CXXRecordDecl *RD = Class->getAsCXXRecordDecl();
> - if (RD && !RD->hasAttr<MSInheritanceAttr>()) {
> - // Lock in the inheritance model on the first use of a member pointer.
> - // Otherwise we may disagree about the size at different points in the TU.
> - // FIXME: MSVC picks a model on the first use that needs to know the size,
> - // rather than on the first mention of the type, e.g. typedefs.
> - if (RequireCompleteType(Loc, Class, 0) && !RD->isBeingDefined()) {
> - // We know it doesn't have an attribute and it's incomplete, so use the
> - // unspecified inheritance model. If we're in the record body, we can
> - // figure out the inheritance model.
> - for (CXXRecordDecl::redecl_iterator I = RD->redecls_begin(),
> - E = RD->redecls_end(); I != E; ++I) {
> - I->addAttr(::new (Context) MSInheritanceAttr(RD->getSourceRange(),
> - Context,
> - MSInheritanceAttr::UnspecifiedSpellingIndex));
> - }
> - }
> - }
> - }
> -
> // Adjust the default free function calling convention to the default method
> // calling convention.
> if (T->isFunctionType())
> @@ -5094,6 +5065,21 @@
> }
> }
>
> + // We lock-in the inheritance model once somebody has asked us to ensure
> + // that a pointer-to-member type is complete.
> + if (Context.getTargetInfo().getCXXABI().isMicrosoft()) {
> + if (const MemberPointerType *MPTy = T->getAs<MemberPointerType>()) {
> + if (!MPTy->getClass()->isDependentType()) {
> + RequireCompleteType(Loc, QualType(MPTy->getClass(), 0), 0);
> +
> + MPTy->getClass()
> + ->getAsCXXRecordDecl()
> + ->getMostRecentDecl()
> + ->setMSInheritanceModel();
> + }
> + }
> + }
> +
> return false;
> }
>
> Index: test/SemaCXX/microsoft-abi-ptm.cpp
> ===================================================================
> --- /dev/null
> +++ test/SemaCXX/microsoft-abi-ptm.cpp
> @@ -0,0 +1,11 @@
> +// RUN: %clang_cc1 -triple %ms_abi_triple -std=c++11 -fsyntax-only %s
> +
> +struct A;
> +void f(int A::*mp);
> +struct A { };
> +static_assert(sizeof(int A::*) == sizeof(int), "pointer-to-member should be sizeof(int)");
> +
> +struct B;
> +void f(int B::*mp);
> +static_assert(sizeof(int B::*) == sizeof(int) * 3, "pointer-to-member should be sizeof(int)*4");
> +struct B { };
>
~Aaron
On Wed, Jan 15, 2014 at 4:09 AM, David Majnemer
<david.majnemer at gmail.com> wrote:
> - Add a test-case for PR18479.
>
> Hi rnk, rsmith,
>
> http://llvm-reviews.chandlerc.com/D2548
>
> CHANGE SINCE LAST DIFF
> http://llvm-reviews.chandlerc.com/D2548?vs=6455&id=6456#toc
>
> Files:
> include/clang/AST/DeclCXX.h
> include/clang/Basic/Attr.td
> lib/AST/MicrosoftCXXABI.cpp
> lib/CodeGen/MicrosoftCXXABI.cpp
> lib/Sema/SemaType.cpp
> test/SemaCXX/microsoft-abi-ptm.cpp
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
More information about the cfe-commits
mailing list