[Mesa-dev] [PATCH 1/3] draw: account for elem size when computing overflow

[Jose Fonseca]

----- Original Message -----
> We weren't taking into account the size of element
> that is to be fetched, which meant that it was possible
> to overflow the buffer reads if the stride was very
> close to the end of the buffer, e.g. stride = 3, buffer
> size = 4, and the element to be read = 4. This should
> be properly detected as an overflow.

Looks good.


> Signed-off-by: Zack Rusin <zackr at vmware.com>
> ---
>  src/gallium/auxiliary/draw/draw_llvm.c |   30 +++++++++++++++++++++++-------
>  1 file changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/src/gallium/auxiliary/draw/draw_llvm.c
> b/src/gallium/auxiliary/draw/draw_llvm.c
> index 5373d1a..f27776a 100644
> --- a/src/gallium/auxiliary/draw/draw_llvm.c
> +++ b/src/gallium/auxiliary/draw/draw_llvm.c
> @@ -695,6 +695,7 @@ generate_fetch(struct gallivm_state *gallivm,
>     LLVMValueRef buffer_size = draw_jit_dvbuffer_size(gallivm, vbuffer_ptr);
>     LLVMValueRef stride;
>     LLVMValueRef buffer_overflowed;
> +   LLVMValueRef needed_buffer_size;
>     LLVMValueRef temp_ptr =
>        lp_build_alloca(gallivm,
>                        lp_build_vec_type(gallivm, lp_float32_vec4_type()),
>                        "");
> @@ -715,15 +716,30 @@ generate_fetch(struct gallivm_state *gallivm,
>     stride = LLVMBuildAdd(builder, stride,
>                           lp_build_const_int32(gallivm, velem->src_offset),
>                           "");
> -
> -   buffer_overflowed = LLVMBuildICmp(builder, LLVMIntUGE,
> -                                     stride, buffer_size,
> +   needed_buffer_size = LLVMBuildAdd(
> +      builder, stride,
> +      lp_build_const_int32(gallivm,
> +                           util_format_get_blocksize(velem->src_format)),
> +      "");
> +
> +   buffer_overflowed = LLVMBuildICmp(builder, LLVMIntUGT,
> +                                     needed_buffer_size, buffer_size,
>                                       "buffer_overflowed");
> -   /*
> -   lp_build_printf(gallivm, "vbuf index = %u, stride is %u\n", index,
> stride);
> -   lp_build_print_value(gallivm, "   buffer size = ", buffer_size);
> +#if 0

I think it is preferable to use if (0) { ... } for this sort of debugging statements, as it allows compilers to ensure the code doesn't rot, and most editors also tend to highlight this as comment.

> +   lp_build_printf(gallivm, "vbuf index = %u, vb_stride is %u\n",
> +                   index, vb_stride);
> +   lp_build_printf(gallivm, "   vb_buffer_offset = %u, src_offset is %u\n",
> +                   vb_buffer_offset,
> +                   lp_build_const_int32(gallivm, velem->src_offset));
> +   lp_build_print_value(gallivm, "   blocksize = ",
> +                        lp_build_const_int32(
> +                           gallivm,
> +                           util_format_get_blocksize(velem->src_format)));
> +   lp_build_printf(gallivm, "   stride = %u\n", stride);
> +   lp_build_printf(gallivm, "   buffer size = %u\n", buffer_size);
> +   lp_build_printf(gallivm, "   needed_buffer_size = %u\n",
> needed_buffer_size);
>     lp_build_print_value(gallivm, "   buffer overflowed = ",
>     buffer_overflowed);
> -   */
> +#endif
>  
>     lp_build_if(&if_ctx, gallivm, buffer_overflowed);
>     {
> --
> 1.7.10.4


Jose
>