[Mesa-dev] [PATCH 3/4] glsl: Emit extra errors for l-value violations in 'out' or 'inout' parameters

[Paul Berry]

On 23 December 2011 14:35, Ian Romanick <idr at freedesktop.org> wrote:

> From: Ian Romanick <ian.d.romanick at intel.com>
>
> Somethings, like pre-increment operations, were not previously caught.
> After the 8.0 release, this code needs some major refactoring and
> clean-up.  It's a mess. :(
>
> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=42755
> ---
>  src/glsl/ast_function.cpp |   57
> +++++++++++++++++++++++++++++++++++++++++---
>  1 files changed, 53 insertions(+), 4 deletions(-)
>
> diff --git a/src/glsl/ast_function.cpp b/src/glsl/ast_function.cpp
> index 126b610..4471c76 100644
> --- a/src/glsl/ast_function.cpp
> +++ b/src/glsl/ast_function.cpp
> @@ -96,6 +96,7 @@ prototype_string(const glsl_type *return_type, const
> char *name,
>  static ir_rvalue *
>  generate_call(exec_list *instructions, ir_function_signature *sig,
>              YYLTYPE *loc, exec_list *actual_parameters,
> +             ir_call **call_ir,
>              struct _mesa_glsl_parse_state *state)
>

Another minor suggestion:  The semantics of call_ir are:

- If generate_call is successful, it is set to the ir_call that is
generated.
- If an error occurred, it is unchanged.

This means that in order for the caller to be robust in the case of error
conditions, it must remember to set call_ir to NULL before calling this
function.

You're doing that, so there's no bug, but I'd feel better about the
situation if we set *call_ir to NULL at the top of generate_call rather
than in the caller.  That way call_ir would behave like a true out
parameter even in the case of error.

As with my other comment, I'm not married to the idea; either way the
series is:

Reviewed-by: Paul Berry <stereotype441 at gmail.com>


>  {
>    void *ctx = state;
> @@ -256,10 +257,12 @@ generate_call(exec_list *instructions,
> ir_function_signature *sig,
>       deref = new(ctx) ir_dereference_variable(var);
>       ir_assignment *assign = new(ctx) ir_assignment(deref, call, NULL);
>       instructions->push_tail(assign);
> +      *call_ir = call;
>
>       deref = new(ctx) ir_dereference_variable(var);
>    } else {
>       instructions->push_tail(call);
> +      *call_ir = call;
>       deref = NULL;
>    }
>    instructions->append_list(&post_call_conversions);
> @@ -269,6 +272,7 @@ generate_call(exec_list *instructions,
> ir_function_signature *sig,
>  static ir_rvalue *
>  match_function_by_name(exec_list *instructions, const char *name,
>                       YYLTYPE *loc, exec_list *actual_parameters,
> +                      ir_call **call_ir,
>                       struct _mesa_glsl_parse_state *state)
>  {
>    void *ctx = state;
> @@ -342,7 +346,8 @@ done:
>       }
>
>       /* Finally, generate a call instruction. */
> -      return generate_call(instructions, sig, loc, actual_parameters,
> state);
> +      return generate_call(instructions, sig, loc, actual_parameters,
> +                          call_ir, state);
>    } else {
>       char *str = prototype_string(NULL, name, actual_parameters);
>
> @@ -1442,9 +1447,53 @@ ast_function_expression::hir(exec_list
> *instructions,
>       process_parameters(instructions, &actual_parameters,
> &this->expressions,
>                         state);
>
> -      return match_function_by_name(instructions,
> -                                   id->primary_expression.identifier, &
> loc,
> -                                   &actual_parameters, state);
> +      ir_call *call = NULL;
> +      ir_rvalue *const value =
> +        match_function_by_name(instructions,
> +                               id->primary_expression.identifier,
> +                               &loc, &actual_parameters, &call, state);
> +
> +      if (call != NULL) {
> +        /* If a function was found, make sure that none of the 'out' or
> 'inout'
> +         * parameters violate the extra l-value rules.
> +         */
> +        ir_function_signature *f = call->get_callee();
> +        assert(f != NULL);
> +
> +        exec_node *formal_node = f->parameters.head;
> +
> +        foreach_list (actual_node, &this->expressions) {
> +           /* Both parameter lists had better be the same length!
> +            */
> +           assert(!actual_node->is_tail_sentinel());
> +
> +           const ir_variable *const formal_parameter =
> +              (ir_variable *) formal_node;
> +           const ast_expression *const actual_parameter =
> +              exec_node_data(ast_expression, actual_node, link);
> +
> +           if ((formal_parameter->mode == ir_var_out
> +                || formal_parameter->mode == ir_var_inout)
> +               && actual_parameter->non_lvalue_description != NULL) {
> +              YYLTYPE loc = actual_parameter->get_location();
> +
> +              _mesa_glsl_error(&loc, state,
> +                               "function parameter '%s %s' references a
> %s",
> +                               (formal_parameter->mode == ir_var_out)
> +                               ? "out" : "inout",
> +                               formal_parameter->name,
> +                               actual_parameter->non_lvalue_description);
> +              return ir_call::get_error_instruction(ctx);
> +           }
> +
> +           /* Only advance the formal_node pointer here because the
> +            * foreach_list business already advances actual_node.
> +            */
> +           formal_node = formal_node->next;
> +        }
> +      }
> +
> +      return value;
>    }
>
>    return ir_call::get_error_instruction(ctx);
> --
> 1.7.6.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120106/3f094b8d/attachment.htm>