diff mbox series

openmp: Improve handling of nested OpenMP metadirectives in C and C++ (was: Re: [PATCH 1/7] openmp: Add C support for parsing metadirectives)

Message ID 0090d882-4c17-3c12-ab54-54670bf32ab0@codesourcery.com
State New
Headers
Series openmp: Improve handling of nested OpenMP metadirectives in C and C++ (was: Re: [PATCH 1/7] openmp: Add C support for parsing metadirectives) |

Commit Message

Kwok Cheung Yeung Feb. 18, 2022, 9:09 p.m. UTC 
  This patch (to be applied on top of the metadirective patch series) 
addresses issues found in the C/C++ parsers when nested metadirectives 
are used.

analyze_metadirective_body when encountering code like:

#pragma omp metadirective when {set={...}: A)
   #pragma omp metadirective when (set={...}: B)

would stop just before ': B' before it naively assumes that the '}' 
marks the end of the body associated with the first metadirective, when 
it needs to include the whole of the second metadirective plus its 
associated body. This is fixed by checking that the nesting level of 
parentheses is zero as well before stopping the gathering of tokens.

The assert on the remaining tokens after parsing a clause can fail 
(resulting in an ICE) if there is a parse error in the directive or the 
body, since in that case not all tokens may be processed before parsing 
aborts early. The assert is therefore not enforced if any parse errors 
occur in the clause.

I have also moved the handling of the metadirective pragma from 
c_parser_omp_construct to c_parser_pragma (and their C++ equivalents), 
since c_parser_omp_construct has some checks that do not apply to 
metadirectives.

Kwok
From a9e4936b8476b97f11bb81b416ef3d28fa60cd37 Mon Sep 17 00:00:00 2001
From: Kwok Cheung Yeung <kcy@codesourcery.com>
Date: Fri, 18 Feb 2022 19:00:57 +0000
Subject: [PATCH] openmp: Improve handling of nested OpenMP metadirectives in C
 and C++

This patch fixes a misparsing issue when encountering code like:

  #pragma omp metadirective when {<selector_set>={...}: A)
    #pragma omp metadirective when (<selector_set>={...}: B)

When called for the first metadirective, analyze_metadirective_body would
stop just before the colon in the second metadirective because it naively
assumes that the '}' marks the end of a code block.

The assertion for clauses to end parsing at the same point is now disabled
if a parse error has occurred during the parsing of the clause, since some
tokens may not be consumed if a parse error cuts parsing short.

2022-02-18  Kwok Cheung Yeung  <kcy@codesourcery.com>

	gcc/c/
	* c-parser.cc (c_parser_omp_construct): Move handling of
	PRAGMA_OMP_METADIRECTIVE from here...
	(c_parser_pragma): ...to here.
	(analyze_metadirective_body): Check that the bracket nesting level
	is also zero before stopping the adding of tokens on encountering a
	close brace.
	(c_parser_omp_metadirective): Modify function signature and update.
	Do not assert on remaining tokens if there has been a parse error.

	gcc/cp/
	* parser.cc (cp_parser_omp_construct): Move handling of
	PRAGMA_OMP_METADIRECTIVE from here...
	(cp_parser_pragma): ...to here.
	(analyze_metadirective_body): Check that the bracket
	nesting level is also zero before stopping the adding of tokens on
	encountering a close brace.
	(cp_parser_omp_metadirective): Modify function signature and update.
	Do not assert on remaining tokens if there has been a parse error.

	gcc/testsuite/
	* c-c++-common/gomp/metadirective-1.c (f): Add test for
	improperly nested metadirectives.
---
 gcc/c/c-parser.cc                             | 47 +++++++++----------
 gcc/cp/parser.cc                              | 33 ++++++-------
 .../c-c++-common/gomp/metadirective-1.c       | 13 +++++
 3 files changed, 51 insertions(+), 42 deletions(-)

Comments

Kwok Cheung Yeung Feb. 18, 2022, 9:26 p.m. UTC | #1 
This patch has been committed to the devel/omp/gcc-11 development branch:

249df772b70f7b9f50f68030d4ea9c25624cc578  openmp: Improve handling of 
nested OpenMP metadirectives in C and C++

Kwok
diff mbox series

Patch

diff --git a/gcc/c/c-parser.cc b/gcc/c/c-parser.cc
index 58fcbb398ee..6a134e0fb50 100644
--- a/gcc/c/c-parser.cc
+++ b/gcc/c/c-parser.cc
@@ -1592,6 +1592,7 @@  static void c_parser_omp_taskwait (c_parser *);
 static void c_parser_omp_taskyield (c_parser *);
 static void c_parser_omp_cancel (c_parser *);
 static void c_parser_omp_nothing (c_parser *);
+static void c_parser_omp_metadirective (c_parser *, bool *);
 
 enum pragma_context { pragma_external, pragma_struct, pragma_param,
 		      pragma_stmt, pragma_compound };
@@ -1600,8 +1601,6 @@  static bool c_parser_omp_cancellation_point (c_parser *, enum pragma_context);
 static bool c_parser_omp_target (c_parser *, enum pragma_context, bool *);
 static void c_parser_omp_end_declare_target (c_parser *);
 static bool c_parser_omp_declare (c_parser *, enum pragma_context);
-static tree c_parser_omp_metadirective (location_t, c_parser *, char *,
-					omp_clause_mask, tree *, bool *);
 static void c_parser_omp_requires (c_parser *);
 static bool c_parser_omp_error (c_parser *, enum pragma_context);
 static bool c_parser_omp_ordered (c_parser *, enum pragma_context, bool *);
@@ -12551,6 +12550,10 @@  c_parser_pragma (c_parser *parser, enum pragma_context context, bool *if_p)
       c_parser_omp_nothing (parser);
       return false;
 
+    case PRAGMA_OMP_METADIRECTIVE:
+      c_parser_omp_metadirective (parser, if_p);
+      return true;
+
     case PRAGMA_OMP_ERROR:
       return c_parser_omp_error (parser, context);
 
@@ -23020,7 +23023,7 @@  analyze_metadirective_body (c_parser *parser,
 	  ++nesting_depth;
 	  goto add;
 	case CPP_CLOSE_BRACE:
-	  if (--nesting_depth == 0)
+	  if (--nesting_depth == 0 && bracket_depth == 0)
 	    stop = true;
 	  goto add;
 	case CPP_OPEN_PAREN:
@@ -23058,10 +23061,8 @@  analyze_metadirective_body (c_parser *parser,
   # pragma omp metadirective [clause[, clause]]
 */
 
-static tree
-c_parser_omp_metadirective (location_t loc, c_parser *parser,
-			    char *p_name, omp_clause_mask, tree *,
-			    bool *if_p)
+static void
+c_parser_omp_metadirective (c_parser *parser, bool *if_p)
 {
   tree ret;
   auto_vec<c_token> directive_tokens;
@@ -23073,13 +23074,14 @@  c_parser_omp_metadirective (location_t loc, c_parser *parser,
   bool default_seen = false;
   int directive_token_idx = 0;
   tree standalone_body = NULL_TREE;
+  location_t pragma_loc = c_parser_peek_token (parser)->location;
 
   ret = make_node (OMP_METADIRECTIVE);
-  SET_EXPR_LOCATION (ret, loc);
+  SET_EXPR_LOCATION (ret, pragma_loc);
   TREE_TYPE (ret) = void_type_node;
   OMP_METADIRECTIVE_CLAUSES (ret) = NULL_TREE;
-  strcat (p_name, " metadirective");
 
+  c_parser_consume_pragma (parser);
   while (c_parser_next_token_is_not (parser, CPP_PRAGMA_EOL))
     {
       if (c_parser_next_token_is_not (parser, CPP_NAME)
@@ -23287,6 +23289,7 @@  c_parser_omp_metadirective (location_t loc, c_parser *parser,
       parser->tokens = tokens.address ();
       parser->tokens_avail = tokens.length ();
 
+      int prev_errorcount = errorcount;
       tree directive = c_begin_compound_stmt (true);
 
       /* Declare all non-local labels that occur within the directive body
@@ -23296,11 +23299,11 @@  c_parser_omp_metadirective (location_t loc, c_parser *parser,
 	  tree label = declare_label (body_labels[j]);
 
 	  C_DECLARED_LABEL_FLAG (label) = 1;
-	  add_stmt (build_stmt (loc, DECL_EXPR, label));
+	  add_stmt (build_stmt (pragma_loc, DECL_EXPR, label));
 	}
 
       c_parser_pragma (parser, pragma_compound, if_p);
-      directive = c_end_compound_stmt (loc, directive, true);
+      directive = c_end_compound_stmt (pragma_loc, directive, true);
       bool standalone_p
 	= directives[i]->kind == C_OMP_DIR_STANDALONE
 	  || directives[i]->kind == C_OMP_DIR_UTILITY;
@@ -23323,10 +23326,14 @@  c_parser_omp_metadirective (location_t loc, c_parser *parser,
       OMP_METADIRECTIVE_CLAUSES (ret)
 	= chainon (OMP_METADIRECTIVE_CLAUSES (ret), variant);
 
-      /* Check that all valid tokens have been consumed.  */
-      gcc_assert (parser->tokens_avail == 2);
-      gcc_assert (c_parser_next_token_is (parser, CPP_EOF));
-      gcc_assert (c_parser_peek_2nd_token (parser)->type == CPP_EOF);
+      /* Check that all valid tokens have been consumed if no parse errors
+	 encountered.  */
+      if (errorcount == prev_errorcount)
+	{
+	  gcc_assert (parser->tokens_avail == 2);
+	  gcc_assert (c_parser_next_token_is (parser, CPP_EOF));
+	  gcc_assert (c_parser_peek_2nd_token (parser)->type == CPP_EOF);
+	}
 
       parser->tokens = old_tokens;
       parser->tokens_avail = old_tokens_avail;
@@ -23338,15 +23345,12 @@  c_parser_omp_metadirective (location_t loc, c_parser *parser,
     ret = c_omp_expand_metadirective (candidates);
 
   add_stmt (ret);
-
-  return ret;
+  return;
 
 error:
   if (parser->in_pragma)
     c_parser_skip_to_pragma_eol (parser);
   c_parser_skip_to_end_of_block_or_statement (parser);
-
-  return NULL_TREE;
 }
 
 /* Main entry point to parsing most OpenMP pragmas.  */
@@ -23422,11 +23426,6 @@  c_parser_omp_construct (c_parser *parser, bool *if_p)
       strcpy (p_name, "#pragma omp");
       stmt = c_parser_omp_master (loc, parser, p_name, mask, NULL, if_p);
       break;
-    case PRAGMA_OMP_METADIRECTIVE:
-      strcpy (p_name, "#pragma omp");
-      stmt = c_parser_omp_metadirective (loc, parser, p_name, mask, NULL,
-					 if_p);
-      break;
     case PRAGMA_OMP_PARALLEL:
       strcpy (p_name, "#pragma omp");
       stmt = c_parser_omp_parallel (loc, parser, p_name, mask, NULL, if_p);
diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
index aa23688814a..ef33cb1611f 100644
--- a/gcc/cp/parser.cc
+++ b/gcc/cp/parser.cc
@@ -45985,7 +45985,7 @@  cp_parser_omp_end_declare_target (cp_parser *parser, cp_token *pragma_tok)
 }
 
 
-/* Helper function for c_parser_omp_metadirective.  */
+/* Helper function for cp_parser_omp_metadirective.  */
 
 static void
 analyze_metadirective_body (cp_parser *parser,
@@ -46021,7 +46021,7 @@  analyze_metadirective_body (cp_parser *parser,
 	  ++nesting_depth;
 	  goto add;
 	case CPP_CLOSE_BRACE:
-	  if (--nesting_depth == 0)
+	  if (--nesting_depth == 0 && bracket_depth == 0)
 	    stop = true;
 	  goto add;
 	case CPP_OPEN_PAREN:
@@ -46056,9 +46056,8 @@  analyze_metadirective_body (cp_parser *parser,
   # pragma omp metadirective [clause[, clause]]
 */
 
-static tree
+static void
 cp_parser_omp_metadirective (cp_parser *parser, cp_token *pragma_tok,
-			     char *p_name, omp_clause_mask, tree *,
 			     bool *if_p)
 {
   tree ret;
@@ -46069,15 +46068,14 @@  cp_parser_omp_metadirective (cp_parser *parser, cp_token *pragma_tok,
   auto_vec<tree> ctxs;
   bool default_seen = false;
   int directive_token_idx = 0;
-  location_t loc = cp_lexer_peek_token (parser->lexer)->location;
+  location_t pragma_loc = pragma_tok->location;
   tree standalone_body = NULL_TREE;
   vec<struct omp_metadirective_variant> candidates;
 
   ret = make_node (OMP_METADIRECTIVE);
-  SET_EXPR_LOCATION (ret, loc);
+  SET_EXPR_LOCATION (ret, pragma_loc);
   TREE_TYPE (ret) = void_type_node;
   OMP_METADIRECTIVE_CLAUSES (ret) = NULL_TREE;
-  strcat (p_name, " metadirective");
 
   while (cp_lexer_next_token_is_not (parser->lexer, CPP_PRAGMA_EOL))
     {
@@ -46296,6 +46294,7 @@  cp_parser_omp_metadirective (cp_parser *parser, cp_token *pragma_tok,
       parser->lexer = lexer;
       cp_lexer_set_source_position_from_token (lexer->next_token);
 
+      int prev_errorcount = errorcount;
       tree directive = push_stmt_list ();
       tree directive_stmt = begin_compound_stmt (0);
 
@@ -46330,8 +46329,10 @@  cp_parser_omp_metadirective (cp_parser *parser, cp_token *pragma_tok,
       OMP_METADIRECTIVE_CLAUSES (ret)
 	= chainon (OMP_METADIRECTIVE_CLAUSES (ret), variant);
 
-      /* Check that all valid tokens have been consumed.  */
-      gcc_assert (cp_lexer_next_token_is (parser->lexer, CPP_EOF));
+      /* Check that all valid tokens have been consumed if no parse errors
+	 encountered.  */
+      gcc_assert (errorcount != prev_errorcount
+		  || cp_lexer_next_token_is (parser->lexer, CPP_EOF));
 
       parser->lexer = old_lexer;
       cp_lexer_set_source_position_from_token (old_lexer->next_token);
@@ -46343,8 +46344,7 @@  cp_parser_omp_metadirective (cp_parser *parser, cp_token *pragma_tok,
     ret = c_omp_expand_metadirective (candidates);
 
   add_stmt (ret);
-
-  return ret;
+  return;
 
 fail:
   /* Skip the metadirective pragma.  */
@@ -46352,7 +46352,6 @@  fail:
 
   /* Skip the metadirective body.  */
   cp_parser_skip_to_end_of_block_or_statement (parser);
-  return error_mark_node;
 }
 
 
@@ -47602,11 +47601,6 @@  cp_parser_omp_construct (cp_parser *parser, cp_token *pragma_tok, bool *if_p)
       stmt = cp_parser_omp_master (parser, pragma_tok, p_name, mask, NULL,
 				   if_p);
       break;
-    case PRAGMA_OMP_METADIRECTIVE:
-      strcpy (p_name, "#pragma omp");
-      stmt = cp_parser_omp_metadirective (parser, pragma_tok, p_name, mask,
-					  NULL, if_p);
-      break;
     case PRAGMA_OMP_PARALLEL:
       strcpy (p_name, "#pragma omp");
       stmt = cp_parser_omp_parallel (parser, pragma_tok, p_name, mask, NULL,
@@ -48257,7 +48251,6 @@  cp_parser_pragma (cp_parser *parser, enum pragma_context context, bool *if_p)
     case PRAGMA_OMP_LOOP:
     case PRAGMA_OMP_MASKED:
     case PRAGMA_OMP_MASTER:
-    case PRAGMA_OMP_METADIRECTIVE:
     case PRAGMA_OMP_PARALLEL:
     case PRAGMA_OMP_SCOPE:
     case PRAGMA_OMP_SECTIONS:
@@ -48289,6 +48282,10 @@  cp_parser_pragma (cp_parser *parser, enum pragma_context context, bool *if_p)
       cp_parser_omp_nothing (parser, pragma_tok);
       return false;
 
+    case PRAGMA_OMP_METADIRECTIVE:
+      cp_parser_omp_metadirective (parser, pragma_tok, if_p);
+      return true;
+
     case PRAGMA_OMP_ERROR:
       return cp_parser_omp_error (parser, pragma_tok, context);
 
diff --git a/gcc/testsuite/c-c++-common/gomp/metadirective-1.c b/gcc/testsuite/c-c++-common/gomp/metadirective-1.c
index 72cf0abbbd7..543063a3324 100644
--- a/gcc/testsuite/c-c++-common/gomp/metadirective-1.c
+++ b/gcc/testsuite/c-c++-common/gomp/metadirective-1.c
@@ -4,6 +4,8 @@ 
 
 void f (int a[], int b[], int c[])
 {
+  int i;
+
   #pragma omp metadirective \
       default (teams loop) \
       default (parallel loop) /* { dg-error "there can only be one default clause in a metadirective before '\\(' token" } */
@@ -26,4 +28,15 @@  void f (int a[], int b[], int c[])
   #pragma omp metadirective \
 	default (metadirective default (flush))	/* { dg-error "metadirectives cannot be used as directive variants before 'default'" } */
     for (i = 0; i < N; i++) c[i] = a[i] * b[i];
+
+  /* Test improperly nested metadirectives - even though the second
+     metadirective resolves to 'omp nothing', that is not the same as there
+     being literally nothing there.  */
+  #pragma omp metadirective \
+      when (implementation={vendor("gnu")}: parallel for)
+    #pragma omp metadirective \
+	when (implementation={vendor("cray")}: parallel for)
+	/* { dg-error "for statement expected before '#pragma'" "" { target c } .-2 } */
+	/* { dg-error "'#pragma' is not allowed here" "" { target c++ } .-3 } */
+      for (i = 0; i < N; i++) c[i] = a[i] * b[i];
 }