[Mesa-dev] [PATCH] util/draw: replace assertions with conditionals in util_draw_max_index()

[Jose Fonseca]

That's verbose enough thanks.


BTW, just noticed another problem in the change, the line

  max_index = ~0;

should be replaced with

  max_index = ~0U - 1;

otherwise 

  return max_index + 1;

will overflow and return 0 when all elements have stride 0, or are per-instance, causing the draw to be erroneously skipped.


Jose


----- Original Message -----
> Don't assert/die if a VBO is too small.  Return zero instead.  For
> debug builds, emit a warning message since this is an unusual
> situation
> that might indicate that there's a bug in the app.
> 
> Note that util_draw_max_index() now returns max_index+1 instead of
> max_index.  This lets us return zero to indicate that one of the VBOs
> is too small to draw anything.
> 
> Fixes a failure with the new piglit vbo-too-small test.
> ---
>  src/gallium/auxiliary/draw/draw_pt.c |   20 +++++++++++++++-----
>  src/gallium/auxiliary/util/u_draw.c  |   30
>  +++++++++++++++++++++++-------
>  2 files changed, 38 insertions(+), 12 deletions(-)
> 
> diff --git a/src/gallium/auxiliary/draw/draw_pt.c
> b/src/gallium/auxiliary/draw/draw_pt.c
> index e0eda67..080e03d 100644
> --- a/src/gallium/auxiliary/draw/draw_pt.c
> +++ b/src/gallium/auxiliary/draw/draw_pt.c
> @@ -422,6 +422,7 @@ draw_vbo(struct draw_context *draw,
>  {
>     unsigned reduced_prim = u_reduced_prim(info->mode);
>     unsigned instance;
> +   unsigned index_limit;
>  
>     assert(info->instance_count > 0);
>     if (info->indexed)
> @@ -470,11 +471,20 @@ draw_vbo(struct draw_context *draw,
>     if (0)
>        draw_print_arrays(draw, info->mode, info->start,
>        MIN2(info->count, 20));
>  
> -   draw->pt.max_index = util_draw_max_index(draw->pt.vertex_buffer,
> -
>                                            draw->pt.nr_vertex_buffers,
> -                                            draw->pt.vertex_element,
> -
>                                            draw->pt.nr_vertex_elements,
> -                                            info);
> +   index_limit = util_draw_max_index(draw->pt.vertex_buffer,
> +                                     draw->pt.nr_vertex_buffers,
> +                                     draw->pt.vertex_element,
> +                                     draw->pt.nr_vertex_elements,
> +                                     info);
> +
> +   if (index_limit == 0) {
> +      /* one of the buffers is too small to do any valid drawing */
> +      debug_warning("draw: VBO too small to draw anything\n");
> +      return;
> +   }
> +
> +   draw->pt.max_index = index_limit - 1;
> +
>  
>     /*
>      * TODO: We could use draw->pt.max_index to further narrow
> diff --git a/src/gallium/auxiliary/util/u_draw.c
> b/src/gallium/auxiliary/util/u_draw.c
> index 20837be..2da1adb 100644
> --- a/src/gallium/auxiliary/util/u_draw.c
> +++ b/src/gallium/auxiliary/util/u_draw.c
> @@ -33,9 +33,13 @@
>  
>  
>  /**
> - * Returns the largest legal index value for the current set of
> bound vertex
> - * buffers.  Regardless of any other consideration, all vertex
> lookups need to
> - * be clamped to 0..max_index to prevent an out-of-bound access.
> + * Returns the largest legal index value plus one for the current
> set
> + * of bound vertex buffers.  Regardless of any other consideration,
> + * all vertex lookups need to be clamped to 0..max_index-1 to
> prevent
> + * an out-of-bound access.
> + *
> + * Note that if zero is returned it means that one or more buffers
> is
> + * too small to contain any valid vertex data.
>   */
>  unsigned
>  util_draw_max_index(
> @@ -68,13 +72,25 @@ util_draw_max_index(
>        assert(format_desc->block.bits % 8 == 0);
>        format_size = format_desc->block.bits/8;
>  
> -      assert(buffer_size - buffer->buffer_offset <= buffer_size);
> +      if (buffer->buffer_offset >= buffer_size) {
> +         /* buffer is too small */
> +         return 0;
> +      }
> +
>        buffer_size -= buffer->buffer_offset;
>  
> -      assert(buffer_size - element->src_offset <= buffer_size);
> +      if (element->src_offset >= buffer_size) {
> +         /* buffer is too small */
> +         return 0;
> +      }
> +
>        buffer_size -= element->src_offset;
>  
> -      assert(buffer_size - format_size <= buffer_size);
> +      if (format_size > buffer_size) {
> +         /* buffer is too small */
> +         return 0;
> +      }
> +
>        buffer_size -= format_size;
>  
>        if (buffer->stride != 0) {
> @@ -95,5 +111,5 @@ util_draw_max_index(
>        }
>     }
>  
> -   return max_index;
> +   return max_index + 1;
>  }
> --
> 1.7.3.4
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>