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.engest...@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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
