Re: [Mesa-dev] [PATCH mesa] u_debug: do not compile asserts when they are disabled
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
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
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
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
