[PATCH 0/3] Fix broken build with -DNDEBUG

[PATCH 0/3] Fix broken build with -DNDEBUG

[David Gibson]

Fix the trivial but nasty with -DNDEBUG builds recently reported by
Jan Palus.  While we're there, add a couple of extra build tests.
These wouldn't actually catch this problem, but they're better than
nothing and do catch the #include problem I also spotted.

David Gibson (3):
  test: Extend exeter build tests to cover more recent binaries
  Fix build with -DNDEBUG
  test: Add test for builds with -DNDEBUG

 test/build/build.py | 25 +++++++++++++++++++++----
 util.h              |  3 ++-
 2 files changed, 23 insertions(+), 5 deletions(-)

-- 
2.54.0

[PATCH 1/3] test: Extend exeter build tests to cover more recent binaries

[David Gibson]

test/build/build.py tests that the Makefile works to build our binaries.
However, it hasn't been updated for a while, and doesn't cover passt-repair
or pesto.  Extend it to cover those too.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 test/build/build.py | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/test/build/build.py b/test/build/build.py
index e3de8305..eeee5acc 100755
--- a/test/build/build.py
+++ b/test/build/build.py
@@ -68,10 +68,13 @@ def test_make(target: str, expected_files: list[str]) -> None:
             assert not p.exists(), f"{p} existed after make clean"
 
 
-exeter.register('make_passt', test_make, 'passt', ['passt'])
-exeter.register('make_pasta', test_make, 'pasta', ['pasta'])
-exeter.register('make_qrap', test_make, 'qrap', ['qrap'])
-exeter.register('make_all', test_make, 'all', ['passt', 'pasta', 'qrap'])
+BINARIES = ['passt', 'pasta', 'qrap', 'passt-repair', 'pesto']
+
+
+for bin in BINARIES:
+    exeter.register(f'make_{bin.replace('-', '_')}', test_make, bin, [bin])
+
+exeter.register('make_all', test_make, 'all', BINARIES)
 
 
 @exeter.test
-- 
2.54.0

[PATCH 2/3] Fix build with -DNDEBUG

[David Gibson]

Since bc872d91765d, our assert() statements are omitted if we compile with
-DNDEBUG, like the standard library assert(3).  Unfortunately a trivial but
embarrassing mistake in that patch means that instead of never aborting in
this case, assert_with_msg() *always* aborts, breaking pretty much
everything.

There's also a missing #include that breaks the build with -DNDEBUG on at
least some library versions.

Reported-by: Jan Palus <jpalus@fastmail.com>
Fixes: bc872d91765d ("treewide: Spell ASSERT() as assert()")
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 util.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/util.h b/util.h
index 70aadeba..11f71d45 100644
--- a/util.h
+++ b/util.h
@@ -6,6 +6,7 @@
 #ifndef UTIL_H
 #define UTIL_H
 
+#include <assert.h>
 #include <stdlib.h>
 #include <stdarg.h>
 #include <stdbool.h>
@@ -60,7 +61,7 @@ void abort_with_msg(const char *fmt, ...)
 			__func__, __FILE__, __LINE__, STRINGIFY(expr))
 #else
 #define assert_with_msg(expr, ...)					\
-	((void)(expr), 0 ? (void)0 : abort_with_msg(__VA_ARGS__))
+	((void)(expr), 1 ? (void)0 : abort_with_msg(__VA_ARGS__))
 #endif
 
 #ifdef P_tmpdir
-- 
2.54.0

Re: [PATCH 2/3] Fix build with -DNDEBUG

[Jan Palus]

On 15.05.2026 14:13, David Gibson wrote:
> Since bc872d91765d, our assert() statements are omitted if we compile with
> -DNDEBUG, like the standard library assert(3).  Unfortunately a trivial but
> embarrassing mistake in that patch means that instead of never aborting in
> this case, assert_with_msg() *always* aborts, breaking pretty much
> everything.
> 
> There's also a missing #include that breaks the build with -DNDEBUG on at
> least some library versions.
> 
> Reported-by: Jan Palus <jpalus@fastmail.com>
> Fixes: bc872d91765d ("treewide: Spell ASSERT() as assert()")
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  util.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/util.h b/util.h
> index 70aadeba..11f71d45 100644
> --- a/util.h
> +++ b/util.h
> @@ -6,6 +6,7 @@
>  #ifndef UTIL_H
>  #define UTIL_H
>  
> +#include <assert.h>
>  #include <stdlib.h>
>  #include <stdarg.h>
>  #include <stdbool.h>
> @@ -60,7 +61,7 @@ void abort_with_msg(const char *fmt, ...)
>  			__func__, __FILE__, __LINE__, STRINGIFY(expr))
>  #else
>  #define assert_with_msg(expr, ...)					\
> -	((void)(expr), 0 ? (void)0 : abort_with_msg(__VA_ARGS__))
> +	((void)(expr), 1 ? (void)0 : abort_with_msg(__VA_ARGS__))

There is a slight semantic difference between assert() and
assert_with_msg() when building with -DNDEBUG -- `expr` is still being
evaluated in abort_with_msg() although likely optimized out in most
builds. I'm assuming you'd prefer to avoid cppcheck suppressions. How
about moving `expr` into branch which is never evaluated then? Would it
keep cppcheck happy?

+	(1 ? (void)0 : ((void)(expr), abort_with_msg(__VA_ARGS__)))

>  #endif
>  
>  #ifdef P_tmpdir
> -- 
> 2.54.0
> 

Re: [PATCH 2/3] Fix build with -DNDEBUG

[David Gibson]

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

On Fri, May 15, 2026 at 01:05:50PM +0200, Jan Palus wrote:
> On 15.05.2026 14:13, David Gibson wrote:
> > Since bc872d91765d, our assert() statements are omitted if we compile with
> > -DNDEBUG, like the standard library assert(3).  Unfortunately a trivial but
> > embarrassing mistake in that patch means that instead of never aborting in
> > this case, assert_with_msg() *always* aborts, breaking pretty much
> > everything.
> > 
> > There's also a missing #include that breaks the build with -DNDEBUG on at
> > least some library versions.
> > 
> > Reported-by: Jan Palus <jpalus@fastmail.com>
> > Fixes: bc872d91765d ("treewide: Spell ASSERT() as assert()")
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  util.h | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/util.h b/util.h
> > index 70aadeba..11f71d45 100644
> > --- a/util.h
> > +++ b/util.h
> > @@ -6,6 +6,7 @@
> >  #ifndef UTIL_H
> >  #define UTIL_H
> >  
> > +#include <assert.h>
> >  #include <stdlib.h>
> >  #include <stdarg.h>
> >  #include <stdbool.h>
> > @@ -60,7 +61,7 @@ void abort_with_msg(const char *fmt, ...)
> >  			__func__, __FILE__, __LINE__, STRINGIFY(expr))
> >  #else
> >  #define assert_with_msg(expr, ...)					\
> > -	((void)(expr), 0 ? (void)0 : abort_with_msg(__VA_ARGS__))
> > +	((void)(expr), 1 ? (void)0 : abort_with_msg(__VA_ARGS__))
> 
> There is a slight semantic difference between assert() and
> assert_with_msg() when building with -DNDEBUG -- `expr` is still being
> evaluated in abort_with_msg() although likely optimized out in most
> builds.

Right, I'm expecting the compiler to optimise this away.  That won't
be the case if the expression has side effects, but side effects in an
assert() expression is already a bug, precisely because of the
possibility NDEBUG.

> I'm assuming you'd prefer to avoid cppcheck suppressions.

cppcheck suppressions would be acceptable if we could put them inside
the macro.  But these are unused variable warnings, which show up at
the site of variable declaration not (obviously) at the place the
variable... isn't used.  Putting suppressions on variables because
they might become unused depending on a macro definition certainly
isn't acceptable.  Both for aesthetics, and because it means if we
removed the assert we'd no longer get a warning that the variable now
really isn't used.

> How
> about moving `expr` into branch which is never evaluated then? Would it
> keep cppcheck happy?
> 
> +	(1 ? (void)0 : ((void)(expr), abort_with_msg(__VA_ARGS__)))

That does work.  Not sure if it's worth the bother of a respin.
Stefano, any opinion?

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

Re: [PATCH 2/3] Fix build with -DNDEBUG

[Stefano Brivio]

On Sat, 16 May 2026 16:28:52 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, May 15, 2026 at 01:05:50PM +0200, Jan Palus wrote:
> > On 15.05.2026 14:13, David Gibson wrote:  
> > > Since bc872d91765d, our assert() statements are omitted if we compile with
> > > -DNDEBUG, like the standard library assert(3).  Unfortunately a trivial but
> > > embarrassing mistake in that patch means that instead of never aborting in
> > > this case, assert_with_msg() *always* aborts, breaking pretty much
> > > everything.
> > > 
> > > There's also a missing #include that breaks the build with -DNDEBUG on at
> > > least some library versions.
> > > 
> > > Reported-by: Jan Palus <jpalus@fastmail.com>
> > > Fixes: bc872d91765d ("treewide: Spell ASSERT() as assert()")
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  util.h | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/util.h b/util.h
> > > index 70aadeba..11f71d45 100644
> > > --- a/util.h
> > > +++ b/util.h
> > > @@ -6,6 +6,7 @@
> > >  #ifndef UTIL_H
> > >  #define UTIL_H
> > >  
> > > +#include <assert.h>
> > >  #include <stdlib.h>
> > >  #include <stdarg.h>
> > >  #include <stdbool.h>
> > > @@ -60,7 +61,7 @@ void abort_with_msg(const char *fmt, ...)
> > >  			__func__, __FILE__, __LINE__, STRINGIFY(expr))
> > >  #else
> > >  #define assert_with_msg(expr, ...)					\
> > > -	((void)(expr), 0 ? (void)0 : abort_with_msg(__VA_ARGS__))
> > > +	((void)(expr), 1 ? (void)0 : abort_with_msg(__VA_ARGS__))  
> > 
> > There is a slight semantic difference between assert() and
> > assert_with_msg() when building with -DNDEBUG -- `expr` is still being
> > evaluated in abort_with_msg() although likely optimized out in most
> > builds.  
> 
> Right, I'm expecting the compiler to optimise this away.  That won't
> be the case if the expression has side effects, but side effects in an
> assert() expression is already a bug, precisely because of the
> possibility NDEBUG.
> 
> > I'm assuming you'd prefer to avoid cppcheck suppressions.  
> 
> cppcheck suppressions would be acceptable if we could put them inside
> the macro.  But these are unused variable warnings, which show up at
> the site of variable declaration not (obviously) at the place the
> variable... isn't used.  Putting suppressions on variables because
> they might become unused depending on a macro definition certainly
> isn't acceptable.  Both for aesthetics, and because it means if we
> removed the assert we'd no longer get a warning that the variable now
> really isn't used.
> 
> > How
> > about moving `expr` into branch which is never evaluated then? Would it
> > keep cppcheck happy?
> > 
> > +	(1 ? (void)0 : ((void)(expr), abort_with_msg(__VA_ARGS__)))  
> 
> That does work.  Not sure if it's worth the bother of a respin.
> Stefano, any opinion?

I think it would be worth changing that given that the only reason why
PLD Linux enables NDEBUG is to avoid evaluating those expressions.

I can also change it on merge though, it's trivial enough. I'll do that
if everybody is fine with it.

-- 
Stefano

Re: [PATCH 2/3] Fix build with -DNDEBUG

[David Gibson]

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

On Sat, May 16, 2026 at 11:27:16AM +0200, Stefano Brivio wrote:
> On Sat, 16 May 2026 16:28:52 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, May 15, 2026 at 01:05:50PM +0200, Jan Palus wrote:
> > > On 15.05.2026 14:13, David Gibson wrote:  
> > > > Since bc872d91765d, our assert() statements are omitted if we compile with
> > > > -DNDEBUG, like the standard library assert(3).  Unfortunately a trivial but
> > > > embarrassing mistake in that patch means that instead of never aborting in
> > > > this case, assert_with_msg() *always* aborts, breaking pretty much
> > > > everything.
> > > > 
> > > > There's also a missing #include that breaks the build with -DNDEBUG on at
> > > > least some library versions.
> > > > 
> > > > Reported-by: Jan Palus <jpalus@fastmail.com>
> > > > Fixes: bc872d91765d ("treewide: Spell ASSERT() as assert()")
> > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > ---
> > > >  util.h | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/util.h b/util.h
> > > > index 70aadeba..11f71d45 100644
> > > > --- a/util.h
> > > > +++ b/util.h
> > > > @@ -6,6 +6,7 @@
> > > >  #ifndef UTIL_H
> > > >  #define UTIL_H
> > > >  
> > > > +#include <assert.h>
> > > >  #include <stdlib.h>
> > > >  #include <stdarg.h>
> > > >  #include <stdbool.h>
> > > > @@ -60,7 +61,7 @@ void abort_with_msg(const char *fmt, ...)
> > > >  			__func__, __FILE__, __LINE__, STRINGIFY(expr))
> > > >  #else
> > > >  #define assert_with_msg(expr, ...)					\
> > > > -	((void)(expr), 0 ? (void)0 : abort_with_msg(__VA_ARGS__))
> > > > +	((void)(expr), 1 ? (void)0 : abort_with_msg(__VA_ARGS__))  
> > > 
> > > There is a slight semantic difference between assert() and
> > > assert_with_msg() when building with -DNDEBUG -- `expr` is still being
> > > evaluated in abort_with_msg() although likely optimized out in most
> > > builds.  
> > 
> > Right, I'm expecting the compiler to optimise this away.  That won't
> > be the case if the expression has side effects, but side effects in an
> > assert() expression is already a bug, precisely because of the
> > possibility NDEBUG.
> > 
> > > I'm assuming you'd prefer to avoid cppcheck suppressions.  
> > 
> > cppcheck suppressions would be acceptable if we could put them inside
> > the macro.  But these are unused variable warnings, which show up at
> > the site of variable declaration not (obviously) at the place the
> > variable... isn't used.  Putting suppressions on variables because
> > they might become unused depending on a macro definition certainly
> > isn't acceptable.  Both for aesthetics, and because it means if we
> > removed the assert we'd no longer get a warning that the variable now
> > really isn't used.
> > 
> > > How
> > > about moving `expr` into branch which is never evaluated then? Would it
> > > keep cppcheck happy?
> > > 
> > > +	(1 ? (void)0 : ((void)(expr), abort_with_msg(__VA_ARGS__)))  
> > 
> > That does work.  Not sure if it's worth the bother of a respin.
> > Stefano, any opinion?
> 
> I think it would be worth changing that given that the only reason why
> PLD Linux enables NDEBUG is to avoid evaluating those expressions.

I guess.  But I'm pretty sure it will come to the same thing unless
you used both -DNDEBUG *and* -O0, which seems a perverse combination.

> I can also change it on merge though, it's trivial enough. I'll do that
> if everybody is fine with it.

Fine by me.

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

[PATCH 3/3] test: Add test for builds with -DNDEBUG

[David Gibson]

Since bc872d91765d we omit assert()s if the NDEBUG preprocessor symbol is
defined, as is the case with the standard library assert(3).  However,
none of our tests verify that we can actually build that way.  Add an
extra test to test/build/build.py tto verify this.

Fixes: bc872d91765d ("treewide: Spell ASSERT() as assert()")

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 test/build/build.py | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/test/build/build.py b/test/build/build.py
index eeee5acc..59de014a 100755
--- a/test/build/build.py
+++ b/test/build/build.py
@@ -77,6 +77,20 @@ for bin in BINARIES:
 exeter.register('make_all', test_make, 'all', BINARIES)
 
 
+@exeter.test
+def test_ndebug() -> None:
+    """Test build with -NDEBUG
+
+    Tests that we can build all binaries with -DNDEBUG (warnings are
+    expected, though).  Doesn't test that they actually work.
+    """
+
+    with clone_sources():
+        sh('make all CPPFLAGS="-DNDEBUG" CFLAGS="-w"')
+        for b in BINARIES:
+            assert Path(b).exists(), f"{b} wasn't made"
+
+
 @exeter.test
 def test_install_uninstall() -> None:
     """Test `make install` and `make uninstall`
-- 
2.54.0