On Tue, Apr 9, 2013 at 7:50 PM, John McCall <[email protected]> wrote: > On Apr 9, 2013, at 3:02 PM, Reid Kleckner <[email protected]> wrote: >> 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: >>> +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. > > Seems reasonable. > > As an aside, I wonder what the right trade-off here actually is: an extra > branch vs. an unnecessary memory dependence... Anyway, it's not > important right now.
Yeah, hard to say.
> + // If the class has an explicit inheritance model attribute, we
> conservatively
> + // use -1. This requires different TUs to see the same attribute or they
> have
> + // an ODR problem.
> + if (RD->hasAttr<MSInheritanceAttr>())
> + return true;
>
> If this is really how MSVC handles this issue — and that seems reasonable
> enough to me — then arguably these cases need to be distinguished in the
> MSInheritanceModel enum. I think we'd have six cases:
> - has no bases at non-zero offsets, known not polymorphic or unknown
> polymorphism
> - has no bases at non-zero offsets, known polymorphic
> - has bases at static non-zero offsets, known not polymorphic or unknown
> polymorphism
> - has bases at static non-zero offsets, known polymorphic
> - has virtual bases
> - unspecified
>
> The inheritance model is fundamentally about member pointers. It makes
> sense for it to capture everything necessary in order to interpret a member
> pointer.
Sounds good, I added MSIM_MultiplePolymorphic and MSIM_SinglePolymorphic.
> That would also make it easier to diagnose the following invalid code (unless
> we already diagnose attempts to add inheritance keywords after the
> definition?).
> class A {
> virtual void foo();
> };
> class __single_inheritance A;
I'm not sure, I need to go back and do a pass for diagnostics.
> + // Since zero is a valid offset for classes without vtables, they use -1.
> For
> + // some reason, classes with vtables use 0 in this ABI. In this
> inheritance
> + // model, only polymorphic classes have vtables.
> + return !RD->isPolymorphic();
>
> The reason's pretty straightforward: it is generally better to use 0 instead
> of -1
> because it's usually easier to produce 0 than -1. That's true even on the
> smallest level — on x86 it's a smaller instruction to load 0 into a register
> than to load -1 — but more importantly, it means that the compiler can do
> things
> like put a zero-initialized global in a zero-fill section or zero-initialize
> an
> object with a simple memset.
>
> + // There is only one kind of class with data member pointers that can be
> zero
> + // initialized: single or multiple inheritance classes with a vfptr.
>
> I'd put it this way: MSVC uses 0 as the null offset if it can guarantee that
> that won't point at a data member. It can make that guarantee:
> - if the class is known to be polymorphic (because offset 0 will be the
> vf-table), or otherwise
> - if the class is known to have virtual bases (because offset 0 will be the
> vb-table)
> class is known to have virtual bases (because offset 0 will be the vb-table).
>
> The way that you've got this logic split between NullFieldOffsetIsAllOnes and
> isZeroInitializable is really confusing to me.
>
> + if (Inheritance >= MSIM_Virtual)
>
> Instead of doing checks like this, consider making little predicate functions:
> static bool hasVirtualBaseAdjustmentField(MSInheritanceModel);
> It would remove the need for quite so many little comments everywhere.
This made the code much better, thanks!
> +llvm::SmallVector<llvm::Constant *, 4>
> +MicrosoftCXXABI::GetNullMemberPointerFields(const MemberPointerType *MPT) {
>
> Please have this take a SmallVectorImpl<llvm::Constant*> by reference instead
> of returning one.
OK.
> + // http://llvm.org/PR15713 : Taking a pointer to a member of a virtual
> + // base is not standard C++. Clang will reject this in SemaCast, so we
> + // don't have to handle it here.
>
> As covered above, when emitting a member pointer constant, you'll always
> be making something in its declaring class. This is why you can just ask
> for the offset of a field instead of computing offsets all along the base
> path.
Yep, I'll document that.
> + // in which case we need to skip the virtual base lookup. If there were a
> + // vbtable, the first entry is a no-op entry that gives back the original
>
> Grammar: "If there is a vbtable..."
OK. I'm confused about subjunctive tense. =/
> + llvm::BasicBlock *OriginalBB;
> + llvm::BasicBlock *SkipAdjustBB;
>
> You'll need to zero-initialize these in order to pacify compiler warnings.
OK. There's been some debate on the list, and I wasn't sure which way to go.
> + // Add it to VBPtr.
> + llvm::Value *AdjustedBase = Builder.CreateInBoundsGEP(VBPtr, VBaseOffs);
>
> On Win64, you need to be sure to sign-extend this. (vb-offsets can be
> negative in very particular circumstances.)
GEP sign extends the i32 implicitly, apparently.
> + assert(MPT->isMemberFunctionPointer());
> + const FunctionProtoType *FPT =
> + MPT->getPointeeType()->getAs<FunctionProtoType>();
>
> This can be castAs<>.
OK.
> Otherwise, this is looking great, thanks!
Thanks for the helpful review!
Should I commit the patch attached?
msvc-memptrs.diff
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
