<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">On Jul 22, 2013, at 6:11 AM, Timur Iskhodzhanov <<a href="mailto:timurrrr@google.com">timurrrr@google.com</a>> wrote:<br><div><blockquote type="cite"> While working on CodeGen, finally realized why we should only put the overriders from the most derived class into MethodVFTableLocations<br></blockquote><div><br></div></div><div>--- lib/AST/MicrosoftMangle.cpp</div><div>+++ lib/AST/MicrosoftMangle.cpp</div><div>@@ -89,6 +89,8 @@</div><div> void mangleNumber(const llvm::APSInt &Value);</div><div> void mangleType(QualType T, SourceRange Range,</div><div> QualifierMangleMode QMM = QMM_Mangle);</div><div>+ void mangleFunctionType(const FunctionType *T, const FunctionDecl *D,</div><div>+ bool IsStructor, bool IsInstMethod);</div><div> </div><div>Is there a reason the mangling changes are part of this patch?</div><div><br></div><div><div>+// The main differences are:</div><div>+// 1. Separate vftable and vbtable.</div><div>+// 2. Each non-primary base class that has new virtual methods gets its</div><div>+// own vfptr and vftable, all the address points are zero.</div></div><div><br></div><div>This is not a difference. Other than the absence of virtual base subobjects,</div><div>the layout of a class is necessarily independent of whether it’s a subobject</div><div>or not — otherwise you wouldn’t be able to convert a derived pointer to a</div><div>base pointer and use the object.</div><div><br></div><div><div>+// 3. Rather than using the most derived class as the type of the hidden 'this'</div><div>+// method parameter, it tries to stick with the original 'this' type as</div><div>+// long as possible.</div><div>+// This yields the following consequences:</div><div>+// - Don't need to add all overriders to the vftables of their parents;</div><div>+// - Don't need adjusting thunks as long as there are no methods overriding</div><div>+// multiple methods at the same time.</div></div><div><br></div><div>Again, this has things a little backwards. You can *never* change the expected</div><div>address point of an existing entry in a v-table, or else you wouldn’t be able</div><div>to use the entry correctly from a base subobject pointer.</div><div><br></div><div>The differences here are:</div><div><br></div><div>1. Virtual method definitions do not necessarily expect ‘this’ to be at the</div><div>address point of the defining class. The address point may be of some</div><div>base sub-object that provides the same method. (You should also</div><div>describe the algorithm for choosing this base sub-object.)</div><div><br></div><div>Crucially, this base sub-object can be in a virtual base, in which case the</div><div>definition hard-codes the appropriate offset from the implementing class.</div><div>Of course, if the implementing class isn’t the most-derived class, that might</div><div>not be the right offset for the real most-derived class, in which case the</div><div>vf-table entry has to be a thunk which adjusts to the arbitrary offset</div><div>before calling the definition.</div><div><br></div><div>2. Overrides do not introduce new vf-table entries just because their</div><div>‘this’ pointer is at a different offset. This is what reduces the number of</div><div>thunks required (except in the virtual-inheritance case described above).</div><div><br></div><div><div>+// 4. At entry, the 'this' parameter always points to the vfptr used</div><div>+// for the vftable lookup.</div></div><div><br></div><div>This is also not a difference.</div><div><br></div><div><div>+// 6. Instead of VTT and constructor vtables, vtordisps are emitted into the</div><div>+// class layout if a class has</div><div>+// a) any ctor/dtor</div><div><br></div></div><div>A user-defined ctor/dtor.</div><div><br></div><div><div>+ OverriddenMethodsSetTy OverriddenMethods;</div><div>+ ComputeAllOverriddenMethods(MD, OverriddenMethods);</div><div>+</div><div>+ // List all the bases that declare but not override this method.</div><div>+ BasesSetVectorTy OverridenBases;</div><div>+ for (OverriddenMethodsSetTy::const_iterator I = OverriddenMethods.begin(),</div><div>+ E = OverriddenMethods.end(); I != E; ++I) {</div><div>+ const CXXMethodDecl *MD = (*I);</div><div>+ if (MD->size_overridden_methods() == 0)</div><div>+ OverridenBases.insert(MD->getParent());</div><div>+ }</div></div><div><br></div><div>Please refactor ComputeAllOverriddenMethods to something like this:</div><div><br></div><div> /// Visit all the methods overridden by the given method, recursively, in a depth-first pre-order.</div><div> template <class VisitorTy></div><div> void visitAllOverriddenMethods(const CXXMethodDecl *method, VisitorTy &visitor) {</div><div> for (const CXXMethodDecl *overridden : method->getOverrides()) { // pseudocode</div><div> // Short-circuit if the visitor or recursive visitation</div><div> visitor.visit(overridden);</div><div> visitAllOverriddenMethods(overridden, visitor);</div><div> }</div><div> }</div><div><br></div><div>The changes to your use and the existing use should be obvious.</div><div><br></div><div><div>+ // This implies that an offset of a non-virtual base will dominate an offset</div><div>+ // of a virtual base.</div></div><div><br></div><div>This comment should say why this is important. (You don’t rely on it in</div><div>the algorithm; it's just an ABI design advantage that potentially reduces</div><div>the number of this-adjustment thunks in derived classes that inherit this</div><div>method.)</div><div><br></div><div><div>+ // See if this class expands a vftable of the base we look at, which is either</div><div>+ // the one defined by the vfptr base path or the primary base.</div><div><br></div><div>Still not sure why you’re not just starting from the right base and then</div><div>walking up the primary base chain.</div><div><br></div><div><div>+ ThisAdjustment ThisAdjustmentOffset;</div><div>+ // Check if this virtual member function overrides</div><div>+ // a method in one of the visited bases.</div><div>+ if (const CXXMethodDecl *OverriddenMD =</div><div>+ FindNearestOverriddenMethod(MD, VisitedBases)) {</div><div><br></div></div><div>I don’t think you need to compute the nearest overridden method.</div><div>The way MSVC does this, you only get an entry if the method doesn’t</div><div>override a method with a trivial (or no) conversion on the return type.</div><div>So you can just walk the directly-overridden methods and do a pretty</div><div>cheap check on the return type, and if you don’t find a match, you</div><div>need an entry.</div><div><br></div><div><div>+ // In case we need a return adjustment, add a new slot for each</div><div>+ // return-adjusting thunk.</div><div><br></div><div>No. The thunks go in the original slots; you’re not adding them here.</div></div></div><div><br></div><div>Whenever you add an entry for a method, you go find the final overrider,</div><div>and then you decide what needs to go in the slot in this concrete vf-table.</div><div><br></div><div>John.</div></body></html>