-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
targets: thumbv8m: Add target for baseline ARMv8-M #55041
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
src/librustc_target/spec/mod.rs
Outdated
@@ -397,6 +397,7 @@ supported_targets! { | |||
("thumbv7m-none-eabi", thumbv7m_none_eabi), | |||
("thumbv7em-none-eabi", thumbv7em_none_eabi), | |||
("thumbv7em-none-eabihf", thumbv7em_none_eabihf), | |||
("thumbv8m-none-eabi", thumbv8m_none_eabi), |
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.
thumbv8m.base-none-eabi
would be better to distinguish from a thumbv8m.main
target in the future.
target_vendor: String::new(), | ||
linker_flavor: LinkerFlavor::Lld(LldFlavor::Ld), | ||
|
||
options: TargetOptions { |
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.
The baseline architecture doesn't support unaligned access, so features: "+strict-align".to_string(),
will need to be added here as I believe it's not enabled by default.
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.
So, it looks like the "Main Extension" from 8-M was a baseline in 7-M? And this newly added target (used for Cortex-M23) is not really a baseline but mainline?
Any other notable changes in 8-M affecting codegen? The "changelog" in ARM ARM doesn't tell much to me.
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.
Aha! Good catch @parched, I will adjust.
I took a look at this description of changes here: https://community.arm.com/processors/trustzone-for-armv8-m/b/blog/posts/five-key-features-of-the-arm-cortex-m23-processor?CommentId=f95326ae-7e58-4829-9874-a1ebd4a141e9
I think I may have atomic instruction support incorrectly configured. Per the above blog post:
In addition, to provide atomic support for C11/C++11, the load-acquire and store-release instructions are included from ARMv8-A
There is no mention of atomic CAS support, I think that I may need to set atomic_cas: false
while keeping max_atomic_width: Some(32),
. https://reviews.llvm.org/D15283?id=42050 also seems to hint at this. Does this seem correct?
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 think I may have atomic instruction support incorrectly configured.
No, you have it correct already :).
ARMv8-M Baseline is a superset of ARMv6-M, containing all ARMv6-M instructions plus ARMv8-A semaphores and atomics, ARMv7-M exclusives
This is what gives it CAS support.
LGTM beside figuring out this baseline vs mainline distinction new to ARMv8-M. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Target stuff LGTM. While testing this I noticed that Cargo puts the artifacts in Before landing this I'd like to check with the @rust-lang/cargo team whether Cargo can be modified to support dots in target names. Or if we should choose a target name without a dot in it. |
Would it be possible to not use a dot in the target name? It deviates a bit from our naming of other targets, and yeah I think Cargo assumes that anything with a |
Yes it would, but that would be deviating from the target clang uses and what |
Perhaps! I'd be fine with either fix |
@alexcrichton can you point me to where this happens in Cargo? happy to open up a PR - I like the idea of matching the clang target name |
I think it's around here |
Ping from triage @evq: What is the status of this PR? |
@evq pinging from triage. Thanks for the PR. Unfortunately we haven't heard from you on this in a while, so I'm closing the PR to keep things tidy. Don't worry though, if you'll have time again in the future please reopen this PR, we'll be happy to review it again! |
@Dylan-DPC I think this is still waiting on rust-lang/cargo#6255 (similar to #56000 ) |
Reopening as blocked. |
@caemor thanks. will reopen it. |
Tweak Layout to allow for non json file targets with internal "." Per @alexcrichton 's comment in rust-lang/rust#55041 (comment), currently cargo assumes that a target with an internal `.` is a custom json target spec, using the file stem for the build directory name. This PR changes cargo to only use the file stem for files with a `json` extension.
The cargo PR has landed. |
I’m not sure there is anything blocking this PR from landing except perhaps that the cargo submodule might need updating? I’ve never seen cargo submodule being updated in a regular PR before, so I don’t think it is necessary, but I might be wrong… |
Ok with Cargo fixes merged I'm gonna go ahead and r+ this. While the "usability interface" through Cargo for this target relies on the Cargo fix merging, this change itself technically does not. As a result should be fine for them to land in parallel! Thanks for the patience @evq! |
@bors: r+ |
📌 Commit 8a0666d has been approved by |
targets: thumbv8m: Add target for baseline ARMv8-M Tested against a SAM L11 MCU.
☀️ Test successful - status-appveyor, status-travis |
Add Armv8-M Mainline targets This commit enables the Armv8-M Mainline architecture profile. It adds two targets: - `thumbv8m.main-none-eabi` - `thumbv8m.main-none-eabihf` The second one uses the Floating Point Unit for floating point operations. It mainly targets the Cortex-M33 processor, which can have the optional Floating Point Unit extension. It follows rust-lang#55041 which does it for Baseline. I will rebase this branch on top of it when it is merged to not create conflicts as we have some files in common. To make it work, it still relies on the Cargo change to be merged (accepting "." in target names, rust-lang/cargo#6255). The goal would also be to add this target in the CI so that the `core` library is available for everybody. To do this, some changes will be needed to compile successfully the needed libraries: * `cc-rs` needs to be updated to allow compiling C code for Armv8-M architectures profiles. It is only a few lines to add [here](https://github.com/alexcrichton/cc-rs/blob/a76611ad9836fa8c44fa8220a1d2a96dd3b7d4b6/src/lib.rs#L1299). * Some assembly files in `builtins` in `compiler-rt` were not assembling for Armv8-M Mainline. I sent changes [upstream](https://reviews.llvm.org/D51854) to that project to fix that. The Rust version of `compiler-rt` will have to be updated to contain [that commit](llvm-mirror/compiler-rt@a34cdf8). I tested it using the [Musca-A Test Chip board](https://developer.arm.com/products/system-design/development-boards/iot-test-chips-and-boards/musca-a-test-chip-board) but more intensively on the [Armv8-M FVP](https://developer.arm.com/products/system-design/fixed-virtual-platforms) (emulation platform). I am going to try to release my test code soon, once I tidy it up 👍
I was able to use the Any way to get this built on stable? (Edit: It looks like they may have just semi-recently landed on nightly? In which case I hope they'll show up on stable soon) |
I was able to get the blinky example to build and run on a SAML11 Xplained board with these changes. Looks like you were able to upstream the `thumbv8m.base-none-eabi` target definition: rust-lang/rust#55041 I was able to use that via rustup on a recent nightly, so no more need for (patched) xargo. I went ahead and bumped all of the crates to the latest releases as I wasn't able to get anything to compile without doing so. All that said, I was able to blink an LED on a SAML11 Xplained.
I don't know much either how the promotion from |
Tested against a SAM L11 MCU.