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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 9753 bytes --]

On Thu, May 14, 2026 at 11:41:02AM +0200, Stefano Brivio wrote:
> On Thu, 14 May 2026 12:01:24 +1000
> David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
> > > > ---
> > > >  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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]