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

Compiler does not recognize the reserve-x18 target feature #121970

Closed
Darksonn opened this issue Mar 4, 2024 · 3 comments · Fixed by #124655
Closed

Compiler does not recognize the reserve-x18 target feature #121970

Darksonn opened this issue Mar 4, 2024 · 3 comments · Fixed by #124655
Labels
A-ABI Area: Concerning the application binary interface (ABI) A-diagnostics Area: Messages for errors, warnings, and lints A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. C-bug Category: This is a bug. D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. O-AArch64 Armv8-A or later processors in AArch64 mode T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Darksonn
Copy link
Contributor

Darksonn commented Mar 4, 2024

On the aarch64-unknown-none target, specifying the reserve-x18 target feature using the flag -Ctarget-feature=+reserve-x18 results in the following warning:

warning: unknown feature specified for `-Ctarget-feature`: `reserve-x18`
  |
  = note: it is still passed through to the codegen backend
  = help: consider filing a feature request

The desired effect of this flag is to reserve the x18 register so that LLVM does not use it during codegen. Note that despite the warning, the flag still works fine today since it is passed on to LLVM.

This feature is necessary for Rust to work when using the Linux Kernel's dynamic shadow call stack feature on aarch64 (more info). The shadow call stack feature requires that the x18 register is reserved, since it is used to store the shadow stack. In C code, the dynamic version of shadow call stack requires you to pass -ffixed-x18 but not -fsanitize=shadow-call-stack (which is only used for non-dynamic SCS).

Clang has a collection of flags for reserving other registers than x18 as well. I think it would make sense to add those as well, even if they are less useful than x18.

Related to #96472.
Related to this thread on LKML.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 4, 2024
@jieyouxu jieyouxu added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. O-AArch64 Armv8-A or later processors in AArch64 mode A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. labels Mar 4, 2024
@jieyouxu jieyouxu added C-bug Category: This is a bug. D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Mar 4, 2024
@bjorn3
Copy link
Member

bjorn3 commented Mar 4, 2024

This affects the ABI, right? I think adding a target spec option makes the most sense (or actually you can already set it as target feature in the target spec without giving a warning). That way you can't enable or disable it for individual functions or crates, which would break the ABI.

Darksonn added a commit to Darksonn/linux that referenced this issue Mar 4, 2024
Add flags to support the shadow call stack sanitizer, both in the
dynamic and non-dynamic modes.

Right now, the compiler will emit the warning "unknown feature specified
for `-Ctarget-feature`: `reserve-x18`". However, the compiler still
passes it to the codegen backend, so the flag will work just fine. Once
rustc starts recognizing the flag (or provides another way to enable the
feature), it will stop emitting this warning. See [1] for the relevant
issue.

Currently, the compiler thinks that the aarch64-unknown-none target
doesn't support -Zsanitizer=shadow-call-stack, so the build will fail if
you enable shadow call stack in non-dynamic mode. However, I still think
it is reasonable to add the flag now, as it will at least fail the build
when using an invalid configuration, until the Rust compiler is fixed to
list -Zsanitizer=shadow-call-stack as supported for the target. See [2]
for the feature request to add this.

I have tested this change with Rust Binder on an Android device using
CONFIG_DYNAMIC_SCS. Without the -Ctarget-feature=+reserve-x18 flag, the
phone crashes immediately on boot, and with the flag, the phone appears
to work normally.

Link: rust-lang/rust#121970 [1]
Link: rust-lang/rust#121972 [2]
Signed-off-by: Alice Ryhl <[email protected]>
Darksonn added a commit to Darksonn/linux that referenced this issue Mar 4, 2024
Add flags to support the shadow call stack sanitizer, both in the
dynamic and non-dynamic modes.

Right now, the compiler will emit the warning "unknown feature specified
for `-Ctarget-feature`: `reserve-x18`". However, the compiler still
passes it to the codegen backend, so the flag will work just fine. Once
rustc starts recognizing the flag (or provides another way to enable the
feature), it will stop emitting this warning. See [1] for the relevant
issue.

Currently, the compiler thinks that the aarch64-unknown-none target
doesn't support -Zsanitizer=shadow-call-stack, so the build will fail if
you enable shadow call stack in non-dynamic mode. However, I still think
it is reasonable to add the flag now, as it will at least fail the build
when using an invalid configuration, until the Rust compiler is fixed to
list -Zsanitizer=shadow-call-stack as supported for the target. See [2]
for the feature request to add this.

I have tested this change with Rust Binder on an Android device using
CONFIG_DYNAMIC_SCS. Without the -Ctarget-feature=+reserve-x18 flag, the
phone crashes immediately on boot, and with the flag, the phone appears
to work normally.

Link: rust-lang/rust#121970 [1]
Link: rust-lang/rust#121972 [2]
Signed-off-by: Alice Ryhl <[email protected]>
Darksonn added a commit to Darksonn/linux that referenced this issue Mar 5, 2024
Add flags to support the shadow call stack sanitizer, both in the
dynamic and non-dynamic modes.

Right now, the compiler will emit the warning "unknown feature specified
for `-Ctarget-feature`: `reserve-x18`". However, the compiler still
passes it to the codegen backend, so the flag will work just fine. Once
rustc starts recognizing the flag (or provides another way to enable the
feature), it will stop emitting this warning. See [1] for the relevant
issue.

Currently, the compiler thinks that the aarch64-unknown-none target
doesn't support -Zsanitizer=shadow-call-stack, so the build will fail if
you enable shadow call stack in non-dynamic mode. However, I still think
it is reasonable to add the flag now, as it will at least fail the build
when using an invalid configuration, until the Rust compiler is fixed to
list -Zsanitizer=shadow-call-stack as supported for the target. See [2]
for the feature request to add this.

I have tested this change with Rust Binder on an Android device using
CONFIG_DYNAMIC_SCS. Without the -Ctarget-feature=+reserve-x18 flag, the
phone crashes immediately on boot, and with the flag, the phone appears
to work normally.

This contains a TODO to add the -Zuse-sync-unwind=n flag. The flag
defaults to n, so it isn't a problem today, but the flag is unstable, so
the default could change in a future compiler release.

Link: rust-lang/rust#121970 [1]
Link: rust-lang/rust#121972 [2]
Signed-off-by: Alice Ryhl <[email protected]>
Darksonn added a commit to Darksonn/linux that referenced this issue Mar 5, 2024
Add flags to support the shadow call stack sanitizer, both in the
dynamic and non-dynamic modes.

Right now, the compiler will emit the warning "unknown feature specified
for `-Ctarget-feature`: `reserve-x18`". However, the compiler still
passes it to the codegen backend, so the flag will work just fine. Once
rustc starts recognizing the flag (or provides another way to enable the
feature), it will stop emitting this warning. See [1] for the relevant
issue.

Currently, the compiler thinks that the aarch64-unknown-none target
doesn't support -Zsanitizer=shadow-call-stack, so the build will fail if
you enable shadow call stack in non-dynamic mode. However, I still think
it is reasonable to add the flag now, as it will at least fail the build
when using an invalid configuration, until the Rust compiler is fixed to
list -Zsanitizer=shadow-call-stack as supported for the target. See [2]
for the feature request to add this.

I have tested this change with Rust Binder on an Android device using
CONFIG_DYNAMIC_SCS. Without the -Ctarget-feature=+reserve-x18 flag, the
phone crashes immediately on boot, and with the flag, the phone appears
to work normally.

This contains a TODO to add the -Zuse-sync-unwind=n flag. The flag
defaults to n, so it isn't a problem today, but the flag is unstable, so
the default could change in a future compiler release.

Link: rust-lang/rust#121970 [1]
Link: rust-lang/rust#121972 [2]
Signed-off-by: Alice Ryhl <[email protected]>
Darksonn added a commit to Darksonn/linux that referenced this issue Mar 5, 2024
Add flags to support the shadow call stack sanitizer, both in the
dynamic and non-dynamic modes.

Right now, the compiler will emit the warning "unknown feature specified
for `-Ctarget-feature`: `reserve-x18`". However, the compiler still
passes it to the codegen backend, so the flag will work just fine. Once
rustc starts recognizing the flag (or provides another way to enable the
feature), it will stop emitting this warning. See [1] for the relevant
issue.

Currently, the compiler thinks that the aarch64-unknown-none target
doesn't support -Zsanitizer=shadow-call-stack, so the build will fail if
you enable shadow call stack in non-dynamic mode. However, I still think
it is reasonable to add the flag now, as it will at least fail the build
when using an invalid configuration, until the Rust compiler is fixed to
list -Zsanitizer=shadow-call-stack as supported for the target. See [2]
for the feature request to add this.

I have tested this change with Rust Binder on an Android device using
CONFIG_DYNAMIC_SCS. Without the -Ctarget-feature=+reserve-x18 flag, the
phone crashes immediately on boot, and with the flag, the phone appears
to work normally.

This contains a TODO to add the -Zuse-sync-unwind=n flag. The flag
defaults to n, so it isn't a problem today, but the flag is unstable, so
the default could change in a future compiler release.

Link: rust-lang/rust#121970 [1]
Link: rust-lang/rust#121972 [2]
Signed-off-by: Alice Ryhl <[email protected]>
@ojeda
Copy link
Contributor

ojeda commented Mar 5, 2024

This affects the ABI, right? I think adding a target spec option makes the most sense (or actually you can already set it as target feature in the target spec without giving a warning). That way you can't enable or disable it for individual functions or crates, which would break the ABI.

Yeah, it would be nice to have a flag for this (or having something like "global target features", i.e. like -Ctarget-feature, but that it ensures it is set the same way for all compilation units).

Darksonn added a commit to Darksonn/linux that referenced this issue Mar 5, 2024
Add flags to support the shadow call stack sanitizer, both in the
dynamic and non-dynamic modes.

Right now, the compiler will emit the warning "unknown feature specified
for `-Ctarget-feature`: `reserve-x18`". However, the compiler still
passes it to the codegen backend, so the flag will work just fine. Once
rustc starts recognizing the flag (or provides another way to enable the
feature), it will stop emitting this warning. See [1] for the relevant
issue.

Currently, the compiler thinks that the aarch64-unknown-none target
doesn't support -Zsanitizer=shadow-call-stack, so the build will fail if
you enable shadow call stack in non-dynamic mode. However, I still think
it is reasonable to add the flag now, as it will at least fail the build
when using an invalid configuration, until the Rust compiler is fixed to
list -Zsanitizer=shadow-call-stack as supported for the target. See [2]
for the feature request to add this.

I have tested this change with Rust Binder on an Android device using
CONFIG_DYNAMIC_SCS. Without the -Ctarget-feature=+reserve-x18 flag, the
phone crashes immediately on boot, and with the flag, the phone appears
to work normally.

This contains a TODO to add the -Zuse-sync-unwind=n flag. The flag
defaults to n, so it isn't a problem today, but the flag is unstable, so
the default could change in a future compiler release.

Link: rust-lang/rust#121970 [1]
Link: rust-lang/rust#121972 [2]
Acked-by: Miguel Ojeda <[email protected]>
Tested-by: Valentin Obst <[email protected]>
Reviewed-by: Valentin Obst <[email protected]>
Signed-off-by: Alice Ryhl <[email protected]>
hubot pushed a commit to aosp-mirror/kernel_common that referenced this issue Mar 5, 2024
Add flags to support the shadow call stack sanitizer, both in the
dynamic and non-dynamic modes.

Right now, the compiler will emit the warning "unknown feature specified
for `-Ctarget-feature`: `reserve-x18`". However, the compiler still
passes it to the codegen backend, so the flag will work just fine. Once
rustc starts recognizing the flag (or provides another way to enable the
feature), it will stop emitting this warning. See [1] for the relevant
issue.

Currently, the compiler thinks that the aarch64-unknown-none target
doesn't support -Zsanitizer=shadow-call-stack, so the build will fail if
you enable shadow call stack in non-dynamic mode. However, I still think
it is reasonable to add the flag now, as it will at least fail the build
when using an invalid configuration, until the Rust compiler is fixed to
list -Zsanitizer=shadow-call-stack as supported for the target. See [2]
for the feature request to add this.

I have tested this change with Rust Binder on an Android device using
CONFIG_DYNAMIC_SCS. Without the -Ctarget-feature=+reserve-x18 flag, the
phone crashes immediately on boot, and with the flag, the phone appears
to work normally.

This contains a TODO to add the -Zuse-sync-unwind=n flag. The flag
defaults to n, so it isn't a problem today, but the flag is unstable, so
the default could change in a future compiler release.

Link: rust-lang/rust#121970 [1]
Link: rust-lang/rust#121972 [2]
Signed-off-by: Alice Ryhl <[email protected]>

Bug: 328033850
Link: https://lore.kernel.org/rust-for-linux/[email protected]/
Change-Id: Ia55287e1ed6da2d5d8d3d6414f2d9a0fc7d23e81
Signed-off-by: Alice Ryhl <[email protected]>
@JamieCunliffe
Copy link
Contributor

This affects the ABI, right?

There could be ABI concerns with this in general. AAPCS64 defines x18 as a platform register if required, or temporary if a platform register isn't needed. For x18 it's probably unlikely for it to have ABI concerns as it's already reserved for use on some platforms, the ones where it's not already reserved, it's only supposed to be a temporary register so not using it should be OK as it is not used for any fixed purpose. That's obviously assuming a platform defines its ABI based on the Arm standards.

LLVM does support reserve on other registers, most notably for ABI reasons, x1 through x7 which would have ABI problems. LLVM does have test cases to ensure that a function call doesn't occur if any of the argument registers have been reserved. I'm not sure what other codegen backends do in that case though, or if they even support reserving those registers. The analysis that LLVM does, doesn't seem to care if it would need to use that register for ABI reasons, even a function such as void test() can't be called in a quick test.

Using target features for this feels strange to me though, I'm wondering if a codegen option to reserve certain registers would be a better approach here. While this issue is specifically about reserving a register on AArch64, LLVM does support this for multiple other architectures and multiple registers, which we should maybe consider for the future.

hubot pushed a commit to aosp-mirror/kernel_common that referenced this issue Mar 13, 2024
Add flags to support the shadow call stack sanitizer, both in the
dynamic and non-dynamic modes.

Right now, the compiler will emit the warning "unknown feature specified
for `-Ctarget-feature`: `reserve-x18`". However, the compiler still
passes it to the codegen backend, so the flag will work just fine. Once
rustc starts recognizing the flag (or provides another way to enable the
feature), it will stop emitting this warning. See [1] for the relevant
issue.

Currently, the compiler thinks that the aarch64-unknown-none target
doesn't support -Zsanitizer=shadow-call-stack, so the build will fail if
you enable shadow call stack in non-dynamic mode. However, I still think
it is reasonable to add the flag now, as it will at least fail the build
when using an invalid configuration, until the Rust compiler is fixed to
list -Zsanitizer=shadow-call-stack as supported for the target. See [2]
for the feature request to add this.

I have tested this change with Rust Binder on an Android device using
CONFIG_DYNAMIC_SCS. Without the -Ctarget-feature=+reserve-x18 flag, the
phone crashes immediately on boot, and with the flag, the phone appears
to work normally.

This contains a TODO to add the -Zuse-sync-unwind=n flag. The flag
defaults to n, so it isn't a problem today, but the flag is unstable, so
the default could change in a future compiler release.

Link: rust-lang/rust#121970 [1]
Link: rust-lang/rust#121972 [2]
Signed-off-by: Alice Ryhl <[email protected]>

Bug: 328033850
Link: https://lore.kernel.org/rust-for-linux/[email protected]/
Change-Id: Ia55287e1ed6da2d5d8d3d6414f2d9a0fc7d23e81
Signed-off-by: Alice Ryhl <[email protected]>
@jieyouxu jieyouxu added A-diagnostics Area: Messages for errors, warnings, and lints A-ABI Area: Concerning the application binary interface (ABI) labels Apr 3, 2024
@bors bors closed this as completed in d0311c1 May 29, 2024
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
hubot pushed a commit to aosp-mirror/kernel_common that referenced this issue Aug 21, 2024
Add flags to support the shadow call stack sanitizer, both in the
dynamic and non-dynamic modes.

Right now, the compiler will emit the warning "unknown feature specified
for `-Ctarget-feature`: `reserve-x18`". However, the compiler still
passes it to the codegen backend, so the flag will work just fine. Once
rustc starts recognizing the flag (or provides another way to enable the
feature), it will stop emitting this warning. See [1] for the relevant
issue.

Currently, the compiler thinks that the aarch64-unknown-none target
doesn't support -Zsanitizer=shadow-call-stack, so the build will fail if
you enable shadow call stack in non-dynamic mode. However, I still think
it is reasonable to add the flag now, as it will at least fail the build
when using an invalid configuration, until the Rust compiler is fixed to
list -Zsanitizer=shadow-call-stack as supported for the target. See [2]
for the feature request to add this.

I have tested this change with Rust Binder on an Android device using
CONFIG_DYNAMIC_SCS. Without the -Ctarget-feature=+reserve-x18 flag, the
phone crashes immediately on boot, and with the flag, the phone appears
to work normally.

This contains a TODO to add the -Zuse-sync-unwind=n flag. The flag
defaults to n, so it isn't a problem today, but the flag is unstable, so
the default could change in a future compiler release.

Link: rust-lang/rust#121970 [1]
Link: rust-lang/rust#121972 [2]
Signed-off-by: Alice Ryhl <[email protected]>

Bug: 328033850
Link: https://lore.kernel.org/rust-for-linux/[email protected]/
Change-Id: Ia55287e1ed6da2d5d8d3d6414f2d9a0fc7d23e81
Signed-off-by: Alice Ryhl <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ABI Area: Concerning the application binary interface (ABI) A-diagnostics Area: Messages for errors, warnings, and lints A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. C-bug Category: This is a bug. D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. O-AArch64 Armv8-A or later processors in AArch64 mode T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants