Add -Zfixed-x18 by Darksonn · Pull Request #124655 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
Add -Zfixed-x18 #124655
Conversation23 Commits6 Checks6 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Signed-off-by: Alice Ryhl aliceryhl@google.com
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
56 tasks
r? @RalfJung
It seems like the right person to review this? The changes look reasonable to me.
Sorry, I can't do reviews outside the interpreter.
r? compiler
@apiraino is an MCP needed?
Posting here the answer from compiler team (see Zulip): it's to review this without MCP since it's an unstable flag
This comment has been minimized.
| // -Zfixed-x18 |
|---|
| if sess.opts.unstable_opts.fixed_x18 { |
| if sess.target.arch != "aarch64" { |
| sess.dcx().emit_fatal(FixedX18InvalidArch { arch: &sess.target.arch }); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used emit_fatal here because with emit_err, the error is printed twice.
This comment has been minimized.
This comment has been minimized.
@oli-obk sure, I can review when the MCP completes if you'd like (I think @estebank already approved)
We probably don't need all the blessed expectations compared to e.g.
//@ error-pattern: the -Zfixed-x18 flag is not supported
//@ dont-check-compiler-stderr
but it's fine.
bsmith mentioned
cc-rswould need to be updated to forward this option
Does this need to be done soon after merging this PR, or when the full design is finalized for stabilization?
On zulip, you, ralf and bjorn were still discussing details, but it's not clear to me whether these mention possible future work, or an approach you three would rather take in this PR? If it's the former, this looks good enough to land unstably to me, and we can iterate from there if need be.
cc @NobodyXu on the cc-rs question
On zulip, you, ralf and bjorn were still discussing details, but it's not clear to me whether these mention possible future work, or an approach you three would rather take in this PR?
I would rather land this PR now as unstable and change it later.
That seems good to me as well if RfL is fine with the possible churn, as I'd expect you to be the only consumers for this flag for a while.
Thanks!
@bors r=lqd,estebank
📌 Commit 4aafecb has been approved by lqd,estebank
It is now in the queue for this repository.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
That seems good to me as well if RfL is fine with the possible churn, as I'd expect you to be the only consumers for this flag for a while.
It should be fine, and this way we can start getting some usage/testing out of it, including in the potential/upcoming Rust for Linux CI job here (where churn should not matter, in the sense that we can change the job at the same time in the same PR to adapt). If we really expect a lot of churn, we could consider avoiding to put it in upstream Linux just yet, but I think it would still be fine.
Thanks Rémy!
bors added a commit to rust-lang-ci/rust that referenced this pull request
…iaskrgr
Rollup of 7 pull requests
Successful merges:
- rust-lang#124655 (Add
-Zfixed-x18) - rust-lang#125693 (Format all source files in
tests/coverage/) - rust-lang#125700 (coverage: Avoid overflow when the MC/DC condition limit is exceeded)
- rust-lang#125705 (Reintroduce name resolution check for trying to access locals from an inline const)
- rust-lang#125708 (tier 3 target policy: clarify the point about producing assembly)
- rust-lang#125715 (remove unneeded extern crate in rmake test)
- rust-lang#125719 (Extract coverage-specific code out of
compiletest::runtest)
r? @ghost
@rustbot modify labels: rollup
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
labels
95 tasks
cc @NobodyXu on the cc-rs question
On zulip, you, ralf and bjorn were still discussing details, but it's not clear to me whether these mention possible future work, or an approach you three would rather take in this PR?
I would rather land this PR now as unstable and change it later.
Do you mean:
That way, cc-rs can translate -Ctarget-feature=-x18-available into -ffixed-x18 as well, instead of emitting -ffixed-x18 when you do nothing.
it sounds alright to me
@NobodyXu What landed yesterday is a -Zfixed-x18 flag that needs to be converted to -ffixed-x18. Potentially we change the flag to an x18-available feature in the future. Does cc-rs need to support -Zfixed-x18 now?
What landed yesterday is a
-Zfixed-x18flag that needs to be converted to-ffixed-x18. Potentially we change the flag to anx18-availablefeature in the future. Does cc-rs need to support-Zfixed-x18now?
Thanks for explanation, currently nobody requested for that feature, but it might be a feature good to have just in case someone needs it.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request
Make -Zfixed-x18 into a target modifier
As part of rust-lang#136966, the -Zfixed-x18 flag should be turned into a target modifier. This is a blocker to stabilization of the flag. The flag was originally added in rust-lang#124655 and the MCP for its addition is MCP#748.
On some aarch64 targets, the x18 register is used as a temporary caller-saved register by default. When the -Zfixed-x18 flag is passed, this is turned off so that the compiler doesn't use the x18 register. This allows end-users to use the x18 register for other purposes. For example, by accessing it with inline asm you can use the register as a very efficient thread-local variable. Another common use-case is to store the stack pointer needed by the shadow-call-stack sanitizer. There are also some aarch64 targets where not using x18 is the default – in those cases the flag is a no-op.
Note that this flag does not on its own cause an ABI mismatch. What actually causes an ABI mismatch is when you have different compilation units that disagree on what it should be used for. But having a CU that uses it and another CU that doesn't normally isn't enough to trigger an ABI problem. However, we still consider the flag to be a target modifier in all cases, since it is assumed that you are passing the flag because you intend to assign some other meaning to the register. Rejecting all flag mismatches even if not all are unsound is consistent with RFC#3716. See the headings "not all mismatches are unsound" and "cases that are not caught" for additional discussion of this.
On aarch64 targets where -Zfixed-x18 is not a no-op, it is an error to pass -Zsanitizer=shadow-call-stack without also passing -Zfixed-x18.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request
Make -Zfixed-x18 into a target modifier
As part of rust-lang#136966, the -Zfixed-x18 flag should be turned into a target modifier. This is a blocker to stabilization of the flag. The flag was originally added in rust-lang#124655 and the MCP for its addition is MCP#748.
On some aarch64 targets, the x18 register is used as a temporary caller-saved register by default. When the -Zfixed-x18 flag is passed, this is turned off so that the compiler doesn't use the x18 register. This allows end-users to use the x18 register for other purposes. For example, by accessing it with inline asm you can use the register as a very efficient thread-local variable. Another common use-case is to store the stack pointer needed by the shadow-call-stack sanitizer. There are also some aarch64 targets where not using x18 is the default – in those cases the flag is a no-op.
Note that this flag does not on its own cause an ABI mismatch. What actually causes an ABI mismatch is when you have different compilation units that disagree on what it should be used for. But having a CU that uses it and another CU that doesn't normally isn't enough to trigger an ABI problem. However, we still consider the flag to be a target modifier in all cases, since it is assumed that you are passing the flag because you intend to assign some other meaning to the register. Rejecting all flag mismatches even if not all are unsound is consistent with RFC#3716. See the headings "not all mismatches are unsound" and "cases that are not caught" for additional discussion of this.
On aarch64 targets where -Zfixed-x18 is not a no-op, it is an error to pass -Zsanitizer=shadow-call-stack without also passing -Zfixed-x18.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Rollup merge of rust-lang#140661 - Darksonn:fixedx18-tm, r=wesleywiser
Make -Zfixed-x18 into a target modifier
As part of rust-lang#136966, the -Zfixed-x18 flag should be turned into a target modifier. This is a blocker to stabilization of the flag. The flag was originally added in rust-lang#124655 and the MCP for its addition is MCP#748.
On some aarch64 targets, the x18 register is used as a temporary caller-saved register by default. When the -Zfixed-x18 flag is passed, this is turned off so that the compiler doesn't use the x18 register. This allows end-users to use the x18 register for other purposes. For example, by accessing it with inline asm you can use the register as a very efficient thread-local variable. Another common use-case is to store the stack pointer needed by the shadow-call-stack sanitizer. There are also some aarch64 targets where not using x18 is the default – in those cases the flag is a no-op.
Note that this flag does not on its own cause an ABI mismatch. What actually causes an ABI mismatch is when you have different compilation units that disagree on what it should be used for. But having a CU that uses it and another CU that doesn't normally isn't enough to trigger an ABI problem. However, we still consider the flag to be a target modifier in all cases, since it is assumed that you are passing the flag because you intend to assign some other meaning to the register. Rejecting all flag mismatches even if not all are unsound is consistent with RFC#3716. See the headings "not all mismatches are unsound" and "cases that are not caught" for additional discussion of this.
On aarch64 targets where -Zfixed-x18 is not a no-op, it is an error to pass -Zsanitizer=shadow-call-stack without also passing -Zfixed-x18.
Labels
Status: This is awaiting some action (such as code changes or more information) from the author.
Relevant to the compiler team, which will review and decide on the PR/issue.