Re: [PATCH v3 04/12] OpenMP: C front end support for metadirectives

[Sandra Loosemore <sloosemore@baylibre.com>]

On 8/16/24 04:30, Jakub Jelinek wrote:
> On Sat, Jul 20, 2024 at 02:42:23PM -0600, Sandra Loosemore wrote:
>> --- a/gcc/c/c-parser.cc
>> +++ b/gcc/c/c-parser.cc
>> @@ -263,9 +263,24 @@ struct GTY(()) c_parser {
>>        otherwise NULL.  */
>>     vec<c_token, va_gc> *in_omp_attribute_pragma;
>>   
>> +  /* When in_omp_attribute_pragma is non-null, these fields save the values
>> +     of the tokens and tokens_avail fields, so that they can be restored
>> +     after parsing the attribute.  Note that parsing the body of a
>> +     metadirective uses its own save/restore mechanism as those can be
>> +     nested with or without the attribute pragmas in the body.  */
>> +    c_token * GTY((skip)) save_tokens;
>> +    unsigned int save_tokens_avail;
> 
> The indentation of the above 2 is wrong.
> Plus if those members are for metadirective parsing, their names are too
> generic.

They are not for metadirective parsing, they are to generalize the 
state-saving for OpenMP attribute-syntax directives that are converted 
to pragmas.  It's related to this patch hunk:

@@ -6846,7 +6884,6 @@ c_parser_handle_statement_omp_attributes (c_parser 
*parser
, tree &attrs,
      return false;

    unsigned int tokens_avail = parser->tokens_avail;
-  gcc_assert (parser->tokens == &parser->tokens_buf[0]);

    tokens++;
    vec<c_token, va_gc> *toks = NULL;

and the multiple instances like this:

@@ -1307,8 +1322,10 @@ c_parser_skip_until_found (c_parser *parser,
           c_token *token = c_parser_peek_token (parser);
           if (token->type == CPP_EOF)
             {
-             parser->tokens = &parser->tokens_buf[0];
-             parser->tokens_avail = token->flags;
+             parser->tokens = parser->save_tokens;
+             parser->save_tokens = NULL;
+             parser->tokens_avail = parser->save_tokens_avail;
+             parser->save_tokens_avail = 0;
               parser->in_omp_attribute_pragma = NULL;
             }
         }

where there presently is a hard-wired assumption that omp attribute 
pragma parsing is the *only* thing that messes with what parser->tokens 
points to.  Metadirectives need to maintain a separate token stream too, 
and attribute-syntax pragma directives can appear inside a 
metadirective, so fixing this somehow is necessary.  Due to the control 
flow of parsing the attribute-syntax pragmas it's not possible to save 
the state it needs in local variables on the stack, so hanging it off 
the parser object is about the only choice.

> But more importantly, for something parsed really rarely, wouldn't it be
> better to just add a single pointer to a new structure that contains
> all you need for metadirective parsing?

Do you want me to create separate new data structures for both the 
attribute-syntax pragma parsing, and the metadirective-specific bits? 
And maybe submit the attribute-syntax pragma parsing state changes as 
its own patch separate from the metadirective pieces?

-Sandra