[PATCH] [ms-cxxabi] Implement codegen for dereferencing member pointers

[Reid Kleckner]

New patch attached

On Mon, Apr 8, 2013 at 7:58 PM, John McCall <rjmccall at apple.com> wrote:
>
> On Apr 2, 2013, at 11:00 AM, Reid Kleckner <rnk at google.com> wrote:
> > Handles all inheritance models for both data and function member
> > pointers.  This does not require knowing the layout of the vbtable,
> > because the member pointer itself carries the offset into the vbtable.
> >
> > Clang still cannot emit multi-field member pointers because it would
> > need to know the layout of the vbtable to generate the correct offsets.
> >
> > Also implements isZeroInitializable() and refactors some of the null
> > member pointer code.
>
> +  llvm::tie(Ptrs, Ints) = MPT->getMSMemberPointerSlots();
>
> I missed this particular method getting added.  I think this is probably
> going a bit too far.  It's not really generally meaningful;  you have two
> clients of it, and one of them has to do weird special-casing anyway
> (i.e. filling in -1 in the right place).

OK.  I ended up having to duplicate switches on the inheritance model,
so I guess I might as well do it here.  I'll do a separate change on
the AST layer to get rid of this helper.  Alternatively I could try to
raise the memptr layout up into the AST layer and query it from here.

>
> Also, your methods aren't consistent about null pointers.  Your isZeroInitializable
> says that all the bits are zero unless the class uses single inheritance and
> isn't polymorphic, but then EmitNullMemberPointer tosses a -1 on the end
> of certain wide null-pointers.

Yeah, this was broken.  The logic I was using only applies to function
member pointers.

> It looks like these are the things we can safely zero initialize:
> - pointers to member functions
> - pointers to data members of single inheritance polymorphic classes
>
> +  switch (Inheritance) {
> +  default:
> +    llvm_unreachable("invalid inheritance");
>
> Kill the default case and put the unreachable after the switch.  Having a
> default case prevents the compiler from warning you about missing cases.

OK.

>
> +  case MSIM_Unspecified: {
> +    CharUnits offs = getContext().getASTRecordLayout(RD).getVBPtrOffset();
> +    llvm::Constant *VBTableOffset =
> +      llvm::ConstantInt::get(CGM.IntTy, offs.getQuantity());
> +    llvm::Constant *fields[] = {
> +      FieldOffset,
> +      VBTableOffset,
> +      getZeroInt()    // FIXME: VirtualBaseAdjustmentOffset
> +    };
>
> The VirtualBaseAdjustmentOffset should be some special value to indicate
> that this isn't a member of a virtual base;  you just need to leave that value
> in there.

The first entry of a vbtable points to the base of the current
subobject, so zero should work for that.

> Also, are you sure you're supposed to leave a meaningful vb-table offset
> in here even when the member *isn't* a member of a virtual base?

I believe zero is correct for that, for the same reason as above.

It looks like taking a pointer to a member of a virtual base is an
MSVC language extension.  Clang rejects my attempts to form member
pointers to virtual bases.  I filed http://llvm.org/PR15713 to track
any decisions made on that front.  Currently, I don't have the decl so
I can't assert without changing the API, so I'll just always emit zero
and comment this.

> Also, recall that the class very well might not have a vb-table;  there might
> be a signal value for that, too.

You're right, I had to fix that.  Can't go dereferencing non-existent vbptrs.

> +  llvm::Type *Int8Ptr = Builder.getInt8Ty()->getPointerTo(0);
>
> This is CGM.Int8PtrTy.

OK.

> +llvm::Value *
> +MicrosoftCXXABI::AdjustVirtualBase(CGBuilderTy &Builder,
> +                                   const CXXRecordDecl *RD, llvm::Value *Base,
> +                                   llvm::Value *VirtualBaseAdjustmentOffset,
> +                                   llvm::Value *VBTableOffset) {
>
> This code is assuming that it *has* to adjust by a virtual base.
> That is not the case (unless MSVC does something interesting
> in its vb-table layouts, like leave a zero offset at a particular offset
> in the table...), and even then it's suspect in the MSIM_unspecified
> case where it's completely legitimate that the record type might not
> have a vb-table.

Good catch: the first vbtable entry points back to the base of the
current subobject.  That means it's safe to perform the adjustment so
long as we know there is a vbptr.  MSVC does this so I followed suit.

However, I had to conditionalize the adjustment in the unspecified
case to avoid dereferencing a vbptr that isn't here.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: msvc-memptrs.diff
Type: application/octet-stream
Size: 27143 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130409/8009612e/attachment.obj>