From d6eb4321d0e62b6b391ad88ce390bd6e23d79747 Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Tue, 26 Nov 2013 14:19:49 -0800 Subject: [PATCH] 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 Reviewed-by: Kenneth Graunke --- 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 40ed33afcee..8f57499ec97 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 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; diff --git a/src/glsl/loop_controls.cpp b/src/glsl/loop_controls.cpp index 26481930db7..0eb103f49b8 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; -- 2.30.2