enable clang builds for Windows #188
Conversation
Most of the warnings are from Windows specific code, as the rest of the codebase is already fairly clean. This change fixes a couple of (benign) bugs discovered by these warnings. In hiro/windows/action/menu.cpp, menu-item.cpp: A function pointer was being passed in place of a bool argument to nall::image::scale(). At one point the name Interpolation::Linear referred to an enum. In any case, the argument in question defaults to true - specifying linear interpolation - so it is unneeded. In hiro/windows/widget/widget.hpp: The function minimumSize() was being overloaded instead of overridden due to a missing const qualifier. It doesn't appear to have had any negative side effects in ares.
These warnings can often catch genuine bugs. The only tautological compare warning I observed was reporting a real bug in the SFC core. The shift count overflow warnings appeared to be for cases that were benign, though only because I already fixed a significant one in the instruction tracer. In general, such shifts are undefined behavior and require surrendering to the whims of the compiler and the processor architecture, so they are worth avoiding. The large diff in ares/component/processor/tlcs900h/instruction.cpp is mostly uninteresting. I had to make the control flow more explicit for undefined instructions to stop the compiler from complaining about bad template specializations. While I was at it, I enabled unused-value warnings for that file, which only required adding a few void casts.
When building with clang, two static inline members of Node subclasses - identifier and register - were not consistently initialized in that order during application startup. Because the constructor of register depends on identifier, this poses a problem. I'm not sure if clang is on the wrong or if the initialization order is actually unspecified by the language standard, but this situation can be easily avoided, so I opted to do just that.
Whatever defining this manually was intended to fix, today it is only breaking things. The mingw headers define this as 0x700 if you are linking against MSVCRT and 0xE00 if you are linking against UCRT. This is a mingw-internal macro, and there is currently no comparison in the headers that evaluates differently for 0x601 than for 0x700. However, always forcing it to 0x601 as nall does breaks linking against UCRT. We should let mingw define this appropriately.
Also, you can now set symbols=true to forcibly generate debugging information for any non-debug build configuration.
There was a problem hiding this comment.
Generally this looks good to me. Would it be difficult to add a Windows/Clang build to CI, so we can be sure this support doesn't regress in future? It doesn't have to publish a binary artefact, but just running the compiler and checking it doesn't explode would be a good start.
I tried setting this up, and it was all very fiddly until I switched to using packages from msys2. Here's what I came up with: invertego@e0ce972 I can make that a part of this PR if it looks reasonable. That commit also switches the existing GCC-based Windows build to use the msys2 packaged version. Our build is currently relying on a Chocolatey MinGW package containing GCC 8.1, which is at this point over 3 years old. |
The LLVM linker is required for both PDB generation and LTO support.
invertego
force-pushed
the
clang_win
branch
from
d8008d4
to
e74390e
Compare
It looks reasonable! I'm a bit disappointed in GitHub Actions. Looking at the list of software on Windows images, it looks like they've been keeping most things (Python, Perl, Rust, Go, Visual Studio, Firefox, Chrome, Git, etc.) quite up-to-date, so I'm not sure why their MinGW package is so stale. It's a bit annoying that we need two separate copies of the "make" step, whose only difference is that the Windows one asks to be executed inside msys2. How difficult would it be to install the latest version from Chocolatey instead? On the other hand, the beta release of the 2022 Windows image specifically encourages installing MSYS2 in the way you're doing, so it's probably not a bad idea to do it that way. |
I tried updating the MinGW Chocolatey package (and installing the LLVM one), and while this did work, it was very fiddly. The LLVM package is built with MSVC as the default target, so it must be overridden to use MinGW headers and libs. Normally it can discover these simply by looking up GCC in your path, but Chocolatey doesn't add the MinGW bin directory to your path. It puts shim executables for all of the individual binaries on your path. Now you have to work out the actual install directory and add that to your path. Do all that, and there's still the fact that our Windows builds were using Powershell, and since the ares makefiles include random bash commands (e.g. cp, test), the only reason these worked is that the Windows build image puts the Git for Windows msys2 /usr/bin directory the path, and it contains standalone executables named after these commands. I found complaints about this in some GitHub Actions issues, as these tools are only meant for use from a GfW bash shell. I believe using msys2 as a shell and installing a deliberately chosen set of packages for our build will create fewer headaches. |
|
Yep, having a second "make" step does sound like less of a hassle than all that other stuff. Thanks for looking into it! |
|
Nice work! Just a heads up that I'm open to having the released builds be produced by clang on all platforms, for consistency. We should ensure that gcc can still build ares though, so that stage should remain in CI, but only clang artefacts should be published imo. |
clang/LLVM has a great tools ecosystem and supports things for Windows that GCC does not (e.g. targeting ARM64, generating PDBs). I ran into several compile and run time issues in the course of building with clang, addressed in this series of commits, broken out for clarity of purpose. I stopped short of changing the default compiler to clang on Windows, but we may want to consider doing so in the future.