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. > 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. > - -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. > - -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. 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. From our point of view, I like the idea of adding -Werror to the default CFLAGS, which fixes that problem. 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. -- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson