This is an archive of the discontinued LLVM Phabricator instance.

Marked call instruction in thunk function with tail attribute when applicable
ClosedPublic

Authored by aaboud on Jul 23 2015, 2:15 PM.

Details

Summary

This patch fix PR24235.
When thunk function is being generated with a call to the original adjusted function, the thunk function will appear in the call stack of the debugger.
To solve this issue, the thunk function should try replace the call instruction with a jump instruction.

This patch mark the call function with tail attribute, when it is applicable, to allow the backend to replace the call with a jump.

Notice that marking the call with a tail might be safer, however, it is not good enough to remove the call in all cases, e.g. when the calling convention is not fastcc.

Diff Detail

Repository
rL LLVM

Event Timeline

aaboud updated this revision to Diff 30524.Jul 23 2015, 2:15 PM
aaboud retitled this revision from to Marked call instruction in thunk function with musttail attribute when applicable.
aaboud updated this object.
aaboud added reviewers: rnk, hans.
aaboud set the repository for this revision to rL LLVM.
aaboud added a subscriber: cfe-commits.
aaboud added a comment.Aug 2 2015, 2:01 AM

I did not get any comment on this patch yet.

ABataev added a subscriber: ABataev.Aug 3 2015, 6:34 AM
ABataev added inline comments.
lib/CodeGen/CGVTables.cpp
315–316 ↗(On Diff #30524)

I think it would better to improve code in lines 252-262 rather than setting musttail attribute manually.

rnk edited edge metadata.Aug 3 2015, 9:22 AM

Musttail is currently x86 only, unfortunately. I started making it work for AArch64, but it was pretty hard and I gave up to work on other things.

That aside, musttail is also somewhat optimization hostile. It interacts with the inliner in surprising ways.

All in all, I'd rather not do this.

Thanks for the comment.
So, do you suggest that we use "tail" instead of "musttail"?
Would it be fine to use "tail" here?

rnk added a comment.Aug 3 2015, 5:11 PM

Thanks for the comment.
So, do you suggest that we use "tail" instead of "musttail"?
Would it be fine to use "tail" here?

Yeah, adding tail is fine.

aaboud updated this revision to Diff 31341.Aug 5 2015, 3:33 AM
aaboud retitled this revision from Marked call instruction in thunk function with musttail attribute when applicable to Marked call instruction in thunk function with tail attribute when applicable.
aaboud updated this object.
aaboud edited edge metadata.
rnk accepted this revision.Aug 5 2015, 8:58 AM
rnk edited edge metadata.

The patch seems right, but I'm surprised there isn't more test fallout.

This revision is now accepted and ready to land.Aug 5 2015, 8:58 AM
This revision was automatically updated to reflect the committed changes.