[Mesa-dev] [PATCH 2/3] glsl: Add validations for ir_call.

[Chad Versace]

On 08/02/2011 06:15 PM, Ian Romanick wrote:
> On 08/02/2011 05:38 PM, Paul Berry wrote:
>> This patch extends ir_validate.cpp to check the following
>> characteristics of each ir_call:
> 
>> - The number of actual parameters must match the number of formal
>>   parameters in the signature.
> 
>> - The type of each actual parameter must match the type of the
>>   corresponding formal parameter in the signature.
> 
>> - Each "out" or "inout" actual parameter must be an lvalue.
>> ---
>>  src/glsl/ir_validate.cpp |   35 +++++++++++++++++++++++++++++++++++
>>  1 files changed, 35 insertions(+), 0 deletions(-)
> 
>> diff --git a/src/glsl/ir_validate.cpp b/src/glsl/ir_validate.cpp
>> index f3fceb2..72e4faf 100644
>> --- a/src/glsl/ir_validate.cpp
>> +++ b/src/glsl/ir_validate.cpp
>> @@ -541,7 +541,42 @@ ir_validate::visit_enter(ir_call *ir)
>>        abort();
>>     }
> 
>> +   exec_list_iterator formal_param_iter = callee->parameters.iterator();
>> +   exec_list_iterator actual_param_iter = ir->actual_parameters.iterator();
> 
> We stopped making new uses of the iterators a long time ago.  As
> implemented, they're a giant pile of fail.  For what you're trying to do
> here, just use node->next and node->is_tail_sentinel().  Note that
> is_tail_sentinel tells you if you're at the first "node" *past* the end
> of the list.
> 
>> +   while (true) {
>> +      if (formal_param_iter.has_next() != actual_param_iter.has_next()) {
>> +         printf("ir_call has the wrong number of parameters:\n");
>> +         goto dump_ir;
>> +      }
>> +      if (!formal_param_iter.has_next()) {
>> +         break;
>> +      }
>> +      const ir_variable *formal_param
>> +         = (const ir_variable *) formal_param_iter.get();
>> +      const ir_rvalue *actual_param
>> +         = (const ir_rvalue *) actual_param_iter.get();
>> +      if (formal_param->type != actual_param->type) {
>> +         printf("ir_call parameter type mismatch:\n");
>> +         goto dump_ir;
>> +      }
>> +      if (formal_param->mode == ir_var_out
>> +          || formal_param->mode == ir_var_inout) {
>> +         if (!actual_param->is_lvalue()) {
>> +            printf("ir_call out/inout parameters must be lvalues:\n");
>> +            goto dump_ir;
>> +         }
>> +      }
>> +      formal_param_iter.next();
>> +      actual_param_iter.next();
>> +   }
>> +
>>     return visit_continue;
>> +
>> +dump_ir:
>> +   ir->print();
>> +   printf("callee:\n");
>> +   callee->print();
>> +   abort();
>>  }

I am not intimate with the implementation details of exec_list_iterator, and so
have nothing to say on that topic. Regardless if you replace the iterators or
not, the patch looks good to me.
Reviewed-by: Chad Versace <chad at chad-versace.us>

-- 
Chad Versace
chad at chad-versace.us

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 900 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20110804/f2119d08/attachment.pgp>