Emit warning when trying to use PGO in conjunction with unwinding on … #61853
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @varkor (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. |
rust-highfive
added
the
S-waiting-on-review
|
@nikomatsakis This changes to just a warning per your suggestion. |
|
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
fbstj
reviewed
Jun 15, 2019
|
Looks good — I'll approve once @fbstj's comment has been fixed. |
|
@EricRahm: could you squash your two commits into one? (It keeps things tidier.) Thanks! |
|
@varkor, I'm not sure how to squash a commit that was added via the suggestion UI. I suppose that means pulling, rebasing and force pushing from the machine I have a checkout out on? If that's the case I can do it in a few days. |
Mark-Simulacrum
force-pushed
the
use_warning
branch
from
521dcd7
to
74a39a3
Compare
|
I've gone ahead and rebased and squashed this PR down for you, thanks! @bors r=varkor |
|
📌 Commit 74a39a3 has been approved by |
bors
added
S-waiting-on-bors
Thank you very much for finishing this off, I completely forgot to push my cleaned up patch! |
Centril
added a commit
to Centril/rust
that referenced
this pull request
Jul 10, 2019
Emit warning when trying to use PGO in conjunction with unwinding on … …Windows. This reduces the error introduced for rust-lang#61002 to just a warning.
bors
added a commit
that referenced
this pull request
Jul 10, 2019
Rollup of 5 pull requests Successful merges: - #61853 (Emit warning when trying to use PGO in conjunction with unwinding on …) - #62278 (Add Iterator::partition_in_place() and is_partitioned()) - #62283 (Target::arch can take more than listed options) - #62393 (Fix pretty-printing of `$crate` (take 4)) - #62474 (Prepare for LLVM 9 update) Failed merges: r? @ghost
nikomatsakis
added
the
beta-nominated
|
I'm nominated this for beta because it would be a big help to FF -- would allow them to make use of the PGO that was stabilized on beta. Moreover, risk is about as low as it can get. |
nikomatsakis
added
the
T-compiler
|
discussed at T-compiler meeting. There were some misgivings about the nature of this PR, and therefore I and others were hesitant to approve a backport without getting some more information first. @wesleywiser is taking point on finding out more about the nature of the failures that occur when you coming PGO with Windows, to try to ensure we are not unduly increasing our surface of undefined behavior. |
nagisa
removed
the
beta-nominated
|
This PR will not be backported, as this is superseded by #62615. |
Mark-Simulacrum
added a commit
to Mark-Simulacrum/rust
that referenced
this pull request
Jul 18, 2019
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…Windows.
This reduces the error introduced for #61002 to just a warning.