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

[Jordan Justen]

On Sun, Jan 26, 2014 at 8:23 PM, Matt Turner <mattst88 at gmail.com> wrote:
> On Sun, Jan 26, 2014 at 8:15 PM, Jordan Justen <jljusten at gmail.com> wrote:
>> On Sun, Jan 26, 2014 at 6:14 PM, Matt Turner <mattst88 at gmail.com> 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() {}
>>>
>>> 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;
>>
>> Would this lead to the version being stuck at the implicit version
>> since parser->version_resolved would be set in the implicit case too?
>>
>> It looks like something like:
>> #define GL_ARB_sample_shading
>> #version 150
>>
>> ...might not end up hitting this code:
>>   if (version >= 150)
>>     add_builtin_define(parser, "GL_core_profile", 1);
>>
>> -Jordan
>
> That's intended. #version has to be the first non-comment token. If
> it's not, the version is 1.10 (or 1.00 in ES).

Series
Reviewed-by: Jordan Justen <jordan.l.justen at intel.com>