list: add some iterator debug

Submitted by Rob Clark on May 25, 2019, 6:03 p.m.

Details

Message ID 20190525180321.8176-1-robdclark@gmail.com
State New
Headers show
Series "list: add some iterator debug" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Rob Clark May 25, 2019, 6:03 p.m.
From: Rob Clark <robdclark@chromium.org>

Debugging use of unsafe iterators when you should have used the _safe
version sucks.  Add some DEBUG build support to catch and assert if
someone does that.

I didn't update the UPPERCASE verions of the iterators.  They should
probably be deprecated/removed.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 src/util/list.h | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/util/list.h b/src/util/list.h
index 09d1b4cae64..6d89a42b226 100644
--- a/src/util/list.h
+++ b/src/util/list.h
@@ -43,6 +43,13 @@ 
 #include <assert.h>
 #include "c99_compat.h"
 
+#ifdef DEBUG
+#  define LIST_DEBUG 1
+#else
+#  define LIST_DEBUG 0
+#endif
+
+#define list_assert(cond, msg)  ({ if (LIST_DEBUG) assert((cond) && msg); })
 
 struct list_head
 {
@@ -212,21 +219,27 @@  static inline void list_validate(const struct list_head *list)
 	pos = container_of(pos->member.prev, pos, member))
 
 #define list_for_each_entry(type, pos, head, member)                    \
-   for (type *pos = LIST_ENTRY(type, (head)->next, member);             \
+   for (type *pos = LIST_ENTRY(type, (head)->next, member),             \
+	     *__next = LIST_ENTRY(type, pos->member.next, member);      \
 	&pos->member != (head);                                         \
-	pos = LIST_ENTRY(type, pos->member.next, member))
+	pos = LIST_ENTRY(type, pos->member.next, member),               \
+	list_assert(pos == __next, "use _safe iterator"),               \
+	__next = LIST_ENTRY(type, __next->member.next, member))
 
 #define list_for_each_entry_safe(type, pos, head, member)               \
    for (type *pos = LIST_ENTRY(type, (head)->next, member),             \
 	     *__next = LIST_ENTRY(type, pos->member.next, member);      \
 	&pos->member != (head);                                         \
 	pos = __next,                                                   \
-        __next = LIST_ENTRY(type, __next->member.next, member))
+	__next = LIST_ENTRY(type, __next->member.next, member))
 
 #define list_for_each_entry_rev(type, pos, head, member)                \
-   for (type *pos = LIST_ENTRY(type, (head)->prev, member);             \
+   for (type *pos = LIST_ENTRY(type, (head)->prev, member),             \
+	     *__prev = LIST_ENTRY(type, pos->member.prev, member);      \
 	&pos->member != (head);                                         \
-	pos = LIST_ENTRY(type, pos->member.prev, member))
+	pos = LIST_ENTRY(type, pos->member.prev, member),               \
+	list_assert(pos == __prev, "use _safe iterator"),               \
+	__prev = LIST_ENTRY(type, __prev->member.prev, member))
 
 #define list_for_each_entry_safe_rev(type, pos, head, member)           \
    for (type *pos = LIST_ENTRY(type, (head)->prev, member),             \

Comments

On Sat, May 25, 2019 at 2:03 PM Rob Clark <robdclark@gmail.com> wrote:
>
> From: Rob Clark <robdclark@chromium.org>
>
> Debugging use of unsafe iterators when you should have used the _safe
> version sucks.  Add some DEBUG build support to catch and assert if
> someone does that.
>
> I didn't update the UPPERCASE verions of the iterators.  They should
> probably be deprecated/removed.
>
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
>  src/util/list.h | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/src/util/list.h b/src/util/list.h
> index 09d1b4cae64..6d89a42b226 100644
> --- a/src/util/list.h
> +++ b/src/util/list.h
> @@ -43,6 +43,13 @@
>  #include <assert.h>
>  #include "c99_compat.h"
>
> +#ifdef DEBUG
> +#  define LIST_DEBUG 1
> +#else
> +#  define LIST_DEBUG 0
> +#endif
> +
> +#define list_assert(cond, msg)  ({ if (LIST_DEBUG) assert((cond) && msg); })

Not sure if it's worth worrying about, but this style of macro
definition can be dangerous. One might use it as

if (x) list_assert()
else blah;

With the macro defined as-is, the "else blah" will get attached to the
if in the macro. I believe the common style is to do do {}while(0) to
avoid such issues (or to use an inline function). Alternatively, just
define it differently for LIST_DEBUG vs not.

Cheers,

  -ilia
On Sat, May 25, 2019 at 11:13 AM Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>
> On Sat, May 25, 2019 at 2:03 PM Rob Clark <robdclark@gmail.com> wrote:
> >
> > From: Rob Clark <robdclark@chromium.org>
> >
> > Debugging use of unsafe iterators when you should have used the _safe
> > version sucks.  Add some DEBUG build support to catch and assert if
> > someone does that.
> >
> > I didn't update the UPPERCASE verions of the iterators.  They should
> > probably be deprecated/removed.
> >
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > ---
> >  src/util/list.h | 23 ++++++++++++++++++-----
> >  1 file changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/util/list.h b/src/util/list.h
> > index 09d1b4cae64..6d89a42b226 100644
> > --- a/src/util/list.h
> > +++ b/src/util/list.h
> > @@ -43,6 +43,13 @@
> >  #include <assert.h>
> >  #include "c99_compat.h"
> >
> > +#ifdef DEBUG
> > +#  define LIST_DEBUG 1
> > +#else
> > +#  define LIST_DEBUG 0
> > +#endif
> > +
> > +#define list_assert(cond, msg)  ({ if (LIST_DEBUG) assert((cond) && msg); })
>
> Not sure if it's worth worrying about, but this style of macro
> definition can be dangerous. One might use it as
>
> if (x) list_assert()
> else blah;
>
> With the macro defined as-is, the "else blah" will get attached to the
> if in the macro. I believe the common style is to do do {}while(0) to
> avoid such issues (or to use an inline function). Alternatively, just
> define it differently for LIST_DEBUG vs not.
>

I think the ({ ... }) should save the day..

(hmm, is that c99 or a gnu thing?  I've it isn't avail on some
compilers I guess we should disable list_assert() for those?)

BR,
-R
Yeah, that's a GNU extension. It also works in clang but not MSVC which is 
used to build NIR.

On May 25, 2019 13:30:29 Rob Clark <robdclark@gmail.com> wrote:

> On Sat, May 25, 2019 at 11:13 AM Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>
>> On Sat, May 25, 2019 at 2:03 PM Rob Clark <robdclark@gmail.com> wrote:
>> >
>> > From: Rob Clark <robdclark@chromium.org>
>> >
>> > Debugging use of unsafe iterators when you should have used the _safe
>> > version sucks.  Add some DEBUG build support to catch and assert if
>> > someone does that.
>> >
>> > I didn't update the UPPERCASE verions of the iterators.  They should
>> > probably be deprecated/removed.
>> >
>> > Signed-off-by: Rob Clark <robdclark@chromium.org>
>> > ---
>> >  src/util/list.h | 23 ++++++++++++++++++-----
>> >  1 file changed, 18 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/src/util/list.h b/src/util/list.h
>> > index 09d1b4cae64..6d89a42b226 100644
>> > --- a/src/util/list.h
>> > +++ b/src/util/list.h
>> > @@ -43,6 +43,13 @@
>> >  #include <assert.h>
>> >  #include "c99_compat.h"
>> >
>> > +#ifdef DEBUG
>> > +#  define LIST_DEBUG 1
>> > +#else
>> > +#  define LIST_DEBUG 0
>> > +#endif
>> > +
>> > +#define list_assert(cond, msg)  ({ if (LIST_DEBUG) assert((cond) && msg); })
>>
>> Not sure if it's worth worrying about, but this style of macro
>> definition can be dangerous. One might use it as
>>
>> if (x) list_assert()
>> else blah;
>>
>> With the macro defined as-is, the "else blah" will get attached to the
>> if in the macro. I believe the common style is to do do {}while(0) to
>> avoid such issues (or to use an inline function). Alternatively, just
>> define it differently for LIST_DEBUG vs not.
>>
>
> I think the ({ ... }) should save the day..
>
> (hmm, is that c99 or a gnu thing?  I've it isn't avail on some
> compilers I guess we should disable list_assert() for those?)
>
> BR,
> -R
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Is there a convenient #ifdef I can use to guard the list_assert()
macro..  I don't really mind if MSVC can't have this, but would rather
not let it prevent the rest of us from having nice things

BR,
-R

On Sat, May 25, 2019 at 1:23 PM Jason Ekstrand <jason@jlekstrand.net> wrote:
>
> Yeah, that's a GNU extension. It also works in clang but not MSVC which is
> used to build NIR.
>
> On May 25, 2019 13:30:29 Rob Clark <robdclark@gmail.com> wrote:
>
> > On Sat, May 25, 2019 at 11:13 AM Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> >>
> >> On Sat, May 25, 2019 at 2:03 PM Rob Clark <robdclark@gmail.com> wrote:
> >> >
> >> > From: Rob Clark <robdclark@chromium.org>
> >> >
> >> > Debugging use of unsafe iterators when you should have used the _safe
> >> > version sucks.  Add some DEBUG build support to catch and assert if
> >> > someone does that.
> >> >
> >> > I didn't update the UPPERCASE verions of the iterators.  They should
> >> > probably be deprecated/removed.
> >> >
> >> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> >> > ---
> >> >  src/util/list.h | 23 ++++++++++++++++++-----
> >> >  1 file changed, 18 insertions(+), 5 deletions(-)
> >> >
> >> > diff --git a/src/util/list.h b/src/util/list.h
> >> > index 09d1b4cae64..6d89a42b226 100644
> >> > --- a/src/util/list.h
> >> > +++ b/src/util/list.h
> >> > @@ -43,6 +43,13 @@
> >> >  #include <assert.h>
> >> >  #include "c99_compat.h"
> >> >
> >> > +#ifdef DEBUG
> >> > +#  define LIST_DEBUG 1
> >> > +#else
> >> > +#  define LIST_DEBUG 0
> >> > +#endif
> >> > +
> >> > +#define list_assert(cond, msg)  ({ if (LIST_DEBUG) assert((cond) && msg); })
> >>
> >> Not sure if it's worth worrying about, but this style of macro
> >> definition can be dangerous. One might use it as
> >>
> >> if (x) list_assert()
> >> else blah;
> >>
> >> With the macro defined as-is, the "else blah" will get attached to the
> >> if in the macro. I believe the common style is to do do {}while(0) to
> >> avoid such issues (or to use an inline function). Alternatively, just
> >> define it differently for LIST_DEBUG vs not.
> >>
> >
> > I think the ({ ... }) should save the day..
> >
> > (hmm, is that c99 or a gnu thing?  I've it isn't avail on some
> > compilers I guess we should disable list_assert() for those?)
> >
> > BR,
> > -R
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
>
Why not just do it in a way that works for everyone? Both the do/while
method and the ifdef-based method that I suggested work everywhere. Or
is there another reason you prefer to use those statement expressions?

On Sat, May 25, 2019 at 6:21 PM Rob Clark <robdclark@gmail.com> wrote:
>
> Is there a convenient #ifdef I can use to guard the list_assert()
> macro..  I don't really mind if MSVC can't have this, but would rather
> not let it prevent the rest of us from having nice things
>
> BR,
> -R
>
> On Sat, May 25, 2019 at 1:23 PM Jason Ekstrand <jason@jlekstrand.net> wrote:
> >
> > Yeah, that's a GNU extension. It also works in clang but not MSVC which is
> > used to build NIR.
> >
> > On May 25, 2019 13:30:29 Rob Clark <robdclark@gmail.com> wrote:
> >
> > > On Sat, May 25, 2019 at 11:13 AM Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> > >>
> > >> On Sat, May 25, 2019 at 2:03 PM Rob Clark <robdclark@gmail.com> wrote:
> > >> >
> > >> > From: Rob Clark <robdclark@chromium.org>
> > >> >
> > >> > Debugging use of unsafe iterators when you should have used the _safe
> > >> > version sucks.  Add some DEBUG build support to catch and assert if
> > >> > someone does that.
> > >> >
> > >> > I didn't update the UPPERCASE verions of the iterators.  They should
> > >> > probably be deprecated/removed.
> > >> >
> > >> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > >> > ---
> > >> >  src/util/list.h | 23 ++++++++++++++++++-----
> > >> >  1 file changed, 18 insertions(+), 5 deletions(-)
> > >> >
> > >> > diff --git a/src/util/list.h b/src/util/list.h
> > >> > index 09d1b4cae64..6d89a42b226 100644
> > >> > --- a/src/util/list.h
> > >> > +++ b/src/util/list.h
> > >> > @@ -43,6 +43,13 @@
> > >> >  #include <assert.h>
> > >> >  #include "c99_compat.h"
> > >> >
> > >> > +#ifdef DEBUG
> > >> > +#  define LIST_DEBUG 1
> > >> > +#else
> > >> > +#  define LIST_DEBUG 0
> > >> > +#endif
> > >> > +
> > >> > +#define list_assert(cond, msg)  ({ if (LIST_DEBUG) assert((cond) && msg); })
> > >>
> > >> Not sure if it's worth worrying about, but this style of macro
> > >> definition can be dangerous. One might use it as
> > >>
> > >> if (x) list_assert()
> > >> else blah;
> > >>
> > >> With the macro defined as-is, the "else blah" will get attached to the
> > >> if in the macro. I believe the common style is to do do {}while(0) to
> > >> avoid such issues (or to use an inline function). Alternatively, just
> > >> define it differently for LIST_DEBUG vs not.
> > >>
> > >
> > > I think the ({ ... }) should save the day..
> > >
> > > (hmm, is that c99 or a gnu thing?  I've it isn't avail on some
> > > compilers I guess we should disable list_assert() for those?)
> > >
> > > BR,
> > > -R
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
> >
> >
This ends up embedded in a for loop expression, ie. the C part in an for (A;B;C)

iirc, that means it needs to be a C expr rather than statement.. or
something roughly like that, I'm too lazy to dig out my C grammar

BR,
-R


On Sat, May 25, 2019 at 3:39 PM Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>
> Why not just do it in a way that works for everyone? Both the do/while
> method and the ifdef-based method that I suggested work everywhere. Or
> is there another reason you prefer to use those statement expressions?
>
> On Sat, May 25, 2019 at 6:21 PM Rob Clark <robdclark@gmail.com> wrote:
> >
> > Is there a convenient #ifdef I can use to guard the list_assert()
> > macro..  I don't really mind if MSVC can't have this, but would rather
> > not let it prevent the rest of us from having nice things
> >
> > BR,
> > -R
> >
> > On Sat, May 25, 2019 at 1:23 PM Jason Ekstrand <jason@jlekstrand.net> wrote:
> > >
> > > Yeah, that's a GNU extension. It also works in clang but not MSVC which is
> > > used to build NIR.
> > >
> > > On May 25, 2019 13:30:29 Rob Clark <robdclark@gmail.com> wrote:
> > >
> > > > On Sat, May 25, 2019 at 11:13 AM Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> > > >>
> > > >> On Sat, May 25, 2019 at 2:03 PM Rob Clark <robdclark@gmail.com> wrote:
> > > >> >
> > > >> > From: Rob Clark <robdclark@chromium.org>
> > > >> >
> > > >> > Debugging use of unsafe iterators when you should have used the _safe
> > > >> > version sucks.  Add some DEBUG build support to catch and assert if
> > > >> > someone does that.
> > > >> >
> > > >> > I didn't update the UPPERCASE verions of the iterators.  They should
> > > >> > probably be deprecated/removed.
> > > >> >
> > > >> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > >> > ---
> > > >> >  src/util/list.h | 23 ++++++++++++++++++-----
> > > >> >  1 file changed, 18 insertions(+), 5 deletions(-)
> > > >> >
> > > >> > diff --git a/src/util/list.h b/src/util/list.h
> > > >> > index 09d1b4cae64..6d89a42b226 100644
> > > >> > --- a/src/util/list.h
> > > >> > +++ b/src/util/list.h
> > > >> > @@ -43,6 +43,13 @@
> > > >> >  #include <assert.h>
> > > >> >  #include "c99_compat.h"
> > > >> >
> > > >> > +#ifdef DEBUG
> > > >> > +#  define LIST_DEBUG 1
> > > >> > +#else
> > > >> > +#  define LIST_DEBUG 0
> > > >> > +#endif
> > > >> > +
> > > >> > +#define list_assert(cond, msg)  ({ if (LIST_DEBUG) assert((cond) && msg); })
> > > >>
> > > >> Not sure if it's worth worrying about, but this style of macro
> > > >> definition can be dangerous. One might use it as
> > > >>
> > > >> if (x) list_assert()
> > > >> else blah;
> > > >>
> > > >> With the macro defined as-is, the "else blah" will get attached to the
> > > >> if in the macro. I believe the common style is to do do {}while(0) to
> > > >> avoid such issues (or to use an inline function). Alternatively, just
> > > >> define it differently for LIST_DEBUG vs not.
> > > >>
> > > >
> > > > I think the ({ ... }) should save the day..
> > > >
> > > > (hmm, is that c99 or a gnu thing?  I've it isn't avail on some
> > > > compilers I guess we should disable list_assert() for those?)
> > > >
> > > > BR,
> > > > -R
> > > > _______________________________________________
> > > > mesa-dev mailing list
> > > > mesa-dev@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > >
> > >
> > >
On Sat, 2019-05-25 at 15:44 -0700, Rob Clark wrote:
> This ends up embedded in a for loop expression, ie. the C part in an
> for (A;B;C)
> 
> iirc, that means it needs to be a C expr rather than statement.. or
> something roughly like that, I'm too lazy to dig out my C grammar
> 

Can't you just call a static helper function to do the validation?
Function calls are valid expressions...

> BR,
> -R
> 
> 
> On Sat, May 25, 2019 at 3:39 PM Ilia Mirkin <imirkin@alum.mit.edu>
> wrote:
> > Why not just do it in a way that works for everyone? Both the
> > do/while
> > method and the ifdef-based method that I suggested work everywhere.
> > Or
> > is there another reason you prefer to use those statement
> > expressions?
> > 
> > On Sat, May 25, 2019 at 6:21 PM Rob Clark <robdclark@gmail.com>
> > wrote:
> > > Is there a convenient #ifdef I can use to guard the list_assert()
> > > macro..  I don't really mind if MSVC can't have this, but would
> > > rather
> > > not let it prevent the rest of us from having nice things
> > > 
> > > BR,
> > > -R
> > > 
> > > On Sat, May 25, 2019 at 1:23 PM Jason Ekstrand <
> > > jason@jlekstrand.net> wrote:
> > > > Yeah, that's a GNU extension. It also works in clang but not
> > > > MSVC which is
> > > > used to build NIR.
> > > > 
> > > > On May 25, 2019 13:30:29 Rob Clark <robdclark@gmail.com> wrote:
> > > > 
> > > > > On Sat, May 25, 2019 at 11:13 AM Ilia Mirkin <
> > > > > imirkin@alum.mit.edu> wrote:
> > > > > > On Sat, May 25, 2019 at 2:03 PM Rob Clark <
> > > > > > robdclark@gmail.com> wrote:
> > > > > > > From: Rob Clark <robdclark@chromium.org>
> > > > > > > 
> > > > > > > Debugging use of unsafe iterators when you should have
> > > > > > > used the _safe
> > > > > > > version sucks.  Add some DEBUG build support to catch and
> > > > > > > assert if
> > > > > > > someone does that.
> > > > > > > 
> > > > > > > I didn't update the UPPERCASE verions of the
> > > > > > > iterators.  They should
> > > > > > > probably be deprecated/removed.
> > > > > > > 
> > > > > > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > > > > > ---
> > > > > > >  src/util/list.h | 23 ++++++++++++++++++-----
> > > > > > >  1 file changed, 18 insertions(+), 5 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/src/util/list.h b/src/util/list.h
> > > > > > > index 09d1b4cae64..6d89a42b226 100644
> > > > > > > --- a/src/util/list.h
> > > > > > > +++ b/src/util/list.h
> > > > > > > @@ -43,6 +43,13 @@
> > > > > > >  #include <assert.h>
> > > > > > >  #include "c99_compat.h"
> > > > > > > 
> > > > > > > +#ifdef DEBUG
> > > > > > > +#  define LIST_DEBUG 1
> > > > > > > +#else
> > > > > > > +#  define LIST_DEBUG 0
> > > > > > > +#endif
> > > > > > > +
> > > > > > > +#define list_assert(cond, msg)  ({ if (LIST_DEBUG)
> > > > > > > assert((cond) && msg); })
> > > > > > 
> > > > > > Not sure if it's worth worrying about, but this style of
> > > > > > macro
> > > > > > definition can be dangerous. One might use it as
> > > > > > 
> > > > > > if (x) list_assert()
> > > > > > else blah;
> > > > > > 
> > > > > > With the macro defined as-is, the "else blah" will get
> > > > > > attached to the
> > > > > > if in the macro. I believe the common style is to do do
> > > > > > {}while(0) to
> > > > > > avoid such issues (or to use an inline function).
> > > > > > Alternatively, just
> > > > > > define it differently for LIST_DEBUG vs not.
> > > > > > 
> > > > > 
> > > > > I think the ({ ... }) should save the day..
> > > > > 
> > > > > (hmm, is that c99 or a gnu thing?  I've it isn't avail on
> > > > > some
> > > > > compilers I guess we should disable list_assert() for those?)
> > > > > 
> > > > > BR,
> > > > > -R
> > > > > _______________________________________________
> > > > > 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
On Mon, May 27, 2019 at 2:50 AM Erik Faye-Lund
<erik.faye-lund@collabora.com> wrote:
>
> On Sat, 2019-05-25 at 15:44 -0700, Rob Clark wrote:
> > This ends up embedded in a for loop expression, ie. the C part in an
> > for (A;B;C)
> >
> > iirc, that means it needs to be a C expr rather than statement.. or
> > something roughly like that, I'm too lazy to dig out my C grammar
> >
>
> Can't you just call a static helper function to do the validation?
> Function calls are valid expressions...


I do like the fact that with the current patch I get the correct line
# in the assert msg.. but perhaps #ifdef MSVC we can make it a static
inline instead?  I'm not sure how many people do active feature dev of
mesa on windows (as opposed to doing dev on linux and then
compiling/shipping non-debug builds on windows), so maybe just
disabling the list debug on MSVC is fine.

BR,
-R
On Mon, 2019-05-27 at 04:23 -0700, Rob Clark wrote:
> On Mon, May 27, 2019 at 2:50 AM Erik Faye-Lund
> <erik.faye-lund@collabora.com> wrote:
> > On Sat, 2019-05-25 at 15:44 -0700, Rob Clark wrote:
> > > This ends up embedded in a for loop expression, ie. the C part in
> > > an
> > > for (A;B;C)
> > > 
> > > iirc, that means it needs to be a C expr rather than statement..
> > > or
> > > something roughly like that, I'm too lazy to dig out my C grammar
> > > 
> > 
> > Can't you just call a static helper function to do the validation?
> > Function calls are valid expressions...
> 
> I do like the fact that with the current patch I get the correct line
> # in the assert msg.. but perhaps #ifdef MSVC we can make it a static
> inline instead?  I'm not sure how many people do active feature dev
> of
> mesa on windows (as opposed to doing dev on linux and then
> compiling/shipping non-debug builds on windows), so maybe just
> disabling the list debug on MSVC is fine.
> 
> BR,
> -R

You can just pass __FILE__ and __LINE__ as arguments to the helper, and
not use assert directy but rather fprintf(stderr, ...) and abort. This
is in fact how most custom assert-macros are implemented.

Something like this:

static void
list_assert_helper(bool cond, const char *expr, const char *msg,
                   const char *file, int line)
{
   if (cond) {
      fprintf(stderr, "list_assert failed: %s:%d: %s",
              file, line, expr);
      abort();
   }
}

#define list_assert(cond, msg) \
      list_assert_helper(LIST_DEBUG && cond, #cond, msg, \
                         __FILE__, __LINE__)
On Mon, 2019-05-27 at 13:37 +0200, Erik Faye-Lund wrote:
> On Mon, 2019-05-27 at 04:23 -0700, Rob Clark wrote:
> > On Mon, May 27, 2019 at 2:50 AM Erik Faye-Lund
> > <erik.faye-lund@collabora.com> wrote:
> > > On Sat, 2019-05-25 at 15:44 -0700, Rob Clark wrote:
> > > > This ends up embedded in a for loop expression, ie. the C part
> > > > in
> > > > an
> > > > for (A;B;C)
> > > > 
> > > > iirc, that means it needs to be a C expr rather than
> > > > statement..
> > > > or
> > > > something roughly like that, I'm too lazy to dig out my C
> > > > grammar
> > > > 
> > > 
> > > Can't you just call a static helper function to do the
> > > validation?
> > > Function calls are valid expressions...
> > 
> > I do like the fact that with the current patch I get the correct
> > line
> > # in the assert msg.. but perhaps #ifdef MSVC we can make it a
> > static
> > inline instead?  I'm not sure how many people do active feature dev
> > of
> > mesa on windows (as opposed to doing dev on linux and then
> > compiling/shipping non-debug builds on windows), so maybe just
> > disabling the list debug on MSVC is fine.
> > 
> > BR,
> > -R

I guess I should also have mentioned that I *do* sometimes do feature
development for Mesa on Windows, so I'd really like to get to benefit
from debug-helpers.
On Mon, May 27, 2019 at 4:39 AM Erik Faye-Lund
<erik.faye-lund@collabora.com> wrote:
>
> On Mon, 2019-05-27 at 13:37 +0200, Erik Faye-Lund wrote:
> > On Mon, 2019-05-27 at 04:23 -0700, Rob Clark wrote:
> > > On Mon, May 27, 2019 at 2:50 AM Erik Faye-Lund
> > > <erik.faye-lund@collabora.com> wrote:
> > > > On Sat, 2019-05-25 at 15:44 -0700, Rob Clark wrote:
> > > > > This ends up embedded in a for loop expression, ie. the C part
> > > > > in
> > > > > an
> > > > > for (A;B;C)
> > > > >
> > > > > iirc, that means it needs to be a C expr rather than
> > > > > statement..
> > > > > or
> > > > > something roughly like that, I'm too lazy to dig out my C
> > > > > grammar
> > > > >
> > > >
> > > > Can't you just call a static helper function to do the
> > > > validation?
> > > > Function calls are valid expressions...
> > >
> > > I do like the fact that with the current patch I get the correct
> > > line
> > > # in the assert msg.. but perhaps #ifdef MSVC we can make it a
> > > static
> > > inline instead?  I'm not sure how many people do active feature dev
> > > of
> > > mesa on windows (as opposed to doing dev on linux and then
> > > compiling/shipping non-debug builds on windows), so maybe just
> > > disabling the list debug on MSVC is fine.
> > >
> > > BR,
> > > -R
>
> I guess I should also have mentioned that I *do* sometimes do feature
> development for Mesa on Windows, so I'd really like to get to benefit
> from debug-helpers.
>

ok, that  was the answer I was looking for, whether it actually
benefits anyone to re-invent assert

BR,
-R
On Mon, May 27, 2019 at 5:06 AM Rob Clark <robdclark@gmail.com> wrote:
>
> On Mon, May 27, 2019 at 4:39 AM Erik Faye-Lund
> <erik.faye-lund@collabora.com> wrote:
> >
> > On Mon, 2019-05-27 at 13:37 +0200, Erik Faye-Lund wrote:
> > > On Mon, 2019-05-27 at 04:23 -0700, Rob Clark wrote:
> > > > On Mon, May 27, 2019 at 2:50 AM Erik Faye-Lund
> > > > <erik.faye-lund@collabora.com> wrote:
> > > > > On Sat, 2019-05-25 at 15:44 -0700, Rob Clark wrote:
> > > > > > This ends up embedded in a for loop expression, ie. the C part
> > > > > > in
> > > > > > an
> > > > > > for (A;B;C)
> > > > > >
> > > > > > iirc, that means it needs to be a C expr rather than
> > > > > > statement..
> > > > > > or
> > > > > > something roughly like that, I'm too lazy to dig out my C
> > > > > > grammar
> > > > > >
> > > > >
> > > > > Can't you just call a static helper function to do the
> > > > > validation?
> > > > > Function calls are valid expressions...
> > > >
> > > > I do like the fact that with the current patch I get the correct
> > > > line
> > > > # in the assert msg.. but perhaps #ifdef MSVC we can make it a
> > > > static
> > > > inline instead?  I'm not sure how many people do active feature dev
> > > > of
> > > > mesa on windows (as opposed to doing dev on linux and then
> > > > compiling/shipping non-debug builds on windows), so maybe just
> > > > disabling the list debug on MSVC is fine.
> > > >
> > > > BR,
> > > > -R
> >
> > I guess I should also have mentioned that I *do* sometimes do feature
> > development for Mesa on Windows, so I'd really like to get to benefit
> > from debug-helpers.
> >
>
> ok, that  was the answer I was looking for, whether it actually
> benefits anyone to re-invent assert
>

I've pushed a MR[1] with a slightly different solution, but I think
this should work for MSVC

-------
#ifdef DEBUG
#  define list_assert(cond, msg)  assert(cond && msg)
#else
#  define list_assert(cond, msg)  (void)(0 && (cond))
#endif
-------

[1] https://gitlab.freedesktop.org/mesa/mesa/merge_requests/962