Skip to content

Only add CFGuard on windows-msvc targets - #74103

Merged
bors merged 1 commit into
rust-lang:masterfrom
ajpaverd:cfguard-msvc-only
Jul 11, 2020
Merged

Only add CFGuard on windows-msvc targets#74103
bors merged 1 commit into
rust-lang:masterfrom
ajpaverd:cfguard-msvc-only

Conversation

@ajpaverd

@ajpaverdajpaverd commented Jul 6, 2020

Copy link
Copy Markdown
Contributor

As @ollie27 pointed out in #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).

@rust-highfive

rust-highfive commented Jul 6, 2020

Copy link
Copy Markdown
Contributor

r? @ecstatic-morse

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 6, 2020
@ollie27

ollie27 commented Jul 7, 2020

Copy link
Copy Markdown
Contributor

I think this could do with a couple of ui tests like:

// run-pass// compile-flags: -Z control-flow-guard// ignore-msvcfnmain(){println!("Hello, world!");}

and

// run-pass// compile-flags: -Z control-flow-guard// only-msvcfnmain(){println!("Hello, world!");}

to make sure we're actually producing working binaries and the warning is printed when it should be.

@ajpaverd

ajpaverd commented Jul 7, 2020

Copy link
Copy Markdown
ContributorAuthor

Thanks @ollie27, do the new ui tests cover the cases you had in mind?

@ollie27

ollie27 commented Jul 7, 2020

Copy link
Copy Markdown
Contributor

Thanks @ollie27, do the new ui tests cover the cases you had in mind?

Yep, looks good.

@nikomatsakis

nikomatsakis commented Jul 8, 2020

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented Jul 8, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit 04a78c6a02db8a70da0fd71f7ad3e878e20c16e7 has been approved by nikomatsakis

@borsbors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 8, 2020
@nikomatsakis

nikomatsakis commented Jul 8, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

@bors r- -- wait, CI looks unhappy?

@borsbors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 8, 2020
@ajpaverd

ajpaverd commented Jul 8, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Yes, I'm wondering if this is the correct way to test compiler warnings? In the CI, the --emit metadata flag seems to suppress this warning.

@nikomatsakis

nikomatsakis commented Jul 8, 2020

Copy link
Copy Markdown
Contributor

@ajpaverd where is that warning emitted, anyway?

I think that with a run-pass test, the stderr file might not include compiler output, I'm not sure.

@ajpaverd

ajpaverd commented Jul 8, 2020

Copy link
Copy Markdown
ContributorAuthor

@nikomatsakis these warnings are emitted from librustc_codegen_ssa/back/linker.rs. So it looks like the test passes when run normally (L1509), but fails when run with --pass=check (L2765) because there's no codegen. The CI seems to run ui tests in both modes, so I'm not sure what to do here. Should the warnings go somewhere else (or be removed)?

@nikomatsakis

nikomatsakis commented Jul 8, 2020

Copy link
Copy Markdown
Contributor

@ajpaverd ah, I see, that's annoying. I guess the question then is whether there is some kind of test mode that only runs normally

@ajpaverd

ajpaverd commented Jul 9, 2020

Copy link
Copy Markdown
ContributorAuthor

So I think changing this to a run-make test gets us some of the way. We can test that this compiler flag produces runnable binaries on all targets (the issue reported by @ollie27). We don't have an explicit check that the warning is emitted on unsupported targets, but I'm not sure that's strictly necessary (and I'm not sure how we would test it with the current CI config).

@petrochenkov

petrochenkov commented Jul 9, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

You can use // ignore-pass to make the test pass in --pass=check mode.

Emitting hard-coded warnings is never a good idea though, they've been gradually removed from the compiler in favor of lints or hard errors.
There are many-C options that don't have any effect for some targets or linkers, sometimes we don't even decide whether they have an effect or not - LLVM does that for us, usually such options are just silently accepted without warnings.

@ajpaverd

ajpaverd commented Jul 9, 2020

Copy link
Copy Markdown
ContributorAuthor

@petrochenkov thanks! I'm actually in favor of removing these warnings to make this cleaner and more in-line with other -C options. I'll update this PR.

@nikomatsakis

nikomatsakis commented Jul 10, 2020

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented Jul 10, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit 1ca7bfe has been approved by nikomatsakis

@borsbors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 10, 2020
@Manishearth

Manishearth commented Jul 10, 2020

Copy link
Copy Markdown
Member

@bors rollup=iffy

Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 10, 2020
@Manishearth
Rollup merge ofrust-lang#74103- ajpaverd:cfguard-msvc-only, r=nikom…
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).
@ManishearthManishearth mentioned this pull request Jul 10, 2020
Closed
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 11, 2020
@Manishearth
Rollup merge ofrust-lang#74103- ajpaverd:cfguard-msvc-only, r=nikom…
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).
@ManishearthManishearth mentioned this pull request Jul 11, 2020
Closed
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 11, 2020
@Manishearth
Rollup merge ofrust-lang#74103- ajpaverd:cfguard-msvc-only, r=nikom…
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).
@ManishearthManishearth mentioned this pull request Jul 11, 2020
Closed
@ManishearthManishearth mentioned this pull request Jul 11, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 11, 2020
@bors
Auto merge ofrust-lang#74235- Manishearth:rollup-bgs3q14, r=Manishe…
346aec9
…arth
Rollup of 19 pull requests
Successful merges:
- rust-lang#71322 (Accept tuple.0.0 as tuple indexing (take 2))
- rust-lang#72303 (Add core::future::{poll_fn, PollFn})
- rust-lang#73862 (Stabilize casts and coercions to `&[T]` in const fn)
- rust-lang#73887 (stabilize const mem::forget)
- rust-lang#73989 (adjust ub-enum test to be endianess-independent)
- rust-lang#74045 (Explain effects of debugging options from config.toml)
- rust-lang#74076 (Add `read_exact_at` and `write_all_at` to WASI's `FileExt`)
- rust-lang#74099 (Add VecDeque::range* methods)
- rust-lang#74100 (Use str::strip* in bootstrap)
- rust-lang#74103 (Only add CFGuard on `windows-msvc` targets)
- rust-lang#74109 (Only allow `repr(i128/u128)` on enum)
- rust-lang#74122 (Start-up clean-up)
- rust-lang#74125 (Correctly mark the ending span of a match arm)
- rust-lang#74127 (Avoid "whitelist")
- rust-lang#74129 (:arrow_up: rust-analyzer)
- rust-lang#74135 (Update books)
- rust-lang#74145 (Update rust-installer to latest version)
- rust-lang#74161 (Fix disabled dockerfiles)
- rust-lang#74162 (take self by value in ToPredicate)
Failed merges:
r? @ghost
@bors
bors merged commit fa50a87 into rust-lang:masterJul 11, 2020
@cuvipercuviper added this to the 1.46 milestone May 2, 2024
@alvinhochunalvinhochun mentioned this pull request Aug 9, 2024
Merged
@ognevny

ognevny commented Aug 10, 2024

Copy link
Copy Markdown
Contributor

did the situation change for non-msvc targets?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@ecstatic-morseecstatic-morse

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

1.46

Development

Successfully merging this pull request may close these issues.

10 participants

@ajpaverd@rust-highfive@ollie27@nikomatsakis@bors@petrochenkov@Manishearth@ognevny@cuviper@ecstatic-morse