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

[Ian Romanick]

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.

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.

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171127/a2b896b4/attachment-0001.sig>