[Mesa-dev] [PATCH v3] glsl/glcpp: Fix glcpp to properly lex entire "preprocessing numbers"

Tue Jul 1 14:26:03 PDT 2014

On Thu, Jun 26, 2014 at 2:54 PM, Carl Worth <cworth at cworth.org> wrote:
> The preprocessor defines a notions of a "preprocessing number" that
> starts with either a digit or a decimal point, and continues with zero
> or more of digits, decimal points, identifier characters, or the sign
> symbols, ('-' and '+').
>
> Prior to this change, preprocessing numbers were lexed as some
> combination of OTHER and IDENTIFIER tokens. This had the problem of
> causing undesired macro expansion in some cases.
>
> We add tests to ensure that the undesired macro expansion does not
> happen in cases such as:
>
>         #define e +1
>         #define xyz -2
>
>         int n = 1e;
>         int p = 1xyz;
>
> In either case these macro definitions have no effect after this
> change, so that the numeric literals, (whether valid or not), will be
> passed on as-is from the preprocessor to the compiler proper.
>
> This fixes the following Khronos GLES3 CTS tests:
>
>         preprocessor.basic.correct_phases_vertex
>         preprocessor.basic.correct_phases_fragment
>
> v2. Thanks to Anuj Phogat for improving the original regular expression,
> (which accepted a '+' or '-', where these are only allowed after one of
> [eEpP]. I also expanded the test to exercise this.
>
> v3. Also fixed regular expression to require at least one digit at the
> beginning (after an optional period). Otherwise, a string such as ".xyz" was
> getting sucked up as a preprocessing number, (where obviously this should be a
> field access). Again, I expanded the test to exercise this.
>
V3 fixes a CTS failure caused by V2 in GL3Tests.shadow.shadow_compilation_frag.

> Reviewed-by: Anuj Phogat <anuj.phogat at gmail.com>
> ---
>
>   > It causes '2.0-MACRO' to incorrectly lexed as a preprocessing number.
>   > Around 30 GLES3 CTS tests failed with this patch.
>   >
>   > I think using below definition of preprocessing numbers should work fine:
>   > PP_NUMBER                   \.?[0-9]([._a-zA-Z0-9]|[eEpP][+-])*
>
>   Thanks, Anuj. When I first saw your follow-up here, I saw the new
>   alternation to pull the exponent piece, ("e+", etc.) separate from
>   the main character class. But I missed that you als fixed the initial
>   optional '.' followed by a required digit.
>
>   I ended up implementing that second fix independently, and only now
>   while sending the patch again do I see that you had that fixed in your
>   regular expression as well.
>
>   Thanks again. This new version also adds a bunch of testing to
>   "make check", (and fixes all the regressions that piglit found with
>   my earlier regular expression.
>
>  src/glsl/glcpp/glcpp-lex.l                         |  6 ++++
>  src/glsl/glcpp/tests/124-preprocessing-numbers.c   | 37 +++++++++++++++++++++
>  .../tests/124-preprocessing-numbers.c.expected     | 38 ++++++++++++++++++++++
>  3 files changed, 81 insertions(+)
>  create mode 100644 src/glsl/glcpp/tests/124-preprocessing-numbers.c
>  create mode 100644 src/glsl/glcpp/tests/124-preprocessing-numbers.c.expected
>
> diff --git a/src/glsl/glcpp/glcpp-lex.l b/src/glsl/glcpp/glcpp-lex.l
> index d5fb087..9563817 100644
> --- a/src/glsl/glcpp/glcpp-lex.l
> +++ b/src/glsl/glcpp/glcpp-lex.l
> @@ -76,6 +76,7 @@ NEWLINE               [\n]
>  HSPACE         [ \t]
>  HASH           ^{HSPACE}*#{HSPACE}*
>  IDENTIFIER     [_a-zA-Z][_a-zA-Z0-9]*
> +PP_NUMBER      [.]?[0-9]([._a-zA-Z0-9]|[eEpP][-+])*
>  PUNCTUATION    [][(){}.&*~!/%<>^|;,=+-]
>
>  /* The OTHER class is simply a catch-all for things that the CPP
> @@ -330,6 +331,11 @@ HEXADECIMAL_INTEGER        0[xX][0-9a-fA-F]+[uU]?
>         return IDENTIFIER;
>  }
>
> +{PP_NUMBER} {
> +       yylval->str = ralloc_strdup (yyextra, yytext);
> +       return OTHER;
> +}
> +
>  {PUNCTUATION} {
>         return yytext[0];
>  }
> diff --git a/src/glsl/glcpp/tests/124-preprocessing-numbers.c b/src/glsl/glcpp/tests/124-preprocessing-numbers.c
> new file mode 100644
> index 0000000..8019804
> --- /dev/null
> +++ b/src/glsl/glcpp/tests/124-preprocessing-numbers.c
> @@ -0,0 +1,37 @@
> +#define e THIS_SHOULD_NOT_BE_EXPANDED
> +#define E NOR_THIS
> +#define p NOT_THIS_EITHER
> +#define P AND_SURELY_NOT_THIS
> +#define OK CRAZY_BUT_TRUE_THIS_NEITHER
> +
> +/* This one is actually meant to be expanded */
> +#define MUST_EXPAND GO
> +
> +/* The following are "preprocessing numbers" and should not trigger macro
> + * expansion. */
> +1e
> +1OK
> +
> +/* These are also "preprocessing numbers", so no expansion */
> +123e+OK
> +.23E+OK
> +1.3e-OK
> +12.E-OK
> +123p+OK
> +.23P+OK
> +1.3p-OK
> +12.P-OK
> +123..OK
> +.23.OK.OK
> +
> +/* Importantly, just before the MUST_EXPAND in each of these, the preceding
> + * "preprocessing number" ends and we have an actual expression. So the
> + * MUST_EXPAND macro must be expanded (who would have though?) in each case. */
> +123ef+MUST_EXPAND
> +.23E3-MUST_EXPAND
> +1.3e--MUST_EXPAND
> +12.E-&MUST_EXPAND
> +123p+OK+MUST_EXPAND
> +.23P+OK;MUST_EXPAND
> +1.3p-OK-MUST_EXPAND
> +12.P-OK&MUST_EXPAND
> diff --git a/src/glsl/glcpp/tests/124-preprocessing-numbers.c.expected b/src/glsl/glcpp/tests/124-preprocessing-numbers.c.expected
> new file mode 100644
> index 0000000..6f5254c
> --- /dev/null
> +++ b/src/glsl/glcpp/tests/124-preprocessing-numbers.c.expected
> @@ -0,0 +1,38 @@
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +1e
> +1OK
> +
> +
> +123e+OK
> +.23E+OK
> +1.3e-OK
> +12.E-OK
> +123p+OK
> +.23P+OK
> +1.3p-OK
> +12.P-OK
> +123..OK
> +.23.OK.OK
> +
> +
> +
> +
> +123ef+GO
> +.23E3-GO
> +1.3e--GO
> +12.E-&GO
> +123p+OK+GO
> +.23P+OK;GO
> +1.3p-OK-GO
> +12.P-OK&GO
> +
Few trailing whitespaces in this file.
> --
> 2.0.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev