Support building with clang-cl

[Nerixyz]

This PR adds support for building crashpad with clang-cl (see getsentry/sentry-native#689). It adds the following flags to crashpad_interface:

• -Wno-missing-field-initializers (e.g. required in util/file/file_io_win.cc)
• -Wno-sign-compare (e.g. required in third_party/mini_chromium/mini_chromium/base/threading/thread_local_storage.h)
• -Wno-microsoft-cast (e.g. required in client/crashpad_client_win.cc)

Crashpad itself (or rather mini_chromium) doesn't appear to support clang-cl and defaults to cl on Windows (though I don't know gn that well).

[supervacuus]

Thanks, @Nerixyz; I started with a clang-cl compatible build some time ago: getsentry/sentry-native@5cc8bf2#diff-d33daaf3c89bd1ad9f7385969b64e8ae2e223e0ad3926feb9d9d2e90b143dd40

As you can see, I also changed the build scripts for mini_chromium, etc., and AFAICR, I was able to build the whole project (but maybe I didn't push my last changes).

What stopped me was that we have a couple of compile conditionals in the code (ours and crashpad) that query the platform/toolchain, where the paths expressed considered clang always not to be MSVC, and that can be problematic even if no compile errors happen.

[Nerixyz]

Sorry for the late reply @supervacuus, I checked a few headers in mini_chromium for checks of __clang__, but most are related to math stuff, which clang-cl does support.

What stopped me was that we have a couple of compile conditionals in the code (ours and crashpad) that query the platform/toolchain, where the paths expressed considered clang always not to be MSVC, and that can be problematic even if no compile errors happen.

Could you point out some conditionals in crashpad that assume this? Maybe these can be changed upstream (or maybe mini_chromium could officially support clang-cl (?)).

Thanks, @Nerixyz; I started with a clang-cl compatible build some time ago: getsentry/sentry-native@5cc8bf2#diff-d33daaf3c89bd1ad9f7385969b64e8ae2e223e0ad3926feb9d9d2e90b143dd40

You added the flags for each CMake target in 2237d97...e5c4398 (that's probably why it looks quite big). I added the flags in crashpad_interface, so they apply to the other targets too (notably mini_chromium).

Looking at compile_commands.json and comparing, for example mini_chromium/base/threading/thread_local_storage_win.cc, there aren't many differences:

diff --git a/build/clang-cl.bat b/build/msvc.bat
index 18964946..fee41ba0 100644
--- a/build/clang-cl.bat
+++ b/build/msvc.bat
@@ -1,6 +1,6 @@
-D:\LLVM\bin\clang-cl.exe
+"D:\VisualStudio 2022\VC\Tools\MSVC\14.39.33519\bin\Hostx64\x64\cl.exe"
 /nologo
--TP
+/TP
 -DCRASHPAD_FLOCK_ALWAYS_SUPPORTED=1
 -DCRASHPAD_LSS_SOURCE_EMBEDDED
 -DNOMINMAX
@@ -35,11 +35,8 @@ D:\LLVM\bin\clang-cl.exe
 /wd4324
 /wd4351
 /wd4577
--Wno-missing-field-initializers
--Wno-sign-compare
--Wno-microsoft-cast
 /Fotools\crash-handler\lib\crashpad\third_party\mini_chromium\CMakeFiles\mini_chromium.dir\mini_chromium\base\threading\thread_local_storage_win.cc.obj
 /FdTARGET_COMPILE_PDB
+/FS
 -c
---
 G:\Projects\c2\tools\crash-handler\lib\crashpad\third_party\mini_chromium\mini_chromium\base\threading\thread_local_storage_win.cc

[supervacuus]

Hi @Nerixyz, sorry for the long response gap.

I cannot compile the current PR proposal with clang-cl. Does a build run successfully if you configure (either this fork or sentry-native with your crashpad changes) with

cmake -B build_cl -G "Visual Studio 17 2022" -T ClangCL

because this immediately leads to further warnings/errors in my setup.

Could you point out some conditionals in crashpad that assume this? Maybe these can be changed upstream (or maybe mini_chromium could officially support clang-cl (?)).

I mostly meant conditionals in the CMake build scripts. But there are a few #ifdefs affecting clang-cl too: __debugbreak(); vs. __builtin_trap(); for instance, though that is less relevant.

You added the flags for each CMake target

Yes, this was intentional because the overlap between the targets was relatively small if you look into my changes. I am more than open to adding warning suppressions to the interface once the build works if those warnings appear in all targets (like -Wno-unused-parameter, which curiously does not appear in your PR), but I would like to limit suppressions as much as possible to the affected targets.

This PR also needs a clang-cl build step, so that changes from upstream or us are checked against clang-cl.

[Nerixyz]

because this immediately leads to further warnings/errors in my setup.

Same for me. It seems like MsBuild or the VS generator swallows the /w[...] flags. If you build with --verbose, you can see that it doesn't specify any of these flags. For whatever reason, the -Wno-missing-field-initializers -Wno-sign-compare -Wno-microsoft-cast flags that I added for clang-cl are present.
If you generate the compile commands, you can see the flags, however if you search for any of them (e.g. /wd4351) in the build_cl directory, they don't show up.
Even when specifying the flags with generator expressions, this will happen ($<$<AND:$<COMPILE_LANGUAGE:C,CXX>,$<C_COMPILER_ID:Clang>>:-Wno-missing-field-initializers> for example).

I usually use Ninja to build, which works fine with both the clang-cl shipped with Visual Studio and the latest clang-cl (18):

cmake -B build_cl2 -GNinja -DCMAKE_C_COMPILER=clang-cl -DCMAKE_CXX_COMPILER=clang-cl

Even Visual Studio itself will use Ninja by default. So if you select clang_cl_x64 as the toolset in the CMake settings, it will work fine.

Omitting the -T ClangCL (i.e. using cl.exe) will have the warnings included (that's what's tested in CI).

(like -Wno-unused-parameter, which curiously does not appear in your PR)

It doesn't appear, because clang translates /wd4100 into -Wno-unused-parameter, but due to the behavior above, this parameter doesn't get passed on.

[supervacuus]

Same for me. It seems like MsBuild or the VS generator swallows the /w[...] flags. If you build with --verbose, you can see that it doesn't specify any of these flags. For whatever reason, the -Wno-missing-field-initializers -Wno-sign-compare -Wno-microsoft-cast flags that I added for clang-cl are present.

Thanks for the check! While the behavior makes little sense, it is consistent with other generator differences on Windows we observe, and I think this is a sufficient discriminator.

That means we only have to add -Wno-unused-parameter and -Wno-deprecated-declarations to your interface-specified compile options, which is fine because the corresponding /wd4100 and /wd4996 are also specified there.

Even Visual Studio itself will use Ninja by default. So if you select clang_cl_x64 as the toolset in the CMake settings, it will work fine.

Of course, switching to Ninja on Windows is an option for us (and preferable, given the significant performance difference and increasing tool support). But if we add support for clang-cl, it should also work with the MSBuild generator. This is a supported generator of the Native SDK on Windows, and I would like to keep toolchain support independent from generator support. That should be easily achievable in this case.

Are you okay with me taking over so that I can work on the Native SDK integration and CI support?

[Nerixyz]

That means we only have to add -Wno-unused-parameter and -Wno-deprecated-declarations to your interface-specified compile options, which is fine because the corresponding /wd4100 and /wd4996 are also specified there.

Are you okay with me taking over so that I can work on the Native SDK integration and CI support?

Sure! We only use this library, so I don't have much experience with the SDK.

I could add the remaining flags later but feel free to just do that if you have time.