-
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
sanitizers: Stabilize AddressSanitizer and LeakSanitizer for the Tier 1 targets #123617
base: master
Are you sure you want to change the base?
Conversation
rustbot has assigned @compiler-errors. Use |
Some changes occurred in src/tools/compiletest cc @jieyouxu These commits modify compiler targets. |
r? @davidtwco |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see tests that exercise things like -Csanitizer=non-existent
and -Zsanitizer=non-existent
, and also -Zsanitizer=stable-sanitizer
1 (e.g. an x86_64-unknown-linux-gnu test for a stable sanitizer) and -Csanitizer=unstable-sanitizer
(I believe you can add a run-make test with a custom target that has no sanitizers enabled for it?)
Footnotes
-
What do we do if we pass
-Zsanitizer
with a stable sanitizer? Should we error? Presumably not, but I would assume we'd want to at least warn the users that the sanitizer has been stabilized and they should be using-C
, just like we do for feature gates. ↩
Documentation will need an update. Is something like |
This is unusable to most stable Rust users, right? It requires either |
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
cec660e
to
17eff53
Compare
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
17eff53
to
f81f25d
Compare
This comment has been minimized.
This comment has been minimized.
f81f25d
to
2cfed6e
Compare
I'll also add that LLVM's own documentation does not yet list Aarch64 as a supported platform for AddressSanitizer: EDIT: A few more concerns:
|
Thanks for pointing this out! @vitalybuka, do you think we should list it there (assuming it's supported)?
I don't think these would work with other backends than LLVM as these are mostly implemented in LLVM.
We could just show an error message saying these are not available when using a backend other than LLVM? (Which I think is what happens now, but I need to confirm.) What do you think?
Yes, @vitalybuka is already participating on this thread. |
The problem is that then it still becomes a breaking change if we want to use Cranelift by default in the future, since with what we're stabilizing e.g. Does that make sense? Not sure I explained it right. In any case, I don't know the plans for Cranelift, and it might already be impossible to make default for other reasons, just a thought. |
Full list is quite long, and quality of implementation can vary. Drop the lists to avoid confusion like rust-lang/rust#123617 (comment) We don't maintain these for other sanitizers.
FYI, @vitalybuka dropped the list of supported architectures on llvm/llvm-project/pull/116302 as per your feedback since it was incomplete and the actual list is quite long.
Yes, I understand. Do you know if there isn't such case of a stabilized flag already? For example, I see a few mentions to LLVM already in https://doc.rust-lang.org/rustc/codegen-options/index.html#codegen-units (e.g., codegen-units, control-flow-guard, embed-bitcode, etc.). Those all seem to be similar cases that would need to be reimplemented if we replace the default backend so they won't become a breaking change as well. Given this, I'm not sure whether adding another unstable option to the compiler, and requiring the user to pass an additional option to be able to use the sanitizers is worth it. What do you think? |
Yeah, that's fair, I guess if we wanted to change the default for debug builds, we'd have to do a dirty "if -Csanitize, -Cembed-bitcode, --emit=llvm-ir, etc. is set, fall back to using LLVM". I'll CC @bjorn3 though, just to make absolutely sure that it doesn't conflict with Cranelift's future plans. |
-Clinker-plugin-lto is another flag that is specific to LLVM. |
8af3b93
to
bed8f9c
Compare
Full list is quite long, and quality of implementation can vary. Drop the lists to avoid confusion like rust-lang/rust#123617 (comment) We don't maintain these for other sanitizers.
Thank you everyone again for the patience and the time while I went through all the issues listed here. Here are the final updates about these issues:
I also added AddressSanitizer for aarch64-apple-darwin since it was promoted to Tier 1 in #128592, but let me know if anyone has any objections. Otherwise, this PR should be ready to be merged. Thank you to everyone that contributed to and in this PR! Also thank you very much for helping me out with pushing this through the last mile, @tmiasko, @1c3t3a, and @jakos-sec! It was much appreciated. |
For more information on sanitizers, see the | ||
[sanitizer | ||
feature](https://doc.rust-lang.org/nightly/unstable-book/compiler-flags/sanitizer.html) | ||
in the [The Rust Unstable | ||
Book](https://doc.rust-lang.org/nightly/unstable-book/). For more information on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should move the relevant documentation for stable sanitizers into this document (we should not need to refer people to the unstable book for information on stable features 🙂)
In particular, the section ASan should be moved and I think it would be extremely helpful to have a section on some of the considerations that need to be taken into account to use sanitizers in mixed C/C++/Rust codebases as well as a general disclaimer that the quality of the sanitizer support varies across platforms and relies heavily on LLVM's implementation.
in the [The Rust Unstable | ||
Book](https://doc.rust-lang.org/nightly/unstable-book/). For more information on | ||
stable and supported sanitizers, see the [supported sanitizers | ||
table](https://github.com/rust-lang/rust/issues/123615#issuecomment-2041791236). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This table should live in this doc as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this table already exists above? I think this sentence can just be removed then?
@@ -104,6 +104,8 @@ | |||
// Do not check link redundancy on bootstraping phase | |||
#![allow(rustdoc::redundant_explicit_links)] | |||
#![warn(rustdoc::unescaped_backticks)] | |||
// FIXME: Remove after `no_sanitize` stabilization (along with `#![feature(no_sanitize)]`) | |||
#![allow(stable_features)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can change the #![feature(no_sanitize)]
below to #![cfg_attr(bootstrap, feature(no_sanitize))]
and that will also ensure it is removed once a compiler with the feature stabilized is used for bootstrapping 🙂
@@ -263,6 +263,8 @@ | |||
#![deny(ffi_unwind_calls)] | |||
// std may use features in a platform-specific way | |||
#![allow(unused_features)] | |||
// FIXME: Remove after `no_sanitize` stabilization (along with `#![feature(no_sanitize)]`) | |||
#![allow(stable_features)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
|
||
------------------------ | ||
|
||
The `no_sanitize` attribute can be used to selectively disable sanitizer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This info will need to go into the Reference (probably here? https://github.com/rust-lang/reference/blob/master/src/attributes/codegen.md)
in the [The Rust Unstable | ||
Book](https://doc.rust-lang.org/nightly/unstable-book/). For more information on | ||
stable and supported sanitizers, see the [supported sanitizers | ||
table](https://github.com/rust-lang/rust/issues/123615#issuecomment-2041791236). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this table already exists above? I think this sentence can just be removed then?
|--------|------------| | ||
| aarch64-unknown-linux-gnu | address, leak | | ||
| i586-pc-windows-msvc | address | | ||
| i586-unknown-linux-gnu | address | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right?
| i586-unknown-linux-gnu | address | | |
| i686-unknown-linux-gnu | address | |
| Target | Sanitizers | | ||
|--------|------------| | ||
| aarch64-unknown-linux-gnu | address, leak | | ||
| i586-pc-windows-msvc | address | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i586-pc-windows-msvc
isn't changed in this PR or a Tier 1 target
| i586-pc-windows-msvc | address | |
72102f8
to
01822f2
Compare
This comment has been minimized.
This comment has been minimized.
01822f2
to
dae2b7a
Compare
This comment has been minimized.
This comment has been minimized.
dae2b7a
to
4f14c34
Compare
This comment has been minimized.
This comment has been minimized.
Add suppport for specifying stable sanitizers in addition to the existing supported sanitizers.
Stabilize AddressSanitizer and LeakSanitizer for the Tier 1 targets that support them.
Stabilize the `no_sanitize` attribute so stable sanitizers can also be selectively disabled for annotated functions.
Stabilize AddressSanitizer for aarch64-apple-darwin since it was promoted to Tier 1 in rust-lang#128592.
4f14c34
to
89d1ada
Compare
Add support for specifying stable sanitizers in addition to the existing supported sanitizers, remove the
-Zsanitizer
unstable option and have only the-Csanitize
codegen option, requiring the-Zunstable-options
to be passed for using unstable sanitizers, add AddressSanitizer and LeakSanitizer for the Tier 1 targets that support them, and also stabilize theno_sanitize
attribute so stable sanitizers can also be selectively disabled for annotated functions.. The tracking issue for stabilizing the sanitizers is #123615. This is part of our work to stabilize support for sanitizers in the Rust compiler. (See our roadmap at https://hackmd.io/@rcvalle/S1Ou9K6H6.)Stabilization Report
Summary
We would like to propose stabilizing AddressSanitizer and LeakSanitizer for the Tier 1 targets that support them, and stabilize the
no_sanitize
attribute so stable sanitizers can also be selectively disabled for annotated functions.. This will be done by-Zsanitizer
unstable option and having only the-Csanitize
codegen option, and requiring the-Zunstable-options
to be passed for using unstable sanitizers.no_sanitize
attribute.After stabilizing these sanitizers, the supported sanitizers will look like this:
The tracking issue for stabilizing the sanitizers is #123615. This is part of our work to stabilize support for sanitizers in the Rust compiler. (See our roadmap at https://hackmd.io/@rcvalle/S1Ou9K6H6.)
Documentation
Documentation will be updated by adding documentation for the
-Csanitizer
codegen option to the Codegen Options in the The rustc book.Tests
You may find current and will find additional test cases for the sanitizers in:
Unresolved questions
We will prioritize stabilizing sanitizers that provide incremental value without requiring rebuilding the Rust Standard Library (i.e., Cargo build-std feature). We're also working on Partial compilation using MIR-only rlibs compiler-team#738, which should help with
-Zbuild-std
.