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

[Stefano Brivio <sbrivio@redhat.com>]

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.

> > 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