[Mesa-dev] [PATCH mesa] u_debug: do not compile asserts when they are disabled

[Eric Engestrom]

On Thursday, 2017-11-30 12:16:06 +0000, Eric Engestrom wrote:
> Commit f0ba7d897d1c22202531a added this code to expose asserts to the
> compiler in an attempt to hide 'unused variable' warnings, incorrectly
> claiming it was a no-op. This has two bad effects:
> - any assert with side-effects are executed when they should be
>   disabled
> - the whole content of the assert must be understandable by the
>   compiler, which isn't true if variable or members are correctly
>   guarded by NDEBUG
> 
> Fix this by effectively reverting f0ba7d897d1c22202531a.
> 
> Unused variables warnings can be addressed by marking the variables as
> MAYBE_UNUSED instead, as is done in the rest of Mesa.

Forgot to mention, I looked at all the debug_assert() and none of them
have side effects, so this change is safe... except that I just
remembered that the other problem in this header is the fact it
overrides the standard assert().
I had a quick grep, and there are almost 6000 `assert()` that are being
replaced by `debug_assert()` because of this line. That's too much for
me to audit.
If someone somewhere started relying on the fact disabled
asserts are still executed, things would break as a result of this
patch. Arguably, those are bugs anyway, just not "active" right now.

I still think this patch should be applied, to fix the second issue, but
one should be aware that breakage are possible if some code relied on
the counter-intuitive behaviour of executing disabled asserts.

(I'll add the short version of this to the commit message)

> 
> Fixes: f0ba7d897d1c22202531a "util: better fix for unused variable
>        warnings with asserts"
> Cc: Keith Whitwell <keithw at vmware.com>
> Reported-by: Gert Wollny <gw.fossdev at gmail.com>
> Signed-off-by: Eric Engestrom <eric.engestrom at imgtec.com>
> ---
>  src/gallium/auxiliary/util/u_debug.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/gallium/auxiliary/util/u_debug.h b/src/gallium/auxiliary/util/u_debug.h
> index d2ea89f59c10e1bc0944..88e9bb8a29d63826167e 100644
> --- a/src/gallium/auxiliary/util/u_debug.h
> +++ b/src/gallium/auxiliary/util/u_debug.h
> @@ -188,7 +188,7 @@ void _debug_assert_fail(const char *expr,
>  #ifndef NDEBUG
>  #define debug_assert(expr) ((expr) ? (void)0 : _debug_assert_fail(#expr, __FILE__, __LINE__, __FUNCTION__))
>  #else
> -#define debug_assert(expr) (void)(0 && (expr))
> +#define debug_assert(expr) ((void)0)
>  #endif
>  
>  
> -- 
> Cheers,
>   Eric
>