[PATCH] Abstract out parts of thunk emission code, add support for simple thunks for -cxx-abi microsoft

[Timur Iskhodzhanov]

  Please take another look


================
Comment at: lib/CodeGen/ItaniumCXXABI.cpp:176
@@ -175,1 +175,3 @@
 
+  bool shouldTryToEmitThunksForExternalVirtualFunctions() { return false; }
+
----------------
Reid Kleckner wrote:
> Is this dead code?
Oops, fixed!

================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:186
@@ -177,1 +185,3 @@
 
+  bool shouldTryToEmitThunksForExternalVirtualFunctions() { return true; }
+
----------------
Reid Kleckner wrote:
> Dead, I believe
Done

================
Comment at: lib/CodeGen/CGVTables.cpp:499
@@ -497,5 +498,3 @@
   if (VFTContext.isValid()) {
-    // FIXME: This is a temporary solution to force generation of vftables in
-    // Microsoft ABI. Remove when we thread VFTableContext through CodeGen.
-    VFTContext->getVFPtrOffsets(MD->getParent());
-    return;
+    // Don't need to generate thunks for complete destructor in this ABI.
+    if (isa<CXXDestructorDecl>(MD) && GD.getDtorType() == Dtor_Complete)
----------------
Reid Kleckner wrote:
> Can you push this logic into VFTContext->getThunkInfo(MD) and have it return null?  We check for that just below.
I wasn't entirely sure I can change the VTableContextBase interface to use GDs, but after some meditation I think I got it right. Added a comment at the code I stared at during meditation :)

================
Comment at: lib/CodeGen/CGVTables.cpp:475
@@ -477,1 +474,3 @@
+      !CGM.getCodeGenOpts().OptimizationLevel) {
+    // If the ABI has key functions, we only want to do this when building with optimizations.
     return;
----------------
Reid Kleckner wrote:
> Can you elaborate on this?  Basically, the thunk should already be available externally if we have key functions, so emitting it to the current TU is just an optimization.
Done

================
Comment at: lib/CodeGen/ItaniumCXXABI.cpp:179
@@ +178,3 @@
+  void setThunkLinkage(llvm::Function *Thunk, bool ForVTable) {
+    if (ForVTable)
+      Thunk->setLinkage(llvm::GlobalValue::AvailableExternallyLinkage);
----------------
Reid Kleckner wrote:
> Can you elaborate on why it's available externally if this is "for the vtable"?
Done

================
Comment at: include/clang/Basic/ABI.h:107
@@ -107,1 +106,3 @@
+  /// \brief Optional ABI-specific data.
+  const void *OpaqueData;
 
----------------
Reid Kleckner wrote:
> You can avoid the void* by forward declaring CXXMethodDecl.  However, it feels weird to have a soft dependency from Basic to AST.
> 
> What are the alternatives to having this here?  It doesn't feel right to keep these out of the comparison operators, even though they are only used to sort the dump.
> You can avoid the void* by forward declaring CXXMethodDecl.

On the second thought - yes. I don't remember the reason I didn't want to do it in the first place...

> However, it feels weird to have a soft dependency from Basic to AST.

ABI.h is not used anywhere in Basic - maybe we can just move it to AST then?

> It doesn't feel right to keep these out of the comparison operators

After looking at this carefully, it **is not right** to keep these out of the comparison.
I've added a test that failed previously and fixed it.


http://llvm-reviews.chandlerc.com/D1787