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

-Zfixed-x18 #748

Closed
2 of 3 tasks
Darksonn opened this issue May 15, 2024 · 6 comments
Closed
2 of 3 tasks

-Zfixed-x18 #748

Darksonn opened this issue May 15, 2024 · 6 comments
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@Darksonn
Copy link

Darksonn commented May 15, 2024

Proposal

This MCP proposes to add a -Zfixed-x18 flag on aarch64 targets. The flag has the same effect as the -ffixed-x18 flag that is supported by clang and gcc. When the flag is passed, the x18 register will not be used as a temporary register in the generated machine code.

Proposed implementation: rust-lang/rust#124655
Feature request: rust-lang/rust#121970

Motivation

This motivation for this change is use in the Linux Kernel. When compiling Rust code for the kernel, the aarch64-unknown-none target is used, and this is a platform where x18 is a temporary caller-saved register by default. I am proposing to add this flag so that the Linux Kernel can make x18 into a reserved register when necessary.

The Linux Kernel has some cases where it needs to reserve x18, but does not pass the -Zsanitizer=shadow-call-stack flag. This is due to the dynamic shadow call stack feature, where the Linux Kernel is able to choose whether SCS should be enabled at boot. This works by having the compiler emit PACIASP/AUTIASP instructions instead of SCS_PUSH/SCS_POP. If Linux decides to enable SCS at boot, then it will use the unwind tables to find the PACIASP/AUTIASP instructions, and modify the machine code at runtime by replacing PACIASP/AUTIASP with SCS_PUSH/SCS_POP instructions in all functions. The transformation from PACIASP/AUTIASP to SCS_PUSH/SCS_POP is only valid if the x18 register is reserved globally.

The Linux Kernel configuration used by Android uses the dynamic shadow call stack feature in production, so a Rust equivalent to clang's -ffixed-x18 flag is a prerequisite for using Rust in the Linux Kernel on Android.

ABI compatibility

Although this flag changes the ABI by changing the purpose of the x18 register, it does so in a way that is not breaking. On its own, mixing code that does or does not use the -Zfixed-x18 flag can't lead to UB. See rust-lang/rust#124323 for a more detailed discussion on this.

Architecture specific flags

Rustc does not currently have any flags that are architecture-specific. This MCP is proposing to add the first such flag.

Alternatives

This MCP proposes to expose this feature via a flag called -Zfixed-x18, however there alternatives.

Adding a new target feature

Internally, clang implements the -ffixed-x18 flag by passing a target feature called reserve-x18 to LLVM, and our -Zfixed-x18 flag would be implemented in the same manner. However, this means that an alternative implementation would be expose reserve-x18 as a stable target feature in the compiler.

This MCP does not propose to add a new target feature because reserving the x18 register does not really seem like it "fits in" with the notion of a target feature. It seems better to keep that as an implementation detail and expose a -ffixed-x18 flag like clang.

The author of this MCP has previously proposed to implement this feature by exposing a target feature: rust-lang/rust#124323.

Global target features / target modifiers

Effectively, this flag could be considered to modify the target that the compiler is compiling for. We could add a more general flag to make modifications to the target. It would be required that the same modifications are made to all compilation units (else UB), so that this could be used with other features that modify the target and can cause UB when mixed.

The author believes that some sort of global target feature / target modifier feature should be added to the language, but the author does not believe that this particular flag should be a global target feature / target modifier because mixing it cannot result in UB.

Add a new target

There are already some aarch64 targets that always reserve the x18 register, so we could add a target that is like aarch64-unknown-none except that the x18 register is reserved. This MCP does not propose that solution due to several disadvantages:

  • Using built-in targets come with the assumption that all flags specified in this way can be split into a short list of targets with particular values for all of the flags. However, in the Linux Kernel, these flags are instead specified using individual on/off toggles. To support this using targets, we need O(2^n) different targets.
  • Using a target.json is perma-unstable. (The Linux Kernel is using target.json on x86 right now due to similar issues with a different flag.)

Finally, the usual argument for why this kind of thing should be part of the target is that mixing it can cause UB, which is not the case for -Zfixed-x18. (Even though all known use-cases require it to not be mixed.)

Mentors or Reviewers

If you have a reviewer or mentor in mind for this work, mention them
here. You can put your own name here if you are planning to mentor the
work.

Process

The main points of the Major Change Process are as follows:

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@Darksonn Darksonn added major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team labels May 15, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 15, 2024

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

Concerns or objections to the proposal should be discussed on Zulip and formally registered here by adding a comment with the following syntax:

@rustbot concern reason-for-concern 
<description of the concern> 

Concerns can be lifted with:

@rustbot resolve reason-for-concern 

See documentation at https://forge.rust-lang.org

cc @rust-lang/compiler @rust-lang/compiler-contributors

@rustbot rustbot added the to-announce Announce this issue on triage meeting label May 15, 2024
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label May 16, 2024
@lqd
Copy link
Member

lqd commented May 17, 2024

The title is still -Cfixed-x18 but the body is for a -Zfixed-x18. A flag is the best sounding alternative to me amongst the ones proposed, and is good enough to land unstably and unblock rust-for-linux here.

Therefore: @rustbot second

For stabilization purposes however, we can discuss in this MCP's zulip thread, as I'd expect we'd want a different naming scheme (but that's about it).

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label May 17, 2024
@Darksonn Darksonn changed the title -Cfixed-x18 -Zfixed-x18 May 17, 2024
@briansmith
Copy link

  • This is a backward-incompatible feature for any target that doesn't reserve x18, isn't it? Up to now, if a library is using external assembly language code, then previously it needed to ensure that x18 was not reserved by the target specification. Now it ALSO needs to check that it wasn't being built with -Zfixed-x18 and avoid using that external assembly language code.

  • Which #[cfg(...)] directive in Rust source code and what logic in build.rs would be used to detect the use of this feature?

  • cc-rs would need to be updated to forward this option.

  • LLVM/clang support many -ffixed- options analogous to this one. See https://clang.llvm.org/docs/ClangCommandLineReference.html and search for -ffixed-. Each one of these is a similar compatibility hazard.

@Darksonn
Copy link
Author

Darksonn commented May 18, 2024

Potential incompatibilities with external assembly is just part of the nature of codegen options. If you change any sort of codegen option, then you'll need to update your external assembly code to follow those same codegen options or accept that they're not applied to your assembly. It's not really unique to reserving registers.

Sometimes, people will use reserved registers as a sort of super fast thread local, and the way you access the value is with inline/external assembly that explicitly uses the register. So we have to allow assembly to access the register even if it's reserved.

As for #[cfg(...)] directives, I suppose this is a disadvantage of not making it a target feature. If it's a target feature, then we can just use the existing logic for target features to provide #[cfg(...)]/build script logic. With a flag, we'll need some other way.

@lqd
Copy link
Member

lqd commented May 18, 2024

So that it's easier to follow in a single place, let's keep technical discussion in the dedicated zulip stream

@apiraino
Copy link
Contributor

@rustbot label -final-comment-period +major-change-accepted

@rustbot rustbot added major-change-accepted A major change proposal that was accepted to-announce Announce this issue on triage meeting and removed final-comment-period The FCP has started, most (if not all) team members are in agreement labels May 28, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue May 29, 2024
Add `-Zfixed-x18`

This PR is a follow-up to rust-lang#124323 that proposes a different implementation. Please read the description of that PR for motivation.

See the equivalent flag in [the clang docs](https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-ffixed-x18).

MCP: rust-lang/compiler-team#748
Fixes rust-lang#121970
r? rust-lang/compiler
rust-timer added a commit to rust-lang-ci/rust that referenced this issue May 29, 2024
Rollup merge of rust-lang#124655 - Darksonn:fixed-x18, r=lqd,estebank

Add `-Zfixed-x18`

This PR is a follow-up to rust-lang#124323 that proposes a different implementation. Please read the description of that PR for motivation.

See the equivalent flag in [the clang docs](https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-ffixed-x18).

MCP: rust-lang/compiler-team#748
Fixes rust-lang#121970
r? rust-lang/compiler
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests

5 participants