Page MenuHomePhabricator

[CodeGen] Store the return value of the target function call to the thunk's return value slot directly when the return type is an aggregate instead of doing so via a temporary
ClosedPublic

Authored by ahatanak on Jun 24 2020, 5:30 PM.

Details

Summary

This fixes PR45997 (https://bugs.llvm.org/show_bug.cgi?id=45997), which is caused by a bug that has existed since we started passing and returning C++ structs with ObjC strong pointer members (see https://reviews.llvm.org/D44908) or structs annotated with trivial_abi directly.

rdar://problem/63740936

Diff Detail

Event Timeline

ahatanak created this revision.Jun 24 2020, 5:30 PM

This seems fine. I do wonder if the "real" bug is that this ought to be handled properly in EmitReturnFromThunk, but regardless, the fix seems acceptable.

Is there a similar bug with trivial_abi? Should we have a test case for that?

ahatanak edited the summary of this revision. (Show Details)Jul 9 2020, 3:40 PM
ahatanak updated this revision to Diff 276858.Jul 9 2020, 3:44 PM

Add test case for trivial_abi.

This seems fine. I do wonder if the "real" bug is that this ought to be handled properly in EmitReturnFromThunk, but regardless, the fix seems acceptable.

Since EmitReturnFromThunk is used only by EmitCallAndReturnForThunk, I think it's better to just avoid the copy in the first place. Perhaps we should teach EmitReturnOfRValue to handle aggregates with non-trivial copy or move assignments correctly, but it seems that it won't have any effect once we commit the changes in this patch.

I agree that avoiding the copy is best. However, at the very least, if that function isn't going to handle the aggregate case correctly, it should assert that it isn't in it.

Otherwise this LGTM.

ahatanak updated this revision to Diff 277163.Jul 10 2020, 4:30 PM

Assert in EmitReturnFromThunk that the result type isn't an aggregate.

rjmccall accepted this revision.Jul 10 2020, 4:51 PM

Thanks! LGTM.

This revision is now accepted and ready to land.Jul 10 2020, 4:51 PM
This revision was automatically updated to reflect the committed changes.