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

Add target for Little-endian ARM Cortex-R4F/R5F MCUs #53663

Closed
wants to merge 3 commits into from
Closed

Add target for Little-endian ARM Cortex-R4F/R5F MCUs #53663

wants to merge 3 commits into from

Conversation

paoloteti
Copy link
Contributor

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

ENV TARGET=armv7r-none-eabihf

ENV CC_armv7r_none_eabihf=arm-none-eabi-gcc \
CFLAGS_armv7r_none_eabihf="-march=armv7-r"
Copy link
Member

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
Copy link
Member

@japaric japaric Aug 24, 2018

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.

Copy link
Member

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.

Copy link
Contributor Author

@paoloteti paoloteti Aug 24, 2018

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.

Copy link
Contributor Author

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.

Copy link
Member

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,
Copy link
Member

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)

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@japaric
Copy link
Member

japaric commented Aug 24, 2018

Thanks for the PR @paoloteti. Do you think you could also add a armv7r-none-eabi target for devices without a FPU like the RM42? if not I can add it in another PR.

@paoloteti
Copy link
Contributor Author

Do you think you could also add a armv7r-none-eabi target for devices without a FPU like the RM42? if not I can add it in another PR.

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(),

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?

Copy link
Contributor Author

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.

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(),
Copy link
Member

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

Copy link

@kenkeiter kenkeiter Aug 24, 2018

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.

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@japaric

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

Copy link
Member

Choose a reason for hiding this comment

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

@paoloteti

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)
@parched
Copy link
Contributor

parched commented Aug 24, 2018

@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 (riscv32imac-unknown-none-elf and aarch64-unknown-none).

@japaric
Copy link
Member

japaric commented Aug 24, 2018

I sent a more complete PR (adds the 3 other possible targets) that builds on top of this one: #53679

@alexcrichton
Copy link
Member

@parched currently we basically have no convention and/or policy for naming targets. It's basically "whatever makes sense" and "first mover advantage"!

bors added a commit that referenced this pull request Aug 28, 2018
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
@TimNN TimNN added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 28, 2018
@bors bors closed this in #53679 Aug 28, 2018
@paoloteti paoloteti deleted the cortex-r-le branch August 29, 2018 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants