cmake: unify GCC, Clang and PNaCl compiler configuration #1197

illwieckz · 2024-06-20T10:14:19Z

unify GCC, Clang and PNaCl compiler configuration
pass more build options to the sub-cmake game build
always set C standard, it will be required for building zlib with Saigo or Wasi
add options to not enforce C/C++ standards
move the target specific build options to another if/else block

- unify GCC, Clang and PNaCl compiler configuration - pass more build options to the sub-cmake game build - always set C standard, it will be required for building zlib with Saigo or Wasi - add options to not enforce C/C++ standards - move the target specific build options to another if/else block

illwieckz · 2024-06-20T10:20:56Z

Now PNaCl uses the same configuration as GCC and Clang, avoids discrepancies like this to be introduced:

cmake: never use strict aliasing #1160

This also make possible to pass build options like LTO to the sub-cmake game call. I'm not sure it really does something with PNaCl as the produced files are the same size (and LTO usually makes our binaries much smaller), but the compiler accepts the option so, it's not bad to pass it. It will be ready for other compilers like Saigo or Wasi.

The C standard is now always enforced, it was required for NaCl because NaCl uses some in-tree zlib source. It makes possible to use an in-tree zlib for things other than PNaCl, something we would want for Saigo or Wasi. And if we ship an own zlib, there is no reason to not build the engine against it. The usage or the in-tree zlib for every binary is not part of that PR, but this PR makes the CMake code ready for it.

Also, for debugging purpose, one can disable the C/C++ standard enforcement with advanced USE_RECOMMENDED_C_STANDARD and USE_RECOMMENDED_CXX_STANDARD variables.

illwieckz · 2024-06-20T18:12:18Z

I forgot to mention it, but this also makes explicit the optimization level used for the selected build profile. In the future I plan to make RelWithDebInfo use -O3 so everything will be there to just flip the number. For releases we currently use RelWithDebInfo with default -O2:

CMAKE Release build defaults to -O3 but RelWithDebInfo build defaults to -O2 #507

illwieckz · 2024-06-21T05:01:01Z

I forgot to mention it, but this also makes explicit the optimization level used for the selected build profile.

Well, this is probably a bad idea, even if someday we override them.

The reason why I went this way was because the NaCl code path as doing it, so when I unified it with other compilers, I thought “why not?”. It happens the NaCl code path doing it is a workaround for CMake failing to detect that PNaCl is a clang-compatible compiler. We better want to rely on extensive compiler detection as implemented in #982 to set the fallbacks for every clang-compatible compiler we detect:

Report what compiler is used to build the game #982

So for now I'll revert the unconditional setting of compiler optimizations and keep the if (NACL) workaround until we can rely on something more generic thanks to #982.

slipher · 2024-06-22T05:17:40Z

Why should there be an option not to use the appropriate language standard? I don't get it. Maybe it was useful one time, but not everything needs to be a checkbox option, if someone needs to do something really obscure they can edit the build system.

illwieckz · 2024-06-22T10:04:27Z

Why should there be an option not to use the appropriate language standard?

Generic answer: I like knobs.

Specific answers:

It makes easier for testing other standards without the default ones getting in the way, so it could be useful as a debug tool the day we raise the standards.
It also makes easy to test the build without those flags, I remember that when I toyed with Android at first it was failing to build with the flags but the without it built. Finding the root cause of why it didn't built with the flag was a specific task that was a different task than testing the rest of the code.
Not all the code base require those standards. For example C++14 isn't required to build a working game server, C++11 is enough.
I really like to be able to disable stuff for testing and debugging purpose in general, and I had to comment out those lines at least three times in the recent years for testing purpose, and editing code breaks rebasing and other things.

slipher · 2024-06-24T02:00:49Z

cmake/DaemonFlags.cmake

+		#   || _BSD_SOURCE || _SVID_SOURCE || _GNU_SOURCE
+		# isascii:
+		#   _XOPEN_SOURCE || _DEFAULT_SOURCE || _SVID_SOURCE
+		add_definitions(-D_POSIX_C_SOURCE=200809L -D_XOPEN_SOURCE=500)


What platform is this stuff needed on?

With -std=c++14 instead of -std=gnu++14.

I would understand -DUSE_RECOMMENDED_C_STANDARD=0 as telling the build system to get out of the way and not set any C standard flags. I oppose having it mean keep setting language standard flags, but it a different way.

The code now only sets GNU dialects.

slipher · 2024-06-24T02:03:22Z

cmake/DaemonFlags.cmake

+	if (USE_HARDENING)
+		message(STATUS "Enabling hardening")
+
+		# PNaCl accepts the flags bug do not define __stack_chk_guard and __stack_chk_fail.


Typos, but does not

slipher · 2024-06-24T02:19:50Z

cmake/DaemonFlags.cmake

+
+	# Link-time optimizations.
+	if (USE_LTO)
+		message(STATUS "Enabling link time optimizations")


The combination of adding these verbose messages, and passing unimplemented-in-NaCl build options such as USE_ARCH_INTRINSICS or USE_LTO to the NaCl build is unfortunate. It just causes the sub-build to lie with false messages such "Enabling link time optimizations"

I removed the “Enabling link time optimizations” message, it was not saying the feature was enabled but the user asked for it so it was wrong anyway.

Passing the USE_LTO option doesn't sound problematic. If the compiler lies about supporting LTO, it's not our fault. I also turned the flto setting as a “try”, so even setting USE_LTO on compilers that would reject the flag wouldn't break the build, and CMake will report the status. PNaCl will lie but that's not our problem.

The USE_ARCH_INTRINSICS would make sense the day we use Wasm, as Wasm support some kind of SIMD. Right now it's a bit useless for PNaCL but PNaCl not doing anything with it doesn't mean it's wrong to pass the option, in case the toolchain can do something with it.

USE_LTO is not passed anymore to the NACL sub-cmake invocation, also the option is ignored if for some reason the option is passed (like doing some manual stuff). After investigation, it happens that neither PNaCl's underlying clang nor Saigo clang support the -flto option. It happens that the pnacl-clang wrapper accepts the option but I don't see how it would do something with it, and the produced binaries are the sames anyway. On Saigo side, clang accepts the option but the link fails (the LTO linker plugin probably doesn't support the NaCl bitcode to begin with).

cmake/DaemonFlags.cmake

slipher · 2024-06-24T02:34:09Z

Having a knob that makes the program fail to build kinda sucks. But on the other hand, it sucks that there is often no way to disable all the flags added by our build system, like you could with something like CXXGLAGS='' in a simpler build system. So I can see the utility of having a way to knock out some flags. 👍

illwieckz · 2024-06-27T19:51:20Z

I moved the setting of PNaCl default build flag to the PNaCl toolchain itself. It makes much sense like that. I haven't set the -O0 for debug as it is not what CMake does for other clang-based compilers:

Default CMake compiler flags for various compilers Unvanquished/Unvanquished#3043

illwieckz · 2024-09-07T12:27:50Z

This now looks ready to me.

slipher · 2024-09-10T04:12:34Z

LGTM

illwieckz added the A-Build label Jun 20, 2024

illwieckz force-pushed the illwieckz/cmake-unify branch 3 times, most recently from 28b8504 to 83e7281 Compare June 20, 2024 17:51

illwieckz force-pushed the illwieckz/cmake-unify branch 3 times, most recently from 39c18bd to a4b5c8d Compare June 21, 2024 07:01

illwieckz force-pushed the illwieckz/cmake-unify branch from a4b5c8d to 5468d65 Compare June 22, 2024 09:37

slipher reviewed Jun 24, 2024

View reviewed changes

illwieckz force-pushed the illwieckz/cmake-unify branch from 5468d65 to a508b0a Compare June 27, 2024 16:00

cmake: set PNaCl default build flags in toolchain

8a3c5e1

illwieckz force-pushed the illwieckz/cmake-unify branch from a508b0a to d70ac44 Compare June 27, 2024 16:58

illwieckz force-pushed the illwieckz/cmake-unify branch 2 times, most recently from 130a688 to 0a35127 Compare June 27, 2024 21:48

cmake: do not test for not-GNU C/C++ dialect with extra knobs

4fa40eb

illwieckz force-pushed the illwieckz/cmake-unify branch 4 times, most recently from 7e0bf64 to 4fa40eb Compare September 7, 2024 12:22

illwieckz merged commit c659393 into master Sep 10, 2024
9 checks passed

illwieckz deleted the illwieckz/cmake-unify branch September 10, 2024 15:01

illwieckz mentioned this pull request Sep 16, 2024

cmake: always use pthread outside of MSVC #1305

Open

cmake: unify GCC, Clang and PNaCl compiler configuration #1197

cmake: unify GCC, Clang and PNaCl compiler configuration #1197

Uh oh!

Conversation

illwieckz commented Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

illwieckz commented Jun 20, 2024

Uh oh!

illwieckz commented Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

illwieckz commented Jun 21, 2024

Uh oh!

slipher commented Jun 22, 2024

Uh oh!

illwieckz commented Jun 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

illwieckz Sep 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

slipher commented Jun 24, 2024

Uh oh!

illwieckz commented Jun 27, 2024

Uh oh!

illwieckz commented Sep 7, 2024

Uh oh!

slipher commented Sep 10, 2024

Uh oh!

Uh oh!

Uh oh!

illwieckz commented Jun 20, 2024 •

edited

Loading

illwieckz commented Jun 20, 2024 •

edited

Loading

illwieckz Sep 7, 2024 •

edited

Loading