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

John McCall rjmccall at apple.com
Fri Feb 10 14:24:20 PST 2012


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

+  /// r4start
+  /// VtorDispOffsets - Offsets of hidden fields.
+  ASTRecordLayout::VtorDispOffsetsMapTy VtorDispOffsets;
+

Please don't tag code with your name.

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

Also, constructors can never be virtual.

+  if (!RD->hasUserDeclaredConstructor() && 
+      !RD->hasUserProvidedDefaultConstructor() &&

I believe this is redundant.

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

+      CharUnits IntSize = 
+        CharUnits::fromQuantity(Context.getTargetInfo().getIntWidth() / 8);

Is this really 'int'?  Like, on Windows 64, is this still only 32-bits?

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

John.



More information about the cfe-dev mailing list