This is an archive of the discontinued LLVM Phabricator instance.

Fix mismatch in prologue and epilogue for funclets on AArch64 Windows
ClosedPublic

Authored by danielframpton on Mar 29 2020, 9:37 AM.

Details

Summary

The generated code for a funclet can have an add to sp in the epilogue for which there is no corresponding sub in the prologue.

This patch removes the early return from emitPrologue that was preventing the sub to sp, and instead conditionalizes the appropriate parts of the rest of the function.

Test case included.

Fixes: https://bugs.llvm.org/show_bug.cgi?id=45345

Diff Detail

Event Timeline

danielframpton created this revision.Mar 29 2020, 9:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 29 2020, 9:37 AM

If I'm understanding correctly, the space that wasn't getting allocated in the prologue is the space for arguments to calls inside the funclet?

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
1038

I think this COPY still needs to happen after the SEH_PrologEnd, or else we need to emit an SEH opcode for it. Otherwise, the SEH opcodes won't match the instructions. (The mismatch probably doesn't have many visible side-effects here, but still.)

If I'm understanding correctly, the space that wasn't getting allocated in the prologue is the space for arguments to calls inside the funclet?

Correct, although the relevant value (MaxCallFrameSize) is computed for the function, so the call does not actually need to be in the funclet.

Move the COPY to after the end of the prologue (SEH_PrologEnd) as suggested in the review.

danielframpton marked 2 inline comments as done.Mar 29 2020, 12:46 PM
danielframpton added inline comments.
llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
1038

Done

danielframpton marked an inline comment as done.

Updated to include context.

LGTM

I assume you don't have commit access, so I'll commit it for you.

efriedma accepted this revision.Mar 30 2020, 2:08 PM
This revision is now accepted and ready to land.Mar 30 2020, 2:08 PM
danielframpton edited the summary of this revision. (Show Details)Mar 30 2020, 5:18 PM

LGTM

I assume you don't have commit access, so I'll commit it for you.

That would be great. I am doing some final validation of the version you accepted (using it to self-host rustc). I will let you know that is complete.

LGTM

I assume you don't have commit access, so I'll commit it for you.

That would be great. I am doing some final validation of the version you accepted (using it to self-host rustc). I will let you know that is complete.

My additional validation showed no issues. Please let me know if there is anything you need me to do.

This revision was automatically updated to reflect the committed changes.

Both patches are merged now. Thanks for the patches!