[PATCH] Add MicrosoftVFTableContext to AST

[John McCall]

+  if (Thunk.This.NonVirtual != 0) {
+    // FIXME: add support for protected/private or use mangleFunctionClass.
+    Out << "W";
+    llvm::APSInt APSNumber(/*BitWidth=*/32 /*TODO: check on x64*/, /*isUnsigned=*/true);
+    APSNumber = -Thunk.This.NonVirtual;
+    Mangler.mangleNumber(APSNumber);

I really wanted to complain about this, but it turns out that the APSInt API is really just that terrible.

+  llvm_unreachable("VBase must be a vbase of Derived");
+  return 0;
+}

You shouldn't need this return here.

+//  2. Each non-primary base class that has new virtual methods gets its
+//     own vfptr and vftable - no address points needed!

The lack of a need for address points is actually because the vf-table
doesn't store this-adjustment offsets for overrides of functions from
virtual bases.  This means that the vf-table only has to grow in one
direction, so the address point can always be a constant offset (zero,
I think).

+//  3. Rather than using the most derived class as the type of the hidden 'this'
+//     method parameter, it tries to stick with the base type as long as
+//     possible. This eliminates the need of this adjusting thunks as long as
+//     there are no methods overriding multiple methods at the same time.

To be precise, the difference here is that Itanium adds a new v-table entry
whenever a function is overridden from a non-primary or virtual base,
but MSVC don't always.  This forces the caller to convert all the way to the
original 'this' value before making the call, which the callee generally then
has to undo; but yes, it does reduce the number of adjusting thunks
required, unless you have virtual inheritance, in which case the number
of adjusting thunks explodes.

+  void LayoutVFTable() {
+    // FIXME: unclear what to do with RTTI in MS ABI as emitting it anywhere
+    // breaks the vftable layout. Just skip RTTI for now, can't mangle anyway.
+

Can you explain how it breaks the vftable layout?  My understanding is that
disabling RTTI actually changes the ABI by causing the RTTI pointer to
be skipped in the layout, thus changing all the offsets; is it more than that?

+  // See if this class expands a vftable of one of its bases.
+  const CXXRecordDecl *NextBase = 0, *NextLastVBase = LastVBase;

Why isn't this always just the primary base?  In what situations do
virtual function entries ever get added to a vf-table from a non-primary
base?  Why do you even need to consider other bases?

You have a lot of very hard-to-follow path logic in this code; it's very
difficult to review this without understanding the invariants better.
It seems like MethodInfoMap is basically trying to track the current
class's notion of the final overriders of all the methods in its primary
vf-table?

+  VFTableBuilder(const CXXRecordDecl *MostDerivedClass, VFPtrInfo Which)

I'm not grokking VFPtrInfo.  What's necessary here beyond a BaseSubobject?
It can be the "most-derived" base subobject, i.e. the base subobject which is
not a primary base of anything else in this complete object.

Random notes that came up before I decided I needed more feedback:

+    CharUnits ThisOffset = Base.getBaseOffset();
+
+    for (int J = 0, F = Path.size(); J != F; ++J) {
+      const CXXBasePathElement &Element = Path[J];
+      const CXXRecordDecl *PrevRD = Element.Class,
+                          *Base = Element.Base->getType()->getAsCXXRecordDecl();

You're shadowing a function parameter here; it's very confusing.

+      if (!MethodInfoMap.count(OverriddenMD))
+        continue;
+
+      MethodInfo &OverriddenMethodInfo = MethodInfoMap[OverriddenMD];

Find an iterator and then dereference it, please.

+      const ASTRecordLayout &Layout = Context.getASTRecordLayout(PrevRD);
+
+      if (Element.Base->isVirtual()) {
+        if (Overrider.Method->getParent() == PrevRD) {
+          // This one's interesting. If the final overrider is in a vbase, it
+          // assumes the this pointer's position to be relative to the start of
+          // the vbase as if it was the most derived class, including the order
+          // of vbases.

This comment doesn't seem to correspond to the code.

John.