On Thu, May 14, 2026 at 11:41:02AM +0200, Stefano Brivio wrote: > 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. Understood, but I don't plan to resume this particularly soon. > > > 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. I mean... the C committee has done some weird stuff, but changing the behaviour of existing valid constructs seems well beyond the pale. > Also mind that building with -std=c99 doesn't work (anymore). Certainly - at this point it's a constraint that CC refer to a C11 compiler, not just a C compiler. > > > - -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. Yeah, ok. > > > - -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. Yeah, fair points. -- 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