Hello Renaud,
I am going to comment a few things to improve your patch submission.
In the email subject we usually put [PATCH vN] to indicate it is the
Nth version of the patch, so the reviewers don't get confused about
which version is the more recent. You can do that by editing the .patch
file generated through 'git format-patch' command.
"fix Bug 85252" is not needed in the subject line because you point to
the bug report at the end of the commit log.
On 09/07/15 15:55, Renaud Gaubert wrote:
> 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 assignement of a void variable
> which will be optimized out (the assignement) during the optimization
> pass.
>
s/assignement/assignment in both places.
> 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)
>
If the fix is more generic, I think you should change the subject line to:
"glsl: avoid compiler's segfault when processing operators with void
arguments"
Or something similar.
I guess the piglit tests you mention in the commit log are going to
check these cases, right?
> 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.
>
> Piglist tests are on the way
>
s/Piglist/Piglit
And there is no need to put this sentence in the commit log, please
remove it. If you want to mention something for the reviewers, write it
down below the "---". For example:
---
Piglit tests are on the way
src/glsl/ast_function.cpp | 7 ++++++-
src/glsl/ast_to_hir.cpp | 10 +++++++++-
2 files changed, 15 insertions(+), 2 deletions(-)
It won't be part of the commit log when the patch is applied to the repo
but it is handy to describe differences to other patch versions or to
mention something relevant for the reviewer that doesn't fit inside the
commit log... like that sentence.
> 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
> ---
> src/glsl/ast_function.cpp | 7 ++++++-
> src/glsl/ast_to_hir.cpp | 10 +++++++++-
> 2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/src/glsl/ast_function.cpp b/src/glsl/ast_function.cpp
> index 92e26bf..3c2b1ea 100644
> --- a/src/glsl/ast_function.cpp
> +++ b/src/glsl/ast_function.cpp
> @@ -1785,7 +1785,12 @@ 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);
> + value = new(ctx) ir_dereference_variable(tmp);
> + instructions->push_tail(tmp);
> + }
> }
>
> return value;
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 8cb46be..00cc16c 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -1270,7 +1270,15 @@ 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) {
> +
Remove this newline.
> + _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) ? "==" : "!=");
Indention is wrong, ""`%1$s' exists [...]" and "right operant [...]"
lines should start below '& loc', like the rest of _mesa_glsl_error()
calls. Look at them as an example of indention for this case.
Beware of the 80 characters line limit (unless there is a good reason to
exceed it)
> +
> + error_emitted = true;
The indention is wrong, there is an extra white-space.
> + } 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 "
>
In general the patch seems right to me (just minor things) but, as I am
not an expert of this part of the code... Have you run piglit to verify
that it doesn't add any regression?
Sam