[Mesa-dev] [PATCH 1/2] glsl hierarchical visitor: Do not overwrite base_ir for parameter lists.

[Ian Romanick]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/15/2011 04:40 PM, Paul Berry wrote:
> This patch fixes a bug in ir_hirearchical_visitor: when traversing
> an exec_list representing the formal or actual parameters of a
> function, it modified base_ir to point to each parameter in turn,
> rather than leaving it as a pointer to the enclosing statement.
> This was a problem, since base_ir is used by visitor classes to
> locate the statement containing the node being visited (usually so
> that additional statements can be inserted before or after it).
> Without this fix, visitors might attempt to insert statements into
> parameter lists.

This sure sounds like a bug, but I'm really surprised that this hasn't
cause catastrophic failures before.  Do you have a shader in mind that
fails due to this issue?

> --- src/glsl/ir_hierarchical_visitor.h |    3 ++- 
> src/glsl/ir_hv_accept.cpp          |   21 +++++++++++++++------ 2
> files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/src/glsl/ir_hierarchical_visitor.h
> b/src/glsl/ir_hierarchical_visitor.h index dc177f5..bba046d 100644 
> --- a/src/glsl/ir_hierarchical_visitor.h +++
> b/src/glsl/ir_hierarchical_visitor.h @@ -178,6 +178,7 @@ void
> visit_tree(ir_instruction *ir, void (*callback)(class
> ir_instruction *ir, void *data), void *data);
> 
> -ir_visitor_status visit_list_elements(ir_hierarchical_visitor *v,
> exec_list *l); +ir_visitor_status
> visit_list_elements(ir_hierarchical_visitor *v, exec_list *l, +
> bool statement_list = true);
> 
> #endif /* IR_HIERARCHICAL_VISITOR_H */ diff --git
> a/src/glsl/ir_hv_accept.cpp b/src/glsl/ir_hv_accept.cpp index
> d33fc85..0e78fda 100644 --- a/src/glsl/ir_hv_accept.cpp +++
> b/src/glsl/ir_hv_accept.cpp @@ -30,7 +30,13 @@ */
> 
> /** - * Process a list of nodes using a hierarchical vistor + *
> Process a list of nodes using a hierarchical vistor. + * + * If
> statement_list is true (the default), this is a list of statements,
> so + * v->base_ir will be set to point to each statement just
> before iterating + * over it, and restored after iteration is
> complete.  If statement_list is + * false, this is a list that
> appears inside a statement (e.g. a parameter + * list), so
> v->base_ir will be left alone. * * \warning * This function will
> operate correctly if a node being processed is removed @@ -38,19
> +44,22 @@ * processed, some of the added nodes may not be
> processed. */ ir_visitor_status 
> -visit_list_elements(ir_hierarchical_visitor *v, exec_list *l) 
> +visit_list_elements(ir_hierarchical_visitor *v, exec_list *l, +
> bool statement_list) { ir_instruction *prev_base_ir = v->base_ir;
> 
> foreach_list_safe(n, l) { ir_instruction *const ir =
> (ir_instruction *) n; -      v->base_ir = ir; +      if
> (statement_list) +         v->base_ir = ir; ir_visitor_status s =
> ir->accept(v);
> 
> if (s != visit_continue) return s; } -   v->base_ir =
> prev_base_ir; +   if (statement_list) +      v->base_ir =
> prev_base_ir;
> 
> return visit_continue; } @@ -129,7 +138,7 @@
> ir_function::accept(ir_hierarchical_visitor *v) if (s !=
> visit_continue) return (s == visit_continue_with_parent) ?
> visit_continue : s;
> 
> -   s = visit_list_elements(v, &this->signatures); +   s =
> visit_list_elements(v, &this->signatures, false); return (s ==
> visit_stop) ? s : v->visit_leave(this); }
> 
> @@ -317,7 +326,7 @@ ir_call::accept(ir_hierarchical_visitor *v) if
> (s != visit_continue) return (s == visit_continue_with_parent) ?
> visit_continue : s;
> 
> -   s = visit_list_elements(v, &this->actual_parameters); +   s =
> visit_list_elements(v, &this->actual_parameters, false); if (s ==
> visit_stop) return s;
> 

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk53gxcACgkQX1gOwKyEAw8UdQCeItlvgwnIpzn0ck7AuQ+bo+9b
3SkAn2cZ/n5BeTDumob9ST4uH21+p+7H
=JEdk
-----END PGP SIGNATURE-----