[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 11:02 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>
>
> 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...

Yeah... currently what compilers do is equivalent to casting to
uintptr_t, but my concern is that eventually, some compiler writer
says "hey, pointer arithmetic never overflows!" and implements tricks
similar to what compilers do today with signed integer overflow. I'm
not sure how likely that is; I'd guess it's not too likely though. So
given that there's not much we can do that's not going to be very
messy, I guess it's ok to leave it how it is now as long as we don't
forget about this in case it does happen eventually.

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