Skip to content

cmake: Restrict MSVC-specific workaround to affected versions #32499

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
Closed
wants to merge 1 commit into from
Closed

cmake: Restrict MSVC-specific workaround to affected versions #32499

wants to merge 1 commit into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented May 14, 2025

The recent MSVC version 17.14 has fixed a bug, so we can now enable compilation of fuzz/utxo_snapshot.cpp with the updated compiler.

@DrahtBot
Copy link
Contributor

DrahtBot commented May 14, 2025 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32499.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK sipsorcery

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #31507 (build: Use clang-cl to build on Windows natively by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@hebasto hebasto mentioned this pull request May 14, 2025
Closed
@maflcko
Copy link
Member

maflcko commented May 14, 2025

No objection, but I guess it also seems fine to wait a few weeks until GitHub bumps their Windows image and then just bump the msvc minimum required version.

@hebasto
Copy link
Member Author

hebasto commented May 14, 2025

No objection, but I guess it also seems fine to wait a few weeks until GitHub bumps their Windows image and then just bump the msvc minimum required version.

Do we really need to force Windows developers to update their toolchains in this case?

@maflcko
Copy link
Member

maflcko commented May 14, 2025

Do we really need to force Windows developers to update their toolchains in this case?

I expect the number of Windows devs is still small enough to just ping and ask them, when the time has come.

@hebasto
Copy link
Member Author

hebasto commented May 14, 2025

Do we really need to force Windows developers to update their toolchains in this case?

I expect the number of Windows devs is still small enough to just ping and ask them, when the time has come.

Friendly ping @davidgumberg @hodlinator @sipsorcery :)

@sipsorcery
Copy link
Contributor

utACK 6779430.

17.14 is still in preview so can't test yet (would rather avoid the >10GB download and install for the sake of a week or two).

@hebasto
Copy link
Member Author

hebasto commented May 14, 2025 •
edited
Loading

17.14 is still in preview so can't test yet (would rather avoid the >10GB download and install for the sake of a week or two).

From https://learn.microsoft.com/en-us/visualstudio/releases/2022/release-notes:

17.14 ... was released on May 13, 2025.

@sipsorcery
Copy link
Contributor

Just checked again (1 hour later) and installing 17.14 now.

Seems it was just my VS Installer instance being crap and telling me there were no new updates and offering 17.14 as a preview.

@DrahtBot DrahtBot mentioned this pull request May 15, 2025
Open
@hodlinator
Copy link
Contributor

nit: Should PR description include "ref #31303"?

Tried removing the compiler_has_bug-logic completely and building with the version I had installed: 17.13.4...

cmake -B build --preset vs2022 -DVCPKG_INSTALL_OPTIONS="--x-buildtrees-root=C:\vcpkg" -DBUILD_FUZZ_BINARY=ON -DBUILD_GUI=OFF
cmake --build build --config Release -j16

...did not encounter the internal compiler error?

Couldn't repro the ICE from https://developercommunity.visualstudio.com/t/C1001:-Internal-compiler-error-with-st/10830350 either:

> cl.exe /std:c++20 /c ice_demo.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.43.34809 for x86
Copyright (C) Microsoft Corporation.  All rights reserved.

ice_demo.cpp

>

davidgumberg
davidgumberg reviewed May 15, 2025
View reviewed changes
src/test/fuzz/CMakeLists.txt
# - https://github.com/bitcoin/bitcoin/issues/31303
# - https://developercommunity.visualstudio.com/t/C1001:-Internal-compiler-error-with-st/10830350
# Fixed in Visual Studio 2022 version 17.14.
set(compiler_has_bug "$<AND:$<CXX_COMPILER_ID:MSVC>,$<VERSION_GREATER_EQUAL:$<CXX_COMPILER_VERSION>,19.42>,$<VERSION_LESS:$<CXX_COMPILER_VERSION>,19.44>>")
Copy link
Contributor

@davidgumberg davidgumberg May 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Question: Where do these version numbers come from? Is there a reference to correlate the Visual Studio version with the MSVC versions, or did you get these by testing manually?

Copy link
Contributor

@davidgumberg davidgumberg May 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Self-answered after asking: There is a table on this page https://learn.microsoft.com/en-us/cpp/overview/compiler-versions?view=msvc-170#version-macros

@davidgumberg
Copy link
Contributor

Tried removing the compiler_has_bug-logic completely and building with the version I had installed: 17.13.4...

[...]

...did not encounter the internal compiler error?

My guess is either that a hotfix was pushed for 19.43, or that this bug was already fixed in 19.43, and the labels / bot response on the msvc bugtracker are inaccurate or canned, don't have an MSVC in front of me, but using Godbolt's MSVC 19.42.34441 the minimal reproduction causes internal error C1001, and MSVC 19.43.34810 does not.

@hebasto
Copy link
Member Author

hebasto commented May 16, 2025

Closing in favour of #32525.

@hebasto hebasto closed this May 16, 2025
@davidgumberg davidgumberg mentioned this pull request May 16, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@davidgumberg davidgumberg davidgumberg left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Build system Windows
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@hebasto @DrahtBot @maflcko @sipsorcery @hodlinator @davidgumberg