WIP: Use BTI c in aarch64_outline_atomic.rs#1063
WIP: Use BTI c in aarch64_outline_atomic.rs#1063taiki-e wants to merge 2 commits intorust-lang:mainfrom
Conversation
|
grep'ing compiler-builtins, I don't see any mention of bti at all. Maybe instead of adding |
|
For normal Rust functions, the BTI c should be inserted by the compiler as needed. For global assemblies, the BTI c should be explicitly written within the assembly as needed. However, it is unclear whose responsibility it should be for naked functions (what we use for outline_atomic). At least currently, the naked function does nothing, but -Z branch-protection is unstable so we can change its behavior. |
| #[cfg(all(branch_protection = "bti", branch_protection = "pac-ret"))] | ||
| ".word 3", | ||
| #[cfg(all(not(branch_protection = "bti"), branch_protection = "pac-ret"))] | ||
| ".word 2", | ||
| #[cfg(all(branch_protection = "bti", not(branch_protection = "pac-ret")))] | ||
| ".word 1", | ||
| #[cfg(all(not(branch_protection = "bti"), not(branch_protection = "pac-ret")))] | ||
| ".word 0", |
There was a problem hiding this comment.
I think cfg_select would be nicer here
| #[cfg(all(branch_protection = "bti", branch_protection = "pac-ret"))] | |
| ".word 3", | |
| #[cfg(all(not(branch_protection = "bti"), branch_protection = "pac-ret"))] | |
| ".word 2", | |
| #[cfg(all(branch_protection = "bti", not(branch_protection = "pac-ret")))] | |
| ".word 1", | |
| #[cfg(all(not(branch_protection = "bti"), not(branch_protection = "pac-ret")))] | |
| ".word 0", | |
| cfg_select! ( | |
| all(branch_protection = "bti", branch_protection = "pac-ret") => ".word 3", | |
| branch_protection = "pac-ret" => ".word 2", | |
| branch_protection = "bti" => ".word 1", | |
| _ => ".word 0", | |
| ), |
|
I'm not that familiar with how this works, but generally the aim is to make naked functions "just work". So I'd say naked functions should support the flag. |
|
There was some discussion about naked functions vs. bti on the llvm side: llvm/llvm-project#56369 but it didn't conclude. I'm not actually sure what the current behavior is. |
|
Rust naked functions are entirely distinct from LLVM naked functions. We lower them to a block of global assembly + an |
|
I would actually prefer not having a BTI automatically inserted at the start of naked functions. The main purpose of naked functions is to support custom calling conventions which cannot be otherwise supported by the Rust compiler. In those cases we don't want to mess with what the user is intending for these functions to contain. |
|
Ah, I misunderstood: this actually adds instructions. Then I agree that we should not add that. And then |
|
Is there any reason these functions need to be naked at all? They don't use a custom calling convention and could just be implemented using plain |
…=lqd Revert enabling `outline-atomics` on various platforms Our implementations in `compiler-builtins` do not yet have BTI landing pads, so platforms using a custom-built std with unstable `-Zbranch-protection=bti` may run into issues. This should resolve rust-lang#151486. There is a PR in progress to add BTI support to compiler-builtins [1], after which this can be re-enabled. Linux has had `outline-atomics` enabled for years [2] with no known problems, so it is not changed here. Partially reverts rust-lang#144938 Reverts rust-lang#149633 Zulip discussion: https://rust-lang.zulipchat.com/#narrow/channel/245100-t-compiler.2Fprioritization.2Falerts/topic/.23151486.20Chromium.20hitting.20SIGILL.20crashes.20on.20aarch64.20after.20.231.E2.80.A6/with/571170956 [1]: rust-lang/compiler-builtins#1063 [2]: tgross35@0f9f241
Rollup merge of #151896 - tgross35:revert-outline-atomics, r=lqd Revert enabling `outline-atomics` on various platforms Our implementations in `compiler-builtins` do not yet have BTI landing pads, so platforms using a custom-built std with unstable `-Zbranch-protection=bti` may run into issues. This should resolve #151486. There is a PR in progress to add BTI support to compiler-builtins [1], after which this can be re-enabled. Linux has had `outline-atomics` enabled for years [2] with no known problems, so it is not changed here. Partially reverts #144938 Reverts #149633 Zulip discussion: https://rust-lang.zulipchat.com/#narrow/channel/245100-t-compiler.2Fprioritization.2Falerts/topic/.23151486.20Chromium.20hitting.20SIGILL.20crashes.20on.20aarch64.20after.20.231.E2.80.A6/with/571170956 [1]: rust-lang/compiler-builtins#1063 [2]: tgross35@0f9f241
|
Did a small sanity check and I think inline asm should be reasonably feasible https://rust.godbolt.org/z/s38z6a6jP. Might be a better way to still keep the early I was originally a bit concerned about paying for frame pointers, which used to be a bit of a problem with fma. But it looks like that has been resolved with rust-lang/rust#141800. |
|
You could write the LSE check in normal rust code and have 2 separate asm blocks in different if branches. |
Potential fix for rust-lang/rust#151486.
TODO: