[Mesa-dev] [PATCH mesa 16/16] util: use NDEBUG to guard asserts

[Eric Engestrom]

On Monday, 2017-11-27 12:53:39 -0800, Ian Romanick wrote:
> On 11/27/2017 10:46 AM, Dylan Baker wrote:
> > Quoting Eric Engestrom (2017-11-27 09:46:57)
> >> On Saturday, 2017-11-25 18:45:14 +0100, Gert Wollny wrote:
> >>> Am Freitag, den 24.11.2017, 18:07 +0000 schrieb Eric Engestrom:
> >>>> Signed-off-by: Eric Engestrom <eric.engestrom at imgtec.com>
> >>>> ---
> >>>>  src/util/ralloc.c | 18 +++++++++---------
> >>>>  src/util/slab.c   |  4 ++--
> >>>>  2 files changed, 11 insertions(+), 11 deletions(-)
> >>>>
> >>>> diff --git a/src/util/ralloc.c b/src/util/ralloc.c
> >>>> index 42cfa2e391d52df68db2..b52079ac075a0fe11944 100644
> >>>> --- a/src/util/ralloc.c
> >>>> +++ b/src/util/ralloc.c
> >>>> @@ -61,7 +61,7 @@ struct
> >>>>  #endif
> >>>>     ralloc_header
> >>>>  {
> >>>> -#ifdef DEBUG
> >>>> +#ifndef NDEBUG
> >>>>     /* A canary value used to determine whether a pointer is
> >>>> ralloc'd. */
> >>>>     unsigned canary;
> >>>>  #endif
> >>>> @@ -88,7 +88,7 @@ get_header(const void *ptr)
> >>>>  {
> >>>>     ralloc_header *info = (ralloc_header *) (((char *) ptr) -
> >>>>                                         sizeof(ralloc_header));
> >>>> -#ifdef DEBUG
> >>>> +#ifndef NDEBUG
> >>>>     assert(info->canary == CANARY);
> >>>>  #endif
> >>>
> >>> With NDEBUG defined "assert" already translates to a no-op, hence the
> >>> extra "#ifndef NDEBUG" block is not needed (same for the other asserts
> >>> below and in the other patches). 
> >>
> >> Indeed, I did so many of those conversions that I wasn't thinking about
> >> that anymore. Thanks for catching that, I'll remove the include guards
> >> altogether around plain asserts()s.
> > 
> > Is that true in this case? info->canary is only defined if NDEBUG is not
> > defined, so this will fail to compile since those symbols are undefined, right?
> 
> The parameters to a preprocessor macro are (basically) just text.  They
> are not evaluated until after the macro is expanded.  I'm 93.2% sure
> removing the guards around the assert() should be fine in this case.

100% sure :)
look at /usr/include/assert.h:

#ifdef NDEBUG
#define assert(expr) ((void) (0))
#endif

The compiler doesn't even see the contents of `assert(foo)`, it's gone
by the time the pre-compiler has done its job.

> 
> As for changing the other guards (here and in the other patches), I'm a
> little less sure.  There are two different debugging macros that have
> two different, partially historical, meanings.
> 
> NDEBUG is part of ANSI C.  When NDEBUG is defined, assert() expands to
> nothing.  In Mesa, individual assertions are usually cheap, but there
> may be a lot of them.  As far as I'm aware, some distros build most
> packages without NDEBUG defined because some developers (incorrectly)
> use assertions in place of regular error checks or have necessary code
> inside the assertion.  Mesa has had some bugs of this nature in the past.
> 
> DEBUG is a Mesa invention.  It has been used over the years to enable
> much more expensive checks.  Both GLSL IR and NIR use DEBUG to enable
> expensive shader IR validation, for example.  I also thought that the
> ralloc code used DEBUG to enable periodic validation of allocation
> headers and parent / child lists, but I may be mistaken there.
> 
> I think there is value in being able to enable cheap and expensive debug
> checks independently.  Enabling IR validation after every optimization
> pass makes a shader-db run take an excruciating amount of time.  At the
> same time, I still want to have some checks to detect problems that I
> may have created.
> 
> The existing situation is a bit confusing.  At times people use DEBUG
> when they probably want NDEBUG.  I think part of the problem is that
> DEBUG is such a generic name, and I think a lot of people don't know
> about NDEBUG.  There are a few ways we could probably clean this up, but
> I don't have strong feeling for which would be best.

Agreed with everything you said :)

> 
> > Dylan
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>