Enable Rust + Sanitizers on Travis #381
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously the sanitizers are forcibly disabled as they were found to be incompatible with Rust code. The nightly channel of Rust, however, now has some fixes which should make this disabling no longer necessary.
This is no longer necessary with upstream rust-lang/rust changes as well as some local tweaks. Namely: * The `-fsanitize=address`-style options are now passed via `-C link-args` through `RUSTFLAGS`. This obviates the need for the shell script. * The `-C default-linker-libraries`, disabling `-nodefaultlibs`, is passed through `RUSTFLAGS`, which is necessary to ensure that `-fsanitize=address` links correctly. * The `-C linker` option is passed to ensure we're using the same C compiler as normal C code, although it has a bit of hackery to only get the `gcc` out of `gcc -std=c99`
It looks to be the case that Rust's standard allocator, jemalloc, is incompatible with sanitizers. The incompatibility, for whatever reason, seems to cause segfaults at runtime when jemalloc is linked with sanitizers. Without actually trying to figure out what's going on here this commit instead takes the hammer of "let's remove jemalloc when testing". The `tor_allocate` crate now by default switches to the system allocator (eventually this will want to be the tor allocator). Most crates then link to `tor_allocate` ot pick this up, but the `smartlist` crate had to manually switch to the system allocator in testing and the `external` crate had to be sure to link to `tor_allocate`. The final gotcha here is that this patch also switches to unconditionally passing `--target` to Cargo. For weird and arcane reasons passing `--target` with the host target of the compiler (which Cargo otherwise uses as the default) is different than not passing `--target` at all. This ensure that our custom `RUSTFLAGS` with sanitizer options doesn't make its way into build scripts, just the final testing artifacts.
Unfortunately Cargo doesn't actually parse these! Cargo should probably print a warning saying they're not used...
Only the final crate needs to be a `staticlib`, no need for all the intermediate steps to produce staticlibs!
|
I also did some investigation into the |
Pull Request Test Coverage Report for Build 2624
💛 - Coveralls |
This'll help retain test compatibility until 1.31.0 is released!
ghost
merged commit
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After discussing with the networking team folks in Mexico this is a series of commits which should get the Travis build green for testing Rust with C code that's been compiled with sanitizers. It turned out that there was a slew of issues that needed to be fixed! Many of them are detailed in the commits but some high level ones are:
-C default-linker-librariesoption rust-lang/rust#54675link_rust.shscript is remove din favor ofRUSTFLAGSused to carry argumentsSome issue links this is relevant to:
-nodefaultlibs(fixed here)Please let me know if anything should be updated here as well! I'd definitely be down for adding more comments here and there.