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

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

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