New patch attached On Mon, Apr 8, 2013 at 7:58 PM, John McCall <[email protected]> wrote: > > On Apr 2, 2013, at 11:00 AM, Reid Kleckner <[email protected]> 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.
msvc-memptrs.diff
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
