[Mesa-dev] [PATCH 1/2] intel: fail to compile in case of bad uniform access

[Ian Romanick]

On 01/28/2013 05:06 PM, Frank Henigman wrote:
> An assertion in fs_visitor::remove_dead_constants() would fail on
> code like this, which accesses a non-existent uniform:
>    uniform vec u[1];
>    ...
>    a += u[0] + u[1];
> Shader code is checked for that error, but apparently only before
> transformations like loop unrolling.  So the following code would get
> transformed to the same as above and then the assertion would fail:
>    for (int i = 0; i < 2; ++i) a += u[i];
>
> This patch changes the assertion failure to a compile failure.

The behavior of an out-of-bounds access is undefined, but compilation is 
not allowed to fail.  I think there are two good solutions:

1. Always generate zero.  I believe this is what DX requires, so there 
may be apps ported from DX that assume this behavior.  This is also what 
happens when you access out of bounds of a UBO.

2. Clamp the index to the bounds of the array.

> https://bugs.freedesktop.org/show_bug.cgi?id=59429

Also... if you're working on a bug, you should assign it to yourself.  I 
was planning to start working on this bug tomorrow. :)

> Signed-off-by: Frank Henigman <fjhenigman at google.com>
> ---
>   src/mesa/drivers/dri/i965/brw_fs.cpp | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index 8e57eb0..548f7d6 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -1467,7 +1467,12 @@ fs_visitor::remove_dead_constants()
>   	    if (inst->src[i].file != UNIFORM)
>   	       continue;
>
> -	    assert(constant_nr < (int)c->prog_data.nr_params);
> +	    if (constant_nr >= (int)c->prog_data.nr_params) {
> +	       ralloc_free(this->params_remap);
> +	       this->params_remap = NULL;
> +	       fail("accessed non-existent uniform");
> +	       return false;
> +	    }
>
>   	    /* For now, set this to non-negative.  We'll give it the
>   	     * actual new number in a moment, in order to keep the
>