[Mesa-dev] [PATCH 2/5] compiler/list: add and use for_range_list_safe

[Ian Romanick]

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.haehnle at 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)
>