Skip to content

[clang][OpenMP] Fix error handling of the adjust_args clause #94696

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 24, 2024

Conversation

mikerice1969
Copy link
Contributor

Static verifier noticed the current code has logically dead code parsing the clause where IsComma is assigned. Fix this and improve the error message received when a bad adjust-op is specified.

This will now be handled like 'map' where a nice diagnostic is given with the correct values, then parsing continues on the next clause reducing unhelpful diagnostics.

Static verifier noticed the current code has logically dead code parsing
the clause where IsComma is assigned. Fix this and improve the error
message received when a bad adjust-op is specified.

This will now be handled like 'map' where a nice diagnostic is given
with the correct values, then parsing continues on the next clause
reducing unhelpful diagnostics.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:openmp OpenMP related changes to Clang labels Jun 6, 2024
@llvmbot
Copy link
Member

llvmbot commented Jun 6, 2024

@llvm/pr-subscribers-clang

Author: Mike Rice (mikerice1969)

Changes

Static verifier noticed the current code has logically dead code parsing the clause where IsComma is assigned. Fix this and improve the error message received when a bad adjust-op is specified.

This will now be handled like 'map' where a nice diagnostic is given with the correct values, then parsing continues on the next clause reducing unhelpful diagnostics.


Full diff: https://github.com/llvm/llvm-project/pull/94696.diff

3 Files Affected:

  • (modified) clang/include/clang/Basic/DiagnosticParseKinds.td (+2)
  • (modified) clang/lib/Parse/ParseOpenMP.cpp (+3-3)
  • (modified) clang/test/OpenMP/declare_variant_clauses_messages.cpp (+10)
diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td b/clang/include/clang/Basic/DiagnosticParseKinds.td
index d8c3fee7841f4..1160b0f7a7a5a 100644
--- a/clang/include/clang/Basic/DiagnosticParseKinds.td
+++ b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -1508,6 +1508,8 @@ def err_omp_unexpected_append_op : Error<
   "unexpected operation specified in 'append_args' clause, expected 'interop'">;
 def err_omp_unexpected_execution_modifier : Error<
   "unexpected 'execution' modifier in non-executable context">;
+def err_omp_unknown_adjust_args_op : Error<
+  "incorrect adjust_args type, expected 'need_device_ptr' or 'nothing'">;
 def err_omp_declare_variant_wrong_clause : Error<
   "expected %select{'match'|'match', 'adjust_args', or 'append_args'}0 clause "
   "on 'omp declare variant' directive">;
diff --git a/clang/lib/Parse/ParseOpenMP.cpp b/clang/lib/Parse/ParseOpenMP.cpp
index 50a872fedebf7..76d1854520382 100644
--- a/clang/lib/Parse/ParseOpenMP.cpp
+++ b/clang/lib/Parse/ParseOpenMP.cpp
@@ -4785,8 +4785,8 @@ bool Parser::ParseOpenMPVarList(OpenMPDirectiveKind DKind,
         getLangOpts());
     Data.ExtraModifierLoc = Tok.getLocation();
     if (Data.ExtraModifier == OMPC_ADJUST_ARGS_unknown) {
-      SkipUntil(tok::colon, tok::r_paren, tok::annot_pragma_openmp_end,
-                StopBeforeMatch);
+      Diag(Tok, diag::err_omp_unknown_adjust_args_op);
+      SkipUntil(tok::r_paren, tok::annot_pragma_openmp_end, StopBeforeMatch);
     } else {
       ConsumeToken();
       if (Tok.is(tok::colon))
@@ -4799,7 +4799,7 @@ bool Parser::ParseOpenMPVarList(OpenMPDirectiveKind DKind,
   bool IsComma =
       (Kind != OMPC_reduction && Kind != OMPC_task_reduction &&
        Kind != OMPC_in_reduction && Kind != OMPC_depend &&
-       Kind != OMPC_doacross && Kind != OMPC_map) ||
+       Kind != OMPC_doacross && Kind != OMPC_map && Kind != OMPC_adjust_args) ||
       (Kind == OMPC_reduction && !InvalidReductionId) ||
       (Kind == OMPC_map && Data.ExtraModifier != OMPC_MAP_unknown) ||
       (Kind == OMPC_depend && Data.ExtraModifier != OMPC_DEPEND_unknown) ||
diff --git a/clang/test/OpenMP/declare_variant_clauses_messages.cpp b/clang/test/OpenMP/declare_variant_clauses_messages.cpp
index 2a9e5385c9ca6..284e49bbd21b4 100644
--- a/clang/test/OpenMP/declare_variant_clauses_messages.cpp
+++ b/clang/test/OpenMP/declare_variant_clauses_messages.cpp
@@ -186,6 +186,16 @@ void vararg_bar2(const char *fmt) { return; }
 // expected-error@+1 {{variant in '#pragma omp declare variant' with type 'void (float *, float *, int *, omp_interop_t)' (aka 'void (float *, float *, int *, void *)') is incompatible with type 'void (float *, float *, int *)'}}
 #pragma omp declare variant(foo_v4) match(construct={dispatch})
 
+// expected-error@+3 {{incorrect adjust_args type, expected 'need_device_ptr' or 'nothing'}}
+#pragma omp declare variant(foo_v1)                        \
+   match(construct={dispatch}, device={arch(arm)})         \
+   adjust_args(badaaop:AAA,BBB)
+
+// expected-error@+3 {{incorrect adjust_args type, expected 'need_device_ptr' or 'nothing'}}
+#pragma omp declare variant(foo_v1)                        \
+   match(construct={dispatch}, device={arch(arm)})         \
+   adjust_args(badaaop AAA,BBB)
+
 #endif // _OPENMP >= 202011
 #if _OPENMP < 202011  // OpenMP 5.0 or lower
 // expected-error@+2 {{expected 'match' clause on 'omp declare variant' directive}}

Copy link
Contributor

@jyu2-git jyu2-git left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@mikerice1969 mikerice1969 merged commit 5413a2b into llvm:main Jun 24, 2024
11 checks passed
@mikerice1969 mikerice1969 deleted the logically-dead-clause-check branch June 24, 2024 20:45
AlexisPerry pushed a commit to llvm-project-tlp/llvm-project that referenced this pull request Jul 9, 2024
)

Static verifier noticed the current code has logically dead code parsing
the clause where IsComma is assigned. Fix this and improve the error
message received when a bad adjust-op is specified.

This will now be handled like 'map' where a nice diagnostic is given
with the correct values, then parsing continues on the next clause
reducing unhelpful diagnostics.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:openmp OpenMP related changes to Clang clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants