Page MenuHomePhabricator

[X86] Fix tailcall return address clobber bug
ClosedPublic

Authored by margnus1 on Jun 14 2016, 7:30 AM.

Details

Summary

This bug (PR28124) was introduced by r237977, which refactored the tail call
sequence to be generated in two passes instead of one.

Unfortunately, the stack adjustment produced by the first pass was not
recognized by X86FrameLowering::mergeSPUpdates() in all cases, causing
code such as the following, which clobbers the return address, to be
generated:

popl    %edi
popl    %edi
pushl   %eax
jmp     tailcallee              # TAILCALL

By moving the stack adjustment by X86MachineFunctionInfo::getTCReturnAddrDelta() from X86FrameLowering::emitEpilogue() to X86ExpandPseudo::ExpandMI(), the bug is fixed.

Diff Detail

Repository
rL LLVM

Event Timeline

margnus1 updated this revision to Diff 60682.Jun 14 2016, 7:30 AM
margnus1 retitled this revision from to [X86] Fix tailcall return address clobber bug.
margnus1 updated this object.
margnus1 added a reviewer: qcolombet.
margnus1 added a subscriber: llvm-commits.

Hi! I wrote this patch under the assumption that you intended X86FrameLowering::mergeSPUpdates() to work this way in your refactoring. Feel free to say so if this isn't the best way of fixing the problem.

qcolombet requested changes to this revision.Jun 27 2016, 9:54 AM
qcolombet edited edge metadata.

Hi Magnus,

The refactoring of findDeadCallerSavedReg looks good, though at this point don't know if it is necessary.
The merge SP update part seems like an optimization to me, and it feels wrong to have to resort on an optimization to generate correct code.

How was this working previously?
Or put different why is the merge SP update necessary to fix the issue?

Cheers,
-Quentin

lib/Target/X86/X86FrameLowering.cpp
200 ↗(On Diff #60682)

remove else {
Per llvm coding style.

200 ↗(On Diff #60682)

No need for brackets for one line block, per llvm coding style.

432 ↗(On Diff #60682)

Add comments on what you are doing here.
For instance, something along the line of: this is fine to drop pop instructions when their results in not used by the return instruction, i.e., the values are not live out.

Explain why we need to make a special case for RIP. That is not clear, at least, to me. I would have expected the proper implicit uses to be set for that register.

test/CodeGen/X86/hipe-cc.ll
76 ↗(On Diff #60682)
  • Add a comment on what is this function testing.
  • Use "opt -instnamer" on this IR to get rid of the [0-9]+ variables.
test/CodeGen/X86/hipe-cc64.ll
86 ↗(On Diff #60682)

Ditto

This revision now requires changes to proceed.Jun 27 2016, 9:54 AM
margnus1 updated this revision to Diff 62093.Jun 28 2016, 7:56 AM
margnus1 updated this object.
margnus1 edited edge metadata.

Hi Quentin!

This worked before by emitting both the adjustment by X86MachineFunctionInfo::getTCReturnAddrDelta() along with the stack adjustment in the tail call instruction in a single call to emitSPUpdate().

While writing this patch, I had initially assumed that it was not possible to do the stack adjustment in a single call to emitSPUpdate() without undoing the refactoring. I have now challenged that assumption and found it to be false, and since I agree that fixing the problem in mergeSPUpdates() is a poor solution, I have changed my patch so that the entire stack adjustment is performed in X86ExpandPseudo::ExpandMI().

qcolombet added inline comments.Jul 7 2016, 3:53 PM
lib/Target/X86/X86ExpandPseudo.cpp
257 ↗(On Diff #62093)

I’d add X86FI as a member so that we do not have to pass it around.

lib/Target/X86/X86FrameLowering.cpp
1628 ↗(On Diff #62093)

add a helper function isTailCallOpcode or something.

margnus1 updated this revision to Diff 63225.Jul 8 2016, 8:00 AM
margnus1 edited edge metadata.

Updated as per your instructions.

qcolombet accepted this revision.Jul 8 2016, 10:43 AM
qcolombet edited edge metadata.

Thanks Magnus.

LGTM.

This revision is now accepted and ready to land.Jul 8 2016, 10:43 AM

Thanks for the review. I would appriciate help to commit this as I lack commit access.

Hi Magnus,

Sure!

Could you rebase the patch?

It does not apply cleaning for me.

Thanks,
-Quentin

margnus1 updated this revision to Diff 63388.Jul 9 2016, 4:50 AM
margnus1 edited edge metadata.

Of course. I should have done so earlier, I apologize.

This revision was automatically updated to reflect the committed changes.