[Mesa-dev] [PATCH] RFC: glsl/glcpp: Allow for '#' characters to appear in shader body

[Kenneth Graunke]

On Thursday, July 31, 2014 11:22:59 AM Carl Worth wrote:
> Strictly speaking, the GLSL specification only allows for the '#' to appear in
> a shader in two places:
> 
> 	1. To introduce a pre-processing directive
> 
> 	   In this case, the '#' must be the first character on a line after
> 	   ignoring any comments and horizontal whitespace.
> 
> 	2. As part of the token-pasting operator
> 
> 	   In this case, the '#' must appear as part of the sequence "##" and
> 	   can only appear in the replacement list of a macro definition.
> 
> Since commit f062f0506a5b827667b7eb52136d8420b7e8113b, glcpp has been strict
> about '#' characters, such that any occurence outside of either of the above
> two cases would result in a syntax error of "Unexpected HASH_TOKEN" from the
> grammar.
> 
> (Prior to that commit, these illegal '#' would be silently ignored and printed
> to stdout. And if it hadn't been for that commit, these illegal '#' would have
> triggered internal errors with later commits.)
> 
> With this commit, glcpp relaxes its strictness and treats such appearances of
> '#' as just another piece of punctuation. This would make it legal to do
> something like:
> 
> 	#define FOO # something here
> 
> and then use "#ifdef FOO" and similar, (though if FOO were ever directly
> expanded, this should trigger a syntax error in the main GLSL compiler).
> 
> Relaxing strict conformance compared to GLSL seems somewhat dubious here, but
> might be justified for a few reasons:
> 
> 	1. GCC's preprocessor allows this, (though C is not GLSL, of course)
> 
> 	2. glcpp is generally soft about illegal characters, (compare to
> 	   non-ASCII characters which glcpp happily passes through, trusting
> 	   the GLSL compiler to catch them).
> 
> 	3. At least one game in the wild (Reactive Quake) has been observed
> 	   with a bogus '#' in it:
> 
> 		#define USE_DISCARD#line 0
> 
> 	   Note that the shader (that we've seen) with this line does not ever
> 	   *use* the USE_DISCARD macro. Note also that this shader is accepted
> 	   by both Nvidia's OpenGL implementation as well as Mesa prior to
> 	   the commit referenced above.
> ---
>  src/glsl/glcpp/glcpp-lex.l   | 12 +++++++-----
>  src/glsl/glcpp/glcpp-parse.y |  1 +
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/src/glsl/glcpp/glcpp-lex.l b/src/glsl/glcpp/glcpp-lex.l
> index c126850..cf517ad 100644
> --- a/src/glsl/glcpp/glcpp-lex.l
> +++ b/src/glsl/glcpp/glcpp-lex.l
> @@ -270,15 +270,17 @@ HEXADECIMAL_INTEGER	0[xX][0-9a-fA-F]+[uU]?
>  
>  	/* If the '#' is the first non-whitespace, non-comment token on this
>  	 * line, then it introduces a directive, switch to the <HASH> start
> -	 * condition.
> +	 * condition and return a HASH_TOKEN token.
>  	 *
> -	 * Otherwise, this is just punctuation, so return the HASH_TOKEN
> -         * token. */
> +	 * Otherwise, this is just punctuation, so just return a literal '#'
> +	 * token, (which will not be interpreted as a directive).
> +	 */
>  	if (parser->first_non_space_token_this_line) {
>  		BEGIN HASH;
> +		RETURN_TOKEN_NEVER_SKIP (HASH_TOKEN);
> +	} else {
> +		RETURN_TOKEN ('#');
>  	}
> -
> -	RETURN_TOKEN_NEVER_SKIP (HASH_TOKEN);
>  }
>  
>  <HASH>version{HSPACE}+ {
> diff --git a/src/glsl/glcpp/glcpp-parse.y b/src/glsl/glcpp/glcpp-parse.y
> index 4ee4110..b281b01 100644
> --- a/src/glsl/glcpp/glcpp-parse.y
> +++ b/src/glsl/glcpp/glcpp-parse.y
> @@ -744,6 +744,7 @@ operator:
>  |	PASTE			{ $$ = PASTE; }
>  |	PLUS_PLUS		{ $$ = PLUS_PLUS; }
>  |	MINUS_MINUS		{ $$ = MINUS_MINUS; }
> +|	'#'			{ $$ = '#'; }
>  ;
>  
>  %%

I agree that this is pretty bogus.  If the latest Reaction Quake still contains this shader, we should probably report a bug.  It's clearly broken - not what the application developer intended - and silently accepting the broken shader is not doing them any favors.  But, we should probably also pass through '#' to be safe.

How about emitting a warning in the RETURN_TOKEN ('#') case?

With or without that change (and I don't need to see a v2), this is:
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140804/b665a445/attachment-0001.sig>