<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 25, 2013, at 10:20 AM, Timur Iskhodzhanov <<a href="mailto:timurrrr@google.com">timurrrr@google.com</a>> wrote:<div><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">2013/7/25 John McCall <<a href="mailto:rjmccall@gmail.com">rjmccall@gmail.com</a>>:<br><blockquote type="cite">On Jul 22, 2013, at 6:11 AM, Timur Iskhodzhanov <<a href="mailto:timurrrr@google.com">timurrrr@google.com</a>> wrote:<br><br>While working on CodeGen, finally realized why we should only put the<br>overriders from the most derived class into MethodVFTableLocations<br><br><br>--- lib/AST/MicrosoftMangle.cpp<br>+++ lib/AST/MicrosoftMangle.cpp<br>@@ -89,6 +89,8 @@<br> void mangleNumber(const llvm::APSInt &Value);<br> void mangleType(QualType T, SourceRange Range,<br> QualifierMangleMode QMM = QMM_Mangle);<br>+ void mangleFunctionType(const FunctionType *T, const FunctionDecl *D,<br>+ bool IsStructor, bool IsInstMethod);<br><br>Is there a reason the mangling changes are part of this patch?<br></blockquote><br>Yes - the still-used Itanium VTableBuilder tries to emit thunks for<br>its vtables from CreateVTableInitialize/GetAddrOfThunk.<br></div></blockquote><div><br></div>Okay.</div><div><br><blockquote type="cite" dir="auto"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite">+// The main differences are:<br>+// 1. Separate vftable and vbtable.<br>+// 2. Each non-primary base class that has new virtual methods gets its<br>+// own vfptr and vftable, all the address points are zero.<br><br>This is not a difference.<br></blockquote><br>As far as I understand, in Itanium ABI each subobject that adds new<br>virtual methods to its bases gets a new *address point* in the shared<br>vtable, but not a new vtable (at least no new sections are generated),<br>which IS different from the Microsoft ABI.<br></div></blockquote><div><br></div>I’m not sure what it means for a subobject to "add virtual methods to its bases”.</div><div><br></div><div><blockquote type="cite" dir="auto"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite">+// 3. Rather than using the most derived class as the type of the hidden<br>'this'<br>+// method parameter, it tries to stick with the original 'this' type as<br>+// long as possible.<br>+// This yields the following consequences:<br>+// - Don't need to add all overriders to the vftables of their parents;<br>+// - Don't need adjusting thunks as long as there are no methods<br>overriding<br>+// multiple methods at the same time.<br><br>Again, this has things a little backwards. You can *never* change the<br>expected<br>address point of an existing entry in a v-table, or else you wouldn’t be<br>able<br>to use the entry correctly from a base subobject pointer.<br></blockquote><br>I don't see any conflict with my comment here.<br>Can you please suggest your variant of this clause?<br></div></blockquote><div><br></div><div>3. Virtual method definitions expect their ‘this’ parameter to point to the</div><div>first vfptr whose table provides a compatible overridden method. In many</div><div>case, this permits the original vf-table entry to directly call the method</div><div>instead of passing through a thunk.</div><div><br></div><div>A compatible overridden method is one which does not have a non-trivial</div><div>covariant-return adjustment.</div><div><br></div><div>The first vfptr is the one with the lowest offset in the complete-object layout</div><div>of the defining class, and the method definition will subtract that constant</div><div>offset from the parameter value to get the real ‘this’ value. Therefore, if the</div><div>offset isn’t really constant (i.e. if the first vfptr is contained in a virtual base</div><div>subobject), the vf-table may require a this-adjustment thunk.</div><div><br></div><div>4. vf-tables do not contain new entries for overrides that merely require</div><div>this-adjustment. Together with #3, this keeps vf-tables smaller and</div><div>eliminates the need for this-adjustment thunks in many cases, at the cost</div><div>of often requiring redundant work to adjust the "this" pointer.</div><div><br></div><blockquote type="cite" dir="auto"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite">+// 4. At entry, the 'this' parameter always points to the vfptr used<br>+// for the vftable lookup.<br><br>This is also not a difference.<br></blockquote><br>It is different in these ABIs.<br><br>Probably we're using different terms, so let's consider this example:<br>-----------<br> class A {<br> virtual void f() {}<br> };<br><br> class B : virtual A {<br> virtual void f();<br> };<br><br> void foo(void*);<br><br> void B::f() {<br> foo(this);<br> }<br>-----------<br>In the Itanium ABI, B::f expects the top of the stack to hold (B*)this at entry:<br>-----------<br> __ZN1B1fEv:<br> jmp __Z3fooPv # TAILCALL<br>-----------<br>thus needing a thunk in the vtable.<br><br>In Microsoft ABI, B::f expects ECX to hold (A*)this and adjusts in the preamble.<br>-----------<br> "?f@B@@EAEXXZ":<br> pushl %eax<br> addl $-4, %ecx # Adjust (A*) to (B*)<br> movl %ecx, (%esp)<br> calll "?foo@@YAXPAX@Z"<br> popl %eax<br> ret<br>-----------<br>thus a thunk is not needed.<br><br>I've rephrased it to "at virtual function entry" - does this make sense now?<br></div></blockquote><div><br></div><div>(This example is misleading because A is actually the primary base of B</div><div>under the Itanium ABI, but I think I see what you’re getting at.)</div><div><br></div><div>This is still not a difference. What you’re describing here is just the effect of</div><div>a previously-enumerated difference.</div><div><br></div><div>In both ABIs, the algorithm for performing a virtual function call is to adjust</div><div>the base pointer to a subobject which contains the method in its primary</div><div>v-table, load a function pointer from the v-table, and call it.</div><div><br></div><div>In your example, the difference is just that, under MSVC, B doesn’t have</div><div>an entry for f() in its vf-table. (In fact, it doesn’t have a vf-table.) So the</div><div>caller has to adjust to something that actually does have an entry for f(),</div><div>namely A.</div><div><br></div><blockquote type="cite" dir="auto"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">Phew, actually changing the FindNearestOverriddenMethod code turned<br>out not to be as trivial, at least I can't justify the change to<br>myself...<br><br>Please take a look at the code - I have several ideas how to change<br>the code there, would be glad if you can suggest which one looks<br>better to you.<br></div></blockquote><div><br></div>I’ll take a look.</div><div><br><blockquote type="cite" dir="auto"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite">+ // See if this class expands a vftable of the base we look at, which is<br>either<br>+ // the one defined by the vfptr base path or the primary base.<br><br>Still not sure why you’re not just starting from the right base and then<br>walking up the primary base chain.<br></blockquote><br>That's not enough even in the simple case of "B: A".<br><br>The vfptr is in the A layout, so the "right base" is A.<br>If we don't go to the most derived class (B) from "the right base"<br>(A), we forget to add the more derived class's own new methods (and<br>probably return-adjusting thunks).<br></div></blockquote><div><br></div><div>My point is that you should just start recursing from B instead of this weird</div><div>combination of walking the path and then falling back on climbing the</div><div>primary base chain.</div><div><br></div><div>Finding B (the most-derived subobject that A is in the primary-base chain of)</div><div>should be really easy — it’s just a depth-first search through the complete</div><div>object’s layout, stopping at the first thing with the same offset as A.</div><br><blockquote type="cite" dir="auto"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite">+ ThisAdjustment ThisAdjustmentOffset;<br>+ // Check if this virtual member function overrides<br>+ // a method in one of the visited bases.<br>+ if (const CXXMethodDecl *OverriddenMD =<br>+ FindNearestOverriddenMethod(MD, VisitedBases)) {<br><br>I don’t think you need to compute the nearest overridden method.<br>The way MSVC does this, you only get an entry if the method doesn’t<br>override a method with a trivial (or no) conversion on the return type.<br>So you can just walk the directly-overridden methods<br></blockquote><br>Good point, change the "iterate over ALL overridden methods" to<br>"iterate over directly overridden methods", though this has spawned<br>some more duplicate code. Is that OK now?<br></div></blockquote><div><br></div>I’ll look at the patch.</div><div><br></div><div>John.</div></body></html>