Add default sanitizers to TargetOptions #147043
Conversation
|
These commits modify compiler targets. |
rustbot
added
A-LLVM
rustbot
added
the
T-compiler
|
rustbot has assigned @petrochenkov. Use |
This comment has been minimized.
This comment has been minimized.
ilovepi
force-pushed
the
default-sanitizers
branch
from
2fa7a35 to
9ee9c2f
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
Note: Some targets, like Android, may also want to set this option, but I'm not sure who's handling rustc integration for them ATM. |
| ) -> SmallVec<[&'ll Attribute; 4]> { | ||
| let mut attrs = SmallVec::new(); | ||
| let enabled = cx.tcx.sess.opts.unstable_opts.sanitizer - no_sanitize; | ||
| let enabled = (cx.sess().target.default_sanitizers | cx.tcx.sess.opts.unstable_opts.sanitizer) |
There was a problem hiding this comment.
There are many places in the compiler in which unstable_opts.sanitizer is used, but only here that list is extended with default_sanitizers.
If the intent for default_sanitizers to work as if one more -C sanitizer was passed implicitly, then the sanitizer list should always be extended.
If for Fuchsia the sanitizers actually only need to be added here in codegen, and not e.g. on linker command line or for #[cfg(sanitize)], then it's probably better to just make a special case for Fuchsia targets here in sanitize_attrs.
There was a problem hiding this comment.
For SCS, the only thing that happens is to add an attribute during code generation, and the backend handles the rest when doing frame lowering. But yeah, that's not quite right for other sanitizers. Clang has the notion of default sanitizers and IIRC a few targets set them. My understanding is that's mostly for SCS, but I think some may set particular flavors of UBSAN to always be on, e.g. for overflows, etc.
Probably adding them in rustc_session is more preferable. I have a vague memory that's what I initially tried but let me try that again, since you're right it should just affect the list from the start.
There was a problem hiding this comment.
Let me know about the new approach. I worked out what blocked my last time. I had misunderstood something about the way options were plumbed into session. Thanks for making me take a second look.
There was a problem hiding this comment.
Usually this is done by having a method on Session like fn sanitizers that would return sess.unstable_opts.sanitizer | sess.target.options.default_sanitizers.
Then #[rustc_lint_opt_deny_field_access] can be used to ensure that the unstable_opts.sanitizer field cannot be accessed directly.
There was a problem hiding this comment.
Usually this is done by having a method on
Sessionlikefn sanitizersthat would returnsess.unstable_opts.sanitizer | sess.target.options.default_sanitizers. Then#[rustc_lint_opt_deny_field_access]can be used to ensure that theunstable_opts.sanitizerfield cannot be accessed directly.
OK. I got that mostly working, but I'm stumped on how to handle the access in options.rs at
rust/compiler/rustc_session/src/options.rs
Line 119 in 42d009c
The issue is that once I slap #[rustc_lint_opt_deny_field_access] on the field, that access is an error, but we don't have access to session from there.
I think the only caller is from consistent(), so ... I guess I could move TargetModifier into session.rs? seems less than desirable, though. I also thought about making the API access be on the options(), but then I don't think you'd get the enforcement you're after...
Any thoughts/preferences? I'm just really not sure what the right tradeoff/convention would be.
There was a problem hiding this comment.
#[allow(rustc::bad_opt_access)] can be used to suppress the lint.
But in the case of fn consistent it looks like it can just accept &Session instead of &Options.
petrochenkov
added
S-waiting-on-author
Some sanitizers are part of a system's ABI, like the shadow call stack on Aarch64 and RISC-V Fuchsia. Typically ABI options have other spellings, but LLVM has, for historical reasons, marked this as a sanitizer instead of an alternate ABI option. As a result, Fuchsia targets may not be compiled against the correct ABI unless this option is set. This hasn't caused correctness problems, since the backend reserves the SCS register, and thus preserves its value. But this is an issue for unwinding, as the SCS will not be an array of PCs describing the call complete call chain, and will have gaps from callers that don't use the correct ABI. In the long term, I'd like to see all the sanitizer configs that all frontends copy from clang moved into llvm's libFrontend, and exposed so that frontend consumers can use a small set of simple APIs to use sanitizers in a consistent way across the LLVM ecosystem, but that work is not yet ready today.
ilovepi
force-pushed
the
default-sanitizers
branch
from
9ee9c2f to
b003810
Compare
petrochenkov
added
S-waiting-on-review
Some sanitizers are part of a system's ABI, like the shadow call stack on Aarch64 and RISC-V Fuchsia. Typically ABI options have other spellings, but LLVM has, for historical reasons, marked this as a sanitizer instead of an alternate ABI option. As a result, Fuchsia targets may not be compiled against the correct ABI unless this option is set. This hasn't caused correctness problems, since the backend reserves the SCS register, and thus preserves its value. But this is an issue for unwinding, as the SCS will not be an array of PCs describing the call complete call chain, and will have gaps from callers that don't use the correct ABI.
In the long term, I'd like to see all the sanitizer configs that all frontends copy from clang moved into llvm's libFrontend, and exposed so that frontend consumers can use a small set of simple APIs to use sanitizers in a consistent way across the LLVM ecosystem, but that work is not yet ready today.