[cfe-commits] r126865 - /cfe/trunk/lib/CodeGen/CGVTables.cpp

[Douglas Gregor]

On Mar 2, 2011, at 11:38 AM, Tilmann Scheller wrote:

> Author: tilmann
> Date: Wed Mar  2 13:38:28 2011
> New Revision: 126865
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=126865&view=rev
> Log:
> Add preliminary support for MSVC-style vtables.

This is the wrong way to introduce support for MSVC vtables. Having a large number of "if (Win64)" branches within the (currently Itanium) vtable builder makes it a mess to read. 

Instead, you should first introduce an abstraction layer, so that the Itanium vtable builder will be cleanly separated from  from MSVC vtable builder, just as we've done with the Objective-C runtimes and which Chip Davis already started work on. Then, you can fill in the MSVC-style vtable builder with all of the Win64-specific code.

Please revert this commit.

  - Doug

> Modified:
>    cfe/trunk/lib/CodeGen/CGVTables.cpp
> 
> Modified: cfe/trunk/lib/CodeGen/CGVTables.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGVTables.cpp?rev=126865&r1=126864&r2=126865&view=diff
> ==============================================================================
> --- cfe/trunk/lib/CodeGen/CGVTables.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CGVTables.cpp Wed Mar  2 13:38:28 2011
> @@ -2266,6 +2266,32 @@
> 
>   int64_t CurrentIndex = 0;
> 
> +  // The MSVC Win64 ABI uses a different ordering of the
> +  // virtual function pointers:
> +  //
> +  // The order of the virtual function pointers in a virtual table is
> +  // the reverse order of declaration of the corresponding member functions
> +  // in the class except for overloaded member functions. Overloaded member
> +  // functions are moved below the first declaration of the
> +  // respective overloaded member function. All declarations for one
> +  // overloaded member function are added in reverse order of declaration.
> +  //
> +  // E.g. for the following class:
> +  // class A {
> +  //   virtual b();
> +  //   virtual a(int a);
> +  //   virtual c();
> +  //   virtual a(float a);
> +  // };
> +  //
> +  // The vtable layout looks like this:
> +  // c()
> +  // a(float b)
> +  // a(int a)
> +  // b()
> +  //
> +  const bool IsWin64 = CGM.getContext().Target.isWin64();
> +
>   const ASTRecordLayout &Layout = CGM.getContext().getASTRecordLayout(RD);
>   const CXXRecordDecl *PrimaryBase = Layout.getPrimaryBase();
> 
> @@ -2285,6 +2311,9 @@
> 
>   const CXXDestructorDecl *ImplicitVirtualDtor = 0;
> 
> +  llvm::StringMap<llvm::SmallVector<const CXXMethodDecl *, 16> *> Methods;
> +  llvm::SmallVector<llvm::SmallVector<const CXXMethodDecl *, 16> *, 16> Order;
> +
>   for (CXXRecordDecl::method_iterator i = RD->method_begin(),
>        e = RD->method_end(); i != e; ++i) {
>     const CXXMethodDecl *MD = *i;
> @@ -2293,6 +2322,20 @@
>     if (!MD->isVirtual())
>       continue;
> 
> +    if (IsWin64) {
> +      if (Methods.count(MD->getNameAsString()) == 0) {
> +        // No entry yet.
> +        llvm::SmallVector<const CXXMethodDecl *, 16> *entry = new llvm::SmallVector<const CXXMethodDecl *, 16>();
> +        entry->push_back(MD);
> +        Methods[MD->getNameAsString()] = entry;
> +        Order.push_back(entry);
> +      } else {
> +        // This is an overloaded method, add to already existing entry.
> +        llvm::SmallVector<const CXXMethodDecl *, 16> *entry = Methods[MD->getNameAsString()];
> +        entry->push_back(MD);
> +      }
> +    }
> +
>     // Check if this method overrides a method in the primary base.
>     if (const CXXMethodDecl *OverriddenMD = 
>           FindNearestOverriddenMethod(MD, PrimaryBases)) {
> @@ -2327,7 +2370,7 @@
>         ImplicitVirtualDtor = DD;
>         continue;
>       } 
> -
> +      assert(!IsWin64 && "Need support for dtor!");
>       // Add the complete dtor.
>       MethodVTableIndices[GlobalDecl(DD, Dtor_Complete)] = CurrentIndex++;
> 
> @@ -2335,11 +2378,14 @@
>       MethodVTableIndices[GlobalDecl(DD, Dtor_Deleting)] = CurrentIndex++;
>     } else {
>       // Add the entry.
> -      MethodVTableIndices[MD] = CurrentIndex++;
> +      if (!IsWin64) {
> +        MethodVTableIndices[MD] = CurrentIndex++;
> +      }
>     }
>   }
> 
>   if (ImplicitVirtualDtor) {
> +    assert(!IsWin64 && "Need support for implicit virtual dtor!");
>     // Itanium C++ ABI 2.5.2:
>     //   If a class has an implicitly-defined virtual destructor, 
>     //   its entries come after the declared virtual function pointers.
> @@ -2353,6 +2399,19 @@
>       CurrentIndex++;
>   }
> 
> +  if (IsWin64) {
> +    for (llvm::SmallVector<llvm::SmallVector<const CXXMethodDecl *, 16> *, 16>::iterator i = Order.begin(),
> +         e = Order.end(); i != e; ++i) {
> +        llvm::SmallVector<const CXXMethodDecl *, 16> *elem = *i;
> +
> +        while (elem->size() > 0) {
> +            const CXXMethodDecl *method = elem->pop_back_val();
> +            // Add the entry.
> +            MethodVTableIndices[method] = CurrentIndex++;
> +        }
> +    }
> +  }
> +
>   NumVirtualFunctionPointers[RD] = CurrentIndex;
> }
> 
> 
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits