<div dir="ltr">On 27 November 2013 12:44, Paul Berry <<a href="mailto:stereotype441@gmail.com" target="_blank">stereotype441@gmail.com</a>> wrote: <div class="gmail_extra"><div class="gmail_quote"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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: <a href="https://bugs.freedesktop.org/show_bug.cgi?id=72026" target="_blank">https://bugs.freedesktop.org/show_bug.cgi?id=72026</a> </blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> --- 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 </blockquote></div> </div></div>