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.

Sign up for GitHub

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

Only error about MSVC + PGO + unwind if we're generating code #62615

Merged
merged 2 commits into from
Jul 18, 2019
Merged

Only error about MSVC + PGO + unwind if we're generating code #62615

merged 2 commits into from
Jul 18, 2019

Conversation

wesleywiser
Copy link
Member

In #61853, we changed the error when using PGO & MSVC toolchain & panic=unwind into a warning. However, in the compiler team meeting on 2019-07-11, we found that not everybody was in favor of that change because of the possibility of broken binaries.

This PR reverts that change so this is again an error. However, to work around an issue the Firefox team is having, we will only emit the error if we're actually supposed to generate a binary. If the rustc is invoked with --print arguments (which means that no binary will actually be emitted), then we won't emit the error because there is not a possibility of the issue occurring.

cc @EricRahm @nikomatsakis @pnkfelix @Centril

@wesleywiser
Revert "Emit warning when trying to use PGO in conjunction with unwin…
374daa7 
…ding on Windows."

This reverts commit 74a39a3.
@wesleywiser
Only error about MSVC + PGO + unwind if we're generating code
3622311 
When `rustc` is invoked with the `--print` argument, we don't actually
generate any code (unless it's the `native-static-libs` option). So we
don't need to error our in this case since there's no risk of generating
either LLVM assertions or corrupted binaries.
@rust-highfive
Copy link
Collaborator

r? @zackmdavis

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 12, 2019
nikomatsakis
nikomatsakis reviewed Jul 12, 2019
View reviewed changes
src/librustc/session/mod.rs
if sess.opts.cg.profile_generate.enabled() &&
sess.target.target.options.is_like_msvc &&
sess.panic_strategy() == PanicStrategy::Unwind {
sess.panic_strategy() == PanicStrategy::Unwind &&
sess.opts.prints.iter().all(|&p| p == PrintRequest::NativeStaticLibs) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this all because, when we are "printing" native static libs, that's the only thing we print? (I would sort of expect any.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PrintRequest::NativeStaticLibs is special in that we still generate binaries and we also emit an info message with details about those native libs during that process. All of the other print options halt compilation after the requested information is printed.

I used .all() here because we want the following behavior:

all(prints: []) => true // this causes the error to be emitted
all(prints: [NativeStaticLibs]) => true // this causes the error to be emitted
all(prints: [ _ ]) => false // skip emitting the error since we're going to stop compilation after printing the requested data

@nikomatsakis nikomatsakis added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jul 12, 2019
@nikomatsakis
Copy link
Contributor

(Nominating for beta since otherwise the version of PGO that ships can't actually be used in FF, and this is a small change.)

@wesleywiser
Copy link
Member Author

In terms of the beta, since 74a39a3 didn't land on beta, the revert of that commit does not need to be applied to beta, only 3622311 needs to be backported.

@pietroalbini pietroalbini added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 16, 2019
@nagisa nagisa mentioned this pull request Jul 18, 2019
Merged
@nagisa nagisa added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jul 18, 2019
@nagisa
Copy link
Member

nagisa commented Jul 18, 2019

Accepted for backport. When backporting keep in mind the comment above.

@nagisa
Copy link
Member

nagisa commented Jul 18, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Jul 18, 2019

📌 Commit 3622311 has been approved by nagisa

@bors bors 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 18, 2019
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jul 18, 2019
@Mark-Simulacrum
Rollup merge of rust-lang#62615 - wesleywiser:pgo_error, r=nagisa
cc16d04 
 Only error about MSVC + PGO + unwind if we're generating code

In rust-lang#61853, we changed the error when using PGO & MSVC toolchain & panic=unwind into a warning. However, in the compiler team meeting on 2019-07-11, we found that not everybody was in favor of that change because of the possibility of broken binaries.

This PR reverts that change so this is again an error. However, to work around an [issue the Firefox team is having](rust-lang#61002 (comment)), we will only emit the error if we're actually supposed to generate a binary. If the `rustc` is invoked with `--print` arguments (which means that no binary will actually be emitted), then we won't emit the error because there is not a possibility of the issue occurring.

cc @EricRahm @nikomatsakis @pnkfelix @Centril
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Jul 18, 2019
Merged
bors added a commit that referenced this pull request Jul 18, 2019
@bors
Auto merge of #62782 - Mark-Simulacrum:rollup-1hz5ya6, r=Mark-Simulacrum
311376d 
Rollup of 15 pull requests

Successful merges:

 - #61926 (Fix hyperlinks in From impls between Vec and VecDeque)
 - #62615 ( Only error about MSVC + PGO + unwind if we're generating code)
 - #62696 (Check that trait is exported or public before adding hint)
 - #62712 (Update the help message on error for self type)
 - #62728 (Fix repeated wording in slice documentation)
 - #62730 (Consolidate hygiene tests)
 - #62732 (Remove last use of mem::uninitialized from std::io::util)
 - #62740 (Add missing link to Infallible in TryFrom doc)
 - #62745 (update data_layout and features for armv7-wrs-vxworks)
 - #62749 (Document link_section arbitrary bytes)
 - #62752 (Disable Z3 in LLVM build)
 - #62764 (normalize use of backticks in compiler messages for librustc/lint)
 - #62774 (Disable simd_select_bitmask test on big endian)
 - #62777 (Self-referencial type now called a recursive type)
 - #62778 (Emit artifact notifications for dependency files)

Failed merges:

 - #62746 ( do not use mem::uninitialized in std::io)

r? @ghost
@bors bors merged commit 3622311 into rust-lang:master Jul 18, 2019
@wesleywiser wesleywiser mentioned this pull request Jul 19, 2019
Merged
@pietroalbini pietroalbini mentioned this pull request Jul 22, 2019
Merged
@pietroalbini pietroalbini removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jul 22, 2019
bors added a commit that referenced this pull request Jul 22, 2019
@bors
Auto merge of #62866 - pietroalbini:beta-rollup, r=pietroalbini
97789c4 
[beta] Rollup backports

Cherry picked:

* rustc_target: avoid negative register counts in the SysV x86_64 ABI. #62380
* Fix ICEs when `Self` is used in type aliases #62417
* Raise the default recursion limit to 128 #62450
* Handle errors during error recovery gracefully #62604
* Correctly break out of recovery loop #62607
* Cancel unemitted diagnostics during error recovery #62666
* ci: pin awscli dependencies #62856
* Ensure that checkout is with \n line endings #62564

Rolled up:

* [beta] Backport #62615 #62793
* [beta] Fix #62660 #62792

r? @ghost
bors added a commit that referenced this pull request Jul 22, 2019
@bors
Auto merge of #62866 - pietroalbini:beta-rollup, r=Centril
61d1607 
[beta] Rollup backports

Cherry picked:

* rustc_target: avoid negative register counts in the SysV x86_64 ABI. #62380
* Fix ICEs when `Self` is used in type aliases #62417
* Raise the default recursion limit to 128 #62450
* Handle errors during error recovery gracefully #62604
* Correctly break out of recovery loop #62607
* Cancel unemitted diagnostics during error recovery #62666
* ci: pin awscli dependencies #62856
* Ensure that checkout is with \n line endings #62564

Rolled up:

* [beta] Backport #62615 #62793
* [beta] Fix #62660 #62792

r? @ghost
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikomatsakis nikomatsakis nikomatsakis left review comments

Assignees

@zackmdavis zackmdavis

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@wesleywiser @rust-highfive @nikomatsakis @nagisa @bors @zackmdavis @pietroalbini