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

[John McCall]

On Mar 27, 2013, at 6:22 PM, Reid Kleckner <rnk at google.com> wrote:
> 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.

Okay.  Mostly, I'd like you to have some comment explaining where
these numbers come from.  It's okay if it's vague about things like ordering.

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

Sorry, I meant that, in Microsoft mode, RequireCompleteType could do
something special for MemberPointerTypes, not just RecordTypes.
That would catch the majority of places where we need to know the size —
for example, we don't call RequireCompleteType for return types when
just declaring a function (or making a function type), but we do call it when
calling or defining the function.

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