Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cranelift AArch64: Fix the get_return_address lowering #4851

Merged
merged 1 commit into from
Sep 7, 2022

Conversation

akirilov-arm
Copy link
Contributor

The previous implementation assumed that nothing had clobbered the LR register since the current function had started executing, so it would be incorrect for a non-leaf function, for example, that contains the get_return_address operation right after a call. The operation is valid only if the preserve_frame_pointers flag is enabled, which implies that the presence of a frame record on the stack is guaranteed.

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:aarch64 Issues related to AArch64 backend. labels Sep 2, 2022
Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! One request for an additional comment below.

At the top level, so I understand correctly: the verifier already ensures that this op can only be used when preserve_frame_pointers is true; so the additional conditions here are to document that / ensure it remains the case?

If so, as another extra assurance, it may be good to put a comment by the verifier check (linked above) indicating that backends may rely on this to be true and the check should not be loosened without auditing all backends.

Thanks!

cranelift/codegen/src/isa/aarch64/inst.isle Outdated Show resolved Hide resolved
@akirilov-arm
Copy link
Contributor Author

At the top level, so I understand correctly: the verifier already ensures that this op can only be used when preserve_frame_pointers is true; so the additional conditions here are to document that / ensure it remains the case?

Yes, I added the check as an additional safety net, and your suggestion to write a comment next to the verifier check makes perfect sense, thanks!

The previous implementation assumed that nothing had clobbered the
LR register since the current function had started executing, so
it would be incorrect for a non-leaf function, for example, that
contains the `get_return_address` operation right after a call.
The operation is valid only if the `preserve_frame_pointers` flag
is enabled, which implies that the presence of a frame record on
the stack is guaranteed.

Copyright (c) 2022, Arm Limited.
@github-actions github-actions bot added the cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. label Sep 7, 2022
@cfallin cfallin merged commit dd07e35 into bytecodealliance:main Sep 7, 2022
@akirilov-arm akirilov-arm deleted the get_return_address branch September 7, 2022 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants