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

[John McCall]

On Mar 26, 2013, at 2:46 PM, Reid Kleckner <rnk at google.com> wrote:
> On Tue, Mar 26, 2013 at 11:08 AM, John McCall <rjmccall at apple.com> wrote:
> 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.

Yep.

> 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.

Yeah, I saw your comment to this effect earlier.  I think adding UnspecifiedInheritanceAttr when you make a member pointer for a forward-declared class that lacks an explicit inheritance attribute makes sense.  I wouldn't go down the road of adding, say, SingleInheritanceAttr to every class with single inheritance.

For sanity's sake, please add the attribute to all the existing declarations, though.

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130326/b4d04316/attachment.html>