Enable array index clamping in shader translator.

Enable array index clamping in shader translator.

BUG=169054
TEST=none


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176889

[Ken Russell (switch to Gerrit), 2013-01-12 01:06:21]

Tested with new layout test in WebKit tree verifying this behavior, as well as
several Three.js demos on Linux; all are working fine with this change.

Emailed the WG regarding writing a conformance test for this.

[greggman, 2013-01-12 01:31:37]

https://codereview.chromium.org/11884007/diff/1/gpu/command_buffer/service/sh...
File gpu/command_buffer/service/shader_translator.cc (right):

https://codereview.chromium.org/11884007/diff/1/gpu/command_buffer/service/sh...
gpu/command_buffer/service/shader_translator.cc:131: SH_MAP_LONG_VARIABLE_NAMES
| SH_CLAMP_INDIRECT_ARRAY_BOUNDS;
Do you want | SH_ENFORCE_PACKING_RESTRICTIONS
or no? Without that the clamping seems useless.

PS: I'm also kind curious why the ANGLE patch uses a function instead of
clamp(v, 0, array_size);

[Ken Russell (switch to Gerrit), 2013-01-14 19:07:51]

On 2013/01/12 01:31:37, greggman wrote:
>
https://codereview.chromium.org/11884007/diff/1/gpu/command_buffer/service/sh...
> File gpu/command_buffer/service/shader_translator.cc (right):
> 
>
https://codereview.chromium.org/11884007/diff/1/gpu/command_buffer/service/sh...
> gpu/command_buffer/service/shader_translator.cc:131:
SH_MAP_LONG_VARIABLE_NAMES
> | SH_CLAMP_INDIRECT_ARRAY_BOUNDS;
> Do you want | SH_ENFORCE_PACKING_RESTRICTIONS
> or no? Without that the clamping seems useless.

I'd like to first update the WebGL spec to require the packing restrictions.
I'll do that this week.

This patch isn't useless without the packing restrictions though. It fixes a
layout test which tests the current behavior, and I believe will fix the
referenced bug.

> PS: I'm also kind curious why the ANGLE patch uses a function instead of
> clamp(v, 0, array_size);

That's a good question. I didn't review the ANGLE patch in detail -- Alok did --
but I agree with you that the injected code should be really simple. Could you
ask Dean? I would be glad to change ANGLE to do it differently.

[greggman, 2013-01-14 19:13:05]

On 2013/01/14 19:07:51, kbr wrote:
> On 2013/01/12 01:31:37, greggman wrote:
> >
>
https://codereview.chromium.org/11884007/diff/1/gpu/command_buffer/service/sh...
> > File gpu/command_buffer/service/shader_translator.cc (right):
> > 
> >
>
https://codereview.chromium.org/11884007/diff/1/gpu/command_buffer/service/sh...
> > gpu/command_buffer/service/shader_translator.cc:131:
> SH_MAP_LONG_VARIABLE_NAMES
> > | SH_CLAMP_INDIRECT_ARRAY_BOUNDS;
> > Do you want | SH_ENFORCE_PACKING_RESTRICTIONS
> > or no? Without that the clamping seems useless.
> 
> I'd like to first update the WebGL spec to require the packing restrictions.
> I'll do that this week.
> 
> This patch isn't useless without the packing restrictions though. It fixes a
> layout test which tests the current behavior, and I believe will fix the
> referenced bug.

It fixes the bug because it clamps negative indices but the fact that it does
not clamp out of range indices worries me because checking this in as is
provides a false sense of security.

but whatever, lgtm

> 
> > PS: I'm also kind curious why the ANGLE patch uses a function instead of
> > clamp(v, 0, array_size);
> 
> That's a good question. I didn't review the ANGLE patch in detail -- Alok did
--
> but I agree with you that the injected code should be really simple. Could you
> ask Dean? I would be glad to change ANGLE to do it differently.

[commit-bot: I haz the power, 2013-01-14 19:41:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kbr@chromium.org/11884007/1

[commit-bot: I haz the power, 2013-01-15 12:38:47]

Change committed as 176889