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

[Stefano Brivio <sbrivio@redhat.com>]

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)
+#else
 #define assert(expr)							\
 	assert_with_msg((expr), "ASSERTION FAILED in %s (%s:%d): %s",	\
 			__func__, __FILE__, __LINE__, STRINGIFY(expr))
+#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.

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

-- 
Stefano