-
Notifications
You must be signed in to change notification settings - Fork 13k
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
asm/arm: allow r9 when usable, make diagnostics more specific #88879
asm/arm: allow r9 when usable, make diagnostics more specific #88879
Conversation
As described in rust-lang#85247, there are a number of cases where inline ARM assembly in Rust programs is prevented from using certain registers. Sometimes these registers are only reserved in certain situations; however, they are all unconditionally restricted by the compiler. Many of rustc's diagnostics describe the registers as being "used internally by LLVM", which obfuscates the meaning of the registers and is ultimately unhelpful to low-level programmers. At the moment, it does not seem possible to ensure that the frame pointer or base pointer will be elided, or to understand the conditions under which this may occur. Some of this is due to the addition of a codegen abstraction layer, where these behaviors may vary between backends and may not be configurable. As such, this information is not available to Rust's assembly parser. However, register r9 has a condition that is rather easily checked: the relocation model. r9 is used as the "static base" (SB) for "read-write position independence" (RWPI) as a base from which to address writable data, as defined in the Procedure Call Standard for the Arm Architecture[1]. The register is always available for use if RWPI is not used[2]. Rust allows RWPI mode to be selected on a per-target basis, as well as making it available as a codegen option in rustc. Thus, we can predicate usage of r9 on this setting with confidence. Allow r9 to be used in `asm!` blocks when the target does not use RWPI, and define what r6 and r9 are reserved for in their respective diagnostics. The frame pointer (r7/r11) and base pointer (r6) remain unconditionally restricted for now. [1]: https://developer.arm.com/documentation/ihi0042/j/ [2]: https://developer.arm.com/documentation/dui0056/d/using-the-procedure-call-standard/read-write-position-independence/register-usage-with-rwpi
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @wesleywiser (or someone else) soon. Please see the contribution instructions for more information. |
Technically we should be checking for the |
That is more precise, but that will still leave out other codegen backends, no? |
_has_feature: impl FnMut(&str) -> bool, | ||
target: &Target, | ||
) -> Result<(), &'static str> { | ||
if let RelocModel::Rwpi | RelocModel::RopiRwpi = target.relocation_model { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we only allow r9 when the relocation model was explicitly given to allow for more freedom in choosing which relocation model will be used by default without risking breaking this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That might be a good idea, though it would make the diagnostic less clear if the user hasn't specifically opted in to that mode. I wanted to add a "note" to it, but this is my first time contributing to rustc and I don't know how to do that, much less how to make it work in the context of def_regs!
.
How does one check if a target attribute is explicitly set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can check the value of sess.opts.cg.relocation_model
. By the way I think target.relocation_model
is only the default relocation model in all cases and not the one that will actually be used when compiling if -Crelocation-model
is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I didn't think of that since in my use case I'm using a custom target. I can fix that up. I'd still like to improve the diagnostics though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, Session
already has a relocation_model
getter that does the logic I was going to write anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, I can't actually do that because the asm parser isn't passed the Session
, only the Target
... Looks like I would need to change the definition of parse
in the entire rustc_target::asm
module at least, including arch-specific filter functions, then alter rustc_ast_lowering::LoweringContext
to pass Session
as well (possibly in place of Target
since it can be accessed through Session
). That feels like a fairly invasive change just to make a small thing more correct, but the overall effect should be negligible. If the maintainers are okay with it, I can go ahead and do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work since rustc_target
can't depend on rustc_session
since that would create a circular dependency.
r? @Amanieu |
Hi @Amanieu ; do you think you'll be able to review this PR, or delegate it to some other expert on ARM |
I still think this should be changed to look for the Finally, the documentation in the unstable book needs to be updated to reflect the new behavior of r9. |
☔ The latest upstream changes (presumably #91692) made this pull request unmergeable. Please resolve the merge conflicts. |
Closing in favor of #91643. |
asm: Allow using r9 (ARM) and x18 (AArch64) if they are not reserved by the current target This supersedes rust-lang#88879. cc `@Skirmisher` r? `@joshtriplett`
asm: Allow using r9 (ARM) and x18 (AArch64) if they are not reserved by the current target This supersedes rust-lang#88879. cc `@Skirmisher` r? `@joshtriplett`
As described in #85247, there are a number of cases where inline ARM assembly in Rust programs is prevented from using certain registers. Sometimes these registers are only reserved in certain situations; however, they are all unconditionally restricted by the compiler. Many of rustc's diagnostics describe the registers as being "used internally by LLVM", which obfuscates the meaning of the registers and is ultimately unhelpful to low-level programmers.
At the moment, it does not seem possible to ensure that the frame pointer or base pointer will be elided, or to understand the conditions under which this may occur. Some of this is due to the addition of a codegen abstraction layer, where these behaviors may vary between backends and may not be configurable. As such, this information is not available to Rust's assembly parser.
However, register r9 has a condition that is rather easily checked: the relocation model. r9 is used as the "static base" (SB) for "read-write position independence" (RWPI) as a base from which to address writable data, as defined in the Procedure Call Standard for the Arm Architecture. The register is always available for use if RWPI is not used.
Rust allows RWPI mode to be selected on a per-target basis, as well as making it available as a codegen option in rustc. Thus, we can predicate usage of r9 on this setting with confidence.
Allow r9 to be used in
asm!
blocks when the target does not use RWPI, and define what r6 and r9 are reserved for in their respective diagnostics. The frame pointer (r7/r11) and base pointer (r6) remain unconditionally restricted for now.