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

[Connor Abbott]

On Mon, Mar 9, 2015 at 10:54 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>
>
> On Mon, Mar 9, 2015 at 7:48 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:
>>
>> On Mon, Mar 9, 2015 at 10:35 PM, Matt Turner <mattst88 at gmail.com> wrote:
>> > On Mon, Mar 9, 2015 at 7:24 PM, Matt Turner <mattst88 at gmail.com> wrote:
>> >> On Mon, Mar 9, 2015 at 6:36 PM, Kenneth Graunke <kenneth at whitecape.org>
>> >> 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.
>> >>>
>> >>> 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'm not understanding now the address of __next->__field can ever be
>> >> NULL.
>> >>
>> >> __next is something with an embedded struct exec_node, so don't we
>> >> want "__next->__field != NULL" without the address-of operator?
>> >
>> > Sorry, that should have been
>> > "exec_node_is_tail_sentinel(&__next->__field)"
>>
>> No, that won't work. We want to bail out if the *current* node is the
>> tail sentinel, not the next one. If the next node is the tail
>> sentinel, then the current one is the last element, so we have to go
>> through the loop once more. We could use exec_node_is_tail_sentinel()
>> on the current node,
>
>
> No, we can't.  The whole point of the *_safe versions is that we never touch
> the current node after we've let the client execute code.  We stash off the
> next one so that, even if the delete the current one, we still have
> something to give them next iteration.
> --Jason

Ah, right. My only concern is that doing pointer arithmetic on NULL
isn't defined, and given that what we're doing involves underflowing
the pointer so it wraps around to a large address (when subtracting in
exec_node_get_data()) and then overflowing it back to 0 (when doing
&__next->field), it's likely that some compiler might come along and
"optimize" this.

>
>>
>> but we've already dereferenced the pointer
>> earlier when getting the next node so it would be less efficient to
>> dereference it again.
>>
>> > _______________________________________________
>> > mesa-dev mailing list
>> > mesa-dev at lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>