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

Support stack probing for arm64 (aarch64) #77071

Closed
tmandry opened this issue Sep 22, 2020 · 22 comments
Closed

Support stack probing for arm64 (aarch64) #77071

tmandry opened this issue Sep 22, 2020 · 22 comments
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-stack-probe Area: Stack probing and guard pages C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC O-AArch64 Armv8-A or later processors in AArch64 mode PG-exploit-mitigations Project group: Exploit mitigations S-blocked Status: Blocked on something else such as an RFC or other implementation work. S-tracking-unimplemented Status: The feature has not been implemented. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@tmandry
Copy link
Member

tmandry commented Sep 22, 2020

Background: #43241

This issue is to track implementation of stack probing on arm64. Most of the work necessary is in in LLVM, but this is a convenient place to track progress and final support in the Rust compiler.

Implementation: @cuviper has suggested using LLVM's inline-asm support for stack probing: #43241 (comment). It does not yet support arm64.

External trackers:

@tmandry tmandry added the C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC label Sep 22, 2020
@jonas-schievink jonas-schievink added the O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state label Sep 22, 2020
@pietroalbini
Copy link
Member

@rustbot ping arm

@rustbot
Copy link
Collaborator

rustbot commented Sep 22, 2020

Hey ARM Group! This bug has been identified as a good "ARM candidate".
In case it's useful, here are some instructions for tackling these sorts of
bugs. Maybe take a look?
Thanks! <3

cc @JamieCunliffe @joaopaulocarreiro @raw-bin @Stammark @vigoux

@tmandry
Copy link
Member Author

tmandry commented Feb 13, 2021

Hi @raw-bin, what's the status of this? As I recall this work was planned to land as part of LLVM 12, but I haven't seen updates on the Linaro tracker yet.

@raw-bin
Copy link

raw-bin commented Feb 13, 2021 via email

@raw-bin
Copy link

raw-bin commented Feb 23, 2021

Hi @tmandry, folks.

Apologies for the delay.

There has been good progress.

Please note the following patches that have been posted by Linaro for review:

https://reviews.llvm.org/D96004
https://reviews.llvm.org/D96005
https://reviews.llvm.org/D96006
https://reviews.llvm.org/D96007

At present Linaro is blocking on review.

If possible, please do chime in.

I will provide further updates as needed.

Cheers

@tmandry
Copy link
Member Author

tmandry commented Feb 23, 2021

Great, thanks for the update! Let me ask around to see if I can help get those patches reviewed.

@jyn514 jyn514 added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label Mar 15, 2021
@workingjubilee workingjubilee added O-AArch64 Armv8-A or later processors in AArch64 mode and removed O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state labels Mar 18, 2022
@joshtriplett
Copy link
Member

What's the current state of this? What needs to happen to get working stack probes on aarch64?

@yaahc yaahc added the A-stack-probe Area: Stack probing and guard pages label Jun 1, 2022
@pnkfelix
Copy link
Member

pnkfelix commented Oct 14, 2022

Visited for T-compiler steering backlog bonanza.

Seems like 3 out of the 4 of the LLVM patches linked above are still undergoing review. So, tagging this as unimplemented. (And also blocked, since its waiting on LLVM review capacity that we really can't do much about ourselves.)

@rustbot label: S-tracking-unimplemented S-blocked

@rustbot rustbot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. S-tracking-unimplemented Status: The feature has not been implemented. labels Oct 14, 2022
@jacobbramley
Copy link
Contributor

Arm's LLVM team have now picked this up, and have told us that the work should be completed by September at the latest (LLVM 17).

@tstellar
Copy link

tstellar commented Feb 3, 2023

The deadline to get changes into LLVM17 is the end of July, so September would be too late.

@jacobbramley
Copy link
Contributor

Yes, sorry, I wasn't very clear. They're hoping to get this reviewed and merged in time for the LLVM 17 release branch, which means by the end of July on main. There should therefore be something for us to see and test somewhat earlier than September, but (according to LLVM's release process documentation)17.0.0-final would take at least an additional six weeks, and of course it's possible that issues are found in that testing time.

@Noratrieb Noratrieb added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 5, 2023
@jonathanpallant
Copy link
Contributor

Does anyone know if these changes made it in?

@jacobbramley
Copy link
Contributor

They didn't! I'd been tracking it, and all seemed to be going well until recently, so I've been digging a bit to work out what's happened.

It seems that the work was progressing as planned, but hit validation issues, so it missed the LLVM17 code freeze, sadly. It was being tracked in Linaro's LLVM-618, but that doesn't describe the exact problem. I'm still hoping to have it landed on main somewhat before the LLVM18 code freeze, and I've stressed its importance.

I apologise for the further delay!

@jacobbramley
Copy link
Contributor

This is in progress here: llvm/llvm-project#66524 (moved from Phabricator)

@slanterns
Copy link
Contributor

It has been merged. Hopefully we can get it after upgrading to LLVM18.

@cuviper
Copy link
Member

cuviper commented Nov 30, 2023

Great! If someone wants to try it, you can add stack_probes: StackProbeType::Inline to the options here:


... and build the compiler with your own LLVM 18 / main.

(The inline option should be a no-op on versions where the target didn't support it.)

Then tests/ui/abi/stack-probes.rs can be updated for a functional test, but even just the normal rustc bootstrap process will probably exercise probing enough to see if there are any rough edges.

@cuviper
Copy link
Member

cuviper commented Dec 1, 2023

I went ahead and tested it myself and found no issues => #118491 flips the switch!

@rcvalle rcvalle added the PG-exploit-mitigations Project group: Exploit mitigations label Dec 1, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 14, 2023
…eywiser

Enable stack probes on aarch64 for LLVM 18

I tested this on `aarch64-unknown-linux-gnu` with LLVM main (~18).

cc rust-lang#77071, to be closed once we upgrade our LLVM submodule.
@cuviper
Copy link
Member

cuviper commented Jan 1, 2024

@erikschul Cross-compilation is a completely different thing -- please open a separate issue.

@adamgemmell
Copy link
Contributor

adamgemmell commented Jan 9, 2024

I've done a top 20k crater run on cuviper's #118491 on an aarch64 machine. I used Rust e51e98d and LLVM c09e6905567a6b546bb2fd9e863511a8fb939b19 and didn't find any non-spurious failures.

@jacobbramley
Copy link
Contributor

Is this issue now complete, or do we need anything else?

@jonathanpallant
Copy link
Contributor

What about the 1 footnote on https://doc.rust-lang.org/nightly/rustc/platform-support.html?

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 6, 2024
…, r=workingjubilee

Remove outdated footnote "missing-stack-probe" in platform-support

... after rust-lang#120055 and rust-lang#118491.

cc rust-lang#77071 (comment).
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 7, 2024
Rollup merge of rust-lang#122094 - slanterns:arm-stack-probe-footnote, r=workingjubilee

Remove outdated footnote "missing-stack-probe" in platform-support

... after rust-lang#120055 and rust-lang#118491.

cc rust-lang#77071 (comment).
@cuviper
Copy link
Member

cuviper commented Mar 7, 2024

I think we're done! Aarch64 stack probes are tested in CI (e.g. here and here), and #122094 removed that footnote.

If there are fundamental problems discovered, we can reopen this, otherwise please open new issues for any bugs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-stack-probe Area: Stack probing and guard pages C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC O-AArch64 Armv8-A or later processors in AArch64 mode PG-exploit-mitigations Project group: Exploit mitigations S-blocked Status: Blocked on something else such as an RFC or other implementation work. S-tracking-unimplemented Status: The feature has not been implemented. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests