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

[Reid Kleckner]

On Tue, Mar 26, 2013 at 3:57 PM, John McCall <rjmccall at apple.com> wrote:

> On Mar 26, 2013, at 2:50 PM, Reid Kleckner <rnk at google.com> wrote:
> >    - Update the test case to pass under x86_64.
> >    - Implement John's comments.
> >    - commit attr tablegen changes, add implicit model attr.
> >
> > Hi rjmccall,
>
> +    std::pair<uint64_t, unsigned> MemPtrInfo =
> +      ABI->getMemberPointerWidthAndAlign(MPT);
> +    Width = MemPtrInfo.first;
> +    Align = MemPtrInfo.second;
>
> This can just be
>   llvm::tie(Width,Align) = ABI->getMemberPointerWidthAndAlign(MPT);
>

Nice.


> Index: lib/AST/CXXABI.h
> ===================================================================
> --- lib/AST/CXXABI.h
> +++ lib/AST/CXXABI.h
> @@ -15,6 +15,7 @@
>  #ifndef LLVM_CLANG_AST_CXXABI_H
>  #define LLVM_CLANG_AST_CXXABI_H
>
> +#include "clang/AST/CharUnits.h"
>
> You don't appear to use anything from this.
>

Gone.


> +  unsigned getMemberPointerAlign(const MemberPointerType *MPT) const {
> +    const TargetInfo &Target = Context.getTargetInfo();
> +    return Target.getTypeAlign(Target.getPtrDiffType(0));
>    }
>
> This is now dead.
>

Gone.


> +  Attr *IA = RD->getAttr<MSInheritanceAttr>();
> +  assert(IA && "should have added implicit attribute in sema");
> +  attr::Kind Inheritance = IA->getKind();
> +
>
> I'm not thrilled by the idea of forcing the existence of the attribute.
> Adding one makes sense as a way to capture the MSVC behavior
> of locking in unspecified sizes as soon as a member pointer is asked
> for, but maybe
>

Yeah, I removed those attributes.


> +
> +  // FIXME: Verify that our alignment matches MSVC.
> +  unsigned Align = Target.getTypeAlign(Target.getPtrDiffType(0));
> +  return std::make_pair(Width, Align);
>
> Okay, I'm willing to give very good odds that the alignment is the
> alignment
> of the notional member-pointer struct, which works out to alignof(int) if
> the
> member pointer is nothing but ints and alignof(ptrdiff_t) otherwise.  You
> should compute this in your switch cases.
>

Done.  It looks like function pointers always have a pointer, and data
pointers never do.


> I would also suggest breaking these down first by IsFunc and then by
> inheritance model.
>

Yep.


> Also, it would make sense to have comments that actually describe these
> structures.  For example, I'm guessing that it's something like this,
> although
> I probably have the fields mixed up:


[snip]

That seems pretty close to me, but I hesitate to clean it up and commit it
without being certain.  :)  When I do codegen later I can commit some
documentation of the fields with greater confidence.


> +  // 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.
>
> You go on to describe this fallback mechanism, so I wouldn't call it
> poorly-understood any more.
>

OK.


> +      // 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.
>
> Well, I guess we could hook this into RequireCompleteType.
>

Hrm?  But then the member pointer size might be picked at some earlier
point in time where we don't have a complete record definition.  I think
it's best to wait until we're really going to use a member pointer type.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130327/441d2833/attachment.html>