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

build-std compatible sanitizer support #65241

Merged
merged 5 commits into from
Jan 11, 2020
Merged

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Oct 9, 2019

Motivation

When using -Z sanitizer=* feature it is essential that both user code and
standard library is instrumented. Otherwise the utility of sanitizer will be
limited, or its use will be impractical like in the case of memory sanitizer.

The recently introduced cargo feature build-std makes it possible to rebuild
standard library with arbitrary rustc flags. Unfortunately, those changes alone
do not make it easy to rebuild standard library with sanitizers, since runtimes
are dependencies of std that have to be build in specific environment,
generally not available outside rustbuild process. Additionally rebuilding them
requires presence of llvm-config and compiler-rt sources.

The goal of changes proposed here is to make it possible to avoid rebuilding
sanitizer runtimes when rebuilding the std, thus making it possible to
instrument standard library for use with sanitizer with simple, although
verbose command:

env CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_RUSTFLAGS=-Zsanitizer=thread cargo test -Zbuild-std --target x86_64-unknown-linux-gnu

Implementation

  • Sanitizer runtimes are no long packed into crates. Instead, libraries build
    from compiler-rt are used as is, after renaming them into librusc_rt.*.
  • rustc obtains runtimes from target libdir for default sysroot, so that
    they are not required in custom build sysroots created with build-std.
  • The runtimes are only linked-in into executables to address issue Rust should not link sanitizer runtimes unconditionally #64629.
    (in previous design it was hard to avoid linking runtimes into static
    libraries produced by rustc as demonstrated by sanitizer-staticlib-link
    test, which still passes despite changes made in Only add sanitizer runtimes when linking an executable (#64629). #64780).

cc @kennytm, @japaric, @Firstyear, @choller

@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 @nikomatsakis (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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 9, 2019
@jonas-schievink jonas-schievink added the A-sanitizers Area: Sanitizers for correctness and code quality label Oct 9, 2019
@Firstyear
Copy link
Contributor

I have OSX but I'm not sure what a suitable test plan or steps would be here?

Otherwise I think everything reads pretty reasonably, but I'd want other better people to put their input into the matter :)

@tmiasko
Copy link
Contributor Author

tmiasko commented Oct 10, 2019

  1. The src/test/run-make-fulldeps contains basic sanitizer tests.
    I would suggest starting with those. Some of them are gated to be run
    on Linux only, so I would check if this is still required.
  2. Following that, I would create a small cargo project with tests that include
    issues that should be reported by given sanitizer, and then test it with
    -Zsanitize=*, and then again with -Zsanitize=* and -Zbuild-std.
  3. Repeat above for some real world project. For a thread sanitizer, test
    suites & benchmarks from crossbeam or tokio crates would be great. I would
    expect to get reports from either of those, so it should be clear if things
    are working on not.

Concrete steps for step 3 suitable for x86_64-unknown-linux-gnu:

~/rust $ ./x.py build --stage 1
~/rust $ mkdir -p build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/src
~/rust $ ln -sT $PWD build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/src/rust
~/rust $ rustup toolchain link stage1 build/x86_64-unknown-linux-gnu/stage1

# Testing on tokio: 
~/tokio $ cargo clean
~/tokio $ env CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_RUSTFLAGS=-Zsanitizer=thread cargo +stage1 test --target x86_64-unknown-linux-gnu
~/tokio $ cargo clean
~/tokio $ env CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_RUSTFLAGS=-Zsanitizer=thread cargo +stage1 -Zbuild-std test --target x86_64-unknown-linux-gnu

@nikomatsakis
Copy link
Contributor

Is there any documentation for how sanitizers work today? I think that before we make any major changes to the setup, I'd like to see that rectified, so that the new design can be described as an update to the documentation. The rustc-guide would be a suitable place for such a thing. We could also hold a design meeting, but I don't think that's necessarily required, since sanitizers are still experimental.

@Firstyear
Copy link
Contributor

Honestly, because they are a -Z flag, and thus nightly only, I think that documentation has not emerged because people don't know they exist, can't access them when they need stable, or have developed other work arounds. For example, I'm required to setup RUSTC_BOOTSTRAP=1 in my makefiles to use stable compilers + -Z with sanitizers and FFI.

I also think that because their major value is with FFI, and that requires the C components to always work with sanitizers, there are not many projects currently using them broadly.

So this all led to the sanitizers being a super niche feature, wildly useful for people like me who has a C code base that is sanitized and able to develop the tricks to use them with Rust FFI, but most people don't have that set of overlapping scenarios to bring this into peoples minds.

I think that if the first line of the documentation was "you need nightly to use sanitizers" that's already a major barrier to getting this used more broadly. So I think we need to seriously discuss how we get features like this out from behind nightly only.

@bors
Copy link
Contributor

bors commented Oct 13, 2019

☔ The latest upstream changes (presumably #65388) made this pull request unmergeable. Please resolve the merge conflicts.

@tmiasko
Copy link
Contributor Author

tmiasko commented Oct 14, 2019

@nikomatsakis japaric published user oriented documentation at https://github.com/japaric/rust-san.

Substantial part of it is dedicated to answering the question how to rebuild
and instrument the std, and how to ignore false positives generated when
previous step wasn't followed. Those parts should now be unnecessary thanks to
-Zbuild-std in cargo and changes proposed here.

I don't think there was any documentation for implementation in rustc, apart
from description on the pull requests and as a part of discussions therein.

Related documentation changes in rustc-guide rust-lang/rustc-dev-guide#466.

@tmiasko tmiasko marked this pull request as ready for review October 14, 2019 22:28
@bors
Copy link
Contributor

bors commented Oct 15, 2019

☔ The latest upstream changes (presumably #65422) made this pull request unmergeable. Please resolve the merge conflicts.

@choller
Copy link
Contributor

choller commented Oct 15, 2019

Thanks for working on this. After some discussions in Discord it seems that this PR is the right way to move forward. As you have noticed, my fix for #64629 was not comprehensive. I am still facing issues where sanitizer runtimes are linked to libraries and we still cannot build Firefox with TSan for that reason.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Oct 15, 2019

@tmiasko first off, thanks a ton for the rustc-guide PR! As I wrote in my review, I suspect that the 'how to use sanitizer' material would be better off in the src/doc/rustc directory, as part of the "rustc book". Do you think you could leave a few notes in the rustc-guide describing how sanitizer integration works? (like, it seems like we have to link some LLVM libraries somewhere, and we are copying some files around, why are we doing that?)

My motivation here is that, particularly for these "little used, perenially unstable" features, I'd like to have some notes on how things work so that if we have to fix bugs we have some idea where to start. (Although this sparks an idea for me that maybe, for features like these, we should also have people kind of "sign up" as maintainers that can be pinged if problems arise... right now, we don't have any notion of who those people might be. I presume you'd be up for that?)

To be clear: if there were some docs on how -Zsanitizer is used (like the ones you already wrote) and how it is implemented (I think those don't quite exist yet), I'd be very happy to r+ the PR. Also, to be clear: I realize I'm asking you to do a bit more than we sometimes do -- I'm just trying to bootstrap an effort to get rustc better documented, one PR at a time. Nothing personal. :)

@ehuss
Copy link
Contributor

ehuss commented Oct 16, 2019

I just wanted to note that adding unstable documentation to the rustc book is a little unusual, as it doesn't contain any unstable docs right now. Normally the unstable documentation goes in the Unstable Book. I personally don't care too much in this case, but thought I would point it out.

EDIT: It may want to at least mention that the chapter is about an unstable feature.

@tmiasko
Copy link
Contributor Author

tmiasko commented Oct 16, 2019

@ehuss thanks for suggestion, the unstable-book seems more appropriate. The
-Zsanitizer feature itself is unstable, moreover I think it is best
combined with -Zbuild-std which is also unstable.

I moved the user oriented parts to unstable-book (new commit here). The
high-level overview of the implementation was left in rustc-guide with
additional details about building runtimes, copying, and linking them.

@nikomatsakis sure, you cc me on issues related to sanitizers.

@nikomatsakis
Copy link
Contributor

@ehuss

I just wanted to note that adding unstable documentation to the rustc book is a little unusual, as it doesn't contain any unstable docs right now

Hmm, I'm ok with the unstable book too. I tend to think that means nobody will ever find it there, but maybe that's the point.

@nikomatsakis
Copy link
Contributor

To elaborate on my previous comment, by "maybe that's the point" I meant:

  • we don't want people to come across unstable content too easily
  • and if they ask questions, we can point them at the right place

That said, maybe it'd be useful to extend the rustc book with a pointer to the unstable guide to say something like "you can find documentation on unstable options here". I do appreciate the idea of making it extra clear when things are unstable. The same could apply to the self-profile options.

@nikomatsakis
Copy link
Contributor

I'm quite satisfied. Thanks again @tmiasko for going to the extra mile to document what's going on, it made reading the PR much easier.

@bors r+

@bors
Copy link
Contributor

bors commented Oct 18, 2019

📌 Commit dd4d6a9 has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 18, 2019
Centril added a commit to Centril/rust that referenced this pull request Oct 19, 2019
build-std compatible sanitizer support

### Motivation

When using `-Z sanitizer=*` feature it is essential that both user code and
standard library is instrumented. Otherwise the utility of sanitizer will be
limited, or its use will be impractical like in the case of memory sanitizer.

The recently introduced cargo feature build-std makes it possible to rebuild
standard library with arbitrary rustc flags. Unfortunately, those changes alone
do not make it easy to rebuild standard library with sanitizers, since runtimes
are dependencies of std that have to be build in specific environment,
generally not available outside rustbuild process. Additionally rebuilding them
requires presence of llvm-config and compiler-rt sources.

The goal of changes proposed here is to make it possible to avoid rebuilding
sanitizer runtimes when rebuilding the std, thus making it possible to
instrument standard library for use with sanitizer with simple, although
verbose command:

```
env CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_RUSTFLAGS=-Zsanitizer=thread cargo test -Zbuild-std --target x86_64-unknown-linux-gnu
```

### Implementation

* Sanitizer runtimes are no long packed into crates. Instead, libraries build
  from compiler-rt are used as is, after renaming them into `librusc_rt.*`.
* rustc obtains runtimes from target libdir for default sysroot, so that
  they are not required in custom build sysroots created with build-std.
* The runtimes are only linked-in into executables to address issue rust-lang#64629.
  (in previous design it was hard to avoid linking runtimes into static
  libraries produced by rustc as demonstrated by sanitizer-staticlib-link
  test, which still passes despite changes made in rust-lang#64780).
* When custom llvm-config is specified during build process, the sanitizer
  runtimes will be obtained from there instead of begin rebuilding from sources
  in src/llvm-project/compiler-rt. This should be preferable since runtimes
  used should match instrumentation passes. For example there have been nine
  version of address sanitizer ABI.

Note this marked as a draft PR, because it is currently untested on OS X (I
would appreciate any help there).

cc @kennytm, @japaric, @Firstyear, @choller
Centril added a commit to Centril/rust that referenced this pull request Oct 19, 2019
build-std compatible sanitizer support

### Motivation

When using `-Z sanitizer=*` feature it is essential that both user code and
standard library is instrumented. Otherwise the utility of sanitizer will be
limited, or its use will be impractical like in the case of memory sanitizer.

The recently introduced cargo feature build-std makes it possible to rebuild
standard library with arbitrary rustc flags. Unfortunately, those changes alone
do not make it easy to rebuild standard library with sanitizers, since runtimes
are dependencies of std that have to be build in specific environment,
generally not available outside rustbuild process. Additionally rebuilding them
requires presence of llvm-config and compiler-rt sources.

The goal of changes proposed here is to make it possible to avoid rebuilding
sanitizer runtimes when rebuilding the std, thus making it possible to
instrument standard library for use with sanitizer with simple, although
verbose command:

```
env CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_RUSTFLAGS=-Zsanitizer=thread cargo test -Zbuild-std --target x86_64-unknown-linux-gnu
```

### Implementation

* Sanitizer runtimes are no long packed into crates. Instead, libraries build
  from compiler-rt are used as is, after renaming them into `librusc_rt.*`.
* rustc obtains runtimes from target libdir for default sysroot, so that
  they are not required in custom build sysroots created with build-std.
* The runtimes are only linked-in into executables to address issue rust-lang#64629.
  (in previous design it was hard to avoid linking runtimes into static
  libraries produced by rustc as demonstrated by sanitizer-staticlib-link
  test, which still passes despite changes made in rust-lang#64780).
* When custom llvm-config is specified during build process, the sanitizer
  runtimes will be obtained from there instead of begin rebuilding from sources
  in src/llvm-project/compiler-rt. This should be preferable since runtimes
  used should match instrumentation passes. For example there have been nine
  version of address sanitizer ABI.

Note this marked as a draft PR, because it is currently untested on OS X (I
would appreciate any help there).

cc @kennytm, @japaric, @Firstyear, @choller
@tmiasko
Copy link
Contributor Author

tmiasko commented Jan 9, 2020

Rebased without commit that attempted to reenable leak sanitizer testcase.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jan 9, 2020

📌 Commit e88f071 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 9, 2020
@bors
Copy link
Contributor

bors commented Jan 9, 2020

⌛ Testing commit e88f071 with merge 41925383b20cd7361be8ef3e8b7a22b61f272611...

@bors
Copy link
Contributor

bors commented Jan 10, 2020

💔 Test failed - checks-azure

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 10, 2020
@Centril
Copy link
Contributor

Centril commented Jan 10, 2020

@bors p=0

@kennytm
Copy link
Member

kennytm commented Jan 10, 2020

@bors retry

macOS dist-x86_64-apple timed out (?) without any logs.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 10, 2020
@bors
Copy link
Contributor

bors commented Jan 10, 2020

⌛ Testing commit e88f071 with merge e621797...

bors added a commit that referenced this pull request Jan 10, 2020
build-std compatible sanitizer support

### Motivation

When using `-Z sanitizer=*` feature it is essential that both user code and
standard library is instrumented. Otherwise the utility of sanitizer will be
limited, or its use will be impractical like in the case of memory sanitizer.

The recently introduced cargo feature build-std makes it possible to rebuild
standard library with arbitrary rustc flags. Unfortunately, those changes alone
do not make it easy to rebuild standard library with sanitizers, since runtimes
are dependencies of std that have to be build in specific environment,
generally not available outside rustbuild process. Additionally rebuilding them
requires presence of llvm-config and compiler-rt sources.

The goal of changes proposed here is to make it possible to avoid rebuilding
sanitizer runtimes when rebuilding the std, thus making it possible to
instrument standard library for use with sanitizer with simple, although
verbose command:

```
env CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_RUSTFLAGS=-Zsanitizer=thread cargo test -Zbuild-std --target x86_64-unknown-linux-gnu
```

### Implementation

* Sanitizer runtimes are no long packed into crates. Instead, libraries build
  from compiler-rt are used as is, after renaming them into `librusc_rt.*`.
* rustc obtains runtimes from target libdir for default sysroot, so that
  they are not required in custom build sysroots created with build-std.
* The runtimes are only linked-in into executables to address issue #64629.
  (in previous design it was hard to avoid linking runtimes into static
  libraries produced by rustc as demonstrated by sanitizer-staticlib-link
  test, which still passes despite changes made in #64780).

cc @kennytm, @japaric, @Firstyear, @choller
@bors
Copy link
Contributor

bors commented Jan 11, 2020

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing e621797 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sanitizers Area: Sanitizers for correctness and code quality merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.