Hi, On Tue, 24 May 2022 16:15:31 +0200 Jakub Jelinek via Fortran wrote: > On Fri, Mar 18, 2022 at 09:26:47AM -0700, Julian Brown wrote: > > --- a/gcc/cp/parser.cc > > +++ b/gcc/cp/parser.cc > > @@ -4266,6 +4266,9 @@ cp_parser_new (cp_lexer *lexer) > > parser->omp_declare_simd = NULL; > > parser->oacc_routine = NULL; > > > > + /* Allow array slice in expression. */ > > Better /* Disallow OpenMP array sections in expressions. */ Fixed. > > + parser->omp_array_section_p = false; > > + > > /* Not declaring an implicit function template. */ > > parser->auto_is_implicit_function_template_parm_p = false; > > parser->fully_implicit_function_template_p = false; > > I think we should figure out when we should temporarily disable > parser->omp_array_section_p = false; > and restore it afterwards to a saved value. E.g. > cp_parser_lambda_expression seems like a good candidate, the fact that > OpenMP array sections are allowed say in map clause doesn't mean they > are allowed inside of lambdas and it would be especially hard when > the lambda is defining a separate function and the search for > OMP_ARRAY_SECTION probably wouldn't be able to discover those. > Other spots to consider might be statement expressions, perhaps type > definitions etc. I've had a go at doing this -- several expression types now forbid array-section syntax (see new "bad-array-section-*" tests added). I'm afraid my C++ isn't quite up to figuring out how it's possible to define a type inside an expression (inside a map clause) if we forbid lambdas and statement expressions though -- can you give an example? > > @@ -8021,6 +8024,7 @@ cp_parser_postfix_open_square_expression > > (cp_parser *parser, releasing_vec expression_list = NULL; > > location_t loc = cp_lexer_peek_token (parser->lexer)->location; > > bool saved_greater_than_is_operator_p; > > + bool saved_colon_corrects_to_scope_p; > > > > /* Consume the `[' token. */ > > cp_lexer_consume_token (parser->lexer); > > @@ -8028,6 +8032,9 @@ cp_parser_postfix_open_square_expression > > (cp_parser *parser, saved_greater_than_is_operator_p = > > parser->greater_than_is_operator_p; > > parser->greater_than_is_operator_p = true; > > + saved_colon_corrects_to_scope_p = > > parser->colon_corrects_to_scope_p; > > + parser->colon_corrects_to_scope_p = false; > > I think the last above line should be guarded on > if (parser->omp_array_section_p) > There is no reason to get worse diagnostics in non-OpenMP code or > even in OpenMP code where array sections aren't allowed. Fixed. > > + > > + /* NOTE: We are reusing using the type of the whole array as > > the type of > > + the array section here, which isn't necessarily entirely > > correct. > > + Might need revisiting. */ > > "reusing using" looks weird. > As for the type of OMP_ARRAY_SECTION trees, perhaps we could > initially use an incomplete array (so array element would be > meaningful) and when we figure out the details and the array section > is contiguous change its type to array type covering it. This version of the patch makes a best-effort attempt to create an exact-sized array type at parse time, else falls back to an incomplete array type if there are e.g. variable bounds. The type is essentially only used for diagnostics anyway, I think, so that should hopefully be good enough. > > + return build3_loc (input_location, OMP_ARRAY_SECTION, > > + TREE_TYPE (postfix_expression), > > + postfix_expression, index, length); > > + } > > + > > + parser->colon_corrects_to_scope_p = > > saved_colon_corrects_to_scope_p; + > > /* Look for the closing `]'. */ > > cp_parser_require (parser, CPP_CLOSE_SQUARE, RT_CLOSE_SQUARE); > > > > @@ -36536,7 +36570,7 @@ struct omp_dim > > static tree > > cp_parser_omp_var_list_no_open (cp_parser *parser, enum > > omp_clause_code kind, tree list, bool *colon, > > - bool allow_deref = false) > > + bool map_lvalue = false) > > { > > auto_vec dims; > > bool array_section_p; > > @@ -36547,12 +36581,95 @@ cp_parser_omp_var_list_no_open (cp_parser > > *parser, enum omp_clause_code kind, > > parser->colon_corrects_to_scope_p = false; *colon = false; > > } > > + begin_scope (sk_omp, NULL); > > Why? Base-language-wise, clauses don't introduce a new scope > for name-lookup. I think this was in aid of a particular test case (c-c++-common/gomp/map-6.c) that tests various bad usages of "always" and "close" modifiers, together with variables called literally "always" and "close". Parse failures during earlier tests could make later tests fail without the scope. I've moved the scope-creation to the appropriate caller. (Is there a better way? Discarding newly-created symbols on error, perhaps?) > And if it is really needed, I'd strongly prefer to either do it solely > for the clauses that might need it, or do begin_scope before first > such clause and finish at the end if it has been introduced. > > > while (1) > > { > > tree name, decl; > > > > if (kind == OMP_CLAUSE_DEPEND || kind == OMP_CLAUSE_AFFINITY) > > cp_parser_parse_tentatively (parser); > > + else if (map_lvalue && kind == OMP_CLAUSE_MAP) > > + { > > This shouldn't be done just for OMP_CLAUSE_MAP, but for all the > other clauses that accept array sections, including > OMP_CLAUSE_DEPEND, OMP_CLAUSE_AFFINITY, OMP_CLAUSE_MAP, OMP_CLAUSE_TO, > OMP_CLAUSE_FROM, OMP_CLAUSE_INCLUSIVE, OMP_CLAUSE_EXCLUSIVE, > OMP_CLAUSE_USE_DEVICE_ADDR, OMP_CLAUSE_HAS_DEVICE_ADDR, > OMP_CLAUSE_*REDUCTION. I'm not too sure about all of those -- Tobias points out that "INCLUSIVE", "EXCLUSIVE", *DEVICE* and *REDUCTION* take "variable list" item types, not "locator list", though sometimes with an array section being permitted (in OpenMP 5.2+). This version of the patch supports MAP, TO and FROM -- it might be possible to unify the parsing for DEPEND and AFFINITY too, maybe as a later patch. > And preferrably, they should be kept in the IL until > *finish_omp_clauses, which should handle those instead of TREE_LIST > that represented them before. The next patch makes that change. > Additionally, something should diagnose > incorrect uses of OMP_ARRAY_SECTION, which is everywhere in the > expressions but as the outermost node(s), i.e. for clauses that do > allow array sections scan OMP_CLAUSE_DECL after handling handleable > array sections and complain about embedded OMP_ARRAY_SECTION, > including OMP_ARRAY_SECTION say in the lower-bound, length and/or > stride expressions of the valid OMP_ARRAY_SECTION. This version of the patch handles low bound/length incorrectly being array sections, though no extra scan has been needed so far for that (I guess "handle_omp_array_sections" or address parsing in gimplify.cc can handle other cases that might arise). > For C++ that also means handling OMP_ARRAY_SECTION code in pt.c. The next patch handles that bit too. Tested (alongside next patch) with offloading to NVPTX -- with my previously-posted "address tokenization" patch also applied. OK? Thanks, Julian