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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
