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

Support AddressSanitizer and ThreadSanitizer on x86_64-apple-darwin #41352

Merged
merged 4 commits into from
Apr 26, 2017

Conversation

kennytm
Copy link
Member

@kennytm kennytm commented Apr 18, 2017

ASan and TSan are supported on macOS, and this commit enables their support.

The sanitizers are always built as *.dylib on Apple platforms, so they cannot be statically linked into the corresponding rustc_?san.rlib. The dylibs are directly copied to lib/rustlib/x86_64-apple-darwin/lib/ instead.

Note, although Xcode also ships with their own copies of ASan/TSan dylibs, we cannot use them due to version mismatch.


There is a caveat: the sanitizer libraries are linked as @rpath/ (due to https://reviews.llvm.org/D6018), so the user needs to additionally pass -C rpath:

Edit: Passing rpath is now automatic.


(cc #39699)

@rust-highfive
Copy link
Collaborator

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

@kennytm
Copy link
Member Author

kennytm commented Apr 18, 2017

So there are two issues making this not mergeable (thus the [WIP]) yet, both related to cargo:

Edit: I have now forced trans to tell the linker to pass -Wl,rpath,<absolute-lib-path> when using the sanitizer with macOS target (ee5212b). The following issues may still worth investigating though.

1. -C rpath is not set automatically when using the sanitizers.

The build script of rustc_?san should be able to set it via the build script (rust-lang/cargo#1293 😉), but well, a user could workaround by adding profile.*.rpath = true on demand; however,

2. The @rpath points to the wrong folder

due to cargo copying/hard-linking the binaries. When you run cargo build, the executable will be placed in:

target/<triple>/debug
    executable*
    deps/
        executable-1a2b3c4d5e6f7890*

The two executables are identical (hard-linked), and cargo run will run the one outside of deps/. However, @rpath is a relative-path, relative to the one in deps/. So cargo run will not be able to find the correct library and fails with the "Library not loaded" error.

Therefore, the rpath needs to be passed as an absolute path (#11746?) via link-args similar to #32886

RUSTFLAGS="-Clink-arg=-rpath -Clink-arg=/Users/kennytm/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/x86_64-apple-darwin/lib" cargo run

or cargo should tell rustc to reduce one level of ../ in the emitted @rpath.

@frewsxcv
Copy link
Member

cc @Manishearth @nagisa @japaric @killercup @ner0x652

@frewsxcv frewsxcv added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 19, 2017
@aidanhs aidanhs added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 19, 2017
@kennytm kennytm changed the title [WIP] Support AddressSanitizer and ThreadSanitizer on x86_64-apple-darwin Support AddressSanitizer and ThreadSanitizer on x86_64-apple-darwin Apr 20, 2017
@alexcrichton
Copy link
Member

Thanks for the PR @kennytm! This looks pretty slick.

Can you describe in a bit more detail what's up w/ the rpath business? Is there something special here as opposed to normal dylib linkage? For example if I created a binary which linked to the asan runtime, and then ran the binary with DYLD_LIBRARY_PATH pointing to the right location, would that work or would that fail?

There's a few opportunities to configuring rpath here as well, I think. We may be able to play with the various LC_* directives in the dylib too. In general though I might prefer if we avoid trying to use the existing rpath code to pass this in. TBH I barely know what it does any more (and I'm not sure many others know more) and I'm a little hesitant to change it. It may end up being shorter regardless as well to just pass/calculate a direct argument.

@kennytm
Copy link
Member Author

kennytm commented Apr 20, 2017

@alexcrichton Thanks :)

tl;dr:

  • DYLD_LIBRARY_PATH works, but I don't think it's a good idea to ask the user or cargo to supply it.
  • Other possible solutions are illustrated at the end of this comment. I think the current solution requires least change for other parties.

LLVM is forcing an @rpath install-name for sanitizers in D6018 and D6176, to fix issue 21316. Because of this, when we link to the sanitizer dylibs, it will have an @rpath too:

$ clang++ -fsanitize=address 1.cpp
$ otool -L a.out
a.out:
	/usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 307.5.0)
	@rpath/libclang_rt.asan_osx_dynamic.dylib (compatibility version 0.0.0, current version 0.0.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1238.51.1)

And the linker would need to provide LC_RPATH load commands, so that dyld can resolve @rpath == "/Applications/Xcode.app/.../darwin/" using the executable's information alone:

$ otool -l a.out
...
Load command 15
          cmd LC_RPATH
      cmdsize 32
         path @executable_path (offset 12)
Load command 16
          cmd LC_RPATH
      cmdsize 136
         path /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/clang/8.1.0/lib/darwin (offset 12)
...

In Rust, unless you manually pass -C rpath, rustc will not instruct the linker to add any LC_RPATH to the binary (flag: -Wl,-rpath,«lib»). And this is why we have issue #17219, because libstd's install-name also has an @rpath, and -C prefer-dynamic alone won't add the LC_RPATH.

The DYLD_LIBRARY_PATH/DYLD_FALLBACK_LIBRARY_PATH environment variables can be used to provide the correct path to @rpath. Yes it works, but I consider it a hack not a solution, as the user or cargo needs to supply it manually. Besides, as clang is also inserting the desired LC_RPATH, I think we should too.

Initially I was thinking of using Cargo to add the -C link-args="-rpath ..." or -C rpath flag via build.rs or the manifest. But it turns out -C rpath won't work in this case. When -C rpath is supplied, rustc will insert at least two LC_RPATH into the binary:

  1. a relative path from the executable's folder to the library's folder, e.g. @loader_path/deps/
  2. an absolute fallback path that points to the standard library folder, e.g. /usr/local/lib/rustlib/x86_64-apple-darwin/lib.
$ rustc -C rpath 1.rs
$ otool -l ./1
...
Load command 13
          cmd LC_RPATH
      cmdsize 120
         path @loader_path/../../../.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/x86_64-apple-darwin/lib (offset 12)
Load command 14
          cmd LC_RPATH
      cmdsize 64
         path /usr/local/lib/rustlib/x86_64-apple-darwin/lib (offset 12)
...

Relative path (#11746) makes perfect sense when the library is a third-party crate obtained by Cargo, because as long as the executable and the linked dylib's path remains the same, they can be distributed and placed anywhere you like. What does not make sense is using relative path to find libstd (#28640 (comment)) and the sanitizers, which should have a fixed location.

Also the relative libstd path also leads to my "The @rpath points to the wrong folder" comment above. After cargo build, the compiled executable will be hard-linked from target/deps/«bin» to target/«bin», and the original relative relationship between the executable and ASan is broken, i.e. the first LC_RPATH becomes invalid. Dyld will then check the second LC_RPATH, which, if we use rustup or build rustc manually, won't be there either. Thus we will see a runtime error.


So, my current solution is to always pass an absolute rpath when -Z sanitizer=* is seen on macOS. Some other possible solutions I could think of:

  1. Allow Cargo's build.rs to supply the rpath, so that we can fix it in librustc_asan level, instead of hacking the compiler (No way of specifing a rpath so custom Frameworks aren't linkable cargo#2171).

  2. The current solution — let rustc insert the required rpath when needed.

  3. Tell user to manually supply -C rpath (or rpath = true in Cargo.toml), at the same time:

    • Fix the current rustc_trans::back::rpath::get_rpath_relative_to_output() to return an absolute rpath for libstd when there is too many ../, e.g., when compiler panic on OS X: 'can't create relative paths across filesystems' (regression) #23140 happens, instead of doing Generate relative paths for -C rpath again #23283, use an absolute path. This may break stuff. Or,
    • Fix the current rustc_trans::back::rpath::get_install_prefix_rpath() to return the actual sysroot path (~/.rustup/toolchains/stable-x86_64-apple-darwin/), instead of / in additional to $CFG_PREFIX. Or,
    • Change Cargo, so that when the executable to lifted from target/deps/ to target/, use a symlink instead of copying/hardlink, so the relative rpath remains the same.
  4. Tell user to manually supply -C link-args="-rpath «lib»" when building.

  5. Tell user to manually run install_name_tool -add_rpath «lib» «executable» after executable is built.

  6. Tell user to manually add DYLD_LIBRARY_PATH=«lib» when running the executable

  7. Try to build ASan and TSan statically. This goes against D216 though which I think would be a terrible move.

I very much prefer solution 1 than the current solution 2, as the reason librustc_asan exists is to configure the linker and use alloc_system right 😁?

@alexcrichton
Copy link
Member

Holy cow, thanks for the detailed explanation @kennytm! That matches most of my hunches and fills in all the gaps as well.

I definitely agree the compiler should fill in the rpath directive here in the binaries it's generating, that feels like the best solution. I also agree that filling in an absolute rpath seems like the best option here as well. Seems like you've additionally come to the same conclusion that rpath in Rust is weird right now...

Ok so with all that in mind, do you mind just moving around a bit where the rpath logic happens? I think it's totally fine for the asan to have specific logic on OSX of how to pass rpath and it doesn't necessarily need to be shared with the rest of the rpath producing code (it's just an absolute path, right?)

@alexcrichton
Copy link
Member

Oh and I'd also be on board with implementing solution (1) above as well. It'd probably just involve some random unstable compiler flag and/or attribute (or something like that). Implementation-wise it probably wouldn't buy too much (may be more complex). Either way's fine by me though.

@nagisa
Copy link
Member

nagisa commented Apr 21, 2017

Try to build ASan and TSan statically. This goes against D216 though which I think would be a terrible move.

Why you say so? That diff is as old as the earth itself and is more than likely to be irrelevant. IMO static linking sanitizers is strongly preferable if it is possible at all.

@kennytm
Copy link
Member Author

kennytm commented Apr 21, 2017

@alexcrichton Changed in c2bd386.

@nagisa The main reason is I don't see any static libraries produced and I don't want to modify compiler-rt 😝.

I believe static-to-dynamic move is still the direction being taken, e.g. the more recent D8471/D8473 in 2015 moves UBSan from an *.a to *.dylib. We could say the decision is made so long ago it is not going to change. We could certainly try to build a static library since we have a fork. But I'm afraid the sanitizer devs have dylib-only on macOS as granted, that adding static-linking support may break something. (Or maybe just reverting the CMakeFiles.txt is enough, I'm no compile-rt export 😃)

@alexcrichton
Copy link
Member

Looks good to me! As one final piece, want to turn on --enable-sanitizers on the builder that runs with DEPLOY=1? That way these'll make their way into the nightlies.

@kennytm
Copy link
Member Author

kennytm commented Apr 21, 2017

@alexcrichton: Do you mean this part?

    # OSX builders producing releases. These do not run the full test suite and
    # just produce a bunch of artifacts.
    #
    # Note that these are running in the `xcode7` image instead of the
    # `xcode8.2` image as above. That's because we want to build releases for
    # OSX 10.7 and `xcode7` is the latest Xcode able to compile LLVM for 10.7.
    - env: >
        RUST_CHECK_TARGET=dist
        RUST_CONFIGURE_ARGS="--build=i686-apple-darwin --enable-extended"
        SRC=.
        DEPLOY=1
        RUSTC_RETRY_LINKER_ON_SEGFAULT=1
        SCCACHE_ERROR_LOG=/tmp/sccache.log
        MACOSX_DEPLOYMENT_TARGET=10.7
      os: osx
      osx_image: xcode7
      install: *osx_install_sccache
    - env: >
        RUST_CHECK_TARGET=dist
        RUST_CONFIGURE_ARGS="--target=aarch64-apple-ios,armv7-apple-ios,armv7s-apple-ios,i386-apple-ios,x86_64-apple-ios --enable-extended"
        SRC=.
        DEPLOY=1
        RUSTC_RETRY_LINKER_ON_SEGFAULT=1
        SCCACHE_ERROR_LOG=/tmp/sccache.log
        MACOSX_DEPLOYMENT_TARGET=10.7
      os: osx
      osx_image: xcode7
      install: *osx_install_sccache

I'm not sure what to change, IIUC the first builder targets i686-apple-darwin (32-bit) and the second one targets iOS, both of them won't support the sanitizers?

@alexcrichton
Copy link
Member

Oh the second one there is also responsible for x86_64-apple-darwin (an implied target). Want to add some specific code to disable sanitizers on iOS and then add --enable-sanitizers to the second one?

@kennytm
Copy link
Member Author

kennytm commented Apr 21, 2017

@alexcrichton Ah I see thanks.

I don't think the code can be placed in .travis.yml or src/ci/run.sh, it can only be done in src/bootstrap?

In this PR, setting --enable-sanitizers will do 3 things:

  1. In check.rs:249, set the environment variable SANITIZER_SUPPORT=1, which will be used to determine if sanitizer-related tests can run.

    This actually only affects the run-make/sanitizer-address test case, since the other two (-leak and -memory) are now guarded by a $TARGET-check, as they are still Linux-only. So I could also change the test case instead of the source code here.

  2. In compile.rs:62, insert the LLVM_CONFIG flag when building libstd, used by the build.rs of librustc_*san. But it is already $TARGET-checked, so nothing needs to be changed.

  3. In compile.rs:119, copy the *.dylib into lib/rustlib/x86_64-apple-darwin/lib, which is specific to macOS x86_64.

So if I understand correctly, besides adding --enable-sanitizers to that build, changing the test case of run-make/sanitizer-address so that it only runs in macOS x86_64 and Linux x86_64 should be enough?

(I'll fix the make tidy issue together after this is confirmed.)

@alexcrichton
Copy link
Member

Oh it's ok to even not update the tests, the builder that's building the release isn't running tests (a different builder does that).

I'd probably just add the flag to .travis.yml, and then fix problems as they arise. If you're lucky none will!

@kennytm
Copy link
Member Author

kennytm commented Apr 22, 2017

@alexcrichton Updated and rebased.

@carols10cents carols10cents added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 24, 2017
@alexcrichton
Copy link
Member

@bors: r+

Thanks @kennytm!

@bors
Copy link
Contributor

bors commented Apr 24, 2017

📌 Commit b455737 has been approved by alexcrichton

@alexcrichton alexcrichton added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Apr 24, 2017
@TimNN
Copy link
Contributor

TimNN commented Apr 25, 2017

This likely caused the failure of #41522:

[01:28:58] failures:
[01:28:58] 
[01:28:58] ---- [run-make] run-make/sysroot-crates-are-unstable stdout ----
[01:28:58] 	
[01:28:58] error: make failed
[01:28:58] status: exit code: 2
[01:28:58] command: "make"
[01:28:58] stdout:
[01:28:58] ------------------------------------------
[01:28:58] verifying alloc is an unstable crate
[01:28:58] verifying alloc_jemalloc is an unstable crate
[01:28:58] verifying alloc_system is an unstable crate
[01:28:58] verifying arena is an unstable crate
[01:28:58] verifying bitflags is an unstable crate
[01:28:58] verifying clang_rt.asan_osx_dynamic.dylib is an unstable crate
[01:28:58] verifying clang_rt.tsan_osx_dynamic.dylib is an unstable crate
[01:28:58] crate clang_rt.asan_osx_dynamic.dylib is not unstable
[01:28:58] error: expected one of `;` or `as`, found `.`
[01:28:58]  --> <anon>:1:22
[01:28:58]   |
[01:28:58] 1 | extern crate clang_rt.asan_osx_dynamic.dylib;
[01:28:58]   |                      ^ expected one of `;` or `as` here
[01:28:58] 
[01:28:58] error: aborting due to previous error
[01:28:58] 
[01:28:58] crate clang_rt.tsan_osx_dynamic.dylib is not unstable
[01:28:58] error: expected one of `;` or `as`, found `.`
[01:28:58]  --> <anon>:1:22
[01:28:58]   |
[01:28:58] 1 | extern crate clang_rt.tsan_osx_dynamic.dylib;
[01:28:58]   |                      ^ expected one of `;` or `as` here
[01:28:58] 
[01:28:58] error: aborting due to previous error
[01:28:58] 
[01:28:58] 
[01:28:58] ------------------------------------------
[01:28:58] stderr:
[01:28:58] ------------------------------------------
[01:28:58] make[1]: *** [check-crate-clang_rt.asan_osx_dynamic.dylib-is-unstable] Error 1
[01:28:58] make[1]: *** Waiting for unfinished jobs....
[01:28:58] make[1]: *** [check-crate-clang_rt.tsan_osx_dynamic.dylib-is-unstable] Error 1
[01:28:58] 
[01:28:58] ------------------------------------------
[01:28:58] 
[01:28:58] thread '[run-make] run-make/sysroot-crates-are-unstable' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:2627
[01:28:58] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:28:58] 
[01:28:58] 
[01:28:58] failures:
[01:28:58]     [run-make] run-make/sysroot-crates-are-unstable
[01:28:58] 
[01:28:58] test result: FAILED. 149 passed; 1 failed; 0 ignored; 0 measured

@kennytm
Copy link
Member Author

kennytm commented Apr 25, 2017

Failed this test https://github.com/rust-lang/rust/blob/master/src/test/run-make/sysroot-crates-are-unstable/Makefile which ensures all non-whitelisted crates are unstable.

But those libclang_rt*.dylib are not crates 😂 I'm going to whitelist them from the test.

@TimNN

@kennytm
Copy link
Member Author

kennytm commented Apr 25, 2017

@alexcrichton @TimNN Fixed in 164fd69.

@eddyb
Copy link
Member

eddyb commented Apr 25, 2017

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Apr 25, 2017

📌 Commit 164fd69 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Apr 25, 2017

⌛ Testing commit 164fd69 with merge 8a34b5c...

@bors
Copy link
Contributor

bors commented Apr 25, 2017

💔 Test failed - status-appveyor

@alexcrichton
Copy link
Member

arielb1 pushed a commit to arielb1/rust that referenced this pull request Apr 25, 2017
…chton

Support AddressSanitizer and ThreadSanitizer on x86_64-apple-darwin

[ASan](https://clang.llvm.org/docs/AddressSanitizer.html#supported-platforms) and [TSan](https://clang.llvm.org/docs/ThreadSanitizer.html#supported-platforms) are supported on macOS, and this commit enables their support.

The sanitizers are always built as `*.dylib` on Apple platforms, so they cannot be statically linked into the corresponding `rustc_?san.rlib`. The dylibs are directly copied to `lib/rustlib/x86_64-apple-darwin/lib/` instead.

Note, although Xcode also ships with their own copies of ASan/TSan dylibs, we cannot use them due to version mismatch.

----

~~There is a caveat: the sanitizer libraries are linked as `@rpath/` (due to https://reviews.llvm.org/D6018), so the user needs to additionally pass `-C rpath`:~~

**Edit:** Passing rpath is now automatic.
@bors
Copy link
Contributor

bors commented Apr 26, 2017

⌛ Testing commit 164fd69 with merge 5fea934...

@bors
Copy link
Contributor

bors commented Apr 26, 2017

💔 Test failed - status-travis

@kennytm
Copy link
Member Author

kennytm commented Apr 26, 2017

https://api.travis-ci.org/jobs/225921077/log.txt?deansi=true

...
[01:01:20] test sync::mpsc::sync_tests::send1 ... ok
[01:01:20] test sync::mpsc::sync_tests::send2 ... ok
[01:01:20] test sync::mpsc::sync_tests::send3 ... ok
[01:01:20] test sync::mpsc::sync_tests::send4 ... ok
[01:01:20] error: process didn't exit successfully: `/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-std/x86_64-unknown-linux-musl/release/deps/std-d02fbe97fc497570` (signal: 11, SIGSEGV: invalid memory reference)
[01:01:20] 
[01:01:20] To learn more, run the command again with --verbose.
[01:01:20] 
[01:01:20] 
[01:01:20] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "-j" "4" "--target" "x86_64-unknown-linux-musl" "--release" "--locked" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--features" "panic-unwind jemalloc backtrace" "-p" "std:0.0.0" "-p" "alloc:0.0.0" "-p" "alloc_system:0.0.0" "-p" "collections:0.0.0" "-p" "rustc_tsan:0.0.0" "-p" "rand:0.0.0" "-p" "core:0.0.0" "-p" "rustc_lsan:0.0.0" "-p" "unwind:0.0.0" "-p" "panic_abort:0.0.0" "-p" "libc:0.0.0" "-p" "compiler_builtins:0.0.0" "-p" "std_unicode:0.0.0" "-p" "rustc_asan:0.0.0" "-p" "rustc_msan:0.0.0" "--"
[01:01:20] expected success, got: exit code: 101
[01:01:20] 
[01:01:20] 
[01:01:20] Build completed unsuccessfully in 1:00:09

spurious error? cc #38618

@TimNN
Copy link
Contributor

TimNN commented Apr 26, 2017

Yes, looks like it.

@bors retry

@bors
Copy link
Contributor

bors commented Apr 26, 2017

⌛ Testing commit 164fd69 with merge 0ee56f6...

bors added a commit that referenced this pull request Apr 26, 2017
Support AddressSanitizer and ThreadSanitizer on x86_64-apple-darwin

[ASan](https://clang.llvm.org/docs/AddressSanitizer.html#supported-platforms) and [TSan](https://clang.llvm.org/docs/ThreadSanitizer.html#supported-platforms) are supported on macOS, and this commit enables their support.

The sanitizers are always built as `*.dylib` on Apple platforms, so they cannot be statically linked into the corresponding `rustc_?san.rlib`. The dylibs are directly copied to `lib/rustlib/x86_64-apple-darwin/lib/` instead.

Note, although Xcode also ships with their own copies of ASan/TSan dylibs, we cannot use them due to version mismatch.

----

~~There is a caveat: the sanitizer libraries are linked as `@rpath/` (due to https://reviews.llvm.org/D6018), so the user needs to additionally pass `-C rpath`:~~

**Edit:** Passing rpath is now automatic.
@bors
Copy link
Contributor

bors commented Apr 26, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 0ee56f6 to master...

@bors bors merged commit 164fd69 into rust-lang:master Apr 26, 2017
@kennytm kennytm deleted the macos-sanitizers branch April 26, 2017 21:55
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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.