Re: [PATCH 4/5] treewide: Spell ASSERT() as assert()

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

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

On Tue, Mar 17, 2026 at 10:36:25AM +0100, Stefano Brivio wrote:
> On Tue, 17 Mar 2026 11:39:42 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Tue, Mar 17, 2026 at 01:02:34AM +0100, Stefano Brivio wrote:
> > > On Mon, 16 Mar 2026 16:46:28 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > +++ b/util.h
> > > > @@ -73,10 +73,14 @@ void abort_with_msg(const char *fmt, ...)
> > > >   * Therefore, avoid using the usual do while wrapper we use to force the macro
> > > >   * to act like a single statement requiring a ';'.
> > > >   */
> > > > -#define ASSERT_WITH_MSG(expr, ...)					\
> > > > +#define assert_with_msg(expr, ...)					\
> > > >  	((expr) ? (void)0 : abort_with_msg(__VA_ARGS__))
> > > > -#define ASSERT(expr)							\
> > > > -	ASSERT_WITH_MSG((expr), "ASSERTION FAILED in %s (%s:%d): %s",	\
> > > > +/* The standard library assert() hits our seccomp filter and dies before it can
> > > > + * actually print a message.  So, replace it with our own version.
> > > > + */
> > > > +#undef assert
> > > > +#define assert(expr)							\
> > > > +	assert_with_msg((expr), "ASSERTION FAILED in %s (%s:%d): %s",	\
> > > >  			__func__, __FILE__, __LINE__, STRINGIFY(expr))  
> > > 
> > > While looking this up to make sure it's specified as a macro (it is,
> > > and this builds against musl as well), I realised that POSIX.1-2024
> > > says:
> > > 
> > >   https://pubs.opengroup.org/onlinepubs/9799919799/functions/assert.html
> > > 
> > >   Forcing a definition of the name NDEBUG, either from the compiler
> > >   command line or with the preprocessor control statement #define NDEBUG
> > >   ahead of the #include <assert.h> statement, shall stop assertions from
> > >   being compiled into the program.
> > > 
> > > ...so, I wonder, now that it's called assert(), should we define it as
> > > "do { } while(0)" #ifdef NDEBUG, for correctness (and maybe somebody
> > > has obscure usages for NDEBUG which we shouldn't sabotage)?  
> > 
> > I like the idea in principle.  Actually implementing it turns out to
> > be kind of a pain in the arse, because if we actually try to compile
> > with -DNDEBUG then we get a much of warnings due to reaching the end
> > of functions (assert(0) stopped us otherwise) or unused variables
> > (they're only used in the assert expression or message).
> > 
> > A project for some other time, I think.
> 
> Well but it's just (probably harmless) warnings right? I tried with
> this on top of your patch:
> 
> ---
> diff --git a/util.h b/util.h
> index dcb79af..77b59bc 100644
> --- a/util.h
> +++ b/util.h
> @@ -75,13 +75,18 @@ void abort_with_msg(const char *fmt, ...)
>   */
>  #define assert_with_msg(expr, ...)					\
>  	((expr) ? (void)0 : abort_with_msg(__VA_ARGS__))
> +
>  /* The standard library assert() hits our seccomp filter and dies before it can
>   * actually print a message.  So, replace it with our own version.
>   */
>  #undef assert
> +#ifdef NDEBUG
> +#define assert(expr)	do { } while(0)

In fact we don't need to explicitly do this.  In the NDEBUG case,
assert() is already a no-op, so we don't need to redefine it.

> +#else
>  #define assert(expr)							\
>  	assert_with_msg((expr), "ASSERTION FAILED in %s (%s:%d): %s",	\
>  			__func__, __FILE__, __LINE__, STRINGIFY(expr))

... and principle of least surprise suggests to me that
assert_with_msg() should also become a no-op with -DNDEBUG.

> +#endif
>  
>  #ifdef P_tmpdir
>  #define TMPDIR		P_tmpdir
> ---
> 
> and 'CFLAGS="-DNDEBUG" make' gives me some stuff such as, for example:
> 
> util.c: In function ‘sock_l4_’:
> util.c:90:14: warning: ‘proto’ may be used uninitialized [-Wmaybe-uninitialized]
>    90 |         fd = socket(af, socktype, proto);
>       |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> because an invalid value for it isn't caught by assert(0) anymore, but
> from a quick review I'd say it's all intended and implied because of the
> missing assert() calls.
> 
> Sure, the output with NDEBUG is not pretty, but we won't be able to "fix"
> most of that anyway. If somebody passes it as extra CFLAGS, I would expect
> they know what they're doing. Nobody will build distribution packages or
> anything "official" with it, I think.

Good point.  I was thinking that compiling clean with -DNDEBUG was a
goal, but it really isn't.

> Either way, should I go ahead and merge this (in the original version or
> amended for NDEBUG, I'm fine with both)?

Not quite - I'll put a version with the tweaks described above applied
in the next spin.

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