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

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

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