-
-
Notifications
You must be signed in to change notification settings - Fork 14.1k
Create x86_64-asan-linux-gnu target which enables ASAN by default #149644
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
base: main
Are you sure you want to change the base?
Conversation
|
This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp. These commits modify compiler targets. |
This comment has been minimized.
This comment has been minimized.
|
Thanks. You will have to modify this PR to meet the guidelines outlined at https://doc.rust-lang.org/rustc/target-tier-policy.html#adding-a-new-target. @rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
d26433e to
9ee5543
Compare
|
Some changes occurred in src/doc/rustc/src/platform-support cc @Noratrieb |
This comment has been minimized.
This comment has been minimized.
|
|
||
| ("x86_64-pc-cygwin", x86_64_pc_cygwin), | ||
|
|
||
| ("x86_64_asan-unknown-linux-gnu", x86_64_asan_unknown_linux_gnu), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about x86_64-unknown-linux-gnu-asan? I think that would make the most sense, adding a new component and leaving all the other components the same. x86_64_asan would imply cfg(target_arch = "x86_64_asan") which is obviously not the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I'd love to have your idea for this. I tried a bunch of things now and couldn't find any great solution.
I tried:
x86_64-unknown-linux-gnu-asan(which you suggested) => cc-rs complains: "too many components in targetx86_64-unknown-linux-gnu-asan"x86_64_asan-unknown-linux-gnu: compiling sanitizer rt complains since the LLVM sees:Compiler-RT supported architectures: x86_64_asan(which is not a valid ARCH).x86_64-asan-linux-gnuworked but I'm not sure if that's ideal eitherx86_64-unknown-linux_asan-gnuworked but I'm not sure if that's ideal either
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x86_64-unknown-linux-gnu-asan (which you suggested) => cc-rs complains: "too many components in target x86_64-unknown-linux-gnu-asan"
That is something that can be changed in the cc crate I think. It tries to parse the target name rather than getting all this information from say rustc --print cfg --target .... The current cc behavior also breaks with custom target json files as those can have an arbitrary target name. I don't know why cc does it this way though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, I can look into supporting it in there then. I wasn't sure if it's desired to actually support a <arch>-<vendor>-<os>-<env>-<config> case, if it is, I agree that would be the best solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a prototype that makes cc-rs work: https://github.com/rust-lang/cc-rs/compare/main...jakos-sec:cc-rs:allow-parsing-sanitizer-targets?expand=1
However I think I'd need some input on how this is best wanted. (Right now I'm basically just throwing away the 5th component. Should we also parse this into the TargetInfo, do we want this sanitizer specific or give this a more generic name?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x86_64-unknown-linux-gnu-asan (which you suggested) => cc-rs complains: "too many components in target x86_64-unknown-linux-gnu-asan"
That is something that can be changed in the cc crate I think. It tries to parse the target name rather than getting all this information from say
rustc --print cfg --target .... The current cc behavior also breaks with custom target json files as those can have an arbitrary target name. I don't know why cc does it this way though.
cc has gone through many iterations of getting target information, including hardcoding it for every target (which was bad). i think the problem is that it wants the underlying llvm-target, which isn't accessible on stable. maybe it should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my opinion here is that we should just give up making target names machine readable and provide the tools people need to get the information from rustc (which today is mainly --print cfg, but that's not always enough)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the discussion on zulip (#t-compiler/major changes > Allow using prebuilt sanitizer libraries compiler-team#943), I've now opted to use the vendor part to specify ASAN (x86_64-asan-linux-gnu).
compiler/rustc_target/src/spec/targets/x86_64_unknown_linux_asan_gnu.rs
Outdated
Show resolved
Hide resolved
9ee5543 to
96ca640
Compare
This comment has been minimized.
This comment has been minimized.
96ca640 to
157ebd5
Compare
This comment has been minimized.
This comment has been minimized.
compiler/rustc_target/src/spec/targets/x86_64_unknown_linux_asan_gnu.rs
Outdated
Show resolved
Hide resolved
30677c5 to
913b7f7
Compare
This comment has been minimized.
This comment has been minimized.
|
@bors retry |
|
@jakos-sec: 🔑 Insufficient privileges: not in try users |
913b7f7 to
a504ca4
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. |
This comment has been minimized.
This comment has been minimized.
a504ca4 to
da92150
Compare
This comment has been minimized.
This comment has been minimized.
As suggested, in order to distribute sanitizer instrumented standard libraries without introducing new rustc flags, this adds a new dedicated target. With the target, we can distribute the instrumented standard libraries through a separate rustup component.
da92150 to
35d2795
Compare
|
@rustbot ready |
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just two more nits. I believe once the discussion in Zulip thread settles, we can more forward.
| The goal of this target is to allow shipping ASAN-instrumented standard | ||
| libraries through rustup, enabling a fully instrumented binary without requiring | ||
| nightly features (build-std). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note, to ship libstd this will have to become a tier 2 target (without host tools).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for pointing this out! I removed host-tools since there is no good reason to have them (nobody needs ASAN within the host tools). But I'll look into what tier 2 changes for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, the tiers are:
- tier 1
- tier 2 w/ host tools
- tier 2 w/o host tools
- tier 3
So, going to tier 2 without host tools would be just a next step on that ladder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean, we should merge this PR as tier 3 and then promote it or is it possible to merge it directly as tier 2 w/o host tools?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's up to you. I think in this case tier 3 target won't be much better than just using -Zbuild-std, but won't hurt.
Tier 2 target will require MCP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be best to simply wait and merge it as Tier 2 instead of going through the process to promote it later.
|
FYI I opened the MCP so we can change this to Tier 2 (to actually allow shipping prebuilt artifacts): rust-lang/compiler-team#951 |
As suggested, in order to distribute sanitizer instrumented standard libraries without introducing new rustc flags, this adds a new dedicated target. With the target, we can distribute the instrumented standard libraries through a separate rustup component.
I pledge to do my best maintaining it and we can also include the folks from the Exploit Mitigations Project Group (rcvalle@ & 1c3t3a@).
I've chosen
x86_64-asan-linux-gnuas the name since the-unknown-linux-gnupart should stay identical to thex86_64-unknown-linux-gnutarget. We could also chooseasan_x86_64-unknown-linux-gnuif that is preferrable.There should be no confusion, it's clear that it's the original target with ASAN enabled.
Only letters, numbers and dashes used.
There are no unusual requirements to build or use it. It's the original
x86_64-unknown-linux-gnutarget with ASAN enabled as a default sanitizer.There are no license implications.
Given, by reusing the existing ASAN code.
There are no new dependencies/features required.
It's using open source tools only.
There are no such terms present.
Understood.
Understood.
The goal is to have ASAN instrumented standard library variants of the existing
x86_64-unknown-linux-gnutarget, so all should be present.I think the explanation in platform support doc is enough to make this aspect clear.
Understood.
Understood.
I don't believe this PR is affected by this.
The target should work on all rustc versions that correctly compile for
x86_64-unknown-linux-gnu.