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

Fix handling of x18 in AArch64 inline assembly on ohos/trusty or with -Zfixed-x18 #133463

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Nov 25, 2024

Currently AArch64 inline assembly allows using x18 on ohos/trusty or with -Zfixed-x18.

pub(crate) fn target_reserves_x18(target: &Target) -> bool {
target.os == "android" || target.os == "fuchsia" || target.is_like_osx || target.is_like_windows
}

However, x18 is reserved in these environments and should not be allowed in the input/output operands of inline assemblies as it is in Android, Windows, etc..


features: "+neon,+fp-armv8,+reserve-x18".into(),

// -Zfixed-x18
if sess.opts.unstable_opts.fixed_x18 {
if sess.target.arch != "aarch64" {
sess.dcx().emit_fatal(FixedX18InvalidArch { arch: &sess.target.arch });
} else {
features.push("+reserve-x18".into());
}
}

(As for ohos, +reserve-x18 is redundant since 7a966b9 that starting using llvm's ohos targets. So removed it from target-spec.)

This fix may potentially break the code for tier 2 target (aarch64-unknown-linux-ohos). (As for others, aarch64-unknown-trusty is tier 3 and -Zfixed-x18 is unstable so breaking them should be fine.)
However, in any case, it seems suspicious that the code that is broken by this was sound.

r? @Amanieu

@rustbot label O-AArch64 +A-inline-assembly

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 25, 2024
@rustbot
Copy link
Collaborator

rustbot commented Nov 25, 2024

These commits modify compiler targets.
(See the Target Tier Policy.)

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

@rustbot rustbot added A-inline-assembly Area: Inline assembly (`asm!(…)`) O-AArch64 Armv8-A or later processors in AArch64 mode labels Nov 25, 2024
@rust-log-analyzer

This comment has been minimized.

@taiki-e
Copy link
Member Author

taiki-e commented Nov 25, 2024

cc @Darksonn (who added -Zfixed-x18 in #124655)
cc @randomPoison (who added aarch64-unknown-trusty in #129490)

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

Comment on lines +74 to +83
pub(crate) fn target_reserves_x18(target: &Target, target_features: &FxIndexSet<Symbol>) -> bool {
// See isX18ReservedByDefault in LLVM for targets reserve x18 by default:
// https://github.com/llvm/llvm-project/blob/llvmorg-19.1.0/llvm/lib/TargetParser/AArch64TargetParser.cpp#L102-L105
// Note that +reserve-x18 is currently not set for the above targets.
target.os == "android"
|| target.os == "fuchsia"
|| target.env == "ohos"
|| target.is_like_osx
|| target.is_like_windows
|| target_features.contains(&sym::reserve_x18)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to avoid duplicating this logic? It seems prone to getting out of sync with llvm.

Copy link
Member

Choose a reason for hiding this comment

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

I can't think of a good way to avoid the duplication unfortunately.

@randomPoison
Copy link
Contributor

This seems correct as far as Trusty is concerned, thanks @taiki-e for addressing this ❤️ I wasn't aware of this when I added the Trusty targets, but as far as I know we don't directly use the x18 register in inline assembly from Rust, so this shouldn't introduce any problems for Trusty 👍

@Amanieu
Copy link
Member

Amanieu commented Nov 28, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Nov 28, 2024

📌 Commit 687dc19 has been approved by Amanieu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors 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 Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-inline-assembly Area: Inline assembly (`asm!(…)`) O-AArch64 Armv8-A or later processors in AArch64 mode S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants