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

On Sat, 16 May 2026 16:28:52 +1000 David Gibson 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 Fixes: bc872d91765d ("treewide: Spell ASSERT() as assert()") Signed-off-by: David Gibson --- 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 #include #include #include @@ -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