Prep for and expand test infrastructure #76

alexcrichton · 2016-09-27T08:06:24Z

This commit implements a few key features:

All tests are now executed against compiler-rt in addition to gcc_s. This is a reference implementation that will be guaranteed to exist so we can ensure that tests always run.
All test infrastructure now lives on the master branch of this repository, allowing it to be version controlled just as the code is. This has the benefits of being able to easily add a new platform, modify an existing platform, or reproduce what a platform is doing locally.

I've gotten a few tests to pass locally, but I suspect this isn't quite ready for merging yet. I'll try to fixup anything in the morning, but figured I'd get some thoughts!

alexcrichton · 2016-09-27T08:07:15Z

src/qc.rs

+// If anything returns `None` then the test is discarded, otherwise the two
+// results are compared for equality and the test fails if this equality check
+// fails.
+macro_rules! check {


This was pretty heinous to write, so I can try to document it a bit more if you'd like or tweak the interface. It was just the first thing I came up with!

Criminally beatiful.

This is pretty much what I had in mind for #72. From the looks of things I'm glad I didn't have to write it 😆.

@alexcrichton do you have any thoughts about benchmarks? I'm assuming the starting performance target is "not significantly worse", which shouldn't be too bad since most of the code is pretty direct translation. I still think some basic benchmarks would be a good sanity check just to ward off possible regressions. I don't know how big the overhead is for dynamic function lookup but we could do that for rustc-builtins as well to make it more 'fair', I suppose.

@mattico that's a good idea! I suspect that this'd be relatively easy to tweak to run benchmarks as well.

alexcrichton · 2016-09-27T08:07:39Z

tests/inspect.rs

+}
+
+#[test]
+fn inspect() {


I'm not sure whether it'd still be worth keeping this around, but curious for your thoughts!

alexcrichton · 2016-09-27T08:08:42Z

compiler-rt/compiler-rt-cdylib/src/lib.rs

+    }
+}
+
+declare!(___ashldi3, __ashldi3);


Reexporting symbols from a native library in a cdylib ended up being pretty tricky, so I opted for this indirect accessor in lieu of other random linker hacks

alexcrichton · 2016-09-27T08:09:10Z

ci/script.sh

-    if [[ $WEAK ]]; then
-        local symbols=( memcmp memcpy memmove memset )
-        for symbol in "${symbols[@]}"; do
-            $PREFIX$NM target/$TARGET/debug/deps/librlibc-*.rlib | grep -q "W $symbol"


I haven't re-added this test, but I figure that it's something that should be tested in rlibc rather than here.

Yeah, removing this is fine.

japaric

I like the new testing infra. It looks much simpler now and we have extra testing against compiler-rt.

Overall 👍 from me but I left some comments.

japaric · 2016-09-27T16:35:58Z

.travis.yml

-      os: linux
+      install: cargo install xargo
+      script: xargo build --target $TARGET
+    - env: target $TARGET


this looks wrong, shouldn't it be TARGET = thumbv6m-none-eabi? Same with other the thumb targets.

japaric · 2016-09-27T16:40:20Z

ci/script.sh

-    if [[ $WEAK ]]; then
-        local symbols=( memcmp memcpy memmove memset )
-        for symbol in "${symbols[@]}"; do
-            $PREFIX$NM target/$TARGET/debug/deps/librlibc-*.rlib | grep -q "W $symbol"


Yeah, removing this is fine.

japaric · 2016-09-27T16:42:19Z

.gitmodules

@@ -0,0 +1,3 @@
+[submodule "compiler-rt/compiler-rt-cdylib/compiler-rt"]
+	path = compiler-rt/compiler-rt-cdylib/compiler-rt
+	url = https://github.com/llvm-mirror/compiler-rt


llvm-mirror's and not rust-lang's compiler-rt? Are all of our patch just Makefiles related and it's OK to drop them?

Ah yeah all our "patches" now are just makefile related I think. I have a feeling getting all tests working here will cause problems though!

japaric · 2016-09-27T17:02:57Z

tests/inspect.rs

+}
+
+#[test]
+fn inspect() {


The nm output in this form is not too important so I'd be OK with dropping this test in this PR.

japaric · 2016-09-27T17:05:16Z

src/qc.rs

+// If anything returns `None` then the test is discarded, otherwise the two
+// results are compared for equality and the test fails if this equality check
+// fails.
+macro_rules! check {


Criminally beatiful.

japaric · 2016-09-27T17:06:19Z

src/qc.rs

+                            // If it's not in libgcc, or we couldn't find
+                            // libgcc, then just ignore this. We should have
+                            // tests through compiler-rt in any case
+                            None => return TestResult::passed(),


could the logic be changed to avoid calling quickcheck::quickcheck at all if the gcc_s symbol is not available?

japaric · 2016-09-27T17:10:46Z

ci/docker/powerpc64-unknown-linux-gnu/Dockerfile

+        qemu-system-ppc
+
+ENV CARGO_TARGET_POWERPC64_UNKNOWN_LINUX_GNU_LINKER=powerpc64-linux-gnu-gcc \
+    CC=powerpc64-linux-gnu-gcc \


Could we use the suffixed version (CC_powerpc64_...) here? My understanding is that CC also overrides the host compiler and, although we aren't using it here, I think it's better to avoid surprises in the future.

Some with the other Dockerfiles.

japaric · 2016-09-27T17:14:06Z

.travis.yml

-      os: linux
-    - env: TARGET=thumbv7em-none-eabihf WEAK=true
-      os: linux
+      install: cargo install xargo


Installing Xargo takes a while. May be worth to (a) set cache: cargo in .travis.yml or (b) use one of the binary releases available via GitHub releases if it slows down the build job considerable.

Also cargo install Xargo requires a few extra packages: libcurl-dev libssl-dev cmake and maybe other stuff. If this command fails, then you can try installing those dependencies.

alexcrichton · 2016-09-28T04:25:52Z

Hm ok, down to a few failures:

i686 Linux/OSX - This is Regression in cross-compiling from 64 -> 32 bit rust#36793
mips-unknown-linux-gnu - SIGTRAP if binary is executed manually, but succeeds if binary is executed with qemu-mips{,el}. Not sure why
powerpc64-unknown-linux-gnu - not sure what's going on here

The "use qemu to emulate binaries we exec" support seems pretty sketchy. It's super nice to have but I can't get it to work...

@japaric any ideas on mips/powerpc64?

japaric · 2016-09-28T04:48:37Z

SIGTRAP if binary is executed manually, but succeeds if binary is executed with qemu-mips{,el}. Not sure why

Sounds like the wrong binfmt interpreter was installed.

powerpc64-unknown-linux-gnu - not sure what's going on here

I think it's the same issue: bad interpreter or perhaps, more likely, the interpreter was not properly registered.

Dou you see the same errors locally, and are you also using the multiarch/qemu-user-static trick locally?

Perhaps the register.sh script in multiarch/qemu-user-static has some errors? I've never seen binfmt related errors like these when letting the binfmt-support/qemu-user-static packages handle the installation of binfmt interpreters (cf smoke). :-/

This commit moves over most of the testing infrastructure to in-tree docker images that are all dispatched to from Travis (no other test configuration). This allows versioning modifications to the test infrastructure as well as the code itself. Additionally separate docker images allows for easy modification of one without worrying about tampering of others as well as easy addition of new targets by simply adding a new `Dockerfile`. Additionally this commit bundles the master version of the `compiler-rt` source repository from `llvm-mirror/compiler-rt` to test against. The compiler-rt library itself is compiled as a `cdylib` which is then dynamically located at runtime and we look for symbols in. There's a few hoops here, but they currently get the job done. All tests now execute against both gcc_s and compiler-rt, and this testing strategy is now all hidden behind a macro as well (refactoring all existing tests along the way).

alexcrichton · 2016-09-29T05:10:48Z

Success! I've finally got everything working. I don't really know how, but at this point I'm not gonna ask...

I've squashed everything into one commit, and restored only testing on the auto/try branch.

r? @japaric

alexcrichton · 2016-09-29T05:11:52Z

Oh so there's a few nightly bugs the travis file also works around. The 32-bit cross-compilation will be fixed tonight but for now I rolled back those nightlies, and there's also a recent bug with ARM and float intrinsics (I think you found this?) that I fixed by rolling back a few nights as well. Fixes are in the pipeline on rust-lang/rust so we should be able to go back to the nightly channel in short order.

japaric · 2016-09-29T05:20:27Z

Success! I've finally got everything working.

An herculean task! Thank you @alexcrichton

I fixed by rolling back a few nights as well.
we should be able to go back to the nightly channel in short order.

👍

@homunkulus r+

homunkulus · 2016-09-29T05:20:27Z

📌 Commit 8e161a7 has been approved by japaric

homunkulus · 2016-09-29T05:20:29Z

⌛ Testing commit 8e161a7 with merge 8e161a7...

homunkulus · 2016-09-29T05:29:20Z

💔 Test failed - status-appveyor

alexcrichton · 2016-09-29T05:30:49Z

Aww :(

Want me to remove the gist part from appveyor as well?

japaric · 2016-09-29T11:43:19Z

Want me to remove the gist part from appveyor as well?

sigh I'll remove it.

The tests passed so I'm going to merge manually.

alexcrichton commented Sep 27, 2016

View reviewed changes

japaric reviewed Sep 27, 2016

View reviewed changes

mattico mentioned this pull request Sep 27, 2016

Add float quickcheck implementations (fixes #45) #48

Merged

japaric merged commit 52383ed into rust-lang:master Sep 29, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prep for and expand test infrastructure #76

Prep for and expand test infrastructure #76

alexcrichton commented Sep 27, 2016

alexcrichton Sep 27, 2016

japaric Sep 27, 2016

mattico Sep 27, 2016

alexcrichton Sep 28, 2016

alexcrichton Sep 27, 2016

alexcrichton Sep 27, 2016

alexcrichton Sep 27, 2016

japaric Sep 27, 2016

japaric left a comment

japaric Sep 27, 2016 •

edited

Loading

japaric Sep 27, 2016

japaric Sep 27, 2016

alexcrichton Sep 27, 2016

japaric Sep 27, 2016

japaric Sep 27, 2016

japaric Sep 27, 2016 •

edited

Loading

japaric Sep 27, 2016

alexcrichton Sep 27, 2016

japaric Sep 27, 2016

alexcrichton commented Sep 28, 2016

japaric commented Sep 28, 2016

alexcrichton commented Sep 29, 2016

alexcrichton commented Sep 29, 2016

japaric commented Sep 29, 2016

homunkulus commented Sep 29, 2016

homunkulus commented Sep 29, 2016

homunkulus commented Sep 29, 2016

alexcrichton commented Sep 29, 2016

japaric commented Sep 29, 2016

Prep for and expand test infrastructure #76

Prep for and expand test infrastructure #76

Conversation

alexcrichton commented Sep 27, 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

Choose a reason for hiding this comment

japaric left a comment

Choose a reason for hiding this comment

japaric Sep 27, 2016 • edited Loading

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

japaric Sep 27, 2016 • edited Loading

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 28, 2016

japaric commented Sep 28, 2016

alexcrichton commented Sep 29, 2016

alexcrichton commented Sep 29, 2016

japaric commented Sep 29, 2016

homunkulus commented Sep 29, 2016

homunkulus commented Sep 29, 2016

homunkulus commented Sep 29, 2016

alexcrichton commented Sep 29, 2016

japaric commented Sep 29, 2016

japaric Sep 27, 2016 •

edited

Loading

japaric Sep 27, 2016 •

edited

Loading