Validate naked functions definitions #79653
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
A-inline-assembly
rust-highfive
added
the
S-waiting-on-review
Amanieu
reviewed
Dec 3, 2020
tmiasko
force-pushed
the
naked-functions
branch
2 times, most recently
from
0436769
to
3b161db
Compare
Given that this starts as a future incompatibility lint, with possibility for migration without causing immediate breakage, and all features involved are still unstable, I would lean towards rejecting |
tmiasko
force-pushed
the
naked-functions
branch
from
3b161db
to
b819c20
Compare
|
Amanieu
reviewed
Dec 7, 2020
tmiasko
force-pushed
the
naked-functions
branch
from
b819c20
to
ad26858
Compare
|
@bors r+ |
|
📌 Commit ad268583b6b7d1431ca681332f5fc3bab2b92a6c has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit ad268583b6b7d1431ca681332f5fc3bab2b92a6c with merge d672c52e967b2159b84a144b3fc85153a05f769a... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
You need to mark the test with |
tmiasko
force-pushed
the
naked-functions
branch
from
ad26858
to
5ce639a
Compare
|
Ignored test on wasm32. The other targets should be fine, I think? (the att_syntax test is conditional, last, and produces no errors). |
|
Not exactly, we have plenty of targets which don't support |
tmiasko
force-pushed
the
naked-functions
branch
from
5ce639a
to
8065dab
Compare
|
Sure, replaced with |
bors
added
the
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
2 tasks
bors bot
added a commit
to tock/tock
that referenced
this pull request
Jan 7, 2021
2290: litex: update LiteX revision, use "Secure"/"TockSecureIMC" cpu variant and integrate PMP r=bradjc a=lschuermann ### Pull Request Overview This pull request - updates the targeted LiteX revision for both boards (`litex/sim` and `litex/arty`) - points to a companion repository ([tock-litex](https://github.com/lschuermann/tock-litex)), which contains prebuilt bitstreams and build scripts for generating these The board definitions in this repository are tested against bitstreams obtained by these build scripts, so it makes sense to publish the bitstreams themselves as well. - changes the LiteX boards to require the "Secure" / "TockSecureIMC" VexRiscv CPU variant containing a PMP and enables PMP support in the `litex_vexriscv` chip crate "TockSecureIMC" is a custom VexRiscv CPU configuration which has support for PMP, hardware multiplication and compressed instructions. It significantly reduces code size and should improve performance. The patch to the upstream `pythondata-cpu-vexriscv` can be found [here](https://github.com/lschuermann/tock-litex/blob/master/pkgs/pythondata-cpu-vexriscv/0001-Add-TockSecureIMC-cpu-variant.patch). Prebuilt Verilog-files and complete bitstreams are published as part of the `tock-litex` repository [releases](https://github.com/lschuermann/tock-litex/releases/). ### Testing Strategy This pull request was tested by running the `mpu` test app with all test cases on both the Arty A7 35T and the Verilated simulation. ### TODO or Help Wanted I don't think it's very elegant to point to some outside repository for the generated bitstreams. It might be more trustworthy to have this repository under the Tock GitHub organization, given that we already have the _Community_ GitHub team. I'd be open to moving it, but also fine if it stays in the current location. ### Documentation Updated - [x] Updated the relevant files in `/docs`, or no updates are required. ### Formatting - [x] Ran `make prepush`. ### Acknowledgements A huge thanks to @lindemer for contributing PMP support to VexRiscv. This must have been a lot of work. I was also impressed by the PMP infrastructure in Tock. Everything worked flawlessly on the first try. Thanks to @bradjc, @alistair23 and all other contributors! 2317: ARM Cortex-m hardfault handler: remove UB and fix warnings on newer nightlies r=bradjc a=hudson-ayers ### Pull Request Overview This pull request modifies the cortex-m hardfault handler to contain only a single, no-return asm!() block, rather than mixing rust code and inline assembly, which is undefined behavior in `#[naked]` functions (see: https://internals.rust-lang.org/t/idea-naked-functions-2-0/11637/25 ). On recent nightlies, because of rust-lang/rust#79653, rustc now warns that our mixing inline assembly and rust code, as well as our use of `llvm_asm` is deprecated and will be a hard error in future versions of rust. In order to update to newer versions of the rust compiler, we will have to fix this. The primary trick here is that the only reason (I think) the hard fault handler needed to be a naked function is so that the stack pointer can be read unmodified, and so that calling the handler does not push to the stack (which could lead to an immediate double fault when the fault was a stack overflow). Therefore the only asm that needs to be in a naked function is the assembly code that: - stores the appropriate stack pointer - determines if a stack overflow occurred - updates the stack pointer in the case of a stack overflow The rest of the hard fault handler can just be placed in a non-naked function, which is called from the inline assembly in the naked function. This involves placing the parameters in the appropriate registers and then branching to the correct function. This allows for the convenience of mixing rust code and assembly in the rest of the hard fault handler. This requires changing `_estack` to be a `static` rather than a `fn()` type, but I think it was only a function type because it is passed as a function in other files (`chips/sam4l/src/lib.rs`), which I think were copied to this file. This change compiles fine and better represents the actual type of `_estack`. ### Testing Strategy This pull request was tested by: - manually inserting a stack overflow in the kernel, and observing that "kernel stack overflow" is printed along with the panic message. - manually inserting a read to an invalid memory address, and observing a normal panic message is printed. ### TODO or Help Wanted I do not fully understand the ARM calling convention -- do I need to be validating the stack pointer is double-word aligned before calling the non-naked function? Also, is my understanding correct that noreturn inline asm does not need to mark its clobbers? It seems that the newer compiler tries to prevent me from doing so. This PR does not fix all warnings surfaced in newer nightlies -- just the warnings surfaced by the hardfault handler. ### Documentation Updated - [x] No updates are required. ### Formatting - [x] Ran `make prepush`. Co-authored-by: Leon Schuermann <[email protected]> Co-authored-by: Hudson Ayers <[email protected]> Co-authored-by: Hudson Ayers <[email protected]>
|
Could someone please tell me why naked functions should be |
|
@Acciente717 That's explained here: https://www.reddit.com/r/rust/comments/kwn43k/why_must_naked_functions_not_return/gj59yqn/ But it would be better to document it officially ? |
|
Yeah, I think so. Also, it seems that I misunderstood the
Are these correct? |
steveklabnik
added a commit
to oxidecomputer/hubris
that referenced
this pull request
Oct 27, 2021
These are always inline(never), and so adding it is considered incorrect. See rust-lang/rust#79653 for more details.
steveklabnik
added a commit
to oxidecomputer/hubris
that referenced
this pull request
Oct 27, 2021
These are always inline(never), and so adding it is considered incorrect. See rust-lang/rust#79653 for more details.
steveklabnik
added a commit
to oxidecomputer/hubris
that referenced
this pull request
Oct 27, 2021
These are always inline(never), and so adding it is considered incorrect. See rust-lang/rust#79653 for more details.
steveklabnik
added a commit
to oxidecomputer/hubris
that referenced
this pull request
Oct 27, 2021
These are always inline(never), and so adding it is considered incorrect. See rust-lang/rust#79653 for more details.
tmiasko
added a commit
to tmiasko/rust
that referenced
this pull request
Jan 21, 2022
Transition unsupported naked functions future incompatibility lint into an error: * Naked functions must contain a single inline assembly block. Introduced as future incompatibility lint in 1.50 rust-lang#79653. Change into an error fixes a soundness issue described in rust-lang#32489. * Naked functions must not use any forms of inline attribute. Introduced as future incompatibility lint in 1.56 rust-lang#87652.
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Jan 22, 2022
…nctions, r=Amanieu Reject unsupported naked functions Transition unsupported naked functions future incompatibility lint into an error: * Naked functions must contain a single inline assembly block. Introduced as future incompatibility lint in 1.50 rust-lang#79653. Change into an error fixes a soundness issue described in rust-lang#32489. * Naked functions must not use any forms of inline attribute. Introduced as future incompatibility lint in 1.56 rust-lang#87652. Closes rust-lang#32490. Closes rust-lang#32489. r? `@Amanieu` `@npmccallum` `@joshtriplett`
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Jan 22, 2022
…nctions, r=Amanieu Reject unsupported naked functions Transition unsupported naked functions future incompatibility lint into an error: * Naked functions must contain a single inline assembly block. Introduced as future incompatibility lint in 1.50 rust-lang#79653. Change into an error fixes a soundness issue described in rust-lang#32489. * Naked functions must not use any forms of inline attribute. Introduced as future incompatibility lint in 1.56 rust-lang#87652. Closes rust-lang#32490. Closes rust-lang#32489. r? ``@Amanieu`` ``@npmccallum`` ``@joshtriplett``
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Jan 22, 2022
…nctions, r=Amanieu Reject unsupported naked functions Transition unsupported naked functions future incompatibility lint into an error: * Naked functions must contain a single inline assembly block. Introduced as future incompatibility lint in 1.50 rust-lang#79653. Change into an error fixes a soundness issue described in rust-lang#32489. * Naked functions must not use any forms of inline attribute. Introduced as future incompatibility lint in 1.56 rust-lang#87652. Closes rust-lang#32490. Closes rust-lang#32489. r? ```@Amanieu``` ```@npmccallum``` ```@joshtriplett```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Validate that naked functions are defined in terms of a single inline assembly
block that uses only
constandsymoperands and hasnoreturnoption.Implemented as future incompatibility lint with intention to migrate it into
hard error. When it becomes a hard error it will ensure that naked functions are
either unsafe or contain an unsafe block around the inline assembly. It will
guarantee that naked functions do not reference functions parameters (obsoleting
part of existing checks from #79411). It will limit the definitions of naked
functions to what can be reliably supported. It will also reject naked functions
implemented using legacy LLVM style assembly since it cannot satisfy those
conditions.
rust-lang/rfcs#2774
rust-lang/rfcs#2972