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

[John McCall]

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);

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.

+  unsigned getMemberPointerAlign(const MemberPointerType *MPT) const {
+    const TargetInfo &Target = Context.getTargetInfo();
+    return Target.getTypeAlign(Target.getPtrDiffType(0));
   }
 
This is now dead.

+  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 

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

I would also suggest breaking these down first by IsFunc and then by
inheritance model.

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:

  if (IsFunc) {
    // Member function pointers have the following general form;  however, fields
    // are dropped as permitted (under the MSVC interpretation) by the inheritance
    // model of the actual class.
    //
    //   struct {
    //     union {
    //       // A pointer to the member function to call.
    //       void *NonVirtualFunctionPointer;
    //
    //       // An offset in the vf-table, distinguished from above by XXX.
    //       // Note that vf-tables are always at offset 0.
    //       ptrdiff_t VirtualFunctionOffset;
    //     };
    //
    //     // An offset to add to 'this' after (possibly) selecting the virtual base but
    //     // before resolving and calling the function.  Only needed if the class has
    //     // any virtual bases or bases at a non-zero offset.
    //     int NonVirtualBaseAdjustment;
    //
    //     // An offset within the vb-table of an offset to add to the 'this' pointer, or
    //     // XXX to indicate that this is not a member of a virtual base.  Only
    //     // needed if the class has virtual inheritance.
    //     int VirtualBaseAdjustmentOffset;
    //
    //     // The offset of the vb-table, or whatever this is.  Only needed for
    //     // incomplete types.
    //     int VBTableOffset;
    //   };
    //
    // Note that this structure is always pointer-aligned and may require
    // tail-padding to bring it up to a multiple of its alignment.
    //
    // A member function pointer is null if XXX.
    switch (Inheritance) { ... }
  } else {

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

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

John.