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

[Samuel Pitoiset]

Looks good to me.

Reviewed-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>

On 05/30/2017 01:25 PM, 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) {
>