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

[Pekka Paalanen]

On Thu, 30 Nov 2017 12:16:06 +
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

Hi,

I believe that is not true. Your patch has:

> -#define debug_assert(expr) (void)(0 && (expr))

which, as I understand, means that (expr) is never evaluated because
the 0 short-circuits the && operator.


Thanks,
pq


> - 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.
> 
> Fixes: f0ba7d897d1c22202531a "util: better fix for unused variable
>warnings with asserts"
> Cc: Keith Whitwell 
> Reported-by: Gert Wollny 
> Signed-off-by: Eric Engestrom 
> ---
>  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
>  
>  



pgpJOge_1WwRL.pgp
Description: OpenPGP digital signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Michel Dänzer]

On 2017-11-30 01:16 PM, 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 "0 &&" should prevent this.


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

Yes, not having to guard things by NDEBUG was a major point of doing it
this way. :)


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Gert Wollny]

There was actually some ping-pong about this issue: 

d885c9dad (Keith Whitwell) introduced it, 
7a2271c65 (José Fonseca) removed it , and  
f0ba7d897 (Keith Whitwell) introduced it again. 

Given this I would give some preference to not to forward standard
assert instead, but don't really see it as an issue to change
debug_assert.

With a31d289de6091 José Fonseca introduced p_debug.h which would
eventually become u_debug,h, and there assert is already channeled to
debug_assert, but it is not clear why it was seen as problematic to
used the standard C assert in 2008.

In any case:
Reviewed-By: Gert Wollny 

Am Donnerstag, den 30.11.2017, 12:16 + schrieb Eric Engestrom:
> 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.
> 
> Fixes: f0ba7d897d1c22202531a "util: better fix for unused variable
>    warnings with asserts"
> Cc: Keith Whitwell 
> Reported-by: Gert Wollny 
> Signed-off-by: Eric Engestrom 
> ---
>  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
>  
>  
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Eric Engestrom]

On Thursday, 2017-11-30 12:16:06 +, 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 
> Reported-by: Gert Wollny 
> Signed-off-by: Eric Engestrom 
> ---
>  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
> 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Eric Engestrom]

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.

Fixes: f0ba7d897d1c22202531a "util: better fix for unused variable
   warnings with asserts"
Cc: Keith Whitwell 
Reported-by: Gert Wollny 
Signed-off-by: Eric Engestrom 
---
 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

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev