Rewrite target checking for #[sanitize]#157332
Conversation
|
Some changes occurred in compiler/rustc_attr_parsing Some changes occurred in compiler/rustc_passes/src/check_attr.rs |
rustbot
added
A-attributes
| Allow(Target::Impl { of_trait: true }), | ||
| Allow(Target::Mod), | ||
| Allow(Target::Crate), | ||
| Allow(Target::Static), |
There was a problem hiding this comment.
Note that attribute parsing has slightly better/different target information than check_attr.rs does
- We allow
Target::Cratehere, which falls underTarget::Modin check_attr.rs - We allow
Target::Method(MethodKind::TraitImpl)here, which falls underTarget::Impl { of_trait: true }in check_attr.rs
There was a problem hiding this comment.
Okay, I raised some concerns on the tracking issue. #39699 (comment)
This pr won't change the behavior and sanitize is still unstable so I don't mind proceeding.
| LL | #[sanitize(address = "off")] | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| | | ||
| = help: sanitize attribute can be applied to a function (with body), impl block, or module |
There was a problem hiding this comment.
function (with body) would be nice to keep. Can we update allowed_targets_applied to do this?
There was a problem hiding this comment.
I've added it as a group to allowed_targets_applied, but this currently means that this group will only be shown if the attribute is applied to a target that is a function without a body. I'll see if I can refactor the allowed_targets_applied to be better about this in a follow-up PR
| Allow(Target::Impl { of_trait: true }), | ||
| Allow(Target::Mod), | ||
| Allow(Target::Crate), | ||
| Allow(Target::Static), |
There was a problem hiding this comment.
Okay, I raised some concerns on the tracking issue. #39699 (comment)
This pr won't change the behavior and sanitize is still unstable so I don't mind proceeding.
| #[derive(Diagnostic)] | ||
| #[diag("`#[sanitize({$field} = ...)]` attribute cannot be used on statics")] | ||
| #[help("`#[sanitize]` can be used on statics if only the address is sanitized")] | ||
| pub(crate) struct SanitizeInvalidStatic { | ||
| #[primary_span] | ||
| pub span: Span, | ||
| pub field: &'static str, | ||
| } |
There was a problem hiding this comment.
You can have multiple fields in this attribute, how does this error display in that case?
There was a problem hiding this comment.
It currently only complains about the first one it finds, I was considering generating a diagnostic for every one but that feels like it would get spammy very quickly
rustbot
removed
the
S-waiting-on-review
|
Reminder, once the PR becomes ready for a review, use |
rustbot
added
the
S-waiting-on-author
|
@rustbot ready |
rustbot
added
S-waiting-on-review
JonathanBrouwer
force-pushed
the
target-sanitize
branch
from
2b59b34 to
b0ccb01
Compare
|
This PR was rebased onto a different main 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. |
rust-bors
Bot
added
S-waiting-on-bors
…=mejrs Rewrite target checking for `#[sanitize]` r? @mejrs
…uwer Rollup of 15 pull requests Successful merges: - #155763 (Promotes 5 Thumb-mode bare-metal Arm targets to Tier 2) - #156953 (delegation: emit error when there is an infer lifetime in user-specified args) - #157248 (delegation: move statements out of the first arg) - #157263 (rustc_codegen_ssa: Refactor `ArchiveEntry` to include entry kind) - #157311 (Use weak linkage for EII defaults) - #156089 (Fix unused_parens for pinned reference patterns) - #156928 (Remove -Zemscripten-wasm-eh) - #157236 (Reorganize `tests/ui/issues` [3/N]) - #157287 (Const generics: remove AliasTerm::kind(), and small fixes) - #157294 (Split coroutine layout computation to its own file) - #157328 (windows: Elide division-by-zero checks in Instant::now()) - #157331 (Rewrite target checking for `#[link]`) - #157336 (Enable `clippy::mem_replace_with_default`) - #157362 (Fix trivial wf module argument/doc comment name mismatches) - #157364 (Rewrite target checking of `rustc_dummy`) Failed merges: - #157332 (Rewrite target checking for `#[sanitize]`)
3 participants
r? @mejrs