[PATCH] MS ABI: Improve selection of an inheritance model
David Majnemer
david.majnemer at gmail.com
Wed Jan 15 09:27:29 PST 2014
On Wednesday, January 15, 2014, Aaron Ballman <aaron at aaronballman.com>
wrote:
> 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).
I chose not to because I wanted a way to mark the Attr implicit so that
tooling, AST dumps and diagnostics wouldn't believe there was one in the
source. I can change it to use the record decl's range but could we
(eventually) get a way to say it was manufactured by clang?
P.S. Personally, I think it would be confusing to use the source range of
this, or any, implicit Attr. The diagnostics wouldn't really make sense to
the user.
> > }
> >
> > // 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
> >
> > _______________________________________________
> > cfe-commits mailing list
> >
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140115/014af3a3/attachment.html>
More information about the cfe-commits
mailing list