Re: [PATCH v3 06/12] Makefile: Split $(FLAGS) into cpp and cc components

On Thu, 14 May 2026 12:01:24 +1000 David Gibson wrote:

On Wed, May 13, 2026 at 09:11:00AM +0200, Stefano Brivio wrote:

...

On Tue, 12 May 2026 15:52:50 +1000 David Gibson wrote:

...

The $(FLAGS) variable contains mandatory compiler flags that should not be overridden. However, it contains a mixture of flags for the preprocessor and for the compiler proper. That's causing some inconvenience for other Makefile cleanups, so split it into $(BASE_CPPFLAGS) and $(BASE_CFLAGS) variables.

Signed-off-by: David Gibson --- Makefile | 40 ++++++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/Makefile b/Makefile index a8f8d06e..697f229f 100644 --- a/Makefile +++ b/Makefile @@ -30,12 +30,17 @@ ifeq ($(shell $(CC) -O2 -dM -E - < /dev/null 2>&1 | grep ' _FORTIFY_SOURCE ' > / FORTIFY_FLAG := -D_FORTIFY_SOURCE=2 endif

-FLAGS := -Wall -Wextra -Wno-format-zero-length -Wformat-security -FLAGS += -pedantic -std=c11 -D_XOPEN_SOURCE=700 -D_GNU_SOURCE -FLAGS += $(FORTIFY_FLAG) -O2 -pie -fPIE -FLAGS += -DPAGE_SIZE=$(shell getconf PAGE_SIZE) -FLAGS += -DVERSION=\"$(VERSION)\" -FLAGS += -DDUAL_STACK_SOCKETS=$(DUAL_STACK_SOCKETS) +# Mandatory preprocessor flags that won't be overridden with $(CPPFLAGS) +# FIXME: Could some of these be default, rather than required? +BASE_CPPFLAGS := -D_XOPEN_SOURCE=700 -D_GNU_SOURCE $(FORTIFY_FLAG) +BASE_CPPFLAGS += -DPAGE_SIZE=$(shell getconf PAGE_SIZE) +BASE_CPPFLAGS += -DVERSION=\"$(VERSION)\" +BASE_CPPFLAGS += -DDUAL_STACK_SOCKETS=$(DUAL_STACK_SOCKETS) + +# Mandatory compiler flags that won't be overridden with $(CFLAGS) +# FIXME: Could some of these be default, rather than required? +BASE_CFLAGS := -std=c11 -pie -fPIE -O2 +BASE_CFLAGS += -pedantic -Wall -Wextra -Wno-format-zero-length -Wformat-security

This new version of the series looks good to me in general (minus potential concern reported below), and everything seems to work on Debian and Fedora, but I would still like to try things out on Alpine or Void Linux because musl might cause surprises. I haven't got to it yet.

Makes sense, though I'm pretty sure it will be safe. I considered having some later patches that started removing non-essential things from BASE_*FLAGS. Those would certainly have required careful testing across distros. But working out what should move became more complicated than I initially thought (that's basically the discussion later in this mail), so I postponed it.

If you plan to submit another series soon, note that I don't plan to re-do the testing I'm currently doing, though, so you should plan to do those yourself and work with maintainers if things break, in case.

...

Meanwhile, regarding these FIXME comments: I think it *is* currently possible to override those flags (with different values for the same options), and overriding -D_FORTIFY_SOURCE on openSUSE (I haven't tried right now) was the initial motivation behind FLAGS.

True, I was thinking "overridden" in the make variable sense, but at the actual compiler that's misleading.

...

That is, the overriding role of CFLAGS seems to be preserved for these BASE_* flags as well, because $CFLAGS is given to the compiler after $BASE_CPPFLAGS, $CPPFLAGS, and $BASE_CFLAGS. So, in this sense, I would already call them "default" flags.

Right, it's kind of the subtle distinction between two types of default: default unless that specific option is overridden (BASE_CFLAGS), or default unless all the "default" options are overridden as a block (CFLAGS).

...

If that's the case, I think it's fine. Otherwise we need to find another solution at least for the short term.

By the way, if it helps addressing those comments at some point (I would apply anyway this series meanwhile if I don't find breakages, because not being able to run static checkers automatically on pesto is pretty nasty), out of those flags:

* -D_XOPEN_SOURCE, -D_GNU_SOURCE, and -DPAGE_SIZE are strictly required to build (at least in some environments)

Yes.

...

* -D_FORTIFY_SOURCE, -pie, -fPIE are not required to build but they are critical for security

Right. I'd kind of consider -pie and -fPIE in a different category than "flags", since they're choosing the kind of output object we're generating.

...

* -DVERSION is not required to build but makes things confusing and issues hard to debug because the version (usually supplied by the distribution) isn't reported in logs and logs of other tools

Right. I guess I'd also consider it mandatory. The fact that you can sort of get away without it seems more an accident of how we include it, rather than policy.

...

- -DDUAL_STACK_SOCKETS doesn't seem to be used anymore starting from commit b8d4fac6a2e7 ("util, pif: Replace sock_l4() with pif_sock_l4()")... was it intended, actually?

Huh, good point. I'm not sure it was exactly intentional. I was certainly grappling with the fact that in a sense it sometimes wanted to return two sockets in the !DUAL_STACK_SOCKETS case was really awkward. My plan, as best I recall was to move the handling of that case up layers, until it reached what would eventually become the forward table - so separate forwarding entries for v4 and v6 if we don't have dual stack sockets.

But, since we introduced DUAL_STACK_SOCKETS, I've discovered that the dual stack socket interface is described by RFC and supported by BSD and Windows as well as Linux. I'm pretty happy killing this define.

...

- -std=c11 is strictly required to ensure we build things correctly

Huh, is it? I would have thought it just catches us from using any C23 or GNU extensions.

It is because, at least in theory, while some constructs we use might be valid C99 or C17, we don't have guarantees that they have the same semantics. I don't see it happening in practice but I wouldn't risk it. Also mind that building with -std=c99 doesn't work (anymore).

...

- -O2 is optional, but dropping it (by default) might require annoying adjustments in distributions

Right. But on the other hand, overriding "all" packages with the distro preferred optimization flags is one of the main uses for distro CFLAGS overrides. Not sure what to do about this one.

Leave it like it is because it works as intended. It's our default which is also the default for most distributions, and Gentoo users (it's pretty much about them) can still override it by passing -O3 later.

...

- -pedantic, -Wall, -Wextra, -Wno-format-zero-length, -Wformat-security are all optional and useful for development (including distribution development), and might be security relevant in some cases

They're security relevant in the sense that it's good for us to get the warnings to catch possible security bugs. But a tree that builds clean with the flags shouldn't have different security properties if it's built without them. So I think it only matters that *we* use the flags, we don't need to impose it on anything else.

See above: "(including distribution development)". If those aren't there, we won't get some warnings from building distribution packages, and a few of those were useful in the past.

But, there's an awkward gotcha here. The build.py testcase needs -Werror to catch warnings, but obviously we don't want it to have to reiterate all the other warning flags. If we put these all into CFLAGS, it's tricky for build.py to just add -Werror without touching the rest.

My whole point was / is that CFLAGS needs to *add* to the existing flags, including warnings, instead of replacing them. You can override warnings flags too by appending options.

From our point of view, I like the idea of adding -Werror to the default CFLAGS, which fixes that problem.

At the moment there's no problem though.

But as you pointed out when I did that in the earlier version that might break distro builds causing use more trouble to fix it.

That *would* break distribution builds at the moment, so it's not acceptable. I constantly have fixes for warnings I spot in the packages I build on my to-do list. -- Stefano