add an opt-in cargo feature to build intrinsics from compiler-rt source #74

japaric · 2016-09-26T21:04:05Z

closes #63
cc #66

This uses the build.rs version from rust-lang/rust#36512 minus the dependency on llvm-target being in rustc --print cfg

cc @alexcrichton I forgot to ask before but how do you feel about using the rustc-cfg crate, which parses the output of rustc --print cfg and exposes the target specification as a Rust struct, in the build.rs? Its functionality overlaps with what you proposed in rust-lang/rfcs#1721 i.e. CARGO_CFG_TARGET_OS, etc.

TODO stop compiling the C version of the intrinsics that we have already ported to Rust.

japaric · 2016-09-26T21:04:15Z

@homunkulus try

homunkulus · 2016-09-26T21:04:19Z

⌛ Trying commit 310aaa9 with merge 310aaa9...

homunkulus · 2016-09-26T21:34:21Z

💔 Test failed - status-travis

alexcrichton · 2016-09-27T00:43:30Z

@japaric I'd personally prefer to keep the dependencies of the rust-lang/rust repo as slim as possible, and in that sense calling rustc --print cfg manually in theory shouldn't be too hard (including parsing). Otherwise using it to inform codegen seems good to me!

japaric · 2016-09-27T00:46:38Z

I'd personally prefer to keep the dependencies of the rust-lang/rust repo as slim as possible

does that include dev dependencies? This repo uses quickcheck for its tests.

japaric · 2016-09-27T00:54:32Z

Also, rustc-cfg has zero dependencies 😉.

japaric · 2016-09-27T01:45:34Z

@homunkulus try

homunkulus · 2016-09-27T01:45:37Z

⌛ Trying commit 82bf52c with merge 82bf52c...

homunkulus · 2016-09-27T02:06:28Z

💔 Test failed - status-travis

japaric · 2016-09-27T02:23:17Z

@alexcrichton having that intrinsics.rs test in this repository has helped me a lot in debugging issues with missing/undefined symbols. Thanks to it I just learned that the thumbv6m-none-eabi target doesn't support some floating point operations because the related compiler-rt intrinsics, which are written in assembly, can't be "compiled" for that target. (cf #75)

I'm actually wondering if it's actually necessary to also run the intrinsics.rs test on the rust-lang/rust repo. Seems like all intrinsics related changes would have to first go through this repository and the CI + intrinsics.rs in this repo should catch any problem as long as we are testing all tier 1 targets (I think I'm only missing android atm) and the intrinsics.rs file covers all the relevant operations (right now, it covers ~70 intrinsics).

japaric · 2016-09-27T02:23:41Z

@homunkulus try

homunkulus · 2016-09-27T02:23:43Z

⌛ Trying commit ee05eb4 with merge ee05eb4...

homunkulus · 2016-09-27T02:44:50Z

💔 Test failed - status-travis

japaric · 2016-09-27T03:01:45Z

@homunkulus try

OK. This should be ready to go now. Who wants to review? @Amanieu @mattico

The build.rs addition is pretty much a copy paste of the one from rust-lang/rust#36512 with additional changes to support the thumb* targets. I should have done that part in two commits so the additional changes were more obvious, sorry; but all the additions should have a "thumb" string near to it :-).

homunkulus · 2016-09-27T03:01:48Z

⌛ Trying commit 7d49d99 with merge 7d49d99...

mattico · 2016-09-27T03:09:03Z

I'll take a look.

homunkulus · 2016-09-27T03:11:19Z

💔 Test failed - status-appveyor

mattico · 2016-09-27T03:16:52Z

Looks like we ran into the gist rate limit on the CI, should be pretty easy to fix.

japaric · 2016-09-27T03:24:07Z

Oh right, I have to update the AppVeyor CI to test these changes. I'll probably have to use ugh powershell so I'm going to leave that for a follow-up PR.

mattico

Just some thoughts about link args for the intrinsics link test. Everything else looks good to me.

mattico · 2016-09-27T03:46:10Z

ci/script.sh

+    $CARGO clean
+
+    # verify we haven't drop any intrinsic/symbol
+    $CARGO build --features c --target $TARGET --bin intrinsics


Would it make sense to have RUSTFLAGS="-C link-args='-Wl,-nostartfiles -Wl,-nodefaultlibs'" so that it applies to all targets? I'm assuming that we don't need libc anywhere in there...

Only -nostartfiles is needed on Linux. I think that would work on Linux, but idk about OSX and Windows. I'm using the libc crate because, on each platform, it links to different libraries needed to produce an executable. Without it the executable may not link at all.

Sounds good 👍. In any case it doesn't make much of a difference.

mattico · 2016-09-27T03:49:56Z

thumbv6m-none-eabi.json

    "features": "+strict-align",
    "llvm-target": "thumbv6m-none-eabi",
    "max-atomic-width": 0,
    "os": "none",
+    "pre-link-args": ["-nostartfiles"],


If so, we wouldn't need these anymore.

closes #63 cc #66

japaric · 2016-09-29T23:54:57Z

@homunkulus try

This should be ready to go.

@alexcrichton Mind taking a look at the changes in the testing infra? Also, are you OK with adding rust-cfg here or would you prefer to just use $TARGET and .contains()?

japaric · 2016-09-29T23:55:17Z

@homunkulus try

homunkulus · 2016-09-29T23:55:19Z

⌛ Trying commit b5797dc with merge b5797dc...

japaric · 2016-09-30T00:21:40Z

@homunkulus try

homunkulus · 2016-09-30T00:21:43Z

⌛ Trying commit 62258b5 with merge 62258b5...

alexcrichton · 2016-09-30T00:36:21Z

build.rs


 fn main() {
-    if env::var("TARGET").unwrap().ends_with("gnueabihf") {
+    println!("cargo:rerun-if-changed=build.rs");


If this is compiling a bunch of C files then this'll probably also want to print a similar key for those files as well.

gcc-rs doesn't generate those for me?

alexcrichton · 2016-09-30T00:37:13Z

build.rs

+        // NOTE Most of the ARM intrinsics are written in assembly. Tell gcc which arch we are going to
+        // target to make sure that the assembly implementations really work for the target. If the
+        // implementation is not valid for the arch, then gcc will error when compiling it.
+        if llvm_target[0].starts_with("thumb") {


Could this logic be added to gcc-rs? (it'll be needed for any C code, right?)

If it's fine to add logic for targets that are not built into the compiler then I can send a PR.

Ah yeah that's true, but it's looking like we'll add these soon so I wouldn't mind adding them.

alexcrichton · 2016-09-30T00:38:28Z

ci/run.sh

+    thumb*)
+        PREFIX=arm-none-eabi-
+        ;;
+    *-unknown-linux-gnu | *-apple-darwin)


this'll catch mips/mipsel/powerpc, right? (is that intended?)

Ah no. I meant to only pick up x86-ish targets. I'll fix it.

alexcrichton · 2016-09-30T00:39:50Z

Mind taking a look at the changes in the testing infra?

Sure!

Also, are you OK with adding rust-cfg here or would you prefer to just use $TARGET and .contains()?

I think we'll end up adding this no matter what, so seems good to me!

japaric · 2016-09-30T00:50:16Z

@alexcrichton I've addressed all your comments I think.

japaric · 2016-09-30T00:51:14Z

Minus the gcc-rs bit because that will take time to land and we can always remove the gcc flags for thumb* later on

alexcrichton · 2016-09-30T01:42:29Z

ci/run.sh

+        xargo build --features c --target $1 --bin intrinsics
+        ;;
+    *)
+        cargo build --features c --target $1 --bin intrinsics


Perhaps this build could also be tested as well?

cargo test --features c would end testing the same thing I think. We'll just be testing less intrinsics on some targets. Also, this would require sprinkling more cfg over the test modules.

Ah, carry on then!

alexcrichton · 2016-09-30T01:44:23Z

src/bin/intrinsics.rs

+// convention for its intrinsics that's different from other architectures; that's why some function
+// have an additional comment: the function name is the ARM name for the intrinsic and the comment
+// in the non-ARM name for the intrinsic.
+#[cfg(feature = "c")]


Some of these are defined even when feature=c is disabled, right?

Want to test those in the cfg(not(feature = "c")) main?

In theory yeah, but it's fine to land this ahead of time.

alexcrichton · 2016-09-30T01:46:10Z

Looking good to me!

The #[cfg(not(all(feature = "c", ..)))] declarations look like they may get a bit unwieldy though in terms of management, but perhaps that can just be dealt with as it happens.

japaric · 2016-09-30T01:50:25Z

The #[cfg(not(all(feature = "c", ..)))] declarations look like they may get a bit unwieldy though in terms of management,

Let's get rid of the C code fast so we can soon get rid of all these cfgs and the C feature as well!

alexcrichton · 2016-09-30T01:58:06Z

Let's get rid of the C code fast so we can soon get rid of all these cfgs and the C feature as well!

Agreed!

japaric · 2016-09-30T02:01:41Z

@homunkulus r+

Alright, let's land this. Then I can try landing this in rust-lang/rust, which is probably going to be awful

homunkulus · 2016-09-30T02:01:41Z

📌 Commit 61484ac has been approved by japaric

homunkulus · 2016-09-30T02:01:43Z

⌛ Testing commit 61484ac with merge 61484ac...

homunkulus · 2016-09-30T02:08:27Z

💔 Test failed - status-travis

japaric · 2016-09-30T02:23:02Z

Did https://static.rust-lang.org/rustup.sh break? All the language: rust builds are failing.

alexcrichton · 2016-09-30T02:52:43Z

I think? But claimed to be fixed.

Maybe it's not though?

japaric · 2016-09-30T17:46:17Z

@homunkulus retry

homunkulus · 2016-09-30T17:46:18Z

⌛ Testing commit 61484ac with merge 61484ac...

japaric · 2016-09-30T18:58:03Z

@homunkulus r+

homunkulus · 2016-09-30T18:58:03Z

📌 Commit cab88e6 has been approved by japaric

homunkulus · 2016-09-30T18:58:09Z

⌛ Testing commit cab88e6 with merge cab88e6...

homunkulus · 2016-09-30T19:23:42Z

☀️ Test successful - status-appveyor, status-travis
Approved by: japaric
Pushing cab88e6 to master...

74: Implement roundf r=japaric a=P1n3appl3 closes rust-lang#32 Co-authored-by: Joseph Ryan <[email protected]>

mattico approved these changes Sep 27, 2016

View reviewed changes

mattico mentioned this pull request Sep 27, 2016

Add float quickcheck implementations (fixes #45) #48

Merged

Jorge Aparicio added 2 commits September 29, 2016 16:06

add an opt-in cargo feature to build intrinsics from compiler-rt source

f77ca91

closes #63 cc #66

adapt to new testing infrastructure

b5797dc

japaric force-pushed the c branch from 7d49d99 to b5797dc Compare September 29, 2016 23:51

ignore duplicated get_pc_thunk symbols

62258b5

alexcrichton reviewed Sep 30, 2016

View reviewed changes

build.rs: print rerun-if-change for all the C/asm sources

7280206

alexcrichton reviewed Sep 30, 2016

View reviewed changes

alexcrichton mentioned this pull request Sep 30, 2016

Trim the list of fallbacks in compiler-rt #78

Closed

only x86-ish targets can use an unprefixed nm

cab88e6

japaric force-pushed the c branch from 61484ac to cab88e6 Compare September 30, 2016 18:57

homunkulus merged commit cab88e6 into master Sep 30, 2016

japaric deleted the c branch September 30, 2016 20:44

tgross35 pushed a commit to tgross35/compiler-builtins that referenced this pull request Feb 23, 2025

Merge rust-lang#74

7379ef8

74: Implement roundf r=japaric a=P1n3appl3 closes rust-lang#32 Co-authored-by: Joseph Ryan <[email protected]>

add an opt-in cargo feature to build intrinsics from compiler-rt source #74

add an opt-in cargo feature to build intrinsics from compiler-rt source #74

Conversation

japaric commented Sep 26, 2016

japaric commented Sep 26, 2016

homunkulus commented Sep 26, 2016

homunkulus commented Sep 26, 2016

alexcrichton commented Sep 27, 2016

japaric commented Sep 27, 2016

japaric commented Sep 27, 2016

japaric commented Sep 27, 2016

homunkulus commented Sep 27, 2016

homunkulus commented Sep 27, 2016

japaric commented Sep 27, 2016

japaric commented Sep 27, 2016

homunkulus commented Sep 27, 2016

homunkulus commented Sep 27, 2016

japaric commented Sep 27, 2016

homunkulus commented Sep 27, 2016

mattico commented Sep 27, 2016

homunkulus commented Sep 27, 2016

mattico commented Sep 27, 2016

japaric commented Sep 27, 2016

mattico 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

japaric commented Sep 29, 2016

japaric commented Sep 29, 2016

homunkulus commented Sep 29, 2016

japaric commented Sep 30, 2016

homunkulus commented Sep 30, 2016

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

alexcrichton commented Sep 30, 2016

japaric commented Sep 30, 2016

japaric commented Sep 30, 2016

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

alexcrichton commented Sep 30, 2016

japaric commented Sep 30, 2016

alexcrichton commented Sep 30, 2016

japaric commented Sep 30, 2016

homunkulus commented Sep 30, 2016

homunkulus commented Sep 30, 2016

homunkulus commented Sep 30, 2016

japaric commented Sep 30, 2016

alexcrichton commented Sep 30, 2016

japaric commented Sep 30, 2016

homunkulus commented Sep 30, 2016

japaric commented Sep 30, 2016

homunkulus commented Sep 30, 2016

homunkulus commented Sep 30, 2016

homunkulus commented Sep 30, 2016