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

[Jason Ekstrand]

On Mon, Mar 9, 2015 at 7:59 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:

> 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.
>

We could cast everything through uintptr_t but that's gonna get messy...


>
> >
> >>
> >> 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
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150309/510e60b9/attachment.html>