[Mesa-dev] [PATCH 1/2] glcpp: Check version_resolved in the proper place.

[Ian Romanick]

On 01/26/2014 07:14 PM, Matt Turner wrote:
> The check was in the wrong place, such that if a shader incorrectly put
> a preprocessor token before the #version declaration, the version would
> be resolved twice, leading to a segmentation fault when attempting to
> redefine the __VERSION__ macro.
> 
>  #define GL_ARB_sample_shading
>  #version 130
>  void main() {}

This is an invalid shader.  Segfaulting is bad, and fixing it is good.
Is the compile error still generated?

> Also, rename glcpp_parser_resolve_version to
>              glcpp_parser_resolve_implicit_version to avoid confusion.
> ---
> I couldn't come up with something better to clarify the naming. Feel free
> to suggest something better.
> 
>  src/glsl/glcpp/glcpp-parse.y | 22 +++++++++++-----------
>  src/glsl/glcpp/glcpp.h       |  2 +-
>  src/glsl/glcpp/pp.c          |  2 +-
>  3 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/src/glsl/glcpp/glcpp-parse.y b/src/glsl/glcpp/glcpp-parse.y
> index 184e5c2..efcf139 100644
> --- a/src/glsl/glcpp/glcpp-parse.y
> +++ b/src/glsl/glcpp/glcpp-parse.y
> @@ -194,7 +194,7 @@ line:
>  		ralloc_asprintf_rewrite_tail (&parser->output, &parser->output_length, "\n");
>  	}
>  |	HASH_LINE {
> -		glcpp_parser_resolve_version(parser);
> +		glcpp_parser_resolve_implicit_version(parser);
>  	} pp_tokens NEWLINE {
>  
>  		if (parser->skip_stack == NULL ||
> @@ -254,10 +254,10 @@ define:
>  
>  control_line:
>  	HASH_DEFINE {
> -		glcpp_parser_resolve_version(parser);
> +		glcpp_parser_resolve_implicit_version(parser);
>  	} define
>  |	HASH_UNDEF {
> -		glcpp_parser_resolve_version(parser);
> +		glcpp_parser_resolve_implicit_version(parser);
>  	} IDENTIFIER NEWLINE {
>  		macro_t *macro = hash_table_find (parser->defines, $3);
>  		if (macro) {
> @@ -267,7 +267,7 @@ control_line:
>  		ralloc_free ($3);
>  	}
>  |	HASH_IF {
> -		glcpp_parser_resolve_version(parser);
> +		glcpp_parser_resolve_implicit_version(parser);
>  	} conditional_tokens NEWLINE {
>  		/* Be careful to only evaluate the 'if' expression if
>  		 * we are not skipping. When we are skipping, we
> @@ -299,14 +299,14 @@ control_line:
>  		_glcpp_parser_skip_stack_push_if (parser, & @1, 0);
>  	}
>  |	HASH_IFDEF {
> -		glcpp_parser_resolve_version(parser);
> +		glcpp_parser_resolve_implicit_version(parser);
>  	} IDENTIFIER junk NEWLINE {
>  		macro_t *macro = hash_table_find (parser->defines, $3);
>  		ralloc_free ($3);
>  		_glcpp_parser_skip_stack_push_if (parser, & @1, macro != NULL);
>  	}
>  |	HASH_IFNDEF {
> -		glcpp_parser_resolve_version(parser);
> +		glcpp_parser_resolve_implicit_version(parser);
>  	} IDENTIFIER junk NEWLINE {
>  		macro_t *macro = hash_table_find (parser->defines, $3);
>  		ralloc_free ($3);
> @@ -380,7 +380,7 @@ control_line:
>  		_glcpp_parser_handle_version_declaration(parser, $2, $3, true);
>  	}
>  |	HASH NEWLINE {
> -		glcpp_parser_resolve_version(parser);
> +		glcpp_parser_resolve_implicit_version(parser);
>  	}
>  ;
>  
> @@ -2024,6 +2024,9 @@ _glcpp_parser_handle_version_declaration(glcpp_parser_t *parser, intmax_t versio
>  {
>  	const struct gl_extensions *extensions = parser->extensions;
>  
> +	if (parser->version_resolved)
> +		return;
> +
>  	parser->version_resolved = true;
>  
>  	add_builtin_define (parser, "__VERSION__", version);
> @@ -2144,11 +2147,8 @@ _glcpp_parser_handle_version_declaration(glcpp_parser_t *parser, intmax_t versio
>  #define IMPLICIT_GLSL_VERSION 110
>  
>  void
> -glcpp_parser_resolve_version(glcpp_parser_t *parser)
> +glcpp_parser_resolve_implicit_version(glcpp_parser_t *parser)
>  {
> -	if (parser->version_resolved)
> -		return;
> -
>  	_glcpp_parser_handle_version_declaration(parser, IMPLICIT_GLSL_VERSION,
>  						 NULL, false);
>  }
> diff --git a/src/glsl/glcpp/glcpp.h b/src/glsl/glcpp/glcpp.h
> index 4aa200a..9d85c16 100644
> --- a/src/glsl/glcpp/glcpp.h
> +++ b/src/glsl/glcpp/glcpp.h
> @@ -203,7 +203,7 @@ void
>  glcpp_parser_destroy (glcpp_parser_t *parser);
>  
>  void
> -glcpp_parser_resolve_version(glcpp_parser_t *parser);
> +glcpp_parser_resolve_implicit_version(glcpp_parser_t *parser);
>  
>  int
>  glcpp_preprocess(void *ralloc_ctx, const char **shader, char **info_log,
> diff --git a/src/glsl/glcpp/pp.c b/src/glsl/glcpp/pp.c
> index 637a58f..fc645f7 100644
> --- a/src/glsl/glcpp/pp.c
> +++ b/src/glsl/glcpp/pp.c
> @@ -151,7 +151,7 @@ glcpp_preprocess(void *ralloc_ctx, const char **shader, char **info_log,
>  	if (parser->skip_stack)
>  		glcpp_error (&parser->skip_stack->loc, parser, "Unterminated #if\n");
>  
> -	glcpp_parser_resolve_version(parser);
> +	glcpp_parser_resolve_implicit_version(parser);
>  
>  	ralloc_strcat(info_log, parser->info_log);
>  
>