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

LLVM does not support PGO on Windows with -Cpanic=unwind #61002

Closed
michaelwoerister opened this issue May 21, 2019 · 12 comments · Fixed by #87307
Closed

LLVM does not support PGO on Windows with -Cpanic=unwind #61002

michaelwoerister opened this issue May 21, 2019 · 12 comments · Fixed by #87307
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. O-windows-msvc Toolchain: MSVC, Operating system: Windows T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@michaelwoerister
Copy link
Member

michaelwoerister commented May 21, 2019 •
edited
Loading

LLVM's "IR-level" instrumentation, which is used by rustc to generate PGO instrumented binaries, does not yet work with exception handling on Windows MSVC. The problem has been reported to LLVM for C++ here: https://bugs.llvm.org/show_bug.cgi?id=41279

This also affects Rust programs built with -Cpanic=unwind for Windows MSVC. As long as LLVM does not support exception handling there, it is a known limitation that PGO can only be used with -Cpanic=abort on this platform.
@jonas-schievink jonas-schievink added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. O-windows-msvc Toolchain: MSVC, Operating system: Windows T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 21, 2019
@michaelwoerister
Copy link
Member Author

PGO does not yet work on windows-gnu for other reasons (see #61002). Once it does, we should check if this problem is specific to MSVC.

This was referenced May 21, 2019
Merged
Closed
@mati865
Copy link
Contributor

mati865 commented May 21, 2019

I have working x86_64 windows-gnu (64 bit builds use SEH) build with PGO I'll open PR once #60981 is merged.
I can do testing in meantime.

FWIW simple hello world example works fine after going through PGO.

@michaelwoerister
Copy link
Member Author

@mati865 if you test, be sure to compile your LLVM with assertions: https://github.com/rust-lang/rust/pull/61005/files#diff-1c226b200b9385d77428f0b4418366e4R1277

Thanks for looking into it!

@mati865
Copy link
Contributor

mati865 commented May 21, 2019

I'll recompile it with assertions today later. Should the test case be using any specific code like panic!()?

@michaelwoerister
Copy link
Member Author

The following test case runs into the problem with MSVC:
https://github.com/rust-lang/rust/blob/master/src/test/run-make-fulldeps/pgo-use/main.rs

See https://github.com/rust-lang/rust/blob/master/src/test/run-make-fulldeps/pgo-use/Makefile for how the file is compiled.

Centril added a commit to Centril/rust that referenced this issue May 21, 2019
@Centril
Rollup merge of rust-lang#61005 - michaelwoerister:error-pgo-windows-…
18d728c 
…unwind, r=zackmdavis

Emit error when trying to use PGO in conjunction with unwinding on Windows.

This PR makes `rustc` emit an error when trying use PGO in conjunction with `-Cpanic=unwind` on Windows, isn't supported by LLVM yet. The error messages points to rust-lang#61002, which documents this known limitation.
@mati865
Copy link
Contributor

mati865 commented May 21, 2019

It does not reproduce with gnu toolchain with Rust and LLVM assertions enabled, I'll try to remember about fixing #61005 along with gnu profiler.

@michaelwoerister
Copy link
Member Author

Cool!

@michaelwoerister
Copy link
Member Author

I just confirmed that (for me locally) PGO seems to work on Windows GNU. That is, I built ripgrep with instrumentation, ran it, and rebuilt it with the profiling data collected. It worked just fine, no crashes. So I updated the original issue text to just mention MSVC instead of Windows in general.

bors added a commit that referenced this issue May 30, 2019
@bors
Auto merge of #61005 - michaelwoerister:error-pgo-windows-unwind, r=z…
c28084a 
…ackmdavis

Emit error when trying to use PGO in conjunction with unwinding on Windows.

This PR makes `rustc` emit an error when trying use PGO in conjunction with `-Cpanic=unwind` on Windows, isn't supported by LLVM yet. The error messages points to #61002, which documents this known limitation.
@EricRahm
Copy link
Contributor

EricRahm commented Jun 8, 2019

It looks like #61005 is a bit too aggressive with its error reporting. For example when invoking rustc, but not compiling in our Firefox build we can the following build error:

00:53:01 INFO - error: failed to run rustc to learn about target-specific information
00:53:01 INFO - Caused by:
00:53:01 INFO - process didn't exit successfully: z:/task_1559774436/build/src/rustc/bin/rustc.exe - --crate-name ___ --print=file-names -C opt-level=2 -C debuginfo=2 --cap-lints warn '-Zpgo-gen=rust-%m.profraw' --target i686-pc-windows-msvc --crate-type bin --crate-type rlib --crate-type dylib --crate-type cdylib --crate-type staticlib --crate-type proc-macro (exit code: 1)
00:53:01 INFO - --- stdout
00:53:01 INFO - .exe
00:53:01 INFO - lib.rlib
00:53:01 INFO - ___.dll
00:53:01 INFO - ___.dll
00:53:01 INFO - ___.lib
00:53:01 INFO - ___.dll
00:53:01 INFO - --- stderr
00:53:01 INFO - error: Profile-guided optimization does not yet work in conjunction with -Cpanic=unwind on Windows when targeting MSVC. See #61002 for details.
00:53:01 ERROR - error: aborting due to previous error

I'm not sure the best solution while mw is away; perhaps we should revert #61005 for now.

@nikomatsakis
Copy link
Contributor

Maybe we can just convert the error to a warning?

@nikomatsakis
Copy link
Contributor

Not the best long-term solution but seems ok

@EricRahm EricRahm mentioned this issue Jun 14, 2019
Merged
Centril added a commit to Centril/rust that referenced this issue Jul 10, 2019
@Centril
Rollup merge of rust-lang#61853 - EricRahm:use_warning, r=varkor
3c3e375 
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.
@wesleywiser wesleywiser mentioned this issue Jul 12, 2019
Merged
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue 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
@tmandry
Copy link
Member

tmandry commented Jul 8, 2020

It looks like the upstream bug was marked fixed a year ago. What's the status of this issue in rustc?

@mati865 mati865 mentioned this issue Jul 15, 2020
Merged
baumanj added a commit to mozilla/mp4parse-rust that referenced this issue Aug 25, 2020
@baumanj
Fix bustage from BMO bug 1660551
b76d66b 
See https://bugzilla.mozilla.org/show_bug.cgi?id=1660551#c3

This appears related to rust-lang/rust#61002
though, I'm not sure what changed between versions of num-traits to make
this an issue, pinning to the version we were already using seems to
function as a workaround for now.
@baumanj baumanj mentioned this issue Aug 25, 2020
Merged
michaelwoerister added a commit to michaelwoerister/rust that referenced this issue Jul 20, 2021
@michaelwoerister
Allow combining -Cprofile-generate and -Cpanic=unwind when targeting
d56c02d 
MSVC.

The LLVM limitation that previously prevented this has been fixed in LLVM
9 which is older than the oldest LLVM version we currently support.

See rust-lang#61002.
@michaelwoerister michaelwoerister mentioned this issue Jul 20, 2021
Merged
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jul 21, 2021
@Dylan-DPC
Rollup merge of rust-lang#87307 - michaelwoerister:pgo-unwind-msvc, r…
7eed57b 
…=nagisa

Allow combining -Cprofile-generate and -Cpanic=unwind when targeting MSVC.

The LLVM limitation that previously prevented this has been fixed in LLVM 9 which is older than the oldest LLVM version we currently support.

Fixes rust-lang#61002.

r? `@nagisa` (or anyone else from `@rust-lang/wg-llvm)`
@bors bors closed this as completed in 90d6d33 Jul 22, 2021
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jun 24, 2023
@glandium
Bug 1840008 - Undo bug 1632776. r=firefox-build-system-reviewers,andi
16bc411 
The bug it worked around
(rust-lang/rust#61002) was fixed in rustc
1.55, and we currently require 1.66.

Differential Revision: https://phabricator.services.mozilla.com/D181843
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Jun 27, 2023
@glandium
Bug 1840008 - Undo bug 1632776. r=firefox-build-system-reviewers,andi
b7e54cc 
The bug it worked around
(rust-lang/rust#61002) was fixed in rustc
1.55, and we currently require 1.66.

Differential Revision: https://phabricator.services.mozilla.com/D181843
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Jul 1, 2023
@marco-c
Bug 1840008 - Undo bug 1632776. r=firefox-build-system-reviewers,andi
9998a0f 
The bug it worked around
(rust-lang/rust#61002) was fixed in rustc
1.55, and we currently require 1.66.

Differential Revision: https://phabricator.services.mozilla.com/D181843

UltraBlame original commit: 70bf3adb2de59cdd39165ebe10cbd76dcc70c6da
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Jul 1, 2023
@marco-c
Bug 1840008 - Undo bug 1632776. r=firefox-build-system-reviewers,andi
3362385 
The bug it worked around
(rust-lang/rust#61002) was fixed in rustc
1.55, and we currently require 1.66.

Differential Revision: https://phabricator.services.mozilla.com/D181843

UltraBlame original commit: 70bf3adb2de59cdd39165ebe10cbd76dcc70c6da
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Jul 1, 2023
@marco-c
Bug 1840008 - Undo bug 1632776. r=firefox-build-system-reviewers,andi
9cf4968 
The bug it worked around
(rust-lang/rust#61002) was fixed in rustc
1.55, and we currently require 1.66.

Differential Revision: https://phabricator.services.mozilla.com/D181843

UltraBlame original commit: 70bf3adb2de59cdd39165ebe10cbd76dcc70c6da
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. O-windows-msvc Toolchain: MSVC, Operating system: Windows 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 a pull request may close this issue.

Allow combining -Cprofile-generate and -Cpanic=unwind when targeting MSVC. michaelwoerister/rust
6 participants
@nikomatsakis @EricRahm @mati865 @jonas-schievink @michaelwoerister @tmandry