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

Swap to just-stabilised asm!() and global_asm!() macros #423

Merged
merged 12 commits into from
Mar 1, 2022

Conversation

adamgreig
Copy link
Member

Once Rust 1.59 is released in a couple of days, the asm!() and global_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 the cortex_m configuration, or where appropriate/convenient simply skipped the asm!() call.

This PR replaces the old gcc-preprocessed asm.S in cortex-m-rt with use of global_asm!(), although since you can't normally use #[cfg(...)] attributes with global_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:

@rust-highfive
Copy link

r? @thalesfragoso

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cortex-m labels Feb 22, 2022
@@ -431,8 +444,148 @@
extern crate cortex_m_rt_macros as macros;

use core::fmt;
use core::arch::global_asm;
Copy link
Contributor

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.

src/asm.rs Outdated Show resolved Hide resolved
@thejpster
Copy link
Contributor

Looks OK to me. Very excited to see this land!

@adamgreig
Copy link
Member Author

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).

@adamgreig adamgreig marked this pull request as ready for review February 24, 2022 17:17
@adamgreig adamgreig requested a review from a team as a code owner February 24, 2022 17:17
…low checking code that uses it on native platform.
@thejpster
Copy link
Contributor

What happened to the CI?

Cargo.toml Show resolved Hide resolved
@adamgreig
Copy link
Member Author

Whew, a few snags from the CI but all green now.

Copy link
Contributor

@therealprof therealprof left a comment

Choose a reason for hiding this comment

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

LGTM

@adamgreig
Copy link
Member Author

bors r=thejpster

@bors
Copy link
Contributor

bors bot commented Mar 1, 2022

Build succeeded:

@bors bors bot merged commit e6c7249 into master Mar 1, 2022
@bors bors bot deleted the stable-inline-asm branch March 1, 2022 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cortex-m
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants