Remove nightly-only target_feature
#3803
Merged
Conversation
This file contains hidden or 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
This was referenced Jan 18, 2024
bradjc
added
the
blocked
bradjc
force-pushed
the
remove-target_feature
branch
from
85840b6 to
de40abb
Compare
The _v7m functions were cfg gated with `target_feature="v7"`, which only works on nightly. To move to stable we need to remove these. Instead we move them to their own crate where they can be included only on v7m platforms.
Since v7m cannot compile with target=v6, we need to move the v7m asm to its own crate.
bradjc
force-pushed
the
remove-target_feature
branch
from
de40abb to
434a80e
Compare
bradjc
removed
the
blocked
Contributor
Author
|
This is rebased and ready to go. I only copied the functions from one file to another without any changes. |
lschuermann
reviewed
Feb 1, 2024
Comment on lines
+1
to
+2
| Cortex-M v7m Architecture | ||
| ========================= |
Member
There was a problem hiding this comment.
Thoughts on adding the cortex-v7m crate (copied over from Slack):
Upsides:
- It removes a nightly feature
- It's consequential, given that we have a common cortex-m crate and a cortex-m0 crate, which contains v6m assembly already
- (I'm biased, but) I think the cortexm::CortexMVariant encapsulates these arch differences very nicely across crates, without involving guesswork as to what the linker's gonna stick together
Downsides:
- Yet another crate
- Increases compilation time slightly
I'm in favor, esp. given we want to compile on stable.
lschuermann
approved these changes
Feb 1, 2024
Member
lschuermann
left a comment
lschuermann
left a comment
There was a problem hiding this comment.
I trust that the assembly has been copied over verbatim and doesn't warrant another review. 😄
ppannuto
approved these changes
Feb 2, 2024
Member
ppannuto
left a comment
ppannuto
left a comment
There was a problem hiding this comment.
This is a good change.
Really, we should get rid of all of the arch/cortex-*; they are not ARM arch's.. v6m, v7m, v7me, etc are. I strongly suspect blame for this falls to me however, for not really understanding things that well in 2015 :/.
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.
Pull Request Overview
Blocked on #3798Part of #1654.
We use
in the cortex-m crate so we can include v7m-specific code in a crate that is also used by v6 arch crates. Using cfgs like this is normally not something we would do, but, since the cfgs are set by the toolchain and not manually by us in tock repo files, it was ok.
The issue is is that
target_feature = "v7"is only set on nightly and not stable. So doing this actually added another nightly only feature although we were not either aware of this or we did not write it down.This removes using
target_feature = "v7"by create a new crate for v7m that only v7m platforms will include.Testing Strategy
travis
TODO or Help Wanted
n/a
This incorporates the changes from #3798 assuming that will be merged as is. Moving code between files is a pain.
Documentation Updated
/docs, or no updates are required.Formatting
make prepush.