[PATCH] D67028: Use musttail for variadic method thunks when possible

[Reid Kleckner via Phabricator via cfe-commits]

rnk marked an inline comment as done.
rnk added a comment.

In D67028#1653439 <https://reviews.llvm.org/D67028#1653439>, @efriedma wrote:

> Do we have test coverage for a variadic, covariant thunk for a function without a definition?  I don't think there's any way for us to actually emit that, but we should make sure the error message is right.


That appears to have been PR18098, so we have a test for it in thunks.cpp. The way the Itanium C++ ABI works, thunks are always emitted as `weak_odr` in TUs that provide the implementation, so emitting them with the vtable is just an optimization. Basically, clang never *has* to emit a thunk when the definition isn't present, it can always choose to simply declare it, and that's what we do here: https://github.com/llvm/llvm-project/blob/7c8952197b86790b31731d34d559281840916e1f/clang/lib/CodeGen/CGVTables.cpp#L558

In the MS ABI, deriving a new class may require the creation of new thunks for methods that were not overridden, so we can't use the same trick.

I think my two new tests are redundant with tests in thunks.cpp, so perhaps I should just add some new RUN lines there.

> I'm a little concerned that using musttail thunks with the Itanium ABI will flush out bugs that have been hiding because we have less test coverage on Windows. But it's probably the right thing to do.

True, it's a risk. One other thing worth mentioning is that the IR cloning doesn't actually work in the presence of labels-as-values, so we are improving conformance in that extra, extra corner case.



================
Comment at: clang/lib/CodeGen/CGVTables.cpp:206
+  AdjustedThisPtr = Builder.CreateBitCast(AdjustedThisPtr,
+                                          ThisStore->getOperand(0)->getType());
   ThisStore->setOperand(0, AdjustedThisPtr);
----------------
efriedma wrote:
> This is fine, I guess, but how is it related?
The MS ABI implementation of `performThisAdjustment` returns a pointer that needs to be cast. The covariant test exercises this codepath in the MS ABI, and we didn't do that before. I guess the Itanium one does the cast internally, but the MS ABI impl has this comment:
  // Don't need to bitcast back, the call CodeGen will handle this.
  return V;

I probably removed the cast when trying to clean up our generated IR.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67028/new/

https://reviews.llvm.org/D67028