Re: [Mesa-dev] [PATCH mesa] u_debug: do not compile asserts when they are disabled
On Thu, 30 Nov 2017 12:16:06 + Eric Engestromwrote: > 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 WollnyAm 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
[Mesa-dev] [PATCH mesa] u_debug: do not compile asserts when they are disabled
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 WhitwellReported-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