Shader translator needs option to clamp uniform array accesses in vertex shaders

[GoogleCodeExporter]

Appendix A, section 5 of the GLSL ES specification states that arbitrary 
indexing of uniform arrays in vertex shaders is mandated. This is specifically 
to support skinning, where matrix indices come in to the shader via vertex 
attributes.

At least for WebGL implementations, and likely for other client libraries using 
the shader translator, an option is needed to guarantee that these indexing 
operations will not go outside the region of the array.

This can be trivially achieved by injecting clamp instructions around the index 
when the index expression references attributes or uniform values. For indexing 
expressions involving only constants or loop indices, it can be determined at 
compile time whether the expression can go out of range, and it should always 
be an error to compile such shaders.

Ideally, the shader translator would be able to provide information to the 
caller about the range of legal values for vertex attributes that are used in 
indexing expressions. For example, a shader like

  attribute int index;
  uniform vec4 positions[4];

  void main() {
    gl_Position = positions[index];
  }

would report that the legal range for the "index" attribute is [0..3]. If the 
indexing expression were instead "positions[2*index]", the legal range would be 
[0..1]. Since WebGL and other libraries using the shader translator tend to 
need to know the maximum value in buffers anyway, these libraries could decide 
whether a "safe" version of the shader with the injected clamp instructions 
must be used, or whether the unmodified shader could be used for higher 
performance.

The reporting of legal ranges for attributes can be fixed in a separate issue 
as it will likely involve significantly more engineering. Optional injection of 
clamp instructions for the appropriate indexing expressions is the most 
immediate need.

Original issue reported on code.google.com by kbr@chromium.org on 30 Sep 2010 at 6:18

[GoogleCodeExporter]

Original comment by alokp@chromium.org on 9 Nov 2010 at 10:22

• Added labels: Type-Enhancement
• Removed labels: Type-Defect

[GoogleCodeExporter]

Should clamping be done in both GLSL and HLSL shaders? If so it would be best 
to inject the clamping instructions into the AST itself so that it is 
automatically handled by both translators.

Original comment by alokp@chromium.org on 10 Nov 2010 at 4:04

[GoogleCodeExporter]

It depends on whether HLSL has semantics that guarantee that out-of-range 
accesses are safe. I know that DX10 class hardware handles out-of-range indices 
during rendering of indexed geometry, but don't know about any rules covering 
out-of-range uniform array accesses.

Agreed that it would probably be best and easiest to just do this at the AST 
level. A first cut, at least, should certainly be done there.

We should only inject the clamp instructions when we can't prove that the 
uniform array accesses are safe. Of course this is a harder problem, but at 
least for simple indexing with just the loop index or a constant, we ought not 
to inject unnecessary clamp instructions.

Original comment by kbr@chromium.org on 10 Nov 2010 at 6:21

[GoogleCodeExporter]

Direct3D 9 guarantees that when a vertex shader attempts to read an out of 
range constant register, a (0, 0, 0, 0) vector is returned. Note that this 
doesn't mean it clamps the index to the array size, but accesses are always 
safe.

Personally I don't think clamping to the actual array size helps. Either way 
the returned values are not what the shader programmer expects, no matter if 
he's reading values from another array, or a null vector. If he expects 
clamping behavior, he should explicitly do the clamping himself.

The GLSL ES spec states the following: "Reading from or writing to an array 
with a non-constant index that is less than zero or greater than or equal to 
the array's size results in undefined behavior. It is platform dependent how 
bounded this undefined behavior may be. It is possible that it leads to 
instability of the underlying system or corruption of memory. However, a 
particular platform may bound the behavior such that this is not the case."

So just to be clear, out-of-range accesses in Direct3D are undefined but well 
bounded. It will not cause instabilities or memory corruption.

Original comment by nicolas....@gmail.com on 10 Nov 2010 at 10:44

[GoogleCodeExporter]

> Personally I don't think clamping to the actual array size
> helps. Either way the returned values are not what the shader
> programmer expects, no matter if he's reading values from another
> array, or a null vector. If he expects clamping behavior, he
> should explicitly do the clamping himself.
> 
> The GLSL ES spec states the following: "Reading from or writing
> to an array with a non-constant index that is less than zero or
> greater than or equal to the array's size results in undefined
> behavior. It is platform dependent how bounded this undefined
> behavior may be. It is possible that it leads to instability of
> the underlying system or corruption of memory. However, a
> particular platform may bound the behavior such that this is not
> the case."

It's not acceptable from the point of view of the WebGL API to potentially 
allow random VRAM to be read or written, especially if the contents of that 
VRAM might not have been written by the current application. For this reason 
the clamping proposed here is essential.

It sounds like it's only needed for the GLSL backend. However, it would be most 
easily done at the AST level, so perhaps a virtual function could be added that 
can be overridden by TOutputGLSL / TOutputHLSL indicating whether the clamp 
instructions are needed.

Original comment by kbr@chromium.org on 10 Nov 2010 at 11:05

[GoogleCodeExporter]

Original comment by kbr@chromium.org on 7 Oct 2011 at 2:36

[GoogleCodeExporter]

Changing this to Type-Defect; needs to be addressed.

Original comment by kbr@chromium.org on 4 Oct 2012 at 8:27

• Added labels: Type-Defect
• Removed labels: Type-Enhancement

[GoogleCodeExporter]

Original comment by c...@chromium.org on 7 Dec 2013 at 4:10

[GoogleCodeExporter]

This was implemented by https://codereview.appspot.com/6999052

Original comment by c...@chromium.org on 21 May 2014 at 2:48

• Changed state: Fixed