glsl: Fix inconsistent assumptions about ir_loop::counter.

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

Reviewed-by: Eric Anholt <[email protected]>
Reviewed-by: Kenneth Graunke <[email protected]>

1 files changed, 6 insertions, 0 deletions

diff --git a/src/glsl/ir_hv_accept.cpp b/src/glsl/ir_hv_accept.cpp
index 941b25e97c4..a0fe3b95d35 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;