vs: update to 17.5 #11599
Merged
vs: update to 17.5 #11599
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
CasualPokePlayer
added a commit
to CasualPokePlayer/dolphin
that referenced
this pull request
Feb 27, 2023
This is a doozy. To start, we have to set -fno-ms-compatibility in order for __VA_OPT__ to work, as clang-cl doesn't understand /Zc:preprocessor. This causes various issues, but most of them are simple fixes (the most guilty of these are reliance of function pointer to object pointer casts done implicitly or with a static_cast, neither of which are allowed in standard C++, they must be done with a reinterpret_cast). The PCH was also broken as it relied on MSVC specific include rules, but fix here is trivial, just send in the absolute path. -U__STDC__ also needs to be sent, as otherwise some MSVC headers decide not to declare certain functions (first case being itoa, as the ucrt stdlib.h only defines _itoa with __STDC__ true, only when it's not true then itoa will be defined, not sure of other cases as this error appears quickly in SDL, the first thing built). In any case, we probably want -fno-ms-compatibility set, as it disallows "illegal" (i.e. not allowed at all by the standard) MSVC extensions, and the "legal" MSVC extensions get enabled still with the implicit -fms-extensions. However, this commit doesn't allow for clang-cl to build yet. It needs PR dolphin-emu#11599 merged first, as otherwise the template shadowing (MSVC specific, disallowed with -fno-ms-compatibility) abused in WIL prevents compilation (the PR updates WIL which no longer faces that issue). Additionally, WIL still needs "help" as it relies on Windows.h's min, and Dolphin defines NOMINMAX everywhere, so it won't be defined and leads to a compile error. This doesn't occur in MSVC due to the delayed template parsing behavior, which prevents this error in practice. It also somewhat doesn't appear in clang-cl, as by default it does this delayed template parsing. However, even if you try to compile, you eventually get to DolphinQt which has /permissive- set. This flag apparently implies -fno-delayed-template-parsing, leading to compile errors. Due to this, I've opted to just switch on -fno-delayed-template-parsing entirely, which seems to work fine (WIL is the only case it's ever a problem, which some macro magic can work around it). It's still not pretty, a more proper fix probably should be sent upstream (probably can just have it use std::min in c++ mode as it's reasonable to expect c++ projects to define NOMINMAX due to std::min/std::max breaking with Windows.h's min/max). mGBA submodule also needed an update to the latest master as of now. In the making of this I encountered a build error with mGBA, and endrift quickly fixed after I reported it, so the latest master has this fix. Various new warnings propped up with clang, which I've disabled most I've came across, with some I've opted to just fix. The more notable of these fixes are just the various uses of null pointer arithmetic, which is just UB. Others are mostly just minor things like constructor order, invalid narrowing, and brace recommendations (nearly all coming from Window's only Dolphin code, I assume this only got past due to MSVC's more lax warnings). Some more of these warnings should probably be fixed, but I haven't bothered with them. Some clang (or just clang-cl) bugs popped up in this, which workarounds have been placed with comments explaining the situation. Even with the above PR mentioned however, this still will not build. It will need a rebuild of the Qt parts with the -ltcg flag not passed to Qt's configure. This flag sets up MSVC's version of LTO, which prevents interoperability with clang-cl and even so is a bad idea in the first place. The resulting builds have very little stablity guarentees with MSVC itself, some MSVC update popping up can potentially just cause issues (either build fail or just miscompilation) a normal build will not have. I believe after all that, it should build completely fine, but without the Qt parts the final link step can't be completed. The no-gui executable builds fine of course, and it appears to be perfectly functional for what it is.
CasualPokePlayer
added a commit
to CasualPokePlayer/dolphin
that referenced
this pull request
Feb 27, 2023
This is a doozy. To start, we have to set -fno-ms-compatibility in order for __VA_OPT__ to work, as clang-cl doesn't understand /Zc:preprocessor. This causes various issues, but most of them are simple fixes (the most guilty of these are reliance of function pointer to object pointer casts done implicitly or with a static_cast, neither of which are allowed in standard C++, they must be done with a reinterpret_cast). The PCH was also broken as it relied on MSVC specific include rules, but fix here is trivial, just send in the absolute path. -U__STDC__ also needs to be sent, as otherwise some MSVC headers decide not to declare certain functions (first case being itoa, as the ucrt stdlib.h only defines _itoa with __STDC__ true, only when it's not true then itoa will be defined, not sure of other cases as this error appears quickly in SDL, the first thing built). In any case, we probably want -fno-ms-compatibility set, as it disallows "illegal" (i.e. not allowed at all by the standard) MSVC extensions, and the "legal" MSVC extensions get enabled still with the implicit -fms-extensions. However, this commit doesn't allow for clang-cl to build yet. It needs PR dolphin-emu#11599 merged first, as otherwise the template shadowing (MSVC specific, disallowed with -fno-ms-compatibility) abused in WIL prevents compilation (the PR updates WIL which no longer faces that issue). Additionally, WIL still needs "help" as it relies on Windows.h's min, and Dolphin defines NOMINMAX everywhere, so it won't be defined and leads to a compile error. This doesn't occur in MSVC due to the delayed template parsing behavior, which prevents this error in practice. It also somewhat doesn't appear in clang-cl, as by default it does this delayed template parsing. However, even if you try to compile, you eventually get to DolphinQt which has /permissive- set. This flag apparently implies -fno-delayed-template-parsing, leading to compile errors. Due to this, I've opted to just switch on -fno-delayed-template-parsing entirely, which seems to work fine (WIL is the only case it's ever a problem, which some macro magic can work around it). It's still not pretty, a more proper fix probably should be sent upstream (probably can just have it use std::min in c++ mode as it's reasonable to expect c++ projects to define NOMINMAX due to std::min/std::max breaking with Windows.h's min/max). mGBA submodule also needed an update to the latest master as of now. In the making of this I encountered a build error with mGBA, and endrift quickly fixed after I reported it, so the latest master has this fix. Various new warnings propped up with clang, which I've disabled most I've came across, with some I've opted to just fix. The more notable of these fixes are just the various uses of null pointer arithmetic, which is just UB. Others are mostly just minor things like constructor order, invalid narrowing, and brace recommendations (nearly all coming from Window's only Dolphin code, I assume this only got past due to MSVC's more lax warnings). Some more of these warnings should probably be fixed, but I haven't bothered with them. Some clang (or just clang-cl) bugs popped up in this, which workarounds have been placed with comments explaining the situation. Even with the above PR mentioned however, this still will not build. It will need a rebuild of the Qt parts with the -ltcg flag not passed to Qt's configure. This flag sets up MSVC's version of LTO, which prevents interoperability with clang-cl and even so is a bad idea in the first place. The resulting builds have very little stablity guarentees with MSVC itself, some MSVC update popping up can potentially just cause issues (either build fail or just miscompilation) a normal build will not have. I believe after all that, it should build completely fine, but without the Qt parts the final link step can't be completed. The no-gui executable builds fine of course, and it appears to be perfectly functional for what it is.
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.
applies some TODOs for vs 17.5, which enable some new compiler checks
the checks fired on WIL code, so updated that to latest