[Mesa-dev] glsl: Fix glcpp to catch garbage after #if 1 ... #else

Submitted by Carl Worth on June 12, 2014, 6:26 p.m.

Details

Message ID 1402597574-8199-1-git-send-email-cworth@cworth.org
State New
Headers show

Not browsing as part of any series.

Commit Message

Carl Worth June 12, 2014, 6:26 p.m.
Previously, a line such as:

	#else garbage

would flag an error if it followed "#if 0", but not if it followed "#if 1".

We fix this by setting a new bit of state (lexing_else) that allows the lexer
to defer switching to the <SKIP> start state until after the NEWLINE following
the #else directive.

A new test case is added for:

	#if 1
	#else garbage
	#endif

which was untested before, (and did not generate the desired error).
---
 src/glsl/glcpp/glcpp-lex.l                               | 10 +++++++++-
 src/glsl/glcpp/glcpp-parse.y                             |  6 ++++--
 src/glsl/glcpp/glcpp.h                                   |  1 +
 src/glsl/glcpp/tests/103-garbage-after-else-0.c          |  3 +++
 src/glsl/glcpp/tests/103-garbage-after-else-0.c.expected |  4 ++++
 src/glsl/glcpp/tests/103-garbage-after-else.c            |  3 ---
 src/glsl/glcpp/tests/103-garbage-after-else.c.expected   |  4 ----
 src/glsl/glcpp/tests/123-garbage-after-else-1.c          |  3 +++
 src/glsl/glcpp/tests/123-garbage-after-else-1.c.expected |  4 ++++
 9 files changed, 28 insertions(+), 10 deletions(-)
 create mode 100644 src/glsl/glcpp/tests/103-garbage-after-else-0.c
 create mode 100644 src/glsl/glcpp/tests/103-garbage-after-else-0.c.expected
 delete mode 100644 src/glsl/glcpp/tests/103-garbage-after-else.c
 delete mode 100644 src/glsl/glcpp/tests/103-garbage-after-else.c.expected
 create mode 100644 src/glsl/glcpp/tests/123-garbage-after-else-1.c
 create mode 100644 src/glsl/glcpp/tests/123-garbage-after-else-1.c.expected

Patch hide | download patch | download mbox

diff --git a/src/glsl/glcpp/glcpp-lex.l b/src/glsl/glcpp/glcpp-lex.l
index 188e454..dea6a6c 100644
--- a/src/glsl/glcpp/glcpp-lex.l
+++ b/src/glsl/glcpp/glcpp-lex.l
@@ -125,7 +125,7 @@  HEXADECIMAL_INTEGER	0[xX][0-9a-fA-F]+[uU]?
 	 * as set by the parser to know whether to change from INITIAL
 	 * to SKIP or from SKIP back to INITIAL.
 	 *
-	 * Three cases cause us to switch out of the SKIP state and
+	 * Four cases cause us to switch out of the SKIP state and
 	 * back to the INITIAL state:
 	 *
 	 *	1. The top of the skip_stack is of type SKIP_NO_SKIP
@@ -142,9 +142,17 @@  HEXADECIMAL_INTEGER	0[xX][0-9a-fA-F]+[uU]?
 	 *	   "#elif". Even inside an "#if 0" we need to lex this
 	 *	   expression so the parser can correctly update the
 	 *	   skip_stack state.
+	 *
+	 *	4. The lexing_else bit is set. This indicates that we
+	 *	   are lexing tokens after #else and before the
+	 *         newline. We don't want to skip here just so that we
+	 *         can correctly flag errors if there is any
+	 *         garbage (other than comments) on the same line as the
+	 *         #else.
 	 */
 	if (YY_START == INITIAL || YY_START == SKIP) {
 		if (parser->lexing_if ||
+                    parser->lexing_else ||
 		    parser->skip_stack == NULL ||
 		    parser->skip_stack->type == SKIP_NO_SKIP)
 		{
diff --git a/src/glsl/glcpp/glcpp-parse.y b/src/glsl/glcpp/glcpp-parse.y
index 650d0d4..57e1ee8 100644
--- a/src/glsl/glcpp/glcpp-parse.y
+++ b/src/glsl/glcpp/glcpp-parse.y
@@ -364,7 +364,7 @@  control_line:
 			glcpp_warning(& @1, parser, "ignoring illegal #elif without expression");
 		}
 	}
-|	HASH_ELSE {
+|	HASH_ELSE { parser->lexing_else = 1; } NEWLINE {
 		if (parser->skip_stack &&
 		    parser->skip_stack->has_else)
 		{
@@ -376,7 +376,8 @@  control_line:
 			if (parser->skip_stack)
 				parser->skip_stack->has_else = true;
 		}
-	} NEWLINE
+                parser->lexing_else = 0;
+	}
 |	HASH_ENDIF {
 		_glcpp_parser_skip_stack_pop (parser, & @1);
 	} NEWLINE
@@ -1212,6 +1213,7 @@  glcpp_parser_create (const struct gl_extensions *extensions, gl_api api)
 					   hash_table_string_compare);
 	parser->active = NULL;
 	parser->lexing_if = 0;
+	parser->lexing_else = 0;
 	parser->space_tokens = 1;
 	parser->newline_as_space = 0;
 	parser->in_control_line = 0;
diff --git a/src/glsl/glcpp/glcpp.h b/src/glsl/glcpp/glcpp.h
index 79ccb23..491263f 100644
--- a/src/glsl/glcpp/glcpp.h
+++ b/src/glsl/glcpp/glcpp.h
@@ -169,6 +169,7 @@  struct glcpp_parser {
 	struct hash_table *defines;
 	active_list_t *active;
 	int lexing_if;
+	int lexing_else;
 	int space_tokens;
 	int newline_as_space;
 	int in_control_line;
diff --git a/src/glsl/glcpp/tests/103-garbage-after-else-0.c b/src/glsl/glcpp/tests/103-garbage-after-else-0.c
new file mode 100644
index 0000000..c460fea
--- /dev/null
+++ b/src/glsl/glcpp/tests/103-garbage-after-else-0.c
@@ -0,0 +1,3 @@ 
+#if 0
+#else garbage
+#endif
diff --git a/src/glsl/glcpp/tests/103-garbage-after-else-0.c.expected b/src/glsl/glcpp/tests/103-garbage-after-else-0.c.expected
new file mode 100644
index 0000000..f9f5f19
--- /dev/null
+++ b/src/glsl/glcpp/tests/103-garbage-after-else-0.c.expected
@@ -0,0 +1,4 @@ 
+0:2(7): preprocessor error: syntax error, unexpected IDENTIFIER, expecting NEWLINE
+0:1(7): preprocessor error: Unterminated #if
+
+
diff --git a/src/glsl/glcpp/tests/103-garbage-after-else.c b/src/glsl/glcpp/tests/103-garbage-after-else.c
deleted file mode 100644
index c460fea..0000000
--- a/src/glsl/glcpp/tests/103-garbage-after-else.c
+++ /dev/null
@@ -1,3 +0,0 @@ 
-#if 0
-#else garbage
-#endif
diff --git a/src/glsl/glcpp/tests/103-garbage-after-else.c.expected b/src/glsl/glcpp/tests/103-garbage-after-else.c.expected
deleted file mode 100644
index f9f5f19..0000000
--- a/src/glsl/glcpp/tests/103-garbage-after-else.c.expected
+++ /dev/null
@@ -1,4 +0,0 @@ 
-0:2(7): preprocessor error: syntax error, unexpected IDENTIFIER, expecting NEWLINE
-0:1(7): preprocessor error: Unterminated #if
-
-
diff --git a/src/glsl/glcpp/tests/123-garbage-after-else-1.c b/src/glsl/glcpp/tests/123-garbage-after-else-1.c
new file mode 100644
index 0000000..0b341a3
--- /dev/null
+++ b/src/glsl/glcpp/tests/123-garbage-after-else-1.c
@@ -0,0 +1,3 @@ 
+#if 1
+#else garbage
+#endif
diff --git a/src/glsl/glcpp/tests/123-garbage-after-else-1.c.expected b/src/glsl/glcpp/tests/123-garbage-after-else-1.c.expected
new file mode 100644
index 0000000..f9f5f19
--- /dev/null
+++ b/src/glsl/glcpp/tests/123-garbage-after-else-1.c.expected
@@ -0,0 +1,4 @@ 
+0:2(7): preprocessor error: syntax error, unexpected IDENTIFIER, expecting NEWLINE
+0:1(7): preprocessor error: Unterminated #if
+
+

Comments

On Thu, Jun 12, 2014 at 11:26 AM, Carl Worth <cworth@cworth.org> wrote:
> Previously, a line such as:
>
>         #else garbage
>
> would flag an error if it followed "#if 0", but not if it followed "#if 1".
>
> We fix this by setting a new bit of state (lexing_else) that allows the lexer
> to defer switching to the <SKIP> start state until after the NEWLINE following
> the #else directive.
>
> A new test case is added for:
>
>         #if 1
>         #else garbage
>         #endif
>
> which was untested before, (and did not generate the desired error).
> ---
>  src/glsl/glcpp/glcpp-lex.l                               | 10 +++++++++-
>  src/glsl/glcpp/glcpp-parse.y                             |  6 ++++--
>  src/glsl/glcpp/glcpp.h                                   |  1 +
>  src/glsl/glcpp/tests/103-garbage-after-else-0.c          |  3 +++
>  src/glsl/glcpp/tests/103-garbage-after-else-0.c.expected |  4 ++++
>  src/glsl/glcpp/tests/103-garbage-after-else.c            |  3 ---
>  src/glsl/glcpp/tests/103-garbage-after-else.c.expected   |  4 ----
>  src/glsl/glcpp/tests/123-garbage-after-else-1.c          |  3 +++
>  src/glsl/glcpp/tests/123-garbage-after-else-1.c.expected |  4 ++++
>  9 files changed, 28 insertions(+), 10 deletions(-)
>  create mode 100644 src/glsl/glcpp/tests/103-garbage-after-else-0.c
>  create mode 100644 src/glsl/glcpp/tests/103-garbage-after-else-0.c.expected
>  delete mode 100644 src/glsl/glcpp/tests/103-garbage-after-else.c
>  delete mode 100644 src/glsl/glcpp/tests/103-garbage-after-else.c.expected
>  create mode 100644 src/glsl/glcpp/tests/123-garbage-after-else-1.c
>  create mode 100644 src/glsl/glcpp/tests/123-garbage-after-else-1.c.expected
>
> diff --git a/src/glsl/glcpp/glcpp-lex.l b/src/glsl/glcpp/glcpp-lex.l
> index 188e454..dea6a6c 100644
> --- a/src/glsl/glcpp/glcpp-lex.l
> +++ b/src/glsl/glcpp/glcpp-lex.l
> @@ -125,7 +125,7 @@ HEXADECIMAL_INTEGER 0[xX][0-9a-fA-F]+[uU]?
>          * as set by the parser to know whether to change from INITIAL
>          * to SKIP or from SKIP back to INITIAL.
>          *
> -        * Three cases cause us to switch out of the SKIP state and
> +        * Four cases cause us to switch out of the SKIP state and
>          * back to the INITIAL state:
>          *
>          *      1. The top of the skip_stack is of type SKIP_NO_SKIP
> @@ -142,9 +142,17 @@ HEXADECIMAL_INTEGER        0[xX][0-9a-fA-F]+[uU]?
>          *         "#elif". Even inside an "#if 0" we need to lex this
>          *         expression so the parser can correctly update the
>          *         skip_stack state.
> +        *
> +        *      4. The lexing_else bit is set. This indicates that we
> +        *         are lexing tokens after #else and before the
> +        *         newline. We don't want to skip here just so that we
> +        *         can correctly flag errors if there is any
> +        *         garbage (other than comments) on the same line as the
> +        *         #else.
>          */
>         if (YY_START == INITIAL || YY_START == SKIP) {
>                 if (parser->lexing_if ||
> +                    parser->lexing_else ||

The preprocessor is one of the only bits that uses tabs. I'd rather we
stick with that, or convert the whole thing.

Otherwise, looks good to me.

Reviewed-by: Matt Turner <mattst88@gmail.com>