Re: [Mesa-dev] [PATCH 7/8] glsl: Replace iterators in ir_reader.cpp with ad-hoc list walking.

2014-01-13 Thread Kenneth Graunke
On 01/13/2014 09:49 AM, Ian Romanick wrote:
> On 01/11/2014 02:37 AM, Kenneth Graunke wrote:
>> These can't use foreach_list since they want to skip over the first few
>> list elements.  Just doing the ad-hoc list walking isn't too bad.
>>
>> Signed-off-by: Kenneth Graunke 
>> ---
>>  src/glsl/ir_reader.cpp | 18 ++
>>  1 file changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/glsl/ir_reader.cpp b/src/glsl/ir_reader.cpp
>> index f5185d2..28923f3 100644
>> --- a/src/glsl/ir_reader.cpp
>> +++ b/src/glsl/ir_reader.cpp
>> @@ -205,11 +205,12 @@ ir_reader::read_function(s_expression *expr, bool 
>> skip_body)
>>assert(added);
>> }
>>  
>> -   exec_list_iterator it = ((s_list *) expr)->subexpressions.iterator();
>> -   it.next(); // skip "function" tag
>> -   it.next(); // skip function name
>> -   for (/* nothing */; it.has_next(); it.next()) {
>> -  s_expression *s_sig = (s_expression *) it.get();
>> +   /* Skip over "function" tag and function name (which are guaranteed to be
>> +* present by the above PARTIAL_MATCH call).
>> +*/
>> +   exec_node *node = ((s_list *) expr)->subexpressions.head->next->next;
>> +   for (/* nothing */; !node->is_tail_sentinel(); node = node->next) {
>> +  s_expression *s_sig = (s_expression *) node;
> 
> This won't behave the same in the (bug) case that the list has too few
> elements.  If the list is empty or as only one element, there will be a
> NULL deref here somewhere.  I believe the iterator version was safe
> against this.
> 
> Do we have some pre-existing guarantee that the list has enough elements?

Yes.  Above:

   s_pattern pat[] = { "function", name };
   if (!PARTIAL_MATCH(expr, pat)) {
  ir_read_error(expr, "Expected (function  (signature ...) ...)");
  return NULL;
   }

If the list doesn't match the (partial) S-Expression

   (function  ...)

we would have bailed by now.  So the list is guaranteed to have at least
two elements.


>>read_function_sig(f, s_sig, skip_body);
>> }
>> return added ? f : NULL;
>> @@ -249,9 +250,10 @@ ir_reader::read_function_sig(ir_function *f, 
>> s_expression *expr, bool skip_body)
>> exec_list hir_parameters;
>> state->symbols->push_scope();
>>  
>> -   exec_list_iterator it = paramlist->subexpressions.iterator();
>> -   for (it.next() /* skip "parameters" */; it.has_next(); it.next()) {
>> -  ir_variable *var = read_declaration((s_expression *) it.get());
>> +   /* Skip over the "parameters" tag. */
>> +   exec_node *node = paramlist->subexpressions.head->next;
>> +   for (/* nothing */; !node->is_tail_sentinel(); node = node->next) {
>> +  ir_variable *var = read_declaration((s_expression *) node);
>>if (var == NULL)
>>   return;
>>  
>>
> 

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev


Re: [Mesa-dev] [PATCH 7/8] glsl: Replace iterators in ir_reader.cpp with ad-hoc list walking.

2014-01-13 Thread Ian Romanick
On 01/11/2014 02:37 AM, Kenneth Graunke wrote:
> These can't use foreach_list since they want to skip over the first few
> list elements.  Just doing the ad-hoc list walking isn't too bad.
> 
> Signed-off-by: Kenneth Graunke 
> ---
>  src/glsl/ir_reader.cpp | 18 ++
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/src/glsl/ir_reader.cpp b/src/glsl/ir_reader.cpp
> index f5185d2..28923f3 100644
> --- a/src/glsl/ir_reader.cpp
> +++ b/src/glsl/ir_reader.cpp
> @@ -205,11 +205,12 @@ ir_reader::read_function(s_expression *expr, bool 
> skip_body)
>assert(added);
> }
>  
> -   exec_list_iterator it = ((s_list *) expr)->subexpressions.iterator();
> -   it.next(); // skip "function" tag
> -   it.next(); // skip function name
> -   for (/* nothing */; it.has_next(); it.next()) {
> -  s_expression *s_sig = (s_expression *) it.get();
> +   /* Skip over "function" tag and function name (which are guaranteed to be
> +* present by the above PARTIAL_MATCH call).
> +*/
> +   exec_node *node = ((s_list *) expr)->subexpressions.head->next->next;
> +   for (/* nothing */; !node->is_tail_sentinel(); node = node->next) {
> +  s_expression *s_sig = (s_expression *) node;

This won't behave the same in the (bug) case that the list has too few
elements.  If the list is empty or as only one element, there will be a
NULL deref here somewhere.  I believe the iterator version was safe
against this.

Do we have some pre-existing guarantee that the list has enough elements?

>read_function_sig(f, s_sig, skip_body);
> }
> return added ? f : NULL;
> @@ -249,9 +250,10 @@ ir_reader::read_function_sig(ir_function *f, 
> s_expression *expr, bool skip_body)
> exec_list hir_parameters;
> state->symbols->push_scope();
>  
> -   exec_list_iterator it = paramlist->subexpressions.iterator();
> -   for (it.next() /* skip "parameters" */; it.has_next(); it.next()) {
> -  ir_variable *var = read_declaration((s_expression *) it.get());
> +   /* Skip over the "parameters" tag. */
> +   exec_node *node = paramlist->subexpressions.head->next;
> +   for (/* nothing */; !node->is_tail_sentinel(); node = node->next) {
> +  ir_variable *var = read_declaration((s_expression *) node);
>if (var == NULL)
>return;
>  
> 

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev