[DoNotMerge][WIP] add support for thumb* (Cortex-M) targets #36526

japaric · 2016-09-16T05:54:35Z

NOTE Do not merge. rust-lang/rfcs#1645 must be approved first.

As specified in the ("less targets" alternative of the) RFC, this PR adds these new, baseline
targets to the compiler:

thumbv6m-none-eabi: Cortex-M0, Cortex-M0+, Cortex-M1
thumbv7m-none-eabi: Cortex-M3
thumbv7em-none-eabi: Cortex-M4, Cortex-M7 -- no FPU, soft float ABI
thumbv7em-none-eabihf: Cortex-M4, Cortex-M7 -- SP FPU, hard float ABI

And tweaks the build script of compiler_builtins to support these targets as well.

With these changes it's possible to compile the following subsets of standard crates for each of
these targets:

For thumbv6m-none-eabi: compiler_builtins, core and rustc_unicode. (*)
For the other 3 targets: collections, compiler_builtins, rand and every other crate below
them.

(*) This target doesn't support atomic operations so crates like alloc, which depend on atomics
(e.g. Arc), and crates above them can't be compiled for this target.

TODOs

The corresponding RFC must be approved.
Teach rustbuild to not build the whole set of standard crates for these targets. Instead tell
it that it must compile only the smaller set of crates that each target supports.
This depends on compiler_builtins: base conditional compilation on the specification #36512. That must land first.

cc @alexcrichton @brson

rust-highfive · 2016-09-16T05:54:45Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @arielb1 (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.

of the target rather than on its triple (i.e. its name). As explained in rust-lang#35474, the current conditional compilation logic, which is based on the target triple (e.g. x86_64-unknown-linux-gnu), is not robust in the presence of custom targets as those can have arbitrary triples (e.g. cortex-m3). To fix that, this commit changes the conditional compilation logic to use the specification of the target (e.g. target_arch, target_os, etc.). For that, compiler_builtins build script now depends on the rustc-cfg crate, whose role is to shell out to `rustc --print cfg` and return its output parsed as a `struct`. With the goal of completely removing any direct dependency of the conditional compilation logic on the target triple, this commit also exposes the llvm-target field of the target specification via `rustc --print cfg`. closes rust-lang#35474

the Target <-> JSON roundtrip.

alexcrichton

Awesome, thanks @japaric! Could you also add comments to all the new target files indicating what the targets are meant to be used for as well? (e.g. justify target-features and whatnot)

alexcrichton · 2016-09-26T17:43:34Z

src/libcompiler_builtins/build.rs

+    }
+
+    if llvm_target[0] == "thumbv7m" {
+        cfg.flag("-march=armv7-m");


This logic should end up in gcc-rs, right?

alexcrichton · 2016-09-26T17:44:43Z

src/libcompiler_builtins/Cargo.toml

@@ -13,3 +13,4 @@ core = { path = "../libcore" }

 [build-dependencies]
 gcc = "0.3.27"
+rustc-cfg = { git = "https://github.com/japaric/rustc-cfg", branch = "llvm-target" }


It looks like this may be something that should be in gcc-rs, but maybe I'm missing something? Couldn't we just look at $TARGET like we do for other platforms?

This change is from #36512. Check that PR for context.

alexcrichton · 2016-09-26T17:45:14Z

src/librustc_back/target/mod.rs


 pub type TargetResult = Result<Target, String>;

 macro_rules! supported_targets {
-    ( $(($triple:expr, $module:ident)),+ ) => (
+    ( $(($triple:expr, $module:ident)),+, ) => (


You can actually move the comma inside the parens to require a trailing comma like:

( $(($triple:expr, $module:ident),)+ )

alexcrichton · 2016-09-26T17:46:40Z

src/librustc_back/target/mod.rs

+            // an impossible value: u64::max_value() because using a valid value like 0 or 8 as the
+            // default would cause the max-atomic-width field to be "lost" (it would get replaced
+            // by target_pointer_width) during the Target <-> JSON round trip
+            max_atomic_width: u64::max_value(),


Hm I'm not sure I quite understand why this is necessary, could you elaborate what you mean by "lost" in the comment?

Sure! Let me ellaborate here, first, what happens today without this change:

All targets go through a round trip from TargetOptions to JSON and back to TargetOptions.

Let's say you set max_atomic_width to the Default value, which is 0 today, for a target specification, when this specification is converted to JSON the max_atomic_width field is not present in that JSON representation.

When that JSON representation is converted back to TargetOptions, the current logic sets max_atomic_width to target_pointer_witdh, i.e. 32 or 64, because the max_atomic_width field is not present in the JSON representation.

TL;DR You set max_atomic_witdth to 0 but the compiler is actually using 32 or 64 for the target.

The bug is subtle and the proposed fix is sort of magical. Perhaps we could solve it in a better way.

alexcrichton · 2016-09-26T17:47:15Z

src/librustc_back/target/thumbv6m_none_eabi.rs

+
+        options: TargetOptions {
+            // NOTE prevents mis-optimizations of `ptr::copy_nonoverlapping` when unaligned loads
+            // are involved


Is this a FIXME or just a NOTE? (e.g. is it a bug we should fix at some point?)

After actually checking the documentation. The ARMv6-M Architecture Reference Manual says:

A3.2 Alignment Support
ARMv6-M always generates a fault when an unaligned access occurs.

So no, this can't be "fixed". The architecture doesn't support this operation so it's correct to disable it here.

alexcrichton · 2016-09-26T17:48:53Z

src/rustc/std_shim/Cargo.toml

+jemalloc = ["std/jemalloc"]
+full = ["core", "std"]
+thumb_no_atomics  = ["compiler_builtins", "core", "rustc_unicode"]
+thumb = ["core", "compiler_builtins", "collections", "rand"]


Perhaps these could be give non-thumb specific names and just mention no-atomics and atomics?

bors · 2016-09-26T21:33:30Z

☔ The latest upstream changes (presumably #36719) made this pull request unmergeable. Please resolve the merge conflicts.

japaric · 2016-09-26T22:14:52Z

@alexcrichton I don't know if you have have seen it yet. But the tools is considering only adding the target specifications and not building binary artifacts cf. rust-lang/rfcs#1645 (comment).

I'll come back to this once that's decided.

alexcrichton · 2016-09-27T20:05:21Z

Sounds good to me!

japaric · 2016-09-30T21:16:34Z

closing in favor of #36874 which only adds the target definitions

rust-highfive assigned arielb1 Sep 16, 2016

Jorge Aparicio added 6 commits September 16, 2016 17:51

add thumb* targets to the compiler

05b64c3

compiler_builtins: support the thumb* targets

40f94c0

fix a bug where max_atomic_width: 0 was lost during ...

9acc050

the Target <-> JSON roundtrip.

whoops, doc comment -> comment

0cff11c

add support for building standard crates for thumb* targets

fec8724

japaric force-pushed the thumbs-up branch from c3d743f to fec8724 Compare September 17, 2016 01:09

alexcrichton reviewed Sep 26, 2016

View reviewed changes

alexcrichton assigned alexcrichton and unassigned arielb1 Sep 26, 2016

japaric closed this Sep 30, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DoNotMerge][WIP] add support for thumb* (Cortex-M) targets #36526

[DoNotMerge][WIP] add support for thumb* (Cortex-M) targets #36526

japaric commented Sep 16, 2016 •

edited

Loading

rust-highfive commented Sep 16, 2016

alexcrichton left a comment

alexcrichton Sep 26, 2016

alexcrichton Sep 26, 2016

japaric Sep 26, 2016

alexcrichton Sep 26, 2016

alexcrichton Sep 26, 2016

japaric Sep 26, 2016

alexcrichton Sep 26, 2016

japaric Sep 28, 2016

alexcrichton Sep 26, 2016

bors commented Sep 26, 2016

japaric commented Sep 26, 2016

alexcrichton commented Sep 27, 2016

japaric commented Sep 30, 2016

[DoNotMerge][WIP] add support for thumb* (Cortex-M) targets #36526

[DoNotMerge][WIP] add support for thumb* (Cortex-M) targets #36526

Conversation

japaric commented Sep 16, 2016 • edited Loading

rust-highfive commented Sep 16, 2016

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Sep 26, 2016

japaric commented Sep 26, 2016

alexcrichton commented Sep 27, 2016

japaric commented Sep 30, 2016

japaric commented Sep 16, 2016 •

edited

Loading