Page MenuHomePhabricator

Explicitly report runtime stack realignment in StackMap section
ClosedPublic

Authored by reames on Jul 17 2014, 2:01 PM.

Details

Summary

This change adds code to explicitly mark a function which requires runtime stack realignment as not having a fixed frame size in the StackMap section. As it happens, this is not actually a functional change. The size that would be reported without the check is also "-1", but as far as I can tell, that's an accident. The code change makes this explicit.

If we decide not to merge the code change, adding the test case to ensure this behaviour is preserved is worthwhile.

Note: There's a separate bug in handling of stackmaps and patchpoints in functions which need dynamic frame realignment. The current code assumes that offsets can be calculated from RBP, but realigned frames must use RSP. (There's a variable gap between RBP and the spill slots.) This change set does not address that issue.

Diff Detail

Repository
rL LLVM

Event Timeline

reames updated this revision to Diff 11612.Jul 17 2014, 2:01 PM
reames retitled this revision from to Explicitly report runtime stack realignment in StackMap section.
reames updated this object.
reames edited the test plan for this revision. (Show Details)
reames added reviewers: atrick, nadav, ributzka.
reames added a subscriber: Unknown Object (MLST).
atrick accepted this revision.Jul 17 2014, 2:09 PM
atrick edited edge metadata.

It looks to me like your change enforces expected behavior. It's great to have a test case. Juergen, do you agree?

Our plan was to add a per function field to the stack map to communicate the dynamic alignment to the runtime. We just haven't done it yet.

Currently, we don't support dynamic alignment, hence no AVX.

This revision is now accepted and ready to land.Jul 17 2014, 2:09 PM
ributzka accepted this revision.Jul 17 2014, 2:12 PM
ributzka edited edge metadata.

Being able to communicate the fact that the stack has been dynamically aligned during runtime is one of the things we want to add in the future to enable AVX.

For now we only have an assert to verify that we use the frame register, but having this test case and an explicit check is even better.

Thanks for adding this.

LGTM

reames closed this revision.Aug 1 2014, 11:35 AM
reames updated this revision to Diff 12114.

Closed by commit rL214534 (authored by @reames).