Clamp numeric overflow rather than failing with an error

Clamp numeric overflow rather than failing with an error

BUG=249086
ANGLEBUG=468
TEST=
R=alokp@chromium.org, kbr@chromium.org

Committed: https://code.google.com/p/angleproject/source/detail?r=521c836

[Zhenyao Mo, 2013-08-23 18:53:22]

Alok: please review.
kbr: FYI.
Shannon: please merge after it lands.

[Zhenyao Mo, 2013-08-23 19:02:27]

I realized the "-" is already stripped out as another token at this point, so no
need to handle the negative cases.

[kbr1, 2013-08-23 21:35:24]

LGTM

Do the new constants cause any problems with OpenGL ES devices that only support
mediump, and therefore can't represent even the maximum float?

https://codereview.appspot.com/13195043/diff/5001/src/compiler/preprocessor/P...
File src/compiler/preprocessor/Preprocessor.cpp (right):

https://codereview.appspot.com/13195043/diff/5001/src/compiler/preprocessor/P...
src/compiler/preprocessor/Preprocessor.cpp:110: val =
std::numeric_limits<int>::max();
Two's complement signed integer max and min values are different; the min is 1
less than the negation of the max. Will this cause any problems?

https://codereview.appspot.com/13195043/diff/5001/src/compiler/preprocessor/P...
src/compiler/preprocessor/Preprocessor.cpp:124: val =
std::numeric_limits<float>::max();
Not sure whether floating-point max and min have a similar issue.

[Zhenyao Mo, 2013-08-24 00:04:32]

It doesn' matter (for all the three questions in kbr's review). We just want to
pass at this stage and replace the out-of-range value with a large value that we
know will be an overflow when passing to the underlying driver.

Assuming underlying driver will handle this (if they are spec-compliant in this
- we have no reason to think they aren't).

[Alok Priyadarshi, 2013-08-26 04:29:39]

https://codereview.appspot.com/13195043/diff/5001/src/compiler/preprocessor/P...
File src/compiler/preprocessor/Preprocessor.cpp (right):

https://codereview.appspot.com/13195043/diff/5001/src/compiler/preprocessor/P...
src/compiler/preprocessor/Preprocessor.cpp:111: std::ostringstream ss;
I think it is fine to clamp, but you might still want to issue at least a
warning.

[Zhenyao Mo, 2013-08-27 23:22:41]

revised per alokp's review.

please have another look.

https://codereview.appspot.com/13195043/diff/12001/src/compiler/preprocessor/...
File src/compiler/preprocessor/DiagnosticsBase.h (right):

https://codereview.appspot.com/13195043/diff/12001/src/compiler/preprocessor/...
src/compiler/preprocessor/DiagnosticsBase.h:60: UNEXPECTED_TOKEN,
Re-ordered alphabetically.

[kbr1, 2013-08-28 01:54:16]

I defer to Alok's review -- I thought in an earlier review that the '-' sign was
handled as a separate token. Regardless, something looks wrong.

https://codereview.appspot.com/13195043/diff/12001/src/compiler/preprocessor/...
File src/compiler/preprocessor/Token.cpp (right):

https://codereview.appspot.com/13195043/diff/12001/src/compiler/preprocessor/...
src/compiler/preprocessor/Token.cpp:70: *value =
std::numeric_limits<int>::min();
Are these backwards?

https://codereview.appspot.com/13195043/diff/12001/src/compiler/preprocessor/...
src/compiler/preprocessor/Token.cpp:107: *value =
-std::numeric_limits<int>::max();
Here as well?

[Zhenyao Mo, 2013-08-28 16:52:12]

Yes, "-" is a separate token, but inside Token where we process int token, we
don't want to make that assumption, especially we have also uint token.

https://codereview.appspot.com/13195043/diff/12001/src/compiler/preprocessor/...
File src/compiler/preprocessor/Token.cpp (right):

https://codereview.appspot.com/13195043/diff/12001/src/compiler/preprocessor/...
src/compiler/preprocessor/Token.cpp:70: *value =
std::numeric_limits<int>::min();
On 2013/08/28 01:54:16, kbr1 wrote:
> Are these backwards?

Done.

https://codereview.appspot.com/13195043/diff/12001/src/compiler/preprocessor/...
src/compiler/preprocessor/Token.cpp:107: *value =
-std::numeric_limits<int>::max();
On 2013/08/28 01:54:16, kbr1 wrote:
> Here as well?

Done.

[Alok Priyadarshi, 2013-08-28 17:25:35]

You might also need to fix preprocessor tests.

https://codereview.appspot.com/13195043/diff/23001/src/compiler/preprocessor/...
File src/compiler/preprocessor/DiagnosticsBase.h (right):

https://codereview.appspot.com/13195043/diff/23001/src/compiler/preprocessor/...
src/compiler/preprocessor/DiagnosticsBase.h:30: CONDITIONAL_ENDIF_WITHOUT_IF,
is there a reason to move these here? Are you trying to sort them by name? If
so, can you please do it in a separate patch.

https://codereview.appspot.com/13195043/diff/23001/src/compiler/preprocessor/...
File src/compiler/preprocessor/ExpressionParser.cpp (right):

https://codereview.appspot.com/13195043/diff/23001/src/compiler/preprocessor/...
src/compiler/preprocessor/ExpressionParser.cpp:1898: token->uValue(&val,
context->diagnostics);
Here we are parsing a directive expression. If the expression overflows, you get
wrong results, which I think is an error, not a warning.

What does the spec say about overflows in preprocessor expressions?

https://codereview.appspot.com/13195043/diff/23001/src/compiler/preprocessor/...
File src/compiler/preprocessor/Token.cpp (right):

https://codereview.appspot.com/13195043/diff/23001/src/compiler/preprocessor/...
src/compiler/preprocessor/Token.cpp:67: if (text[0] == '-')
This will never be true. Token text does not contain sign.

https://codereview.appspot.com/13195043/diff/23001/src/compiler/preprocessor/...
src/compiler/preprocessor/Token.cpp:104: if (text[0] == '-')
same here

[Zhenyao Mo, 2013-08-28 23:44:49]

alok, please take another look.

I'll clean up the glslang_tab stuff once this CL is reviewed.

Otherwise if I modify anything and run the script, my manual cleanup will be
lost.

[Alok Priyadarshi, 2013-08-28 23:55:50]

https://codereview.appspot.com/13195043/diff/36001/src/compiler/glslang.l
File src/compiler/glslang.l (right):

https://codereview.appspot.com/13195043/diff/36001/src/compiler/glslang.l#new...
src/compiler/glslang.l:194: 0[xX]{H}+         { if (!atoi_clamp(yytext,
&(yylval->lex.i))) warn_int_overflow(yyscanner); return INTCONSTANT; }
To reduce code duplication I would create something like:
return int_constant(yyscanner);

This function will parse the value and issue warning if appropriate. The same
for float_constant(yyscanner).

https://codereview.appspot.com/13195043/diff/36001/src/compiler/util.cpp
File src/compiler/util.cpp (right):

https://codereview.appspot.com/13195043/diff/36001/src/compiler/util.cpp#newc...
src/compiler/util.cpp:44: if (str[0] == '-')
This is not necessary. The token string does not contain sign.

[Zhenyao Mo, 2013-08-29 00:18:03]

https://codereview.appspot.com/13195043/diff/36001/src/compiler/glslang.l
File src/compiler/glslang.l (right):

https://codereview.appspot.com/13195043/diff/36001/src/compiler/glslang.l#new...
src/compiler/glslang.l:194: 0[xX]{H}+         { if (!atoi_clamp(yytext,
&(yylval->lex.i))) warn_int_overflow(yyscanner); return INTCONSTANT; }
On 2013/08/28 23:55:50, Alok Priyadarshi wrote:
> To reduce code duplication I would create something like:
> return int_constant(yyscanner);
> 
> This function will parse the value and issue warning if appropriate. The same
> for float_constant(yyscanner).

Done.

https://codereview.appspot.com/13195043/diff/36001/src/compiler/util.cpp
File src/compiler/util.cpp (right):

https://codereview.appspot.com/13195043/diff/36001/src/compiler/util.cpp#newc...
src/compiler/util.cpp:44: if (str[0] == '-')
On 2013/08/28 23:55:50, Alok Priyadarshi wrote:
> This is not necessary. The token string does not contain sign.

Done.

[Alok Priyadarshi, 2013-08-29 05:27:39]

lgtm after you address these comments.

Please revert the changes in glslang_tab.[h,cpp]. And generate glslang_lex.cpp
with the correct flex version.

https://codereview.appspot.com/13195043/diff/37009/src/compiler/util.cpp
File src/compiler/util.cpp (right):

https://codereview.appspot.com/13195043/diff/37009/src/compiler/util.cpp#newc...
src/compiler/util.cpp:22: {
this function should return bool

https://codereview.appspot.com/13195043/diff/37009/src/compiler/util.cpp#newc...
src/compiler/util.cpp:23: int rt = 1;
nit: rename rt -> success?

https://codereview.appspot.com/13195043/diff/37009/src/compiler/util.cpp#newc...
src/compiler/util.cpp:41: *value = -std::numeric_limits<float>::max();
why is it negative?

https://codereview.appspot.com/13195043/diff/37009/src/compiler/util.cpp#newc...
src/compiler/util.cpp:45: int atoi_clamp(const char *str, int *value)
int -> bool

[Zhenyao Mo, 2013-08-29 21:55:18]

https://codereview.appspot.com/13195043/diff/37009/src/compiler/util.cpp
File src/compiler/util.cpp (right):

https://codereview.appspot.com/13195043/diff/37009/src/compiler/util.cpp#newc...
src/compiler/util.cpp:22: {
On 2013/08/29 05:27:39, Alok Priyadarshi wrote:
> this function should return bool

Done.

https://codereview.appspot.com/13195043/diff/37009/src/compiler/util.cpp#newc...
src/compiler/util.cpp:23: int rt = 1;
On 2013/08/29 05:27:39, Alok Priyadarshi wrote:
> nit: rename rt -> success?

Done.

https://codereview.appspot.com/13195043/diff/37009/src/compiler/util.cpp#newc...
src/compiler/util.cpp:41: *value = -std::numeric_limits<float>::max();
On 2013/08/29 05:27:39, Alok Priyadarshi wrote:
> why is it negative?

Done.

https://codereview.appspot.com/13195043/diff/37009/src/compiler/util.cpp#newc...
src/compiler/util.cpp:45: int atoi_clamp(const char *str, int *value)
On 2013/08/29 05:27:39, Alok Priyadarshi wrote:
> int -> bool

Done.

[Alok Priyadarshi, 2013-08-29 22:05:29]

lgtm

[Zhenyao Mo, 2013-08-29 22:17:09]

Committed patchset #10 manually as r521c836 (presubmit successful).