[PATCH] Add MicrosoftVFTableContext to AST

[John McCall]

On Jul 22, 2013, at 6:11 AM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
>  While working on CodeGen, finally realized why we should only put the overriders from the most derived class into MethodVFTableLocations

--- lib/AST/MicrosoftMangle.cpp
+++ lib/AST/MicrosoftMangle.cpp
@@ -89,6 +89,8 @@
   void mangleNumber(const llvm::APSInt &Value);
   void mangleType(QualType T, SourceRange Range,
                   QualifierMangleMode QMM = QMM_Mangle);
+  void mangleFunctionType(const FunctionType *T, const FunctionDecl *D,
+                          bool IsStructor, bool IsInstMethod);
 
Is there a reason the mangling changes are part of this patch?

+// The main differences are:
+//  1. Separate vftable and vbtable.
+//  2. Each non-primary base class that has new virtual methods gets its
+//     own vfptr and vftable, all the address points are zero.

This is not a difference.  Other than the absence of virtual base subobjects,
the layout of a class is necessarily independent of whether it’s a subobject
or not — otherwise you wouldn’t be able to convert a derived pointer to a
base pointer and use the object.

+//  3. Rather than using the most derived class as the type of the hidden 'this'
+//     method parameter, it tries to stick with the original 'this' type as
+//     long as possible.
+//     This yields the following consequences:
+//     - Don't need to add all overriders to the vftables of their parents;
+//     - Don't need adjusting thunks as long as there are no methods overriding
+//       multiple methods at the same time.

Again, this has things a little backwards.  You can *never* change the expected
address point of an existing entry in a v-table, or else you wouldn’t be able
to use the entry correctly from a base subobject pointer.

The differences here are:

1. Virtual method definitions do not necessarily expect ‘this’ to be at the
address point of the defining class.  The address point may be of some
base sub-object that provides the same method.  (You should also
describe the algorithm for choosing this base sub-object.)

Crucially, this base sub-object can be in a virtual base, in which case the
definition hard-codes the appropriate offset from the implementing class.
Of course, if the implementing class isn’t the most-derived class, that might
not be the right offset for the real most-derived class, in which case the
vf-table entry has to be a thunk which adjusts to the arbitrary offset
before calling the definition.

2. Overrides do not introduce new vf-table entries just because their
‘this’ pointer is at a different offset.  This is what reduces the number of
thunks required (except in the virtual-inheritance case described above).

+//  4. At entry, the 'this' parameter always points to the vfptr used
+//     for the vftable lookup.

This is also not a difference.

+//  6. Instead of VTT and constructor vtables, vtordisps are emitted into the
+//     class layout if a class has
+//      a) any ctor/dtor

A user-defined ctor/dtor.

+  OverriddenMethodsSetTy OverriddenMethods;
+  ComputeAllOverriddenMethods(MD, OverriddenMethods);
+
+  // List all the bases that declare but not override this method.
+  BasesSetVectorTy OverridenBases;
+  for (OverriddenMethodsSetTy::const_iterator I = OverriddenMethods.begin(),
+       E = OverriddenMethods.end(); I != E; ++I) {
+    const CXXMethodDecl *MD = (*I);
+    if (MD->size_overridden_methods() == 0)
+      OverridenBases.insert(MD->getParent());
+  }

Please refactor ComputeAllOverriddenMethods to something like this:

  /// Visit all the methods overridden by the given method, recursively, in a depth-first pre-order.
  template <class VisitorTy>
  void visitAllOverriddenMethods(const CXXMethodDecl *method, VisitorTy &visitor) {
    for (const CXXMethodDecl *overridden : method->getOverrides()) { // pseudocode
      // Short-circuit if the visitor or recursive visitation
      visitor.visit(overridden);
      visitAllOverriddenMethods(overridden, visitor);
    }
  }

The changes to your use and the existing use should be obvious.

+  // This implies that an offset of a non-virtual base will dominate an offset
+  // of a virtual base.

This comment should say why this is important.  (You don’t rely on it in
the algorithm; it's just an ABI design advantage that potentially reduces
the number of this-adjustment thunks in derived classes that inherit this
method.)

+  // See if this class expands a vftable of the base we look at, which is either
+  // the one defined by the vfptr base path or the primary base.

Still not sure why you’re not just starting from the right base and then
walking up the primary base chain.

+    ThisAdjustment ThisAdjustmentOffset;
+    // Check if this virtual member function overrides
+    // a method in one of the visited bases.
+    if (const CXXMethodDecl *OverriddenMD =
+            FindNearestOverriddenMethod(MD, VisitedBases)) {

I don’t think you need to compute the nearest overridden method.
The way MSVC does this, you only get an entry if the method doesn’t
override a method with a trivial (or no) conversion on the return type.
So you can just walk the directly-overridden methods and do a pretty
cheap check on the return type, and if you don’t find a match, you
need an entry.

+        // In case we need a return adjustment, add a new slot for each
+        // return-adjusting thunk.

No.  The thunks go in the original slots; you’re not adding them here.

Whenever you add an entry for a method, you go find the final overrider,
and then you decide what needs to go in the slot in this concrete vf-table.

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130724/d20069c7/attachment.html>