Maintain supported sanitizers as a target property #81866
Conversation
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
nagisa
force-pushed
the
nagisa/sanitizer-support-target-prop
branch
from
09c94a7
to
bdee081
Compare
This comment has been minimized.
This comment has been minimized.
|
Hm, this JSON encoding roundtrip test puts some sticks into the wheels :( |
|
☔ The latest upstream changes (presumably #82045) made this pull request unmergeable. Please resolve the merge conflicts. |
nagisa
force-pushed
the
nagisa/sanitizer-support-target-prop
branch
from
bdee081
to
1c3740e
Compare
|
This is good to go now. |
| )), | ||
| } | ||
| // Cannot mix and match sanitizers | ||
| // FIXME(nagisa): is there a really good reason that is the case? |
There was a problem hiding this comment.
All supported sanitizers are pairwise incompatible (we could allow address + leak, but that would be equivalent to address alone, and confusing on targets where leak detection is not enabled by default in ASAN).
Aha, I knew I had missed something! Done. |
nagisa
force-pushed
the
nagisa/sanitizer-support-target-prop
branch
from
f08d194
to
874c67f
Compare
crlf0710
added
A-sanitizers
|
@matthewjasper this is ready for review |
JohnCSimon
added
S-waiting-on-review
|
☔ The latest upstream changes (presumably #83307) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@tmiasko may I consider your checkmark above as an r=tmiasko? |
|
The changes LGTM, but I don't have approval privileges. |
|
will r+ it after the conflict is resolved |
This commit adds an additional target property – `supported_sanitizers`, and replaces the hardcoded allowlists in argument parsing to use this new property. Fixes rust-lang#81802
nagisa
force-pushed
the
nagisa/sanitizer-support-target-prop
branch
from
874c67f
to
41875c8
Compare
|
@bors r=tmiasko Thanks! |
|
📌 Commit 41875c8 has been approved by |
bors
added
S-waiting-on-bors
…get-prop, r=tmiasko
Maintain supported sanitizers as a target property
In an effort to remove a hard-coded allow-list for target-sanitizer support correspondence, this PR moves the configuration to the target options.
Perhaps the one notable change made in this PR is this doc-comment:
```rust
/// The sanitizers supported by this target
///
/// Note that the support here is at a codegen level. If the machine code with sanitizer
/// enabled can generated on this target, but the necessary supporting libraries are not
/// distributed with the target, the sanitizer should still appear in this list for the target.
```
Previously the target would typically be added to the allow-list at the same time as the supporting runtime libraries are shipped for the target. However whether we ship the runtime libraries or not needn't be baked into the compiler; and if we don't users will receive a significantly more directed error about library not being found.
Fixes rust-lang#81802
|
☀️ Test successful - checks-actions |
add address sanitizer fo android We have been being using asan to debug the rust/cpp/c mixed android application in production for months: recompile the rust library with a patched rustc, everything just works fine. The patch is really small thanks to `@nagisa` 's refactoring in rust-lang#81866 r? `@nagisa`
In an effort to remove a hard-coded allow-list for target-sanitizer support correspondence, this PR moves the configuration to the target options.
Perhaps the one notable change made in this PR is this doc-comment:
Previously the target would typically be added to the allow-list at the same time as the supporting runtime libraries are shipped for the target. However whether we ship the runtime libraries or not needn't be baked into the compiler; and if we don't users will receive a significantly more directed error about library not being found.
Fixes #81802