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

[Carl Worth]

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.

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
+
-- 
2.0.0