[Mesa-dev] [PATCH 2/4] glsl: Fix inconsistent assumptions about ir_loop::counter.

[Paul Berry]

On 27 November 2013 12:44, Paul Berry <stereotype441 at gmail.com> wrote:

> The compiler back-ends (i965's fs_visitor and brw_visitor,
> ir_to_mesa_visitor, and glsl_to_tgsi_visitor) assume that when
> ir_loop::counter is non-null, it points to a fresh ir_variable that
> should be used as the loop counter (as opposed to an ir_variable that
> exists elsewhere in the instruction stream).
>
> However, previous to this patch:
>
> (1) loop_control_visitor did not create a new variable for
>     ir_loop::counter; instead it re-used the existing ir_variable.
>     This caused the loop counter to be double-incremented (once
>     explicitly by the body of the loop, and once implicitly by
>     ir_loop::increment).
>
> (2) ir_clone did not clone ir_loop::counter properly, resulting in the
>     cloned ir_loop pointing to the source ir_loop's counter.
>
> (3) ir_hierarchical_visitor did not visit ir_loop::counter, resulting
>     in the ir_variable being missed by reparenting.
>
> Additionally, most optimization passes (e.g. loop unrolling) assume
> that the variable mentioned by ir_loop::counter is not accessed in the
> body of the loop (an assumption which (1) violates).
>
> The combination of these factors caused a perfect storm in which the
> code worked properly nearly all of the time: for loops that got
> unrolled, (1) would introduce a double-increment, but loop unrolling
> would fail to notice it (since it assumes that ir_loop::counter is not
> accessed in the body of the loop), so it would unroll the loop the
> correct number of times.  For loops that didn't get unrolled, (1)
> would introduce a double-increment, but then later when the IR was
> cloned for linking, (2) would prevent the loop counter from being
> cloned properly, so it would look to further analysis stages like an
> independent variable (and hence the double-increment would stop
> occurring).  At the end of linking, (3) would prevent the loop counter
> from being reparented, so it would still belong to the shader object
> rather than the linked program object.  Provided that the client
> program didn't delete the shader object, the memory would never get
> reclaimed, and so the shader would function properly.
>
> However, for loops that didn't get unrolled, if the client program did
> delete the shader object, and the memory belonging to the loop counter
> got re-used, this could cause a use-after-free bug, leading to a
> crash.
>
> This patch fixes loop_control_visitor, ir_clone, and
> ir_hierarchical_visitor to treat ir_loop::counter the same way the
> back-ends treat it: as a freshly allocated ir_variable that needs to
> be visited and cloned independently of other ir_variables.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=72026
>

Note: this change is just invasive enough that it probably shouldn't be
picked over to stable before the 10.0 release.  But I recommend picking it
over to stable after the 10.0 release.


> ---
>  src/glsl/ir_clone.cpp      | 3 ++-
>  src/glsl/ir_hv_accept.cpp  | 6 ++++++
>  src/glsl/loop_controls.cpp | 2 +-
>  3 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/src/glsl/ir_clone.cpp b/src/glsl/ir_clone.cpp
> index 40ed33a..8f57499 100644
> --- a/src/glsl/ir_clone.cpp
> +++ b/src/glsl/ir_clone.cpp
> @@ -163,7 +163,8 @@ ir_loop::clone(void *mem_ctx, struct hash_table *ht)
> const
>        new_loop->to = this->to->clone(mem_ctx, ht);
>     if (this->increment)
>        new_loop->increment = this->increment->clone(mem_ctx, ht);
> -   new_loop->counter = counter;
> +   if (this->counter)
> +      new_loop->counter = this->counter->clone(mem_ctx, ht);
>
>     foreach_iter(exec_list_iterator, iter, this->body_instructions) {
>        ir_instruction *ir = (ir_instruction *)iter.get();
> diff --git a/src/glsl/ir_hv_accept.cpp b/src/glsl/ir_hv_accept.cpp
> index 941b25e..a0fe3b9 100644
> --- a/src/glsl/ir_hv_accept.cpp
> +++ b/src/glsl/ir_hv_accept.cpp
> @@ -87,6 +87,12 @@ ir_loop::accept(ir_hierarchical_visitor *v)
>     if (s != visit_continue)
>        return (s == visit_continue_with_parent) ? visit_continue : s;
>
> +   if (this->counter) {
> +      s = this->counter->accept(v);
> +      if (s != visit_continue)
> +         return (s == visit_continue_with_parent) ? visit_continue : s;
> +   }
> +
>     s = visit_list_elements(v, &this->body_instructions);
>     if (s == visit_stop)
>        return s;
> diff --git a/src/glsl/loop_controls.cpp b/src/glsl/loop_controls.cpp
> index 2648193..0eb103f 100644
> --- a/src/glsl/loop_controls.cpp
> +++ b/src/glsl/loop_controls.cpp
> @@ -254,7 +254,7 @@ loop_control_visitor::visit_leave(ir_loop *ir)
>                      ir->from = init->clone(ir, NULL);
>                      ir->to = limit->clone(ir, NULL);
>                      ir->increment = lv->increment->clone(ir, NULL);
> -                    ir->counter = lv->var;
> +                    ir->counter = lv->var->clone(ir, NULL);
>                      ir->cmp = cmp;
>
>                      max_iterations = iterations;
> --
> 1.8.4.2
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131127/25d62a4a/attachment.html>