Re: [Mesa-dev] [PATCH 1/2] util: Fix foreach_list_typed_safe when exec_node is not at offset 0.
On Tue, Mar 10, 2015 at 8:31 PM, Ian Romanick wrote: > 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. I don't know, I think with the bugs shaken out, the prevent a lot of mistakes among other benefits. Think for instance of the number of times we've fixed node->next != NULL to be node->next->next != NULL before the macros existed. They also cut boilerplate from every linked list walk. These were some diff-stats from some of the commits that started using them: 184 insertions(+), 291 deletions (commit 4d78446d) 41 insertions(+), 85 deletions (commit e0cb82d0) Using macros also massively simplified the transition in the i965 backend from a single huge instruction list to a list of basic blocks, each with their own instruction lists -- see commit 49374fab. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] util: Fix foreach_list_typed_safe when exec_node is not at offset 0.
On Tuesday, March 10, 2015 05:48:20 PM Jason Ekstrand wrote: > On Tue, Mar 10, 2015 at 5:45 PM, Matt Turner wrote: > > > On Tue, Mar 10, 2015 at 1:09 PM, Jason Ekstrand > > wrote: > > > How about we do things slightly differently and check > > "(__node)->field.next > > > != NULL" just like we do on regular versions. Since the check happens > > > between the increment step and running the user's code, __node is valid > > for > > > every invocation of the checking condition. Would that make you feel > > better > > > about it? > > > > Yeah, that seems a lot clearer. > > > > Ken, > Are you ok with that? If so, do you want to make the change or shall I? > --Jason I think that should work, and is probably clearer. I'll let you do it. signature.asc Description: This is a digitally signed message part. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] util: Fix foreach_list_typed_safe when exec_node is not at offset 0.
On 03/10/2015 08:04 PM, Connor Abbott wrote: > On Tue, Mar 10, 2015 at 10:54 PM, Ian Romanick wrote: >> On 03/09/2015 06:36 PM, Kenneth Graunke wrote: >>> From: Jason Ekstrand >>> >>> __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 >>> Reviewed-by: Kenneth Graunke >>> --- >>> 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)) >>> ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] util: Fix foreach_list_typed_safe when exec_node is not at offset 0.
On Tue, Mar 10, 2015 at 10:54 PM, Ian Romanick wrote: > On 03/09/2015 06:36 PM, Kenneth Graunke wrote: >> From: Jason Ekstrand >> >> __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))" > >> Signed-off-by: Jason Ekstrand >> Reviewed-by: Kenneth Graunke >> --- >> 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; \ >> __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)) >> >> > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] util: Fix foreach_list_typed_safe when exec_node is not at offset 0.
On 03/09/2015 06:36 PM, Kenneth Graunke wrote: > From: Jason Ekstrand > > __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. > Signed-off-by: Jason Ekstrand > Reviewed-by: Kenneth Graunke > --- > 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; \ > __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)) > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] util: Fix foreach_list_typed_safe when exec_node is not at offset 0.
On Tue, Mar 10, 2015 at 5:45 PM, Matt Turner wrote: > On Tue, Mar 10, 2015 at 1:09 PM, Jason Ekstrand > wrote: > > How about we do things slightly differently and check > "(__node)->field.next > > != NULL" just like we do on regular versions. Since the check happens > > between the increment step and running the user's code, __node is valid > for > > every invocation of the checking condition. Would that make you feel > better > > about it? > > Yeah, that seems a lot clearer. > Ken, Are you ok with that? If so, do you want to make the change or shall I? --Jason ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] util: Fix foreach_list_typed_safe when exec_node is not at offset 0.
On Tue, Mar 10, 2015 at 1:09 PM, Jason Ekstrand wrote: > How about we do things slightly differently and check "(__node)->field.next > != NULL" just like we do on regular versions. Since the check happens > between the increment step and running the user's code, __node is valid for > every invocation of the checking condition. Would that make you feel better > about it? Yeah, that seems a lot clearer. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] util: Fix foreach_list_typed_safe when exec_node is not at offset 0.
On Mon, Mar 9, 2015 at 11:06 PM, Matt Turner wrote: > On Mon, Mar 9, 2015 at 6:36 PM, Kenneth Graunke > wrote: > > From: Jason Ekstrand > > > > __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 > > Reviewed-by: Kenneth Graunke > > --- > > 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; > \ > > Wow. Okay, so this apparently relies on exec_node_data (which is > pretty confusingly named, I think) returning a negative pointer value > if passed NULL as its node parameter, such that &...->__field > effectively adds back the offset of the field in the struct, giving a > NULL pointer? > > That's sufficiently confusing to warrant a lengthy comment at the very > least. > > Testing that the address of a field in a struct is NULL... just looks > insane. > How about we do things slightly differently and check "(__node)->field.next != NULL" just like we do on regular versions. Since the check happens between the increment step and running the user's code, __node is valid for every invocation of the checking condition. Would that make you feel better about it? --Jason ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] util: Fix foreach_list_typed_safe when exec_node is not at offset 0.
On Mon, Mar 9, 2015 at 6:36 PM, Kenneth Graunke wrote: > From: Jason Ekstrand > > __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 > Reviewed-by: Kenneth Graunke > --- > 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; \ Wow. Okay, so this apparently relies on exec_node_data (which is pretty confusingly named, I think) returning a negative pointer value if passed NULL as its node parameter, such that &...->__field effectively adds back the offset of the field in the struct, giving a NULL pointer? That's sufficiently confusing to warrant a lengthy comment at the very least. Testing that the address of a field in a struct is NULL... just looks insane. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] util: Fix foreach_list_typed_safe when exec_node is not at offset 0.
On Mon, Mar 9, 2015 at 11:02 PM, Jason Ekstrand wrote: > > > On Mon, Mar 9, 2015 at 7:59 PM, Connor Abbott wrote: >> >> On Mon, Mar 9, 2015 at 10:54 PM, Jason Ekstrand >> wrote: >> > >> > >> > On Mon, Mar 9, 2015 at 7:48 PM, Connor Abbott >> > wrote: >> >> >> >> On Mon, Mar 9, 2015 at 10:35 PM, Matt Turner >> >> wrote: >> >> > On Mon, Mar 9, 2015 at 7:24 PM, Matt Turner >> >> > wrote: >> >> >> On Mon, Mar 9, 2015 at 6:36 PM, Kenneth Graunke >> >> >> >> >> >> wrote: >> >> >>> From: Jason Ekstrand >> >> >>> >> >> >>> __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 >> >> >>> Reviewed-by: Kenneth Graunke >> >> >>> --- >> >> >>> 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@lists.freedesktop.org >> >> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev >> >> ___ >> >> mesa-dev mailing list >> >> mesa-dev@lists.freedesktop.org >> >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev >> > >> > > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] util: Fix foreach_list_typed_safe when exec_node is not at offset 0.
On Mon, Mar 9, 2015 at 7:59 PM, Connor Abbott wrote: > On Mon, Mar 9, 2015 at 10:54 PM, Jason Ekstrand > wrote: > > > > > > On Mon, Mar 9, 2015 at 7:48 PM, Connor Abbott > wrote: > >> > >> On Mon, Mar 9, 2015 at 10:35 PM, Matt Turner > wrote: > >> > On Mon, Mar 9, 2015 at 7:24 PM, Matt Turner > wrote: > >> >> On Mon, Mar 9, 2015 at 6:36 PM, Kenneth Graunke < > kenn...@whitecape.org> > >> >> wrote: > >> >>> From: Jason Ekstrand > >> >>> > >> >>> __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 > >> >>> Reviewed-by: Kenneth Graunke > >> >>> --- > >> >>> 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@lists.freedesktop.org > >> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev > >> ___ > >> mesa-dev mailing list > >> mesa-dev@lists.freedesktop.org > >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev > > > > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] util: Fix foreach_list_typed_safe when exec_node is not at offset 0.
On Mon, Mar 9, 2015 at 7:58 PM, Matt Turner wrote:
> On Mon, Mar 9, 2015 at 7:32 PM, Jason Ekstrand
> wrote:
> >
> >
> > On Mon, Mar 9, 2015 at 7:24 PM, Matt Turner wrote:
> >>
> >> On Mon, Mar 9, 2015 at 6:36 PM, Kenneth Graunke
> >> wrote:
> >> > From: Jason Ekstrand
> >> >
> >> > __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
> >> > Reviewed-by: Kenneth Graunke
> >> > ---
> >> > 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?
> >
> >
> > No, "__field" is the name of the exec_node field embedded in the __type
> > struct. So if I have
> >
> > struct foo {
> >/* stuff */
> >struct exec_node bar;
> > }
> >
> > as my type, __type is "struct foo", __next and __node are both of type
> > "__type *", and __field is "bar". So, in order to get form __next to an
> > exec_node, you have to do &__next->__field because we need the actual
> > exec_node back.
>
> More concretely, for something like:
>
> foreach_list_typed_safe (bblock_link, successor, link,
>&predecessor->block->children)
>
> I think this macro expands to (after this patch)
>
>for (bblock_link * successor =
>exec_node_data(bblock_link,
> (&predecessor->block->children)->head, link),
>* __next =
>exec_node_data(bblock_link, (successor)->link.next, link);
> &__next->link != NULL;
> successor = __next, __next =
>exec_node_data(bblock_link, (__next)->link.next, link))
>
> How can the address of a field in a struct (e.g., __next->link) be NULL?
>
If the address of the struct is (void *)-8 and the link field is 8 bytes
into the struct. In this case, assuming unsigned overflow (I think that's
reasonably safe), the address of the link will be (void *)0
--Jason
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] util: Fix foreach_list_typed_safe when exec_node is not at offset 0.
On Mon, Mar 9, 2015 at 10:54 PM, Jason Ekstrand wrote: > > > On Mon, Mar 9, 2015 at 7:48 PM, Connor Abbott wrote: >> >> On Mon, Mar 9, 2015 at 10:35 PM, Matt Turner wrote: >> > On Mon, Mar 9, 2015 at 7:24 PM, Matt Turner wrote: >> >> On Mon, Mar 9, 2015 at 6:36 PM, Kenneth Graunke >> >> wrote: >> >>> From: Jason Ekstrand >> >>> >> >>> __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 >> >>> Reviewed-by: Kenneth Graunke >> >>> --- >> >>> 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@lists.freedesktop.org >> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev >> ___ >> mesa-dev mailing list >> mesa-dev@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] util: Fix foreach_list_typed_safe when exec_node is not at offset 0.
On Mon, Mar 9, 2015 at 7:32 PM, Jason Ekstrand wrote:
>
>
> On Mon, Mar 9, 2015 at 7:24 PM, Matt Turner wrote:
>>
>> On Mon, Mar 9, 2015 at 6:36 PM, Kenneth Graunke
>> wrote:
>> > From: Jason Ekstrand
>> >
>> > __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
>> > Reviewed-by: Kenneth Graunke
>> > ---
>> > 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?
>
>
> No, "__field" is the name of the exec_node field embedded in the __type
> struct. So if I have
>
> struct foo {
>/* stuff */
>struct exec_node bar;
> }
>
> as my type, __type is "struct foo", __next and __node are both of type
> "__type *", and __field is "bar". So, in order to get form __next to an
> exec_node, you have to do &__next->__field because we need the actual
> exec_node back.
More concretely, for something like:
foreach_list_typed_safe (bblock_link, successor, link,
&predecessor->block->children)
I think this macro expands to (after this patch)
for (bblock_link * successor =
exec_node_data(bblock_link,
(&predecessor->block->children)->head, link),
* __next =
exec_node_data(bblock_link, (successor)->link.next, link);
&__next->link != NULL;
successor = __next, __next =
exec_node_data(bblock_link, (__next)->link.next, link))
How can the address of a field in a struct (e.g., __next->link) be NULL?
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] util: Fix foreach_list_typed_safe when exec_node is not at offset 0.
On Mon, Mar 9, 2015 at 7:48 PM, Connor Abbott wrote: > On Mon, Mar 9, 2015 at 10:35 PM, Matt Turner wrote: > > On Mon, Mar 9, 2015 at 7:24 PM, Matt Turner wrote: > >> On Mon, Mar 9, 2015 at 6:36 PM, Kenneth Graunke > wrote: > >>> From: Jason Ekstrand > >>> > >>> __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 > >>> Reviewed-by: Kenneth Graunke > >>> --- > >>> 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 > 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@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] util: Fix foreach_list_typed_safe when exec_node is not at offset 0.
On Mon, Mar 9, 2015 at 10:35 PM, Matt Turner wrote: > On Mon, Mar 9, 2015 at 7:24 PM, Matt Turner wrote: >> On Mon, Mar 9, 2015 at 6:36 PM, Kenneth Graunke >> wrote: >>> From: Jason Ekstrand >>> >>> __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 >>> Reviewed-by: Kenneth Graunke >>> --- >>> 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, 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] util: Fix foreach_list_typed_safe when exec_node is not at offset 0.
On Mon, Mar 9, 2015 at 7:24 PM, Matt Turner wrote: > On Mon, Mar 9, 2015 at 6:36 PM, Kenneth Graunke wrote: >> From: Jason Ekstrand >> >> __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 >> Reviewed-by: Kenneth Graunke >> --- >> 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)" ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] util: Fix foreach_list_typed_safe when exec_node is not at offset 0.
On Mon, Mar 9, 2015 at 7:24 PM, Matt Turner wrote:
> On Mon, Mar 9, 2015 at 6:36 PM, Kenneth Graunke
> wrote:
> > From: Jason Ekstrand
> >
> > __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
> > Reviewed-by: Kenneth Graunke
> > ---
> > 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?
>
No, "__field" is the name of the exec_node field embedded in the __type
struct. So if I have
struct foo {
/* stuff */
struct exec_node bar;
}
as my type, __type is "struct foo", __next and __node are both of type
"__type *", and __field is "bar". So, in order to get form __next to an
exec_node, you have to do &__next->__field because we need the actual
exec_node back.
Put differently, &__next->field undoes the pointer arithmatic that
exec_node_data(__type, ptr, __field) does. Ideallly, we would like __next
to be a pointer of type "struct exec_node" and do the conversion to "__type
*" later. Unfortunately, C doesn't let us do that inside the for loop. So
we settle for extra pointer arithmetic.
The other option, of course, would be to use a struct but then we couldn't
make a variable named __node on behalf of the caller.
--Jason
>
> > __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))
> >
> > --
> > 2.2.2
> >
> > ___
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] util: Fix foreach_list_typed_safe when exec_node is not at offset 0.
On Mon, Mar 9, 2015 at 6:36 PM, Kenneth Graunke wrote: > From: Jason Ekstrand > > __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 > Reviewed-by: Kenneth Graunke > --- > 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? > __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)) > > -- > 2.2.2 > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] util: Fix foreach_list_typed_safe when exec_node is not at offset 0.
Reviewed-by: Connor Abbott On Mon, Mar 9, 2015 at 9:36 PM, Kenneth Graunke wrote: > From: Jason Ekstrand > > __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 > Reviewed-by: Kenneth Graunke > --- > 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; \ > __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)) > > -- > 2.2.2 > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/2] util: Fix foreach_list_typed_safe when exec_node is not at offset 0.
From: Jason Ekstrand __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 Reviewed-by: Kenneth Graunke --- 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; \ __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)) -- 2.2.2 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
