-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Add support for Control Flow Guard on Windows. #68180
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthewjasper (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
8cafeb7
to
885f977
Compare
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.
Please remove yet
from not yet supported
.
r? @nagisa |
☔ The latest upstream changes (presumably #68474) made this pull request unmergeable. Please resolve the merge conflicts. |
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.
r=me sans some cosmetic nits.
src/librustc_session/config.rs
Outdated
#[derive(Clone, Hash, Debug)] | ||
pub enum CFGuard { | ||
/// Emit Control Flow Guard metadata but no checks. | ||
Nochecks, |
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.
Please rename this variant to MetadataOnly
or something else along the lines. Nochecks
seems to imply CFG is disabled entirely, which it isn’t.
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.
The nochecks
variant was intended to match the corresponding -cfguard,nochecks
option in Clang (https://reviews.llvm.org/rL339420). Should we keep this terminology for consistency?
The intention is really to convey that this is "Control Flow Guard without any run-time checks". Is there somewhere in the documentation I could explain this?
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 guess you can keep it in that case. Just s/Nochecks/NoChecks/
. There are a couple of places for this documentation to go, including in the manpages and the unstable book.
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
592b3dd
to
33df727
Compare
@bors r+ Thanks! |
📌 Commit 3b91aad265e977a59d3efa20bb31b213520fdd03 has been approved by |
@ajpaverd: 🔑 Insufficient privileges: Not in reviewers |
Sure, please do squash. |
This patch enables rustc to emit the required LLVM module flags to enable Control Flow Guard metadata (cfguard=1) or metadata and checks (cfguard=2). The LLVM module flags are ignored on unsupported targets and operating systems.
⌛ Testing commit 3b91aad265e977a59d3efa20bb31b213520fdd03 with merge 176b807bde66b7a680917edbe4a731cff8c1f68b... |
3b91aad
to
c0744e1
Compare
Did my squash+rebase inadvertently interrupt bors' testing and merging? |
Yes, any push will prevent a PR from being merged before someone approves it again. |
@bors r+ Sorry didn’t notice the squash. Thanks for doing it. |
📌 Commit c0744e1 has been approved by |
Add support for Control Flow Guard on Windows. LLVM now supports Windows Control Flow Guard (CFG): llvm/llvm-project@d157a9b This patch adds support for rustc to emit the required LLVM module flags to enable CFG metadata (cfguard=1) or metadata and checks (cfguard=2). The LLVM module flags are ignored on unsupported targets and operating systems.
💥 Test timed out |
Is there a way to figure out what caused the bors tests to time out (the logs just seem to say the operation was cancelled)? |
Looks like it may have been cancelled manually (https://dev.azure.com/rust-lang/rust/_build/results?buildId=19568&view=results) probably to give way to something more important. Lets just retry for now. Thanks for bringing it to my attention. @bors r+ |
💡 This pull request was already approved, no need to approve it again.
|
📌 Commit c0744e1 has been approved by |
Add support for Control Flow Guard on Windows. LLVM now supports Windows Control Flow Guard (CFG): llvm/llvm-project@d157a9b This patch adds support for rustc to emit the required LLVM module flags to enable CFG metadata (cfguard=1) or metadata and checks (cfguard=2). The LLVM module flags are ignored on unsupported targets and operating systems.
☀️ Test successful - checks-azure |
…Simulacrum Enable Control Flow Guard in rustbuild Now that Rust supports Control Flow Guard (rust-lang#68180), add a config.toml option to build the standard library with CFG enabled. r? @nagisa
LLVM now supports Windows Control Flow Guard (CFG): llvm/llvm-project@d157a9b
This patch adds support for rustc to emit the required LLVM module flags to enable CFG metadata (cfguard=1) or metadata and checks (cfguard=2). The LLVM module flags are ignored on unsupported targets and operating systems.