[PATCH] [ms-cxxabi] Correctly compute the size of member pointers

[Reid Kleckner]

On Tue, Mar 26, 2013 at 11:08 AM, John McCall <rjmccall at apple.com> wrote:

> On Mar 22, 2013, at 4:50 PM, Reid Kleckner <rnk at google.com> wrote:
> >    - Implement John's comments.
>
> -    std::pair<uint64_t, unsigned> PtrDiffInfo =
> -      getTypeInfo(getPointerDiffType());
> -    Width = PtrDiffInfo.first * ABI->getMemberPointerSize(MPT);
> -    Align = PtrDiffInfo.second;
> +    Width = ABI->getMemberPointerWidth(MPT);
> +    Align = ABI->getMemberPointerAlign(MPT);
>
> Please have one function that returns both of these.  No reason to
> do two virtual calls when we can do one.
>
> This will also allow you to get the alignment right more easily on 64-bit.
>

Done.  It just didn't seem like the style of the rest of the ASTContext
queries.


> +static bool usesMultipleInheritanceModel(const MemberPointerType *MPT,
> +                                         const CXXRecordDecl *RD) {
> +  // For data pointers, the representation is the same for single and
> multiple
> +  // inheritance classes.  We return false to use the single inheritance
> model.
> +  if (MPT->isMemberDataPointerType())
> +    return false;
> +
>
> This isn't quite what I meant.  The top-level routine already decides
> whether
> the member type is a function or not.  Just don't call this function if it
> is.
>

I'm adding the implicit attribute now, so I don't think I can do this
optimization any more.


> That is, change this:
> +    else if (usesMultipleInheritanceModel(MPT, RD))
> to
>  +    else if (isFunc && usesMultipleInheritanceModel(MPT, RD))
>
> +  if (RD->getTypeForDecl()->isIncompleteType()) {
> +    if (Attr *IA = RD->getAttr<SingleInheritanceAttr>())
> +      Inheritance = IA->getKind();
> +    else if (Attr *IA = RD->getAttr<MultipleInheritanceAttr>())
> +      Inheritance = IA->getKind();
> +    else if (Attr *IA = RD->getAttr<VirtualInheritanceAttr>())
> +      Inheritance = IA->getKind();
> +    else
> +      Inheritance = attr::UnspecifiedInheritance;
> +  } else {
>
> Check for the attribute first, and check for it with:
>   if (Attr *IA = RD->getAttr<MSInheritanceAttr>()) {
>   }
>

Much better.  :)  There needs to be a diagnostic if we have multiple
inheritance attrs, though.


> That way you can e.g. implicitly add your UnspecifiedInheritanceAttr to a
> class decl and it'll just work.  Also it means you don't have to enumerate
> your
> inheritance attribute kinds.
>
> +  // If template instantiation fails or the type is just incomplete, the
> ABI has
> +  // a poorly understood fallback mechanism in place, so we continue
> without any
> +  // diagnostics.
> +  if (Context.getTargetInfo().getCXXABI().isMicrosoft())
> +    RequireCompleteType(Loc, Class, 0);
>
> This would be an excellent opportunity to implicitly add your
> UnspecifiedInheritanceAttr to all the existing declarations. :)
>

Is it OK if I just implicitly add the right attribute if there isn't one
present already?  I suppose the unspecified model is the only one that I
need to add for correctness.

It's actually important to lock this in at some point so we don't have
different sizes at different TU points.  In MSVC, if you declare a member
pointer variable before the class definition, it uses the unspecified
model.  If you wait until after the definition, it will pick a more
restrictive model.


> The changes to ClangAttrEmitter look fine.


Committed separately as r178054.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130326/429ecf00/attachment.html>