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

Only add sanitizer runtimes when linking an executable (#64629). #64780

Merged
merged 1 commit into from
Oct 7, 2019

Conversation

choller
Copy link
Contributor

@choller choller commented Sep 25, 2019

This change modifies the code to only add sanitizer runtimes if we are linking an executable, as those runtimes should never be part of libraries. I successfully compiled mozilla-central with ASan using this patch.

@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 @zackmdavis (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 Sep 25, 2019
@choller
Copy link
Contributor Author

choller commented Sep 25, 2019

Just to clarify, this will fix issue #64629 and ensure that we match Clang's behavior with respect to DSO linking. There is no corresponding code for the static library case because static libraries are typically produced directly by invoking ar. Especially since static libraries can be linked into DSOs again, they should never contain sanitizer runtimes either.

@michaelwoerister
Copy link
Member

Thanks for the PR, @choller! This looks correct to me. The logic in Clang seems to be quite a bit more complicated but for rustc's limited support, this looks good.

@bors r+

@bors
Copy link
Contributor

bors commented Oct 2, 2019

📌 Commit 640c261 has been approved by michaelwoerister

@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 2, 2019
@bors
Copy link
Contributor

bors commented Oct 2, 2019

⌛ Testing commit 640c261 with merge 9de12ba...

bors added a commit that referenced this pull request Oct 2, 2019
Only add sanitizer runtimes when linking an executable (#64629).

This change modifies the code to only add sanitizer runtimes if we are linking an executable, as those runtimes should never be part of libraries. I successfully compiled `mozilla-central` with ASan using this patch.
@bors
Copy link
Contributor

bors commented Oct 2, 2019

💥 Test timed out

@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 Oct 2, 2019
@michaelwoerister
Copy link
Member

@bors retry

@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 7, 2019
@bors
Copy link
Contributor

bors commented Oct 7, 2019

⌛ Testing commit 640c261 with merge 59a31c8...

bors added a commit that referenced this pull request Oct 7, 2019
Only add sanitizer runtimes when linking an executable (#64629).

This change modifies the code to only add sanitizer runtimes if we are linking an executable, as those runtimes should never be part of libraries. I successfully compiled `mozilla-central` with ASan using this patch.
@bors
Copy link
Contributor

bors commented Oct 7, 2019

☀️ Test successful - checks-azure
Approved by: michaelwoerister
Pushing 59a31c8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 7, 2019
@bors bors merged commit 640c261 into rust-lang:master Oct 7, 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
bors added a commit that referenced this pull request Oct 22, 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 #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).
* 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
tmandry added a commit to tmandry/rust that referenced this pull request Oct 30, 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
bors added a commit that referenced this pull request Dec 11, 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 #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 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants