This is an archive of the discontinued LLVM Phabricator instance.

Use musttail for variadic method thunks when possible
ClosedPublic

Authored by rnk on Aug 30 2019, 5:00 PM.

Details

Summary

This avoids cloning variadic virtual methods when the target supports
musttail and the return type is not covariant. I think we never
implemented this previously because it doesn't handle the covariant
case. But, in the MS ABI, there are some cases where vtable thunks must
be emitted even when the variadic method defintion is not available, so
it looks like we need to implement this. Do it for both ABIs, since it's
a nice size improvement and simplification for Itanium.

Fixes PR43173.

Event Timeline

rnk created this revision.Aug 30 2019, 5:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 30 2019, 5:00 PM

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.

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.

clang/lib/CodeGen/CGVTables.cpp
214

This is fine, I guess, but how is it related?

rnk marked an inline comment as done.Sep 5 2019, 1:36 PM

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.

clang/lib/CodeGen/CGVTables.cpp
214

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.

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.

Yes. MSVC emits an error message "covariant returns with multiple or virtual inheritance not supported for varargs functions" in cases like the following. It looks like the equivalent isn't implemented in clang? (Is that the llvm_unreachable in this patch?)

Oops, meant to actually include the testcase in my last comment:

struct V1 { };
struct V2 : virtual V1 { };
struct A {

virtual V1 *f(int,...);

};
struct B : A {

virtual void b();
virtual V2 *f(int,...);

};
B b;

rnk added a comment.Sep 5 2019, 2:46 PM

I think what I said applies to your test case. Basically, in the Itanium C++ ABI, virtual method definitions provide all their thunks as weak_odr. We only emit thunks referenced by vtables as an optimization, and they are marked available_externally. In your test case, we hit the early return that I linked to, so we don't try to clone, and we don't need to emit an error.

In your test case, we hit the early return that I linked to, so we don't try to clone, and we don't need to emit an error.

Yes, that's what happens with the Itanium ABI; what happens with the MS ABI?

rnk updated this revision to Diff 218992.Sep 5 2019, 3:11 PM
  • merge into thunks.cpp
rnk added a comment.Sep 5 2019, 3:29 PM

In your test case, we hit the early return that I linked to, so we don't try to clone, and we don't need to emit an error.

Yes, that's what happens with the Itanium ABI; what happens with the MS ABI?

I see, it doesn't work. There's also the case of the MS ABI for ISAs that don't support musttail, although that's less interesting.

rnk updated this revision to Diff 218995.Sep 5 2019, 3:29 PM
  • emit an error if we try to clone without a definition
rnk marked an inline comment as done.Sep 6 2019, 3:37 PM
rnk added inline comments.
clang/test/CodeGenCXX/ms-thunks-variadic-return.cpp
9

I do love the optimism of the CGM.ErrorUnsupported diagnostic: "cannot compile this ${unimplementable_feature} yet".

I suppose it could be done if we standardized a new ABI for variadic virtual methods with pointer-like return values, so that they emit the main implementation under a new symbol name that takes a va_list and then the variadic one thunks to it. =P

efriedma accepted this revision.Sep 6 2019, 3:44 PM

LGTM

This revision is now accepted and ready to land.Sep 6 2019, 3:44 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 6 2019, 3:54 PM