[Mesa-dev] [Mesa-stable] [PATCH v2] glsl: avoid compiler's segfault when processing operators with void arguments

[Ian Romanick]

On 08/10/2015 11:32 AM, Mark Janes wrote:
> Should this patch be cc'd to stable branches?
> 
> Without it, the compiler crashes on invalid inputs, instead of
> generating an error.  
> 
> This patch applies cleanly to 10.5 and 10.6.

I don't care too much either.  I submitted the original bug report after
some conversations about what ?: should do with void operands in
Khronos.  It's really unlikely to occur in practice. *shrug*

Although... if it makes it even a tiny bit easier to maintain the
Jenkins configuration... ;)

> -Mark
> 
> Renaud Gaubert <renaud at lse.epita.fr> writes:
> 
>> This is done by returning an rvalue of type void in the
>> ast_function_expression::hir function instead of a void expression.
>>
>> This produces (in the case of the ternary) an hir with a call
>> to the void returning function and an assignment of a void variable
>> which will be optimized out (the assignment) during the optimization
>> pass.
>>
>> This fix results in having a valid subexpression in the many
>> different cases where the subexpressions are functions whose
>> return values are void.
>>
>> Thus preventing to dereference NULL in the following cases:
>>   * binary operator
>>   * unary operators
>>   * ternary operator
>>   * comparison operators (except equal and nequal operator)
>>
>> Equal and nequal had to be handled as a special case because
>> instead of segfaulting on a forbidden syntax it was now accepting
>> expressions with a void return value on either (or both) side of
>> the expression.
>>
>> Signed-off-by: Renaud Gaubert <renaud at lse.epita.fr>
>> Reviewed-by: Gabriel Laskar <gabriel at lse.epita.fr>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85252
>>
>> ---
>> Piglit tests were sent to the Piglit mailing list:
>>   * glsl-1.10 Adds tests on how void functions are handled
>>
>>  src/glsl/ast_function.cpp | 9 ++++++++-
>>  src/glsl/ast_to_hir.cpp   | 9 ++++++++-
>>  2 files changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/glsl/ast_function.cpp b/src/glsl/ast_function.cpp
>> index 92e26bf..ac32723 100644
>> --- a/src/glsl/ast_function.cpp
>> +++ b/src/glsl/ast_function.cpp
>> @@ -1785,7 +1785,14 @@ ast_function_expression::hir(exec_list *instructions,
>>  	 /* an error has already been emitted */
>>  	 value = ir_rvalue::error_value(ctx);
>>        } else {
>> -	 value = generate_call(instructions, sig, &actual_parameters, state);
>> +        value = generate_call(instructions, sig, &actual_parameters, state);
>> +        if (!value) {
>> +           ir_variable *const tmp = new(ctx) ir_variable(glsl_type::void_type,
>> +                                                         "void_var",
>> +                                                         ir_var_temporary);
>> +           instructions->push_tail(tmp);
>> +           value = new(ctx) ir_dereference_variable(tmp);
>> +        }
>>        }
>>  
>>        return value;
>> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
>> index 8cb46be..0d0ad2a 100644
>> --- a/src/glsl/ast_to_hir.cpp
>> +++ b/src/glsl/ast_to_hir.cpp
>> @@ -1270,7 +1270,14 @@ ast_expression::do_hir(exec_list *instructions,
>>         *    applied to one operand that can make them match, in which
>>         *    case this conversion is done."
>>         */
>> -      if ((!apply_implicit_conversion(op[0]->type, op[1], state)
>> +
>> +      if (op[0]->type == glsl_type::void_type || op[1]->type == glsl_type::void_type) {
>> +         _mesa_glsl_error(& loc, state, "`%s':  wrong operand types: "
>> +                         "no operation `%1$s' exists that takes a left-hand "
>> +                         "operand of type 'void' or a right operand of type "
>> +                         "'void'", (this->oper == ast_equal) ? "==" : "!=");
>> +         error_emitted = true;
>> +      } else if ((!apply_implicit_conversion(op[0]->type, op[1], state)
>>             && !apply_implicit_conversion(op[1]->type, op[0], state))
>>            || (op[0]->type != op[1]->type)) {
>>           _mesa_glsl_error(& loc, state, "operands of `%s' must have the same "
>> -- 
>> 2.4.5
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-stable mailing list
> mesa-stable at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-stable