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

[r4start]

On 11/02/2012 02:24, John McCall wrote:
> On Feb 6, 2012, at 3:04 AM, r4start wrote:
>> This patch provide support for vtordisp fields.
>> About this hidden fields you can read here http://msdn.microsoft.com/ru-ru/library/453x4xdd.aspx .
>> By default Microsoft compiler set /vd1 option and this behavior is default in my patch.
>> In future I can add /vd0 and /vd2 options to clang if somebody can tell me how can I do this.
> Looks like it should be a LangOption.  You should add a -cc1 option to
> control it;  if/when we add a VS-compatible driver, it would map /vd0 and /vd2
> down to that.
>
> I guess we'll also need to implement the pragma.
>
> The information about this thing is very incomplete;  it's not really clear
> to me exactly how it's used.  I guess we can worry about that later.
>
> +    /// VtorDispOffsets - Offsets for adjusting 'this' pointer
> +    /// in thunks (Microsoft-only).
> +    VtorDispOffsetsMapTy VtorDispOffsets;
> +
>
> The values in this map are always just sizeof(int) less than the
> corresponding VBaseOffset, right?
Yes, now I change VtorDispOffsetsMapTy to SmallVector<const 
CXXRecordDecl*, 4>.
>   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;
> +  /// r4start
> +  /// VtorDispOffsets - Offsets of hidden fields.
> +  ASTRecordLayout::VtorDispOffsetsMapTy VtorDispOffsets;
> +
>
> Please don't tag code with your name.
Removed.
> +  bool hasNewVirtualFunction(const CXXRecordDecl *RD,
> +                             bool OnlyMethods = false) const;
>
> This parameter name makes no sense.  All virtual functions are methods.
>
> +    if (method->isVirtual()&&  !method->size_overridden_methods()&&
> +        !(OnlyMethods&&  (method->getKind() == Decl::CXXConstructor ||
> +                          method->getKind() == Decl::CXXDestructor))) {
>
> I see.  Maybe you should call it something like IgnoreDestructor.
Yes,  this name more clear.
> Also, constructors can never be virtual.
>
> +  if (!RD->hasUserDeclaredConstructor()&&
> +      !RD->hasUserProvidedDefaultConstructor()&&
>
> I believe this is redundant.
Removed.
> +  // If class has primary base, then it can be primary base.
> +  if (Context.getASTRecordLayout(Base).getPrimaryBase()) return true;
>
> Please split the layout fixes unrelated to vtordisp as a separate commit.
> Additionally, the next line calls getASTRecordLayout again;  please just
> re-use the result.
Ok.
> +      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.
> +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.

  - Dmitry.