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

[Reid Kleckner]

On Tue, Apr 9, 2013 at 7:50 PM, John McCall <rjmccall at apple.com> wrote:
> On Apr 9, 2013, at 3:02 PM, Reid Kleckner <rnk at google.com> wrote:
>> 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:
>>> +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?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: msvc-memptrs.diff
Type: application/octet-stream
Size: 31444 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130411/4ad57d1f/attachment.obj>