[PATCH] Add MicrosoftVFTableContext to AST

Timur Iskhodzhanov timurrrr at google.com
Fri Jul 19 08:33:40 PDT 2013


Please have another look

2013/7/19 John McCall <rjmccall at apple.com>:
> +  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.

Agree, fixed.

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

OK, I've updated the comment to reflect your suggestions.

> +  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?

Please note this comment is copied from
VTableBuilder::LayoutPrimaryAndSecondaryVTables.

As far as I understand, if we emit RTTI, it should go to vftable[-1].
Otherwise there's even no slot allocated for it.
That means the vftable symbol should point in the middle of a section
(if RTTI is emitted), which is not yet supported by LLVM IIRC (see the
"Symbol in the middle of a global" thread).
I think we should also use weak/strong-odr linkage for no-RTTI/RTTI
cases so the more complete vftables are chosen at link-time.

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

consider
  struct A;
  struct B;
  struct C: B;
  struct MDC: A, C;
if C adds new methods to B, they are put into the B's vftable.
Basically, each class in the hierarchy can add methods to its own
primary base vftable.

> Why do you even need to consider other bases?

VFTableBuilder in general and AddMethods in particular are used to
build any of the vftables, not only the primary one - in contrast with
the Itanium builder which has different codepaths for
primary/secondary tables.
That means we have to consider each base subobject on the vftable path
recursively, e.g. building its vftable almost as if it was the most
derived class (e.g. right bases's vftable). This includes adding "new"
methods to the vftable being built for the subobject.

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

I had the same complaint while reading VTableBuilder/VTableContext :)
That said, I'll be happy to put explaining comments into the code
where necessary, but it's already hard for me to decide what needs
more comments...

> 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?

Not in the primary vftable but rather in the vftable it's building
(e.g. first vbase's vftable).

Other than that - yes.
I think my MethodInfoMap is similar to MethodInfoMap from
VTableBuilder::AddMethods.
Please note that using FinalOverriders alone would not suffice due to
a different "this parameter" logic in this ABI.

> +  VFTableBuilder(const CXXRecordDecl *MostDerivedClass, VFPtrInfo Which)
>
> I'm not grokking VFPtrInfo.  What's necessary here beyond a BaseSubobject?

I don't think there's anything *necessary* but having the BasePath
calculated once and the last vbase stored too IMO simplifies the
remaining code (e.g. AddMethods traversal or mangling, which is not
included in this patch).
Otherwise, I'll have to re-calculate the all paths and pick one each
time I want to traverse the hierarchy to this base subobject, right?

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

Good catch - fixed!

> +      if (!MethodInfoMap.count(OverriddenMD))
> +        continue;
> +
> +      MethodInfo &OverriddenMethodInfo = MethodInfoMap[OverriddenMD];
>
> Find an iterator and then dereference it, please.

Done.
I agree this could be better performance-wise, but not sure if it's as
readable now.

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

Maybe the comment wasn't clear enough, I've rephrased it.

> John.



More information about the cfe-commits mailing list