[cfe-dev] Microsoft vtordisp field support for clang.

[John McCall]

On Feb 13, 2012, at 4:29 AM, r4start wrote:
> On 11/02/2012 02:24, John McCall wrote:
>> 
>> I guess we still need to record whether
>> a base actually requires this at all.  I'd be more comfortable with storing this
>> as additional value information in VBaseOffsets;  that will avoid bloating
>> the RecordLayout for the common case of a class with no virtual bases.
> Are you suggest something like this?
> struct VBaseInfo {
>  CharUnits Offset;
>  bool IsVtorDispPresent;
> };
> llvm::DenseMap<const CXXRecordDecl *, VBaseInfo> VBaseOffsets;

Yep, that's what I had in mind.

>> +      CharUnits IntSize =
>> +        CharUnits::fromQuantity(Context.getTargetInfo().getIntWidth() / 8);
>> 
>> Is this really 'int'?  Like, on Windows 64, is this still only 32-bits?
> Yes, this field on Windows x64 is still int.

Okay, that looks good, then.

>> +bool RecordLayoutBuilder::needsVtorDisp(const CXXRecordDecl *RD,
>> +                                        const CXXRecordDecl *Base) const {
>> 
>> This entire algorithm is grossly inefficient.  I believe that all you want is:
>>   1) Create a set of classes, and fill it with the all virtual bases.
>>   2) For each virtual method in the entire class hierarchy, remove all the
>>     classes declaring a method overridden by that method.
>>   3) A vtordisp field is required for every virtual base class which is no
>>     longer in the set.
>> In addition, you can avoid the full hierarchy walk by immediately
>> removing virtual bases which are known by a direct base class to
>> require a vtordisp field.
>> 
>> This algorithm has the side-effect of working when a function overrides
>> two different virtual functions from virtual bases at once.
> Please tell me if I am not right.
> You suggest remove this function with function like this "void RecordLayoutBuilder::FillVtorDispBases(const CXXRecordDecl *RD)".
> 
> This function in pseudo code:
> 
> Vbases = getAllVBasesForRD; // First step in your algorithm.
> VBasesVtorDisp;
> 
> // Then goes step 2, but if we want traverse all virtual methods we must walk throw inheritance path.
> foreach (VBase in Vbases) {
>  CXXBasePath &Path = getPathFromRDToVBase;
>  bool needsVtorDisp = false;
> 
>  foreach(Class in Path) {
>    // Here we will have another 'for'  from Class->method_begin() to Class->method_end(),
>    // and check that Class overrides virtual method from VBase.
>    if ((Class.hasCtorDeclared || Class.hasDtorDeclared) && Class.overridesVirtualFunctionOfBase) {
>      needsVtorDisp = true;
>      break;
>    }
>  }
>  if (needsVtorDisp)
>    VBasesVtorDisp.push_back(VBase);
> }
> If I am right then I am not sure that this algorithm is better.

I don't think this needs to be path-dependent, or at least not path-dependent in any way beyond what's encapsulated in the usual override rules.  Either the most-derived class directly overrides a method from a virtual base class, or one of its base classes does.  That is, I think the algorithm just needs to be something like this:

  var basesThatNeedVtorDisps : set<class>
  for each (base in mostDerivedClass.directBases)
    basesThatNeedVtorDisps.addAll(base.basesThatNeedVtorDisps)
  for each (method in mostDerivedClass.virtualMethods)
    for each (overriddenMethod in method.overriddenMethods)
      if (mostDerivedClass.virtualBases.contains(overriddenMethod.declaringClass))
        basesThatNeedVtorDisps.add(overriddenMethod.declaringClass);

The only tricky thing is that you might be overriding a method from a non-virtual base of a virtual base;  I don't even know what that's supposed to do for these vtordisp fields, though.  Could you check?  The test case would be something like this:

  class A { virtual void foo(); }
  class B : public A { };
  class C : public virtual B { C(); virtual void foo(); };

John.