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