The Wayback Machine - https://web.archive.org/web/20201019181210/https://github.com/rust-lang/rust/pull/73893
Skip to content

/ rust

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

Jump to bottom

Stabilize control-flow-guard codegen option #73893

Merged
merged 1 commit into from Jul 22, 2020
Merged

Stabilize control-flow-guard codegen option #73893

merged 1 commit into from Jul 22, 2020

Conversation

@ajpaverd
Copy link
Contributor

@ajpaverd ajpaverd commented Jun 30, 2020 •
edited

This is the stabilization PR discussed in #68793. It converts the -Z control-flow-guard debugging option into a codegen option (-C control-flow-guard), and changes the associated tests.
@rust-highfive
Copy link
Collaborator

@rust-highfive rust-highfive commented Jun 30, 2020

r? @GuillaumeGomez

(rust_highfive has picked a reviewer for you, use r? to override)
@jonas-schievink jonas-schievink added this to the 1.46 milestone Jun 30, 2020
@jonas-schievink
Copy link
Member

@jonas-schievink jonas-schievink commented Jun 30, 2020

r? @nikomatsakis
@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jun 30, 2020

@rfcbot fcp merge

Dear @rust-lang/compiler -- I move that we stabilize the control-flow-guard codegen option. You can find write-ups with the details in this comment from the tracking issue.
@rfcbot
Copy link

@rfcbot rfcbot commented Jun 30, 2020 •
edited by estebank

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.
@ollie27
Copy link
Contributor

@ollie27 ollie27 commented Jul 6, 2020

I tried compiling a simple hello world binary for none MSVC targets with -Zcontrol-flow-guard=y and it doesn't seem to be ignored. On x86_64-pc-windows-gnu along with the warning: Windows Control Flow Guard is not supported by this linker. message I'm getting an undefined reference to `__guard_dispatch_icall_fptr' error. On x86_64-unknown-linux-gnu rustc just segfaults. I can investigate further if needed but I assume we just need to make sure the cfguard module flag is only emitted when actually targeting windows-msvc.
@ajpaverd
Copy link
Contributor Author

@ajpaverd ajpaverd commented Jul 6, 2020

I tried compiling a simple hello world binary for none MSVC targets with -Zcontrol-flow-guard=y and it doesn't seem to be ignored. On x86_64-pc-windows-gnu along with the warning: Windows Control Flow Guard is not supported by this linker. message I'm getting an undefined reference to `__guard_dispatch_icall_fptr' error. On x86_64-unknown-linux-gnu rustc just segfaults. I can investigate further if needed but I assume we just need to make sure the cfguard module flag is only emitted when actually targeting windows-msvc.

Thanks for reporting this bug @ollie27! The windows-gnu target was indeed a missed cased. As far as I can see, LLVM should still be ignoring this module flag for any non-Windows target (see e.g. X86TargetMachine.cpp), so the segfault on x86_64-unknown-linux-gnu is surprising, but there could of course be an LLVM bug as well - I'll investigate further. In any case, I agree that only emitting the module flag in rustc if targeting windows-msvc would solve both of these issues, and would also be more robust. I'll submit a separate PR for this.
@ajpaverd ajpaverd mentioned this pull request Jul 6, 2020
Merged
@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jul 8, 2020

ping @petrochenkov @estebank and @eddyb
@rfcbot
Copy link

@rfcbot rfcbot commented Jul 8, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 10, 2020
@Manishearth
Rollup merge of rust-lang#74103 - ajpaverd:cfguard-msvc-only, r=nikom…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
f254701 
…atsakis

Only add CFGuard on `windows-msvc` targets

As @ollie27 pointed out in rust-lang#73893, the `cfguard` module flag causes incorrect behavior on `windows-gnu` targets. This patch restricts rustc to only add this flag for `windows-msvc` targets (this may need to be changed if other linkers gain support for CFGuard).
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 11, 2020
@Manishearth
Rollup merge of rust-lang#74103 - ajpaverd:cfguard-msvc-only, r=nikom…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
e048504 
…atsakis

Only add CFGuard on `windows-msvc` targets

As @ollie27 pointed out in rust-lang#73893, the `cfguard` module flag causes incorrect behavior on `windows-gnu` targets. This patch restricts rustc to only add this flag for `windows-msvc` targets (this may need to be changed if other linkers gain support for CFGuard).
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 11, 2020
@Manishearth
Rollup merge of rust-lang#74103 - ajpaverd:cfguard-msvc-only, r=nikom…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
34e083f 
…atsakis

Only add CFGuard on `windows-msvc` targets

As @ollie27 pointed out in rust-lang#73893, the `cfguard` module flag causes incorrect behavior on `windows-gnu` targets. This patch restricts rustc to only add this flag for `windows-msvc` targets (this may need to be changed if other linkers gain support for CFGuard).
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 11, 2020
@Manishearth
Rollup merge of rust-lang#74103 - ajpaverd:cfguard-msvc-only, r=nikom…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
fa50a87 
…atsakis

Only add CFGuard on `windows-msvc` targets

As @ollie27 pointed out in rust-lang#73893, the `cfguard` module flag causes incorrect behavior on `windows-gnu` targets. This patch restricts rustc to only add this flag for `windows-msvc` targets (this may need to be changed if other linkers gain support for CFGuard).
@bors
Copy link
Contributor

@bors bors commented Jul 11, 2020

☔ The latest upstream changes (presumably #74235) made this pull request unmergeable. Please resolve the merge conflicts.
@ajpaverd
Stabilize control-flow-guard codegen option
Loading status checks…
31c7aae
@ajpaverd ajpaverd force-pushed the ajpaverd:cfguard-stabilize branch from 23d8b24 to 31c7aae Jul 14, 2020
@rfcbot
Copy link

@rfcbot rfcbot commented Jul 18, 2020

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.
@Dylan-DPC Dylan-DPC modified the milestones: 1.46, 1.47 Jul 18, 2020
@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jul 20, 2020

@ajpaverd I believe this is ready to merge -- any final changes you had in mind?
@ajpaverd
Copy link
Contributor Author

@ajpaverd ajpaverd commented Jul 20, 2020

Thanks @nikomatsakis and all the reviewers! I think this is ready to merge. I guess the question of enabling this mitigation in libstd should be a separate discussion/PR.
@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jul 21, 2020

@bors r+
@bors
Copy link
Contributor

@bors bors commented Jul 21, 2020

📌 Commit 31c7aae has been approved by nikomatsakis
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 22, 2020
@Manishearth
Rollup merge of rust-lang#73893 - ajpaverd:cfguard-stabilize, r=nikom…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
7a8e644 
…atsakis

Stabilize control-flow-guard codegen option

This is the stabilization PR discussed in rust-lang#68793. It converts the `-Z control-flow-guard` debugging option into a codegen option (`-C control-flow-guard`), and changes the associated tests.
@Manishearth Manishearth mentioned this pull request Jul 22, 2020
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 22, 2020
@bors
Auto merge of rust-lang#74619 - Manishearth:rollup-j8gqd3r, r=Manishe…
Loading status checks…
9b76013 
…arth

Rollup of 9 pull requests

Successful merges:

 - rust-lang#73270 ([AVR] Correctly set the pointer address space when constructing pointers to functions)
 - rust-lang#73655 (va_args implementation for AAPCS.)
 - rust-lang#73893 (Stabilize control-flow-guard codegen option)
 - rust-lang#74237 (compiletest: Rewrite extract_*_version functions)
 - rust-lang#74454 (small coherence cleanup)
 - rust-lang#74538 (Guard against non-monomorphized type_id intrinsic call)
 - rust-lang#74568 (Apply rust-lang#66379 to `*mut T` `as_ref`)
 - rust-lang#74570 (Use forge links for prioritization procedure)
 - rust-lang#74589 (Update books)

Failed merges:

r? @ghost
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 22, 2020
@bors
Auto merge of rust-lang#74619 - Manishearth:rollup-j8gqd3r, r=Manishe…
Loading status checks…
af7dadb 
…arth

Rollup of 9 pull requests

Successful merges:

 - rust-lang#73270 ([AVR] Correctly set the pointer address space when constructing pointers to functions)
 - rust-lang#73655 (va_args implementation for AAPCS.)
 - rust-lang#73893 (Stabilize control-flow-guard codegen option)
 - rust-lang#74237 (compiletest: Rewrite extract_*_version functions)
 - rust-lang#74454 (small coherence cleanup)
 - rust-lang#74538 (Guard against non-monomorphized type_id intrinsic call)
 - rust-lang#74568 (Apply rust-lang#66379 to `*mut T` `as_ref`)
 - rust-lang#74570 (Use forge links for prioritization procedure)
 - rust-lang#74589 (Update books)

Failed merges:

r? @ghost
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 22, 2020
@Manishearth
Rollup merge of rust-lang#73893 - ajpaverd:cfguard-stabilize, r=nikom…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
ae9c9b0 
…atsakis

Stabilize control-flow-guard codegen option

This is the stabilization PR discussed in rust-lang#68793. It converts the `-Z control-flow-guard` debugging option into a codegen option (`-C control-flow-guard`), and changes the associated tests.
@Manishearth Manishearth mentioned this pull request Jul 22, 2020
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 22, 2020
@bors
Auto merge of rust-lang#74638 - Manishearth:rollup-l94txmo, r=Manishe…
Loading status checks…
154c3c5 
…arth

Rollup of 10 pull requests

Successful merges:

 - rust-lang#73655 (va_args implementation for AAPCS.)
 - rust-lang#73783 (Detect when `'static` obligation might come from an `impl`)
 - rust-lang#73893 (Stabilize control-flow-guard codegen option)
 - rust-lang#74237 (compiletest: Rewrite extract_*_version functions)
 - rust-lang#74528 (refactor and reword intra-doc link errors)
 - rust-lang#74538 (Guard against non-monomorphized type_id intrinsic call)
 - rust-lang#74541 (Add the aarch64-apple-darwin target )
 - rust-lang#74570 (Use forge links for prioritization procedure)
 - rust-lang#74589 (Update books)
 - rust-lang#74635 (Fix tooltip position if the documentation starts with a code block)

Failed merges:

r? @ghost
@Manishearth Manishearth mentioned this pull request Jul 22, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 22, 2020
@bors
Auto merge of rust-lang#74642 - Manishearth:rollup-148kz52, r=Manishe…
Loading status checks…
9e92106 
…arth

Rollup of 9 pull requests

Successful merges:

 - rust-lang#73655 (va_args implementation for AAPCS.)
 - rust-lang#73893 (Stabilize control-flow-guard codegen option)
 - rust-lang#74237 (compiletest: Rewrite extract_*_version functions)
 - rust-lang#74454 (small coherence cleanup)
 - rust-lang#74528 (refactor and reword intra-doc link errors)
 - rust-lang#74568 (Apply rust-lang#66379 to `*mut T` `as_ref`)
 - rust-lang#74570 (Use forge links for prioritization procedure)
 - rust-lang#74589 (Update books)
 - rust-lang#74635 (Fix tooltip position if the documentation starts with a code block)

Failed merges:

r? @ghost
@bors bors merged commit 8afb305 into rust-lang:master Jul 22, 2020
15 checks passed
15 checks passed
PR (mingw-check, ubuntu-latest-xl)
Details
PR (x86_64-gnu-llvm-8, ubuntu-latest-xl)
Details
PR (x86_64-gnu-tools, 1, ubuntu-latest-xl)
Details
try
Details
auto
Details
auto-fallible
Details
master
Details
bors build finished
Details
bors build finished
Details
bors build finished
Details
bors build finished
Details
pr Build #20200714.60 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-8) Linux x86_64-gnu-llvm-8 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors T-compiler disposition-merge finished-final-comment-period relnotes
9 participants
@ajpaverd @rust-highfive @jonas-schievink @nikomatsakis @rfcbot @ollie27 @bors @GuillaumeGomez @Dylan-DPC
You can’t perform that action at this time.