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

[Timur Iskhodzhanov]

================
Comment at: test/CodeGenCXX/microsoft-abi-thunks.cpp:44
@@ +43,3 @@
+ protected:
+  virtual void protected_f();
+  // MANGLING-DAG: @"\01?protected_f at C@@O3AEXXZ"
----------------
Reid Kleckner wrote:
> Why no thunks for the protected one?
O3/O7 **is** the thunk mangling.

I've added the non-thunk version mangling check just to avoid confusion.

================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:189
@@ +188,3 @@
+  void setThunkLinkage(llvm::Function *Thunk, bool AvailableExternally) {
+    assert(AvailableExternally && "All thunks should be available externally");
+    Thunk->setLinkage(llvm::GlobalValue::WeakAnyLinkage);
----------------
Reid Kleckner wrote:
> Why?  The MS C++ ABI has no key functions, so nothing is guaranteed to be available externally, basically ever.
Hm, it looks like I've just put a redundant assert at the wrong place so it stopped making any sense.
If AvailableExternally==false, EmitThunk() will assert anyways when called from EmitThunks() on out-of-line method definition - because EmitThunk() was already called by MaybeEmitThunkAvailableExternally.

I've just removed this assert.

================
Comment at: lib/AST/MicrosoftMangle.cpp:1420
@@ -1418,2 +1419,3 @@
     switch (MD->getAccess()) {
       default:
+        llvm_unreachable("Unsupported access specifier");
----------------
Reid Kleckner wrote:
> Make this "case AS_none", and rely on -Wcovered-switch to ensure that we don't miss any cases.
OK, done

================
Comment at: lib/AST/MicrosoftMangle.cpp:1881
@@ +1880,3 @@
+    switch (MD->getAccess()) {
+    default:
+      llvm_unreachable("Unsupported access specifier");
----------------
Reid Kleckner wrote:
> AS_none
Done

================
Comment at: lib/AST/MicrosoftMangle.cpp:1884
@@ +1883,3 @@
+    case AS_private:
+      Out << "G";
+      break;
----------------
Reid Kleckner wrote:
> Nitty micro-optimization: when you have a character, just stream the character.  It's hard to optimize the string stream back the character stream.
Good catch - done

================
Comment at: lib/AST/MicrosoftMangle.cpp:1897
@@ +1896,3 @@
+    switch (MD->getAccess()) {
+    default:
+      llvm_unreachable("Unsupported access specifier");
----------------
Reid Kleckner wrote:
> AS_none
Done

================
Comment at: lib/AST/MicrosoftMangle.cpp:1933
@@ +1932,3 @@
+    const ThisAdjustment &Adjustment, raw_ostream &Out) {
+  // FIXME: Actually, the dtor thunk should be emitted for vector deleting
+  // dtors rather than scalar deleting dtors. Just use the vector deleting dtor
----------------
Reid Kleckner wrote:
> Peter Collingbourne published a patch for these many months ago that I think works.  Feel free to ping it or take it over.  I'm OK with this FIXME for now though.
That patch had some problems that you've later resolved while working on regular dtors. It's very likely it still need some work, but probably not too much.
Let's leave the FIXME here for a while, I'll consider taking over Peter's patch later.

================
Comment at: lib/AST/MicrosoftMangle.cpp:1942
@@ -1902,1 +1941,3 @@
+  Mangler.mangleFunctionType(DD->getType()->castAs<FunctionProtoType>(),
+                             DD, true, true);
 }
----------------
Reid Kleckner wrote:
> "true, true" here is gross.  The other booleans without names are also gross.  We should make up and pass in a proper enum along the lines of:
> enum FunctionDeclType {
> FDT_FreeFunctoin
> FDT_InstanceMethod
> FDT_StructorMethod
> };
> 
> All structors are instance methods, so we don't need the 4th state.
> 
> Feel free to commit that cleanup change separately with post-commit review.
Come on, we can do better than that - see r191950 :)
Thanks for catching!

================
Comment at: lib/CodeGen/CGCXXABI.h:357
@@ -347,1 +356,3 @@
+                               bool AvailableExternally) = 0;
+
   virtual void EmitReturnFromThunk(CodeGenFunction &CGF,
----------------
Reid Kleckner wrote:
> These two together don't seem like the right interface.  I'll have more comments later.
After thinking for some time, I've changed the way CGVTables handles thunks and now the interface is simpler and (likely) more correct.
PTAL!


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