From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=RzfYfQJ1; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTPS id 7577D5A0262 for ; Thu, 14 May 2026 11:41:09 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1778751668; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=bUS/khuAC+pckPDLXaSpyq3M64q3XJaH9AsrPuzpIwA=; b=RzfYfQJ1B+/JeRNdmWwllWz8el7NaXN5RVTOZ4zSeWN4ck5XCbVlS5mWsW3CKP317389/k SbYBUrYg5mPjRdW9Uesz0AwIDkLMbukhACVYARfWgujbTU+ZbGLEQVdoukbcP3WuTlx9MG t5Gjwre7SBoO51QrDAXmLZeU4M4rS+s= Received: from mail-wr1-f72.google.com (mail-wr1-f72.google.com [209.85.221.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-66-YNB6GfjsPpOTRWZNCwdPmQ-1; Thu, 14 May 2026 05:41:06 -0400 X-MC-Unique: YNB6GfjsPpOTRWZNCwdPmQ-1 X-Mimecast-MFC-AGG-ID: YNB6GfjsPpOTRWZNCwdPmQ_1778751666 Received: by mail-wr1-f72.google.com with SMTP id ffacd0b85a97d-44a122a5128so5924256f8f.0 for ; Thu, 14 May 2026 02:41:06 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1778751665; x=1779356465; h=date:content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=bUS/khuAC+pckPDLXaSpyq3M64q3XJaH9AsrPuzpIwA=; b=Zt6ZUtY4p5cx8suwXopFrRohq7t5OwY5hgkjS0m8K/WzcJ4JbCxNyFaZHYtntv5UVp MDmKGiMPJ7ylB8FJTTgqcW4bDqiix01N2H76IcmwZWei1lRjoIZyzhA49XOVZmQfA8N3 Bmdm/OADY/8oNmuqeU2PK/ZxLZxCGu/ckfG9keO7wb9nWLAha9j+1XMSVueiI2o2ElxH QzEhF4hFFBs48wqPzsLLsmF/TTx48PT1BE86lar7DyGyD2oOqgjGysUsCNSd3ulAl+AR DNFtvOUoSsZBz1GlHwi2OHqh6HBtfwbBN7HtuQAKSoeUn1seVTDvnA2LJi+e7DPo3SWc MFvg== X-Gm-Message-State: AOJu0Yyd2uGEkblb+7FetZz8eeM4P92wtdKTNPgqHeCroqQLZRQbLjeM y9JTYdKEiONDVu7WCIOxbwKnFx/WeQOqadSn4sHslaLzu8fdeOhrsSF1kmHxThDPAittazGIF8B +3N0FcwYjHjm8nn1bB/NFCkI2XRQIc0FcuRKJCyTw5JgOG7IqGaDmisIZRacBkA== X-Gm-Gg: Acq92OEGsMrQRkLgYBWpXFSWdg33rUJRRYf7FiZuEw/5WeJEjKR+xuOf5Y62enqypAO D44DFCibuqO2KjtvfC8NeYmrzGgBjEwsA6YIhoJLTesG6dxFQfFAe25flruyGnZdSIpTGp6PgF4 559eQlOm+1IFRgXfTxA6opH/EmG1wnj/ybolNJXOaigU7GV9nec7aHe3Y/rdCof+E5UAJVjBzg6 C5C1nm27+HRxdWh3QsMkG6Pn47MRpMTrrzxwQDl0jNz8DcJ3BuD65EGmj9/Fn8SzR4agT1yjXwb GnYnUrTbiZd2zBn0LQwUXrjESvB+4zpwTpQU6Ncbb+OectYc1qloJCeRodOiB5rWUBWifxJtdB3 vdO/V5RgUVKvgCwgFdQemU8Sbu2Y0UvGK X-Received: by 2002:a05:6000:2087:b0:454:9655:43af with SMTP id ffacd0b85a97d-45c5556cdd4mr10911649f8f.0.1778751665050; Thu, 14 May 2026 02:41:05 -0700 (PDT) X-Received: by 2002:a05:6000:2087:b0:454:9655:43af with SMTP id ffacd0b85a97d-45c5556cdd4mr10911589f8f.0.1778751664373; Thu, 14 May 2026 02:41:04 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-45d9ed30110sm5759547f8f.13.2026.05.14.02.41.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 14 May 2026 02:41:03 -0700 (PDT) From: Stefano Brivio To: David Gibson Subject: Re: [PATCH v3 06/12] Makefile: Split $(FLAGS) into cpp and cc components Message-ID: <20260514114102.294abd87@elisabeth> In-Reply-To: References: <20260512055256.1800449-1-david@gibson.dropbear.id.au> <20260512055256.1800449-7-david@gibson.dropbear.id.au> <20260513091059.051d41c5@elisabeth> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 Date: Thu, 14 May 2026 11:41:02 +0200 (CEST) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: Ys0YiCJef8ct7BwF9yfLdgW0VKpoNFtRJv1_XOT1oE4_1778751666 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: OQ3WHBO7GY53SCCHAL2CPGQIETYCLCO6 X-Message-ID-Hash: OQ3WHBO7GY53SCCHAL2CPGQIETYCLCO6 X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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. > > 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