Only add sanitizer runtimes when linking an executable (#64629). #64780
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
|
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
added
the
S-waiting-on-review
|
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 |
|
Thanks for the PR, @choller! This looks correct to me. The logic in Clang seems to be quite a bit more complicated but for @bors r+ |
|
📌 Commit 640c261 has been approved by |
bors
added
S-waiting-on-bors
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.
|
💥 Test timed out |
bors
added
S-waiting-on-review
|
@bors retry |
bors
added
S-waiting-on-bors
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.
|
☀️ Test successful - checks-azure |
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
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 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-centralwith ASan using this patch.