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

[Reid Kleckner]

On Fri, Mar 22, 2013 at 4:44 PM, John McCall <rjmccall at apple.com> wrote:

> On Mar 22, 2013, at 11:41 AM, Reid Kleckner <rnk at google.com> wrote:
> >    - Update the test case to pass under x86_64.
> >
> > Hi rjmccall,
> >
> > http://llvm-reviews.chandlerc.com/D568
>
> +    // FIXME: Handling x86_64 will require changing this method to return
> sizes
> +    // in bytes instead of pointers.  Some member pointer sizes are not
> +    // multiples of a pointer size.
>
> So, change the method to return sizes in bytes instead of pointers instead
> of reporting a diagnostic in a core AST routine.
>

OK, I'll change the method.  I assumed there'd be more than one caller.  :)

What should I do for non-x86 architectures then?  I could assert, since
presumably we'll have errorred out much earlier if someone asks the
frontend for mips code.

There's prior art for issuing diagnostics in AST from RecordLayoutBuilder,
but maybe that's different.

+  attr::Kind Inheritance;
>
> +      // This last case seems like an error, but MSVC allocates one more
> slot
> +      // than is used for virtual inheritance.  We follow suit for
> +      // compatibility.
>
> My understanding is that it's required for vtordisps or something.
>
> +      // FIXME: Issue a warning.
>
> Well, you wouldn't issue a warning in the AST library, so the FIXME doesn't
> go here.
>
> +    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();
>
> You should really make a common base class for these attributes.
> I believe this is just as simple as adding:
>   class MSInheritanceAttr : InheritableAttr;
> to Attr.td, making the attribute defs inherit from that, and then defining
> that
> class in Attr.h.
>

I can do that.  It'll be in the next patch, but I'll commit it separately.


> +// getNumBases() seems to only give us the number of direct bases, and
> not the
> +// total.  This function tells us if we inherit from anybody that uses MI.
> +static bool recordHasMultipleInheritance(const CXXRecordDecl *RD) {
> +  while (RD->getNumBases() > 0) {
> +    if (RD->getNumBases() > 1)
> +      return true;
> +    assert(RD->getNumBases() == 1);
> +    RD = RD->bases_begin()->getType()->getAsCXXRecordDecl();
> +  }
> +  return false;
> +}
>
> Hmm.  I bet that non-primary base class forces the "multiple inheritance"
> case:
>   struct A { int x; void bar(); };
>   struct B : A { virtual void foo(); };
> What's sizeof(void (B::*)())?
>

Nice catch, 2 * sizeof(void*), so it's the "multiple" case.  Added a test
case.

What do I need to look for to identify this case?  A class that is
polymorphic, but whose base is not?


> +  switch (Inheritance) {
> +  case attr::SingleInheritance:
> +    return 1;
> +  case attr::MultipleInheritance:
> +    return 1 + int(Pointee->isFunctionType());
> +  case attr::VirtualInheritance:
> +    return 2 + int(Pointee->isFunctionType());
>
> Please try to avoid doing the somewhat-expensive query about multiple
> inheritance if this is a non-function member pointer.
>

OK.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130322/4afe6e76/attachment.html>