Skip to content
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

Sanitizer support allow-list should not be baked into the compiler (outside of the built-in targets) #81802

Closed
nagisa opened this issue Feb 5, 2021 · 0 comments · Fixed by #81866
Assignees
Labels
A-sanitizers Area: Sanitizers for correctness and code quality. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nagisa
Copy link
Member

nagisa commented Feb 5, 2021

We have code such as this in our codebase that encodes an allow-list of targets that support for a given sanitizer:

const ASAN_SUPPORTED_TARGETS: &[&str] = &[

Instead, the code should be adjusted to only encode such support table in the target definitions. The compiler otherwise should be pretty agnostic to both the list of available sanitizers (outside of the backend) and targets that support it.

@nagisa nagisa added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-sanitizers Area: Sanitizers for correctness and code quality. labels Feb 5, 2021
@nagisa nagisa self-assigned this Feb 5, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Apr 2, 2021
…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
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 2, 2021
…t-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
@bors bors closed this as completed in 16c1d0a Apr 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sanitizers Area: Sanitizers for correctness and code quality. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant