[Mesa-dev] [PATCH] glsl: Implement GLSL 1.30's literal integer range restrictions.

[Ian Romanick]

On 10/03/2011 05:03 PM, Eric Anholt wrote:
>  From page 22 (28 of PDF) of GLSL 1.30 spec:
>      It is an error to provide a literal integer whose magnitude is too
>      large to store in a variable of matching signed or unsigned type.
>
>      Unsigned integers have exactly 32 bits of precision.  Signed integers
>      use 32 bits, including a sign bit, in two's complement form.
> ---
>
> Others read this as "0xffffffff" being a valid literal integer of -1,
> like in C, right?

Given the paragraph before the one you quoted, I think not.

     "No white space is allowed between the digits of an integer
     constant, including after the leading 0 or after the leading 0x
     or 0X of a constant, or before the suffix u or U. When the suffix u
     or U is present, the literal has type uint, otherwise the type is
     int. A leading unary minus sign (-) is interpreted as an arithmetic
     unary negation, not as part of the constant."

I think you'd have to say 0xffffffffu.  This is a little weird because 
~0 is valid.  It may seem obvious that 0xffffffff should "just work," 
but what about 4294967294?  GLSL wants there to be an error in the cases 
where GCC would generate a warning with -Wconversion.

There is a bit here that's broken.  Going by the letter of the spec, you 
can't say 'int i = -2147483648;' because 2147483648 has a magnitude too 
large for a signed integer.  Ugh.

This makes 4 spec bugs in 2 days.  I'm on a roll.

>   src/glsl/glsl_lexer.ll |   38 ++++++++++++++++++++++++++++++--------
>   1 files changed, 30 insertions(+), 8 deletions(-)
>
> diff --git a/src/glsl/glsl_lexer.ll b/src/glsl/glsl_lexer.ll
> index cfd8926..b01dcde 100644
> --- a/src/glsl/glsl_lexer.ll
> +++ b/src/glsl/glsl_lexer.ll
> @@ -22,6 +22,7 @@
>    * DEALINGS IN THE SOFTWARE.
>    */
>   #include<ctype.h>
> +#include<limits.h>
>   #include "strtod.h"
>   #include "ast.h"
>   #include "glsl_parser_extras.h"
> @@ -43,8 +44,6 @@ static int classify_identifier(struct _mesa_glsl_parse_state *, const char *);
>
>   #define YY_USER_INIT yylineno = 0; yycolumn = 0;
>
> -#define IS_UINT (yytext[yyleng - 1] == 'u' || yytext[yyleng - 1] == 'U')
> -
>   /* A macro for handling reserved words and keywords across language versions.
>    *
>    * Certain words start out as identifiers, become reserved words in
> @@ -81,6 +80,32 @@ static int classify_identifier(struct _mesa_glsl_parse_state *, const char *);
>    * ...means the word is a legal keyword in GLSL ES 1.00.
>    */
>   #define ES yyextra->es_shader
> +
> +#define LITERAL_INTEGER(base)						\
> +do {									\
> +   bool is_uint = (yytext[yyleng - 1] == 'u' ||				\
> +		   yytext[yyleng - 1] == 'U');				\
> +   const char *digits = yytext;						\
> +									\
> +   if (base == 16)							\
> +      digits += 2;							\
> +   long long value = strtoll(digits, NULL, base);			\
> +									\
> +   yylval->n = (int)value;						\
> +									\
> +   if (value>  UINT_MAX) {						\

I had this big long bit written about why this isn't correct, but it is 
correct.  It's just really non-obvious.  The non-obvious bit is that 
digits can never have a negative sign, so 'value' will always be 
positive.  I think I'd use 'unsigned long long' and strtoull just to 
make that more obvious.

> +      /* Note that signed 0xffffffff is valid, not out of range! */	\
> +      if (yyextra->language_version>= 130) {				\
> +	 _mesa_glsl_error(yylloc, yyextra,				\
> +			  "Literal value `%s' out of range", yytext);	\
> +      } else {								\
> +	 _mesa_glsl_warning(yylloc, yyextra,				\
> +			   "Literal value `%s' out of range", yytext);	\

If we're going to emit a warning in the < 1.30 case, it should be based 
on the representable size of integers on the target machine.  We have 
that information in ctx->Const.*Int.Precision.

> +      }									\
> +   }									\
> +   return is_uint ? UINTCONSTANT : INTCONSTANT;				\
> +} while (0)
> +
>   %}
>
>   %option bison-bridge bison-locations reentrant noyywrap
> @@ -292,16 +317,13 @@ layout		{
>   -=		return SUB_ASSIGN;
>
>   [1-9][0-9]*[uU]?	{
> -			    yylval->n = strtol(yytext, NULL, 10);
> -			    return IS_UINT ? UINTCONSTANT : INTCONSTANT;
> +			    LITERAL_INTEGER(10);
>   			}
>   0[xX][0-9a-fA-F]+[uU]?	{
> -			    yylval->n = strtol(yytext + 2, NULL, 16);
> -			    return IS_UINT ? UINTCONSTANT : INTCONSTANT;
> +			    LITERAL_INTEGER(16);
>   			}
>   0[0-7]*[uU]?		{
> -			    yylval->n = strtol(yytext, NULL, 8);
> -			    return IS_UINT ? UINTCONSTANT : INTCONSTANT;
> +			    LITERAL_INTEGER(8);
>   			}
>
>   [0-9]+\.[0-9]+([eE][+-]?[0-9]+)?[fF]?	{