Skip to content

Rewrite target checking for #[sanitize]#157332

Open
JonathanBrouwer wants to merge 3 commits into
rust-lang:mainfrom
JonathanBrouwer:target-sanitize
Open

Rewrite target checking for #[sanitize]#157332
JonathanBrouwer wants to merge 3 commits into
rust-lang:mainfrom
JonathanBrouwer:target-sanitize

Conversation

@JonathanBrouwer
Copy link
Copy Markdown
Contributor

r? @mejrs

@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Jun 2, 2026

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 2, 2026
Allow(Target::Impl { of_trait: true }),
Allow(Target::Mod),
Allow(Target::Crate),
Allow(Target::Static),
Copy link
Copy Markdown
Contributor Author

@JonathanBrouwer JonathanBrouwer Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that attribute parsing has slightly better/different target information than check_attr.rs does

  • We allow Target::Crate here, which falls under Target::Mod in check_attr.rs
  • We allow Target::Method(MethodKind::TraitImpl) here, which falls under Target::Impl { of_trait: true } in check_attr.rs

View changes since the review

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@mejrs mejrs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a macro call test here as well.

View changes since this review

LL | #[sanitize(address = "off")]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: sanitize attribute can be applied to a function (with body), impl block, or module
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

function (with body) would be nice to keep. Can we update allowed_targets_applied to do this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +1016 to +1023
#[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,
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can have multiple fields in this attribute, how does this error display in that case?

Copy link
Copy Markdown
Contributor Author

@JonathanBrouwer JonathanBrouwer Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 2, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Jun 2, 2026

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 2, 2026
@JonathanBrouwer
Copy link
Copy Markdown
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 3, 2026
@rustbot

This comment has been minimized.

Copy link
Copy Markdown
Contributor

@mejrs mejrs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors Bot commented Jun 3, 2026

📌 Commit b0ccb01 has been approved by mejrs

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 5. This pull request will be tested once the tree is reopened.

@rust-bors rust-bors Bot 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 Jun 3, 2026
JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Jun 3, 2026
…=mejrs

Rewrite target checking for `#[sanitize]`

r? @mejrs
rust-bors Bot pushed a commit that referenced this pull request Jun 3, 2026
…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]`)
@rust-bors rust-bors Bot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 3, 2026
@rust-bors

This comment has been minimized.

@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Jun 3, 2026

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.

@JonathanBrouwer
Copy link
Copy Markdown
Contributor Author

@bors r=mejrs

@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors Bot commented Jun 3, 2026

📌 Commit 97dc1b1 has been approved by mejrs

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 5. This pull request will be tested once the tree is reopened.

@rust-bors rust-bors Bot 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 3, 2026
JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Jun 3, 2026
…=mejrs

Rewrite target checking for `#[sanitize]`

r? @mejrs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants