On 27 November 2013 12:44, Paul Berry <stereotype...@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 > >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
