[flang][OpenMP] Skip assertion while processing default clause on disallowed constructs

[NimishMishra]

Currently, the handling of default clause on directives enforces an assertion on whether default clause is allowed on the directive. This causes crashes when default is erroneously defined on a directive. This PR skips the assertion, as such semantic checks are handled elsewhere.

Fixes #93437

[llvmbot]

@llvm/pr-subscribers-flang-semantics

Author: None (NimishMishra)

Changes

Currently, the handling of default clause on directives enforces an assertion on whether default clause is allowed on the directive. This causes crashes when default is erroneously defined on a directive. This PR converts the assertion into a semantic failure.

Fixes #93437

---

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

1 Files Affected:

• (modified) flang/lib/Semantics/resolve-directives.cpp (+7-2)

diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index dbc531372c3f4..659f508e1c193 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -2109,8 +2109,13 @@ void OmpAttributeVisitor::Post(const parser::Name &name) {
           dirContext.defaultDSA == Symbol::Flag::OmpShared) {
         // 1) default
         // Allowed only with parallel, teams and task generating constructs.
-        assert(parallelDir || taskGenDir ||
-            llvm::omp::allTeamsSet.test(dirContext.directive));
+        if (!(parallelDir || taskGenDir ||
+                llvm::omp::allTeamsSet.test(dirContext.directive)))
+          context_.Say(dirContext.directiveSource,
+              "%s directive cannot have default clause"_err_en_US,
+              parser::ToUpperCaseLetters(
+                  llvm::omp::getOpenMPDirectiveName(dirContext.directive)
+                      .str()));
         if (dirContext.defaultDSA != Symbol::Flag::OmpShared)
           declNewSymbol(dirContext.defaultDSA);
         else

[llvmbot]

@llvm/pr-subscribers-flang-openmp

Author: None (NimishMishra)

Changes

Currently, the handling of default clause on directives enforces an assertion on whether default clause is allowed on the directive. This causes crashes when default is erroneously defined on a directive. This PR converts the assertion into a semantic failure.

Fixes #93437

---

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

1 Files Affected:

• (modified) flang/lib/Semantics/resolve-directives.cpp (+7-2)

diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index dbc531372c3f4..659f508e1c193 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -2109,8 +2109,13 @@ void OmpAttributeVisitor::Post(const parser::Name &name) {
           dirContext.defaultDSA == Symbol::Flag::OmpShared) {
         // 1) default
         // Allowed only with parallel, teams and task generating constructs.
-        assert(parallelDir || taskGenDir ||
-            llvm::omp::allTeamsSet.test(dirContext.directive));
+        if (!(parallelDir || taskGenDir ||
+                llvm::omp::allTeamsSet.test(dirContext.directive)))
+          context_.Say(dirContext.directiveSource,
+              "%s directive cannot have default clause"_err_en_US,
+              parser::ToUpperCaseLetters(
+                  llvm::omp::getOpenMPDirectiveName(dirContext.directive)
+                      .str()));
         if (dirContext.defaultDSA != Symbol::Flag::OmpShared)
           declNewSymbol(dirContext.defaultDSA);
         else

[NimishMishra]

Another probable way of handling this crash is to let the check for directives handle whether default is indeed allowed or now. During processing of a Name, we silently skip processing symbols (i.e. not emit any error).

Indeed, for the linked reproducer, should we silently skip without emitting any error, at a later point, another semantic error "DEFAULT clause is not allowed on the DO directive" is emitted.

[kiranchandramohan]

Isn't OMP.td responsible for what clauses are allowed on a directive? Could you add to the summary why this does not work for default in this case?

Is it possible to add this check in

llvm-project/flang/lib/Semantics/resolve-directives.cpp

Line 1907 in 7a28a5b

void OmpAttributeVisitor::Post(const parser::OmpDefaultClause &x) {

?

Can you add a test?

[NimishMishra]

Hi @kiranchandramohan, thanks for the review.

Yes indeed there is a semantic error in place. However, that is emitted after the assertion failure. That is why I was thinking that one way of working with this could be to silently return from this function (i.e. do not process default symbols) and let later checks emit the semantic error.

Currently, there are two semantic errors being emitted for the same violation.

[kiranchandramohan]

Yes indeed there is a semantic error in place. However, that is emitted after the assertion failure. That is why I was thinking that one way of working with this could be to silently return from this function (i.e. do not process default symbols) and let later checks emit the semantic error.

This is fine, if the later code emits all the errors caught by the check in this function.

[kiranchandramohan]

The error message has to improve.

[NimishMishra]

This message is coming from CheckAllowed

llvm-project/flang/lib/Semantics/check-directive-structure.h

Line 487 in cd9a02e

void DirectiveStructureChecker<D, C, PC, ClauseEnumSize>::CheckAllowed(

. The message needs to be generic, since this is shared across different directives. Do you have anything specific in mind which we could add? Should we need something specific to default, then it would be better to resort to emitting the error at the position where the previous commit of this PR was doing.

[kiranchandramohan]

Sorry, I was a bit confused by the presence of ordered and was thinking this is a restriction only if ordered is present. I see that this is a general constraint for the DO directive, so it is fine.

[kiranchandramohan]

LG.

[kiranchandramohan]

I think ordered is not necessary for this test.

[kiranchandramohan]

Sorry, I was a bit confused by the presence of ordered and was thinking this is a restriction only if ordered is present. I see that this is a general constraint for the DO directive, so it is fine.

[luporl]

LGTM, thanks for the fix.
I thought the check of whether a default clause is allowed on a directive would happen before the assert.

[NimishMishra]

Already fixed by #107586, hence closing.