On Tue, May 05, 2026 at 01:10:30AM +0200, Stefano Brivio wrote: > On Mon, 4 May 2026 14:47:05 +1000 > David Gibson wrote: > > > On Sun, May 03, 2026 at 11:56:47PM +0200, Stefano Brivio wrote: > > > On Wed, 29 Apr 2026 13:47:13 +1000 > > > David Gibson wrote: > > > > > > > On Tue, Apr 28, 2026 at 09:17:31AM +0200, Stefano Brivio wrote: > > > > > On Tue, 21 Apr 2026 13:23:29 +1000 > > > > > David Gibson wrote: > > > > > > > > > > > FLAGS was introduced over the more standard CFLAGS, because there are some > > > > > > options we can't compile without, so overriding CFLAGS from the command > > > > > > line wasn't practical. We've now better dealt with that using > > > > > > BASE_CPPFLAGS, so there's no real need for FLAGS any more. > > > > > > I was about to add CFLAGS back to test/build/build.py (see below), but > > > then I realised that this patch actually defeats the purpose of FLAGS, > > > see commit 512f5b1aab2a ("Makefile: Allow define overrides by > > > prepending, not appending, CFLAGS") for the actual reason behind it. > > > > > > That is, with this patch: > > > > > > > > > Replace it > > > > > > with the more conventional CFLAGS, which now *can* be reasonable overridden > > > > > > from the command line. > > > > > > ...passing CFLAGS completely overrides the default CFLAGS (instead of > > > prepending them, that is, overriding single existing options). > > > > Yes, that was intentional. Fully overriding CFLAGS isn't always the > > most convenient, but it is how CFLAGS conventionally works in > > Makefiles, so least surprise and all that. > > > > > So you can't practically pass (add) "-Werror" anymore, as that will > > > drop stuff like -std=c11, which means one can't use -Werror in build > > > tests. > > > > Right, that's the most awkward consequence that I've spotted. For > > this case we can neutralize that by including -Werror by default, > > which I think is worthwhile anyway. > > ...except we can't do that because of the other reasons I mentioned. Yeah, sorry, I hadn't read that far when I wrote this. > > > It also drops bits like -pie -fPIE, so distributions (e.g. > > > openSUSE) can't use that to override FORTIFY_SOURCE. > > > > Hm, ok. I guess I'm not really sure what the purpose of the -pie > > -fPIE is, so I'm not clear whether it's essential for the build, or > > just a sensible default that users/distros could have reason to > > override. > > It's essential for security as it enables usage of ASLR (address space > layout randomization) and most distributions turn it on by default (or > require it in guidelines these days). > > We want to avoid that some distributions might miss that by mistake. Ah, ok. Yeah, that sounds essential to me. I think you could also make the argument that -pie is about the type of output we want, more than a "flag" per se, so it doesn't belong in CFLAGS any more than -c or -o would. -fPIE follows as a requirement for -pie. > > I didn't realise it was connected to FORTIFY_SOURCE. > > It's not directly connected, but if openSUSE just needs to redefine > FORTIFY_SOURCE, then they can't just add it by passing CFLAGS, because > that would also disable -pie -fPIE, plus whatever flag we want to have > on for security reasons that are independent from a specific > distribution. Ok, I see. > > > When I committed 512f5b1aab2a, I realised that, with the "prepending" > > > semantics, there's no way to completely override / clear CFLAGS, but: > > > > Right. I think gcc is "last option wins". > > Yes, as described in that commit (same for Clang). Ah, yes. I think som of the $(filter) and $(filter-out) confused be in terms of how FLAGS and CFLAGS interact(ed). > > > - it doesn't seem like a common use case anyway > > > > Maybe? > > > > > - what passing CFLAGS from the command line does isn't really > > > standardised, and it seems to be common to use it as "extra" > > > CFLAGS > > > > Huh.. ok. That's the opposite of the impression I've generally had. > > Although that said I guess "essential" compiler flags aren't usually > > in CFLAGS, which could be interpreted as extra flags. > > Maybe EXTRA_CFLAGS is more common, but we don't have it... I don't recall EXTRA_CFLAGS being particularly standard, but I'm not very certain. > > > Now, it would be possible to introduce something like EXTRA_CFLAGS and > > > ask users and distributions to switch to it, but it needs to happen in > > > two steps: > > > > Actually, I'd suggest inverting the pattern. Put vital flags in > > another variable (say BASE_CFLAGS) and have the default CFLAGS contain > > only flags that are a suggested default build, but can be safely > > overridden. This is the same pattern I've used for > > BASE_CPPFLAGS/CPPFLAGS in the earlier patches. > > Ah, good idea, that would avoid bothering users and distributions. By > the way, I think all current FLAGS are vital flags that should be in > BASE_CFLAGS, I'm not really convinced for -Owhatever, -pedantic, -fstack-protectr-strong or the warning options. > and one can still override -O or similar by passing CFLAGS > if CFLAGS is appended. It kind of depends depends what the caller is expecting. Do they expect omitting -Oxyz from CFLAGS to imply -O0, or "project default"? > > I didn't implement that in this patch originally, because I hadn't > > recognized that anything in FLAGS (other than the -D options I already > > moved to BASE_CPPFLAGS) was virtal in that sense. But it sounds like > > I was incorrect about that, so we can change accordingly. > > > > > - add EXTRA_CFLAGS and ask distribution maintainers to switch to that > > > > > > - after a reasonable amount of time (I would say six months or so) make > > > command-line CFLAGS replace the default CFLAGS > > > > Splitting BASE_CPPFLAGS/CFLAGS instead of CFLAGS/EXTRA_CFLAGS should > > make that transitions significantly less awkward. > > Right. Details about what belongs in BASE_CFLAGS versus CFLAGS can be thrashed out later. The vital thing for this series is to split $(FLAGS) into preprocessor versus compiler flags (with clearer names for both). That's what we need in order to properly invoke the static checkers for both passt and pesto (more particularly, cppcheck needs -DPESTO with pesto and -UPESTO with passt, or log.h will give it conniptions). > > > > I don't really see a strong motivation to do this though, at the moment. > > > > > > > > > > > > > > > Signed-off-by: David Gibson > > > > > > --- > > > > > > Makefile | 21 ++++++++++----------- > > > > > > test/build/build.py | 4 ++-- > > > > > > 2 files changed, 12 insertions(+), 13 deletions(-) > > > > > > > > > > > > diff --git a/Makefile b/Makefile > > > > > > index e89e5556..1e5f0282 100644 > > > > > > --- a/Makefile > > > > > > +++ b/Makefile > > > > > > @@ -36,8 +36,8 @@ BASE_CPPFLAGS := -D_XOPEN_SOURCE=700 -D_GNU_SOURCE \ > > > > > > -DVERSION=\"$(VERSION)\" > > > > > > CPPFLAGS := $(FORTIFY_FLAG) -DDUAL_STACK_SOCKETS=$(DUAL_STACK_SOCKETS) > > > > > > > > > > > > -FLAGS := -Wall -Wextra -Wno-format-zero-length -Wformat-security > > > > > > -FLAGS += -pedantic -std=c11 -O2 -pie -fPIE > > > > > > +WARNINGS = -Wall -Wextra -Wno-format-zero-length -Wformat-security > > > > > > +CFLAGS = -pedantic -std=c11 -O2 -pie -fPIE $(WARNINGS) > > > > > > > > > > > > PASST_SRCS = arch.c arp.c bitmap.c checksum.c conf.c dhcp.c dhcpv6.c \ > > > > > > epoll_ctl.c flow.c fwd.c fwd_rule.c icmp.c igmp.c inany.c iov.c ip.c \ > > > > > > @@ -66,7 +66,7 @@ ifeq ($(shell printf "$(C)" | $(CC) -S -xc - -o - >/dev/null 2>&1; echo $$?),0) > > > > > > endif > > > > > > > > > > > > ifeq ($(shell :|$(CC) -fstack-protector-strong -S -xc - -o - >/dev/null 2>&1; echo $$?),0) > > > > > > - FLAGS += -fstack-protector-strong > > > > > > + CFLAGS += -fstack-protector-strong > > > > > > endif > > > > > > > > > > > > prefix ?= /usr/local > > > > > > @@ -85,7 +85,7 @@ endif > > > > > > > > > > > > all: $(BIN) $(MANPAGES) docs > > > > > > > > > > > > -static: FLAGS += -static > > > > > > +static: CFLAGS += -static > > > > > > static: CPPFLAGS += -DGLIBC_NO_STATIC_NSS > > > > > > static: clean all > > > > > > > > > > > > @@ -96,12 +96,11 @@ seccomp_repair.h: seccomp.sh $(PASST_REPAIR_SRCS) > > > > > > @ ARCH="$(TARGET_ARCH)" CC="$(CC)" ./seccomp.sh seccomp_repair.h $(PASST_REPAIR_SRCS) > > > > > > > > > > > > passt: $(PASST_SRCS) $(HEADERS) > > > > > > - $(CC) $(FLAGS) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) $(PASST_SRCS) -o passt $(LDFLAGS) > > > > > > + $(CC) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) $(PASST_SRCS) -o passt $(LDFLAGS) > > > > > > > > > > > > -passt.avx2: FLAGS += -Ofast -mavx2 -ftree-vectorize -funroll-loops > > > > > > +passt.avx2: CFLAGS += -Ofast -mavx2 -ftree-vectorize -funroll-loops > > > > > > passt.avx2: $(PASST_SRCS) $(HEADERS) > > > > > > - $(CC) $(filter-out -O2,$(FLAGS)) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) \ > > > > > > - $(PASST_SRCS) -o passt.avx2 $(LDFLAGS) > > > > > > + $(CC) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) $(PASST_SRCS) -o passt.avx2 $(LDFLAGS) > > > > > > > > > > > > passt.avx2: passt > > > > > > > > > > > > @@ -109,16 +108,16 @@ pasta.avx2 pasta.1 pasta: pasta%: passt% > > > > > > ln -sf $< $@ > > > > > > > > > > > > qrap: $(QRAP_SRCS) passt.h > > > > > > - $(CC) $(FLAGS) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) -DARCH=\"$(TARGET_ARCH)\" $(QRAP_SRCS) -o qrap $(LDFLAGS) > > > > > > + $(CC) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) -DARCH=\"$(TARGET_ARCH)\" $(QRAP_SRCS) -o qrap $(LDFLAGS) > > > > > > > > > > > > passt-repair: $(PASST_REPAIR_SRCS) seccomp_repair.h > > > > > > - $(CC) $(FLAGS) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) $(PASST_REPAIR_SRCS) -o passt-repair $(LDFLAGS) > > > > > > + $(CC) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) $(PASST_REPAIR_SRCS) -o passt-repair $(LDFLAGS) > > > > > > > > > > > > valgrind: EXTRA_SYSCALLS += rt_sigprocmask rt_sigtimedwait rt_sigaction \ > > > > > > rt_sigreturn getpid gettid kill clock_gettime \ > > > > > > mmap|mmap2 munmap open unlink gettimeofday futex \ > > > > > > statx readlink > > > > > > -valgrind: FLAGS += -g > > > > > > +valgrind: CFLAGS += -g > > > > > > valgrind: CPPFLAGS += -DVALGRIND > > > > > > valgrind: all > > > > > > > > > > > > diff --git a/test/build/build.py b/test/build/build.py > > > > > > index e3de8305..7c9cbb44 100755 > > > > > > --- a/test/build/build.py > > > > > > +++ b/test/build/build.py > > > > > > @@ -60,7 +60,7 @@ def test_make(target: str, expected_files: list[str]) -> None: > > > > > > with clone_sources(): > > > > > > for p in ex_paths: > > > > > > assert not p.exists(), f"{p} existed before make" > > > > > > - sh(f'make {target} CFLAGS="-Werror"') > > > > > > + sh(f'make {target}') > > > > > > for p in ex_paths: > > > > > > assert p.exists(), f"{p} wasn't made" > > > > > > sh('make clean') > > > > > > @@ -90,7 +90,7 @@ def test_install_uninstall() -> None: > > > > > > progs = ['passt', 'pasta', 'qrap'] > > > > > > > > > > > > # Install > > > > > > - sh(f'make install CFLAGS="-Werror" prefix={prefix}') > > > > > > + sh(f'make install prefix={prefix}') > > > > > > > > > > Here, and above: I don't understand what (if anything) implies -Werror > > > > > now. > > > > > > > > Ah, oops. I misread the -Werror in test/Makefile, thinking it was in > > > > Makefile and applied to everything. I think the right fix is to put > > > > -Werror in the default CFLAGS and remove it from here. > > > > > > That's not really doable as it would occasionally break distributions, > > > where specific toolchain or architecture combinations lead quite often > > > to harmless warnings. I can remember dozens of those, and not a single > > > one that was actually a critical problem that made it preferable to > > > have missing packages. > > > > Ah, right. And those distributions don't already override CFLAGS with > > something that doesn't include -Werror? > > We don't have -Werror on. That's just for tests. > > > > It's also very annoying for developers, especially for myself as I > > > often have to run quick tests with different compilers. > > > > Ok, fair point. > > > > > I would rather add -Werror back to test/build/build.py for the moment > > > being by dropping this patch, and then drop patches that non-trivially > > > depend on this one (due to lack of time, not because I have anything > > > against the other patches, which look good to me except for 13/13). > > > > > > That is, I would reduce this series to the bare minimum that's needed > > > for the "RFC: Dynamic configuration update implementation" series, to > > > avoid blocking progress there. I haven't quite figured out how to do > > > that yet, but that's next on my list unless you get to that first. > > > > I'm still unwell, so unlikely. > > Maybe it can all be made simpler by keeping this series substantially > as it is with the change you suggested above (essentially, BASE_CFLAGS > instead of FLAGS, and CFLAGS adding on top of those). Right, sounds basically the same as my suggestion above. -- 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