expand osx sanitizer support (lsan, dylib, cdylib, staticlib) #62681
Conversation
This file contains 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
|
r? @varkor (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
mtak-
commented
Jul 14, 2019
| @@ -210,6 +210,8 @@ fn calculate_type(tcx: TyCtxt<'_>, ty: config::CrateType) -> DependencyList { | |||
| // quite yet, so do so here. | |||
| activate_injected_dep(*sess.injected_panic_runtime.get(), &mut ret, | |||
| &|cnum| tcx.is_panic_runtime(cnum)); | |||
| activate_injected_dep(*sess.injected_sanitizer_runtime.get(), &mut ret, | |||
| &|cnum| tcx.is_sanitizer_runtime(cnum)); | |||
There was a problem hiding this comment.
I am not sure if this is necessary. It would only really affect linux targets which I don't have access to for testing. My thinking was changing the sanitizer crate depkind to Implicit requires adding activate_injected_dep in suitable places.
mtak-
commented
Jul 15, 2019
| @@ -3,14 +3,9 @@ | |||
| -include ../tools.mk | |||
There was a problem hiding this comment.
I did not expect this test to pass. Is it run for PRs?
There was a problem hiding this comment.
None of the tests tagged with # needs-sanitizer-support are run because the Dockerfile for x86_64-gnu-llvm-6.0 does not have --enable-sanitizers set. The Dockerfile for x86_64-gnu does have --enable-sanitizers, but is not currently being run on azure.
There was a problem hiding this comment.
This should pass now - if it were run.
There was a problem hiding this comment.
cc @rust-lang/infra is there anything that should/could be done here to get these tests running?
There was a problem hiding this comment.
You can edit .azure-pipelines/pr.yml to temporarily run the x86_64-gnu builder on this one PR, and then that change would be reverted before approving the PR
mtak-
commented
Jul 15, 2019
|
@mtak- could we add docs of a full cargo workflow somewhere here: https://github.com/rust-lang/rust/tree/master/src/doc/rustc/src ? E.g. see the docs for I often find it tricky to enable It would be worth it to properly document a cargo workflow for the sanitizers somewhere, even if the current workflow is a bit annoying. (IIUC This does not need to be done in this PR, so maybe I should open an issue for this. |
|
@gnzlbg I'm up for writing some docs on the sanitizers, but I think it should be in a separate PR so folks can chime in with their experiences. I am only familiar with using the sanitizers on macos. On macos you have to dynamically link to the sanitizers - which mostly comes when building a dylib, cdylib, or staticlib. The most annoying thing IMO is that if you have any proc-macro dependencies then the target has to be explicit. $ RUSTFLAGS="-Zsanitizer=address" cargo run # error
$ RUSTFLAGS="-Zsanitizer=address" cargo run --target x86_64-apple-darwin # ok
The sanitizer code on master should error out for |
|
I'm on macosx right now, so can't really test msan, but the problem I had last time I tried on linux was msan trying to override some symbols from the C library (e.g. memcpy, strcmp, etc.). Anyways documenting this in a different PR sounds good. |
|
Hey sorry I don't have a ton of time to review nowadays, would someone on @rust-lang/compiler be willing to review this? |
|
r? @nagisa |
nagisa
reviewed
Jul 25, 2019
|
Overall LGTM, waiting to hear on infra to see if we can do something about tests. |
|
Hopefully we can get some CI on these tests. If not, I would be very grateful if someone on linux would run these tests. I would not be shocked if the |
|
Ping from triage: @rust-lang/infra, is anybody available to help with #62681 (review)? |
mtak-
force-pushed
the
osx-improved-sanitizers
branch
from
278e341 to
e85a37e
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Ping from triage. @mtak- any update on this. Some checks are failing. Thanks. |
Alexendoo
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #64264) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Ping from Triage, closing due to inactivity. Please re-open when or if there are updates. Thanks for the PR @mtak- |
joelpalmer
added
S-inactive
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.
This PR adds support for leak sanitizer to macos as well as support for macos instrumenting dylib, cdylib, and staticlib crate types.
I kinda fumbled my way through this PR. Hoping for some feedback!
The sanitizers on macos are always built as dylibs, so packaging up a staticlib with all of
asan, for example, is not possible (I couldn't figure out how to do it). EDIT: sourceThis comment indicated it was probably ok to re-enable the
lsantest. EDIT: Which means that #46126 can likely be closed.