[Mesa-dev] [PATCH 1/2] util: Fix foreach_list_typed_safe when exec_node is not at offset 0.

[Ian Romanick]

On 03/10/2015 08:04 PM, Connor Abbott wrote:
> On Tue, Mar 10, 2015 at 10:54 PM, Ian Romanick <idr at freedesktop.org> wrote:
>> On 03/09/2015 06:36 PM, Kenneth Graunke wrote:
>>> From: Jason Ekstrand <jason.ekstrand at intel.com>
>>>
>>> __next and __prev are pointers to the structure containing the exec_node
>>> link, not the embedded exec_node.  NULL checks would fail unless the
>>> embedded exec_node happened to be at offset 0 in the parent struct.
>>
>> Wait... what?  That is 100% wrong.  They are pointers to the exec_node,
>> or every assumption of the list structure (like checking for the end of
>> the list) breaks.
> 
> No, this is the foreach_list_safe code so __next is the user-specified
> variable pointing to the structure containing the next exec_node. Look
> at the code below the change, where we say "__next =
> exec_node_data(__type, (__next)->__field.next, __field))"

Which doesn't completely show up in the diff context.  Okay.  But in
that case...

>>> Signed-off-by: Jason Ekstrand <jason.ekstrand at intel.com>
>>> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
>>> ---
>>>  src/glsl/list.h | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/glsl/list.h b/src/glsl/list.h
>>> index ddb98f7..680e963 100644
>>> --- a/src/glsl/list.h
>>> +++ b/src/glsl/list.h
>>> @@ -684,7 +684,7 @@ inline void exec_node::insert_before(exec_list *before)
>>>             exec_node_data(__type, (__list)->head, __field),                \
>>>                 * __next =                                                  \
>>>             exec_node_data(__type, (__node)->__field.next, __field);        \
>>> -        __next != NULL;                                                    \
>>> +        &__next->__field != NULL;                                          \

I agree with Matt that this looks really insane.  Maybe just add
another variable that points to the exec_node?  I guess you'd need to
be able to declare variables with multiple types in the initializer of
the for-statement.

Also... I'm not a fan of these ever-growing macros.  They're a lot like
alligators.  They're pretty cute when they're small, but when they grow
up they drown you in a river and eat you.  I feel like the users of
this macro should just use foreach_list_safe, and use exec_node_data by
hand.  That would simplify things a lot by hiding less from the
programmer.

>>>          __node = __next, __next =                                          \
>>>             exec_node_data(__type, (__next)->__field.next, __field))
>>>
>>> @@ -693,7 +693,7 @@ inline void exec_node::insert_before(exec_list *before)
>>>             exec_node_data(__type, (__list)->tail_pred, __field),           \
>>>                 * __prev =                                                  \
>>>             exec_node_data(__type, (__node)->__field.prev, __field);        \
>>> -        __prev != NULL;                                                    \
>>> +        &__prev->__field != NULL;                                          \
>>>          __node = __prev, __prev =                                          \
>>>             exec_node_data(__type, (__prev)->__field.prev, __field))
>>>