-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Conversation
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. |
Edit: I have now forced trans to tell the linker to pass 1.
|
daf4b7e
to
91106e3
Compare
cc @Manishearth @nagisa @japaric @killercup @ner0x652 |
91106e3
to
ee5212b
Compare
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 There's a few opportunities to configuring rpath here as well, I think. We may be able to play with the various |
@alexcrichton Thanks :) tl;dr:
LLVM is forcing an $ 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 $ 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 The Initially I was thinking of using Cargo to add the
$ 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 Also the relative libstd path also leads to my "The So, my current solution is to always pass an absolute rpath when
I very much prefer solution 1 than the current solution 2, as the reason |
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?) |
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. |
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. |
ee5212b
to
c2bd386
Compare
@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 😃) |
Looks good to me! As one final piece, want to turn on |
@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 |
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 |
@alexcrichton Ah I see thanks. I don't think the code can be placed in In this PR, setting
So if I understand correctly, besides adding (I'll fix the |
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! |
c2bd386
to
b455737
Compare
@alexcrichton Updated and rebased. |
📌 Commit b455737 has been approved by |
This likely caused the failure of #41522:
|
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 |
These are not even crates...
ec4c9b9
to
164fd69
Compare
@alexcrichton @TimNN Fixed in 164fd69. |
@bors r=alexcrichton |
📌 Commit 164fd69 has been approved by |
⌛ Testing commit 164fd69 with merge 8a34b5c... |
💔 Test failed - status-appveyor |
@bors: retry
|
…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.
⌛ Testing commit 164fd69 with merge 5fea934... |
💔 Test failed - status-travis |
https://api.travis-ci.org/jobs/225921077/log.txt?deansi=true
spurious error? cc #38618 |
Yes, looks like it. @bors retry |
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.
☀️ Test successful - status-appveyor, status-travis |
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 correspondingrustc_?san.rlib
. The dylibs are directly copied tolib/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)