dolphin
dolphin copied to clipboard
dolphin-emu
clang-cl compatibility
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, specify an include directory to the PCH dir. -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 #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). WIL also had another issue as it relied on Windows.h's min, and Dolphin defines NOMINMAX everywhere, so min wouldn't be defined and lead to a compile error. This wasn't an issue in MSVC due to its template parsing behavior, which prevented this error in practice. The fix was sent upstream and was merged, and included in the linked PR now.
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 it after I reported it, so the latest master has this fix. This has been done in PR #11626
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.
qt_no_entrypoint also needs to be placed in the dolphin-emu properties. This prevents Qt6Entrypoint from being linked, which it shouldn't in the first place as Dolphin handles the entrypoint itself. lld-link would run into 3 different kinds of errors trying to link Qt6Entrypoint: the Qt6Entrypoint objects were compiled with /GL, which can't be used with lld-link as the object would be in some MSVC internal format, Qt6Entrypoint has a function calling main, which is never defined, and Qt6Entrypoint has a definition of wWinMain, which would conflict with Dolphin's own wWinMain definition.
After all these changes, it builds fine with clang-cl, and appears to work correctly (well, playing various games seems to work out fine).
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.
I'm actually slightly wrong on this. MSVC turns off the delayed template parsing behavior with /permissive-. It does not error still however, because the min use depends on TString (although even then the types for the parameters are still known so this somewhat doesn't make sense), so it decides to delay parsing of that part of the template function despite /permissive- being set. clang-cl does not have the same behavior, instead it will still parse that part of the template function and error due to no min function/macro existing.
is the mgba project file update supposed to be along with updating the submodule? maybe move the submodule + project update into a different PR.
is the mgba project file update supposed to be along with updating the submodule? maybe move the submodule + project update into a different PR.
Without the project files also updated, the build will fail using the VS sln (leading to Windows buildbot failures), cmake builds are fine anyways as it uses the cmake scripts from mGBA upstream here.
Without the mGBA submodule update, the build will fail when using clang-cl (due to struct option being declared within a function declaration, which errored once it got to the function definition; this is something which is probably UB anyways and MSVC accepted it for ??? reasons; both gcc and clang just error no matter what with this scenario)
i'm saying: put the submodule update in a separate pr. the vcxproj update will go in that pr as well.