-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Add target for Little-endian ARM Cortex-R4F/R5F MCUs #53663
Conversation
ENV TARGET=armv7r-none-eabihf | ||
|
||
ENV CC_armv7r_none_eabihf=arm-none-eabi-gcc \ | ||
CFLAGS_armv7r_none_eabihf="-march=armv7-r" |
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.
Instead of hardcoding cflags in the docker image let's add proper support to the cc crate. I have sent PR rust-lang/cc-rs#339 for this.
|
||
# GNU Arm Embedded Toolchain 7-2018-q2-update (June 27,2018) | ||
ENV BASE_URL=https://developer.arm.com/-/media/Files/downloads/gnu-rm/ | ||
RUN curl -L $BASE_URL/7-2018q2/gcc-arm-none-eabi-7-2018-q2-update-linux.tar.bz2 | tar -xj |
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.
Any reason we should use this specific GCC version rather than the one packaged by Ubuntu? Are there known codegen bugs?
Because if the Ubuntu version is enough we can build this target in the dist-various-1 image where all the Cortex-M targets are built.
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.
Another reason to avoid these toolchains is that these links are not permanent. For example the other docker image you added for the armebv7r target no longer builds because this URL no longer exists.
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.
Any reason we should use this specific GCC version
Do you mean the default Ubuntu 16.04 package or the one in the PPA channel (ppa:team-gcc-arm-embedded/ppa)?
I use 7-2018-q2-update mainly for Armv8r, BTW I can use Ubuntu package here.
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.
Another reason to avoid these toolchains is that these links are not permanent. For example the other docker image you added for the armebv7r target no longer builds because this URL no longer exists.
Here is "mandatory" because it is an armeb toolchain. I'll update the link. The issue here is that use 'latest' folder instead of '7.2.1' and latest now is 7.3.x.
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.
Why is it mandatory? The arm-none-eabi-gcc
toolchain can also generate big endian code and the toolchain is used by the build system only to compile C code from source -- it doesn't link to binaries provides by the toolchain (like startup object or libc.a).
Furthermore this toolchain doesn't contain pre-compiled binaries for the v7-R architecture (or the M profile):
$ arm-none-eabi-readelf -A $(find -name crti.o) | grep _profile
Tag_CPU_arch_profile: Application
Tag_CPU_arch_profile: Application
Tag_CPU_arch_profile: Application
Tag_CPU_arch_profile: Application
Tag_CPU_arch_profile: Application
(this repeats 20+ more times)
target_os: "none".to_string(), | ||
target_env: "".to_string(), | ||
target_vendor: "".to_string(), | ||
linker_flavor: LinkerFlavor::Gcc, |
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.
Do you know if LLD supports linking Cortex-R object files? Could we use rust-lld
here instead of leaving the linker undefined? (which, IIRC, will default to cc
and will fail to link unless overridden)
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 don't know if LLD support Cortex-R, I'll try.
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.
Do you know if LLD supports linking Cortex-R object files
I confirm that it works for Cortex-R too. I'll wait next cc-rs
to update everything.
Thanks for the PR @paoloteti. Do you think you could also add a |
I'll try to find spare time during the week-end, otherwise feel free to add it and add me in cc. (both LE and BE). |
executables: true, | ||
relocation_model: "static".to_string(), | ||
panic_strategy: PanicStrategy::Abort, | ||
features: "+vfp3,+d16,+fp-only-sp".to_string(), |
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.
@paoloteti I was working on a PR for this already (although I'm more than happy to defer to yours!). The set of features I pulled from the LLVM .ts
are a bit different than yours (see this gist) – any thoughts?
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.
There are redundant features in your spec: for example v7 is superfluous because part of llvm_target (the v7 in the triple name), the same for v7r, also thumb2 is already part of +v7. +dsp and +hwdiv are superfluous too.
BTW the idea is to provide only a set of minimal features for a give target and to override/extend features at command line.
I found an RM4x board in a box in my lab. I did some test and I'm confident enough.
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.
Ahh, okay – wonderful. I had a feeling that you were trying to maintain a minimum subset to address the largest number of MCUs, but I wasn't aware you could apply additional features using the CLI. In that case, the set you specified here obviously looks sufficient.
executables: true, | ||
relocation_model: "static".to_string(), | ||
panic_strategy: PanicStrategy::Abort, | ||
features: "+vfp3,+d16,+fp-only-sp".to_string(), |
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 armebv7r-none-eabihf
target has a +v7
here. Why does that target need it and this one doesn't? I would expect LLVM to enable the v7
feature if the llvm-triple
includes the word arm*v7
in it.
Semi on-topic: anyone knows how to make llc, rustc, or in general LLVM, output all the enabled target features for a particular triple? Something like llc -triple=armebv7r-none-eabihf --print enabled-target-features
(which of course that doesn't work) that prints +v7,+rclass,+dsp,..
. I know that llc -mattr=help -march=arm
prints all the target features that makes sense for the ARM architecture but I want to know which ones are enabled. rustc --print cfg
almost does what I want but doesn't include target features that have not been whitelisted. cc @parched @nagisa
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.
During the development process, I pulled the list of features from here when I thought the fault that I was encountering was unrelated to the issue that rust-lang/cc-rs#339 addresses. That being said, L422-425 of that file actually define the [sub]features that enabling that feature enables. I was a little verbose (as mentioned) and chose to be very explicit about enabling the feature groups AND individual features, because I wasn't entirely sure where Rust was pulling its version of LLVM from at that time. There are, however, specific features for the ARMv7r processors, as well as for the Cortex R5, specifically.
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.
There is a way to obtain current features if LLVM is built with debug assertions through -debug
flag to the LLVM tools.
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 armebv7r-none-eabihf target has a +v7 here. Why does that target need it and this one doesn't? I would expect LLVM to enable the v7 feature if the llvm-triple includes the word arm*v7 in it.
Yes right, it is correct here and just redundant in armebv7r-none-eabihf
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.
Yes right, it is correct here and just redundant in armebv7r-none-eabihf
Let's be consistent and either include in all the Cortex-R targets, or not include it anywhere. Otherwise it's confusing.
Similar to `armebv7r-none-eabihf`, but for Little-endian MCUs. As example TI RM4x/RM5x are Little-endian Cortex-R4F/R5F MCUs. CI/Dockerfile is intentionally in the disabled folder.
Change linker and linker_flavour to use LLD
- use gcc-arm-none-eabi from Ubuntu repository - remove hard-coded CC flags (require cc-rs v1.0.21)
@paoloteti Looks good to me, I just have more general question, probably for @alexcrichton? What's the Rust policy for putting the vendor in the triple? Most target targets have it, but the bare metal ones don't, although the last two bare metal ones do ( |
I sent a more complete PR (adds the 3 other possible targets) that builds on top of this one: #53679 |
@parched currently we basically have no convention and/or policy for naming targets. It's basically "whatever makes sense" and "first mover advantage"! |
add more Cortex-R targets This expands on PR #53663 to complete the set of Cortex-R targets and builds rust-std components for them. r? @alexcrichton each extra rust-std component (there's 4 of them) takes about 3 minutes to build on my local machine. In terms of stability (LLVM codegen bugs) these new targets should be as stable as the Cortex-M ones (e.g. `thumbv7m-none-eabi`). If the extra build time is too much we can leave the rust-std components out for now closes #53663 cc @paoloteti
Similar to
armebv7r-none-eabihf
, but for Little-endian MCUs.As example: TI RM4x/RM5x are Little-endian Cortex-R4F/R5F MCUs.
CI/Dockerfile is intentionally in the disabled folder.
r? @parched
r? @alexcrichton
cc @japaric