As time goes on, I become less and less a fan of the proliferation of for_* macros... especially ones that are only used in one or two places.
On 05/07/2016 03:05 PM, Nicolai Hähnle wrote: > From: Nicolai Hähnle <nicolai.haeh...@amd.com> > > This macro avoids undefined behaviour that crashes gcc's ubsan. > --- > src/compiler/glsl/list.h | 13 +++++++++++++ > src/compiler/glsl/opt_dead_code_local.cpp | 7 +------ > src/compiler/glsl/opt_tree_grafting.cpp | 5 +---- > 3 files changed, 15 insertions(+), 10 deletions(-) > > diff --git a/src/compiler/glsl/list.h b/src/compiler/glsl/list.h > index 8da9514..12389aa 100644 > --- a/src/compiler/glsl/list.h > +++ b/src/compiler/glsl/list.h > @@ -716,4 +716,17 @@ inline void exec_node::insert_before(exec_list *before) > (((__node) = exec_node_data(__type, __cur, __field)) || true); \ > __cur = __prev, __prev = __prev->prev) > > +/** > + * Iterate over a range [begin, end) of nodes. > + */ > +#define for_range_list_safe(__type, __node, __begin, __end) \ > + for (__type *(__node), **__flag = &(__node); __flag; __flag = NULL) \ > + for (struct exec_node *__cur = (__begin), \ > + *__next = __cur->next, \ > + *__end_stored = (__end); \ > + __cur != __end_stored && \ > + (((__node) = (__type *) __cur) || true); \ > + __cur = __next, __next = __next->next) > + > + > #endif /* LIST_CONTAINER_H */ > diff --git a/src/compiler/glsl/opt_dead_code_local.cpp > b/src/compiler/glsl/opt_dead_code_local.cpp > index d38fd2b..5dd3bfd 100644 > --- a/src/compiler/glsl/opt_dead_code_local.cpp > +++ b/src/compiler/glsl/opt_dead_code_local.cpp > @@ -291,7 +291,6 @@ dead_code_local_basic_block(ir_instruction *first, > ir_instruction *last, > void *data) > { > - ir_instruction *ir, *ir_next; > /* List of avaialble_copy */ > exec_list assignments; > bool *out_progress = (bool *)data; > @@ -299,8 +298,7 @@ dead_code_local_basic_block(ir_instruction *first, > > void *ctx = ralloc_context(NULL); > /* Safe looping, since process_assignment */ > - for (ir = first, ir_next = (ir_instruction *)first->next;; > - ir = ir_next, ir_next = (ir_instruction *)ir->next) { > + for_range_list_safe(ir_instruction, ir, first, last->next) { In this case it seems like changing all the types to exec_node* and adding ir_instruction *ir = (ir_instruction *) node; right here would be sufficient. Unless you're able to hit a case where first or last isn't really an ir_instruction. If that's the case, there's a much bigger bug. > ir_assignment *ir_assign = ir->as_assignment(); > > if (debug) { > @@ -314,9 +312,6 @@ dead_code_local_basic_block(ir_instruction *first, > kill_for_derefs_visitor kill(&assignments); > ir->accept(&kill); > } > - > - if (ir == last) > - break; > } > *out_progress = progress; > ralloc_free(ctx); > diff --git a/src/compiler/glsl/opt_tree_grafting.cpp > b/src/compiler/glsl/opt_tree_grafting.cpp > index a40e5f7..47fca7d 100644 > --- a/src/compiler/glsl/opt_tree_grafting.cpp > +++ b/src/compiler/glsl/opt_tree_grafting.cpp > @@ -347,11 +347,8 @@ tree_grafting_basic_block(ir_instruction *bb_first, > void *data) > { > struct tree_grafting_info *info = (struct tree_grafting_info *)data; > - ir_instruction *ir, *next; > > - for (ir = bb_first, next = (ir_instruction *)ir->next; > - ir != bb_last->next; > - ir = next, next = (ir_instruction *)ir->next) { I think this could be fixed by changing next to be exec_node*. > + for_range_list_safe(ir_instruction, ir, bb_first, bb_last->next) { > ir_assignment *assign = ir->as_assignment(); > > if (!assign) > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev