[Mesa-dev] [PATCH] glcpp: fix #undef to match latest spec update and GLSLang implementation

[Iago Toral]

This didn't get any reviews, any takers? It fixes a couple of CTS tests
so it would be good to have it merged.

Iago

On Tue, 2017-05-30 at 13:25 +0200, Iago Toral Quiroga wrote:
> GLSL ES spec includes the following:
> 
>    "It is an error to undefine or to redefine a built-in
>     (pre-defined) macro name."
> 
> But desktop GLSL doesn't. This has sparked some discussion
> in Khronos, and the final conclusion was to update the
> GLSL 4.50 spec to include the following:
> 
>    "By convention, all macro names containing two consecutive
>     underscores ( __ ) are reserved for use by underlying
>     software layers.  Defining or undefining such a name in a
>     shader does not itself result in an error, but may result
>     in unintended behaviors that stem from having multiple
>     definitions of the same name.  All macro names prefixed
>     with “GL_” (“GL” followed by a single underscore) are also
>     reserved, and defining or undefining such a name results in
>     a compile-time error."
> 
> In other words, undefining GL_* names should be an error, but
> undefining other names with a double underscore in them is
> not strictly prohibited in desktop GLSL.
> 
> This patch fixes the preprocessor to apply these rules,
> following exactly the implementation already present
> in GLSLang. This fixes some tests in CTS.
> 
> Kronos bug:
> https://cvs.khronos.org/bugzilla/show_bug.cgi?id=16003
> 
> Fixes:
> KHR-
> GL45.shaders.preprocessor.definitions.undefine_core_profile_vertex
> KHR-
> GL45.shaders.preprocessor.definitions.undefine_core_profile_fragment
> ---
>  src/compiler/glsl/glcpp/glcpp-parse.y | 45 ++++++++++++++++++++++++-
> ----------
>  1 file changed, 31 insertions(+), 14 deletions(-)
> 
> diff --git a/src/compiler/glsl/glcpp/glcpp-parse.y
> b/src/compiler/glsl/glcpp/glcpp-parse.y
> index fe211a0..f1719f9 100644
> --- a/src/compiler/glsl/glcpp/glcpp-parse.y
> +++ b/src/compiler/glsl/glcpp/glcpp-parse.y
> @@ -287,24 +287,41 @@ control_line_success:
>                   * The GLSL ES 1.00 spec does not contain this text,
> but
>                   * dEQP's preprocess test in GLES2 checks for it.
>                   *
> -                 * Section 3.3 (Preprocessor) of the GLSL 1.30 spec
> says:
> +                 * Section 3.3 (Preprocessor) revision 7, of the
> GLSL 4.50
> +                 * spec says:
>                   *
> -                 *    #define and #undef functionality are defined
> as is
> -                 *    standard for C++ preprocessors for macro
> definitions
> -                 *    both with and without macro parameters.
> +                 *    By convention, all macro names containing two
> consecutive
> +                 *    underscores ( __ ) are reserved for use by
> underlying
> +                 *    software layers. Defining or undefining such a
> name
> +                 *    in a shader does not itself result in an
> error, but may
> +                 *    result in unintended behaviors that stem from
> having
> +                 *    multiple definitions of the same name. All
> macro names
> +                 *    prefixed with "GL_" (...) are also reseved,
> and defining
> +                 *    such a name results in a compile-time error.
>                   *
> -                 * At least as far as I can tell GCC allow '#undef
> __FILE__'.
> -                 * Furthermore, there are desktop OpenGL conformance
> tests
> -                 * that expect '#undef __VERSION__' and '#undef
> -                 * GL_core_profile' to work.
> +                 * The code below implements the same checks as
> GLSLang.
>                   */
> -		if (parser->is_gles &&
> -                    (strcmp("__LINE__", $3) == 0
> -                     || strcmp("__FILE__", $3) == 0
> -                     || strcmp("__VERSION__", $3) == 0
> -                     || strncmp("GL_", $3, 3) == 0))
> +		if (strncmp("GL_", $3, 3) == 0)
>  			glcpp_error(& @1, parser, "Built-in (pre-
> defined)"
> -				    " macro names cannot be
> undefined.");
> +				    " names beginning with GL_
> cannot be undefined.");
> +		else if (strstr($3, "__") != NULL) {
> +			if (parser->is_gles
> +			    && parser->version >= 300
> +			    && (strcmp("__LINE__", $3) == 0
> +				|| strcmp("__FILE__", $3) == 0
> +				|| strcmp("__VERSION__", $3) == 0))
> {
> +				glcpp_error(& @1, parser, "Built-in
> (pre-defined)"
> +					    " names cannot be
> undefined.");
> +			} else if (parser->is_gles && parser-
> >version <= 300) {
> +				glcpp_error(& @1, parser,
> +					    " names containing
> consecutive underscores"
> +					    " are reserved.");
> +			} else {
> +				glcpp_warning(& @1, parser,
> +					      " names containing
> consecutive underscores"
> +					      " are reserved.");
> +			}
> +		}
>  
>  		entry = _mesa_hash_table_search (parser->defines,
> $3);
>  		if (entry) {