-
Notifications
You must be signed in to change notification settings - Fork 157
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
Swap to just-stabilised asm!() and global_asm!() macros #423
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
@@ -431,8 +444,148 @@ | |||
extern crate cortex_m_rt_macros as macros; | |||
|
|||
use core::fmt; | |||
use core::arch::global_asm; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope this clippy lint goes away in 1.59.
Looks OK to me. Very excited to see this land! |
5a1dabe
to
21837dc
Compare
21837dc
to
c39dfb3
Compare
I've fixed that uppercase BXNS, set rust-version in the Cargo.toml (except the unchanged panic-itm), bumped those Cargos to edition 2021 while we're at it, and removed outdated references to old Rust versions from cortex-m-rt docs (since we now require 1.59 anyway, and they were already outdated with 1.42). |
…low checking code that uses it on native platform.
f3a4818
to
db8bea2
Compare
What happened to the CI? |
…l asm, add inline to most cortex_m::asm methods
dcbcbef
to
3e8a5be
Compare
Whew, a few snags from the CI but all green now. |
…to depend on cortex-m 0.8
e58d223
to
ac2a836
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
bors r=thejpster |
Once Rust 1.59 is released in a couple of days, the
asm!()
andglobal_asm!()
macros will become stable, and we will no longer require the various precompiled binaries we've used until now in cortex-m and cortex-m-rt. cc #420.This PR uses
asm!()
in cortex-m, and removes the inline-asm feature, since I anticipate this going into cortex-m 0.8 and so we don't need to leave it behind for compatibility. In various places the previous version would call an extern C method when built for the native target, which would only fail at link time; to preserve the ability to build on x86 I've either made the whole method require thecortex_m
configuration, or where appropriate/convenient simply skipped theasm!()
call.This PR replaces the old gcc-preprocessed
asm.S
in cortex-m-rt with use ofglobal_asm!()
, although since you can't normally use#[cfg(...)]
attributes withglobal_asm!()
, there's also a slightly scary macro modified from one invented by @Dirbaio for a similar purpose. I considered putting the initialisation of LR behind an armv6m flag, but since we want to restore it after calling__pre_init
it seemed better to just leave it the same on both targets. I added Cargo features to optionally set SP and VTOR at startup, which has been variously requested but would previously have required multiplicatively more pre-built binaries. Now: no problem. Relevant issues:I've tested these on a couple of targets (and updated the CI): on the whole there's a small improvement in code size due to everyone getting inlined asm, especially in
cortex_m::interrupt::free()
.The major downside is we bump our MSRV from 1.42 (March 2020) to 1.59 (Feb 2022). For cortex-m, I propose putting these changes in the upcoming 0.8 release (which is technically what the master branch is already on) and not backporting. For cortex-m-rt I'm not sure: we don't have any other pending breaking changes, so we could consider a patch release. Anyway, this PR doesn't commit to any particular releases, so we can decide that later.
For cortex-m-semihosting/panic-semihosting I think a patch release would be ideal, especially since we had to yank the last c-m-sh release due to conflicting prebuilt binaries (a problem that should now vanish).
Also tagging these issues that I think might also benefit from new inline asm:
HardFault
trampoline opt-in #406