From b34629f252da8079829d92eaef33837b46963636 Mon Sep 17 00:00:00 2001 From: Krzysztof Parzyszek Date: Thu, 18 Sep 2025 13:57:38 -0500 Subject: [PATCH 1/3] [flang][OpenMP] Reject blank common blocks more gracefully Parse them as "invalid" OmpObjects, then emit a diagnostic in semantic checks. --- flang/include/flang/Parser/dump-parse-tree.h | 2 + flang/include/flang/Parser/parse-tree.h | 9 +++- flang/lib/Parser/openmp-parsers.cpp | 7 ++- flang/lib/Parser/unparse.cpp | 20 +++++++-- flang/lib/Semantics/check-omp-loop.cpp | 5 ++- flang/lib/Semantics/check-omp-structure.cpp | 44 ++++++++++++------ flang/lib/Semantics/openmp-utils.cpp | 4 +- flang/lib/Semantics/resolve-directives.cpp | 27 +++++++---- flang/lib/Semantics/resolve-names.cpp | 45 +++++++++++-------- .../threadprivate-blank-common-block.f90 | 9 ---- .../Semantics/OpenMP/blank-common-block.f90 | 18 ++++++++ 11 files changed, 132 insertions(+), 58 deletions(-) delete mode 100644 flang/test/Parser/OpenMP/threadprivate-blank-common-block.f90 create mode 100644 flang/test/Semantics/OpenMP/blank-common-block.f90 diff --git a/flang/include/flang/Parser/dump-parse-tree.h b/flang/include/flang/Parser/dump-parse-tree.h index 1c9fd7673e06d..0b6e3fd6a3b6b 100644 --- a/flang/include/flang/Parser/dump-parse-tree.h +++ b/flang/include/flang/Parser/dump-parse-tree.h @@ -634,6 +634,8 @@ class ParseTreeDumper { NODE(parser, OmpNumTasksClause) NODE(OmpNumTasksClause, Modifier) NODE(parser, OmpObject) + NODE(OmpObject, Invalid) + NODE_ENUM(OmpObject::Invalid, Kind) NODE(parser, OmpObjectList) NODE(parser, OmpOrderClause) NODE(OmpOrderClause, Modifier) diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h index 7307283eb91ec..09a45476420df 100644 --- a/flang/include/flang/Parser/parse-tree.h +++ b/flang/include/flang/Parser/parse-tree.h @@ -3505,8 +3505,15 @@ struct OmpDirectiveName { // in slashes). An extended list item is a list item or a procedure Name. // variable-name | / common-block / | array-sections struct OmpObject { + // Blank common blocks are not valid objects. Parse them to emit meaningful + // diagnostics. + struct Invalid { + ENUM_CLASS(Kind, BlankCommonBlock); + WRAPPER_CLASS_BOILERPLATE(Invalid, Kind); + CharBlock source; + }; UNION_CLASS_BOILERPLATE(OmpObject); - std::variant u; + std::variant u; }; WRAPPER_CLASS(OmpObjectList, std::list); diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp index c6d4de108fb59..66526ba00b5ed 100644 --- a/flang/lib/Parser/openmp-parsers.cpp +++ b/flang/lib/Parser/openmp-parsers.cpp @@ -1024,8 +1024,11 @@ TYPE_PARSER(construct( maybe(nonemptyList(Parser{}) / ":"), scalarIntExpr)) -TYPE_PARSER( - construct(designator) || "/" >> construct(name) / "/") +TYPE_PARSER( // + construct(designator) || + "/" >> construct(name) / "/" || + construct(sourced(construct( + "//"_tok >> pure(OmpObject::Invalid::Kind::BlankCommonBlock))))) // OMP 5.0 2.19.4.5 LASTPRIVATE ([lastprivate-modifier :] list) TYPE_PARSER(construct( diff --git a/flang/lib/Parser/unparse.cpp b/flang/lib/Parser/unparse.cpp index 73bbbc04f46b1..189a34ee1dc56 100644 --- a/flang/lib/Parser/unparse.cpp +++ b/flang/lib/Parser/unparse.cpp @@ -2168,10 +2168,22 @@ class UnparseVisitor { void Unparse(const OmpContextSelectorSpecification &x) { Walk(x.v, ", "); } void Unparse(const OmpObject &x) { - common::visit(common::visitors{ - [&](const Designator &y) { Walk(y); }, - [&](const Name &y) { Put("/"), Walk(y), Put("/"); }, - }, + common::visit( // + common::visitors{ + [&](const Designator &y) { Walk(y); }, + [&](const Name &y) { + Put("/"); + Walk(y); + Put("/"); + }, + [&](const OmpObject::Invalid &y) { + switch (y.v) { + case OmpObject::Invalid::Kind::BlankCommonBlock: + Put("//"); + break; + } + }, + }, x.u); } void Unparse(const OmpDirectiveNameModifier &x) { diff --git a/flang/lib/Semantics/check-omp-loop.cpp b/flang/lib/Semantics/check-omp-loop.cpp index 562bd1b4e79a4..c9d0495850b6e 100644 --- a/flang/lib/Semantics/check-omp-loop.cpp +++ b/flang/lib/Semantics/check-omp-loop.cpp @@ -491,7 +491,10 @@ void OmpStructureChecker::Leave(const parser::OpenMPLoopConstruct &x) { checkReductionSymbolInScan(name); } }, - [&](const auto &name) { checkReductionSymbolInScan(&name); }, + [&](const parser::Name &name) { + checkReductionSymbolInScan(&name); + }, + [&](const parser::OmpObject::Invalid &invalid) {}, }, ompObj.u); } diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp index 4c7cd1734e0e7..1ee5385fb38a1 100644 --- a/flang/lib/Semantics/check-omp-structure.cpp +++ b/flang/lib/Semantics/check-omp-structure.cpp @@ -269,7 +269,8 @@ bool OmpStructureChecker::CheckAllowedClause(llvmOmpClause clause) { } void OmpStructureChecker::AnalyzeObject(const parser::OmpObject &object) { - if (std::holds_alternative(object.u)) { + if (std::holds_alternative(object.u) || + std::holds_alternative(object.u)) { // Do not analyze common block names. The analyzer will flag an error // on those. return; @@ -294,7 +295,12 @@ void OmpStructureChecker::AnalyzeObject(const parser::OmpObject &object) { } evaluate::ExpressionAnalyzer ea{context_}; auto restore{ea.AllowWholeAssumedSizeArray(true)}; - common::visit([&](auto &&s) { ea.Analyze(s); }, object.u); + common::visit( // + common::visitors{ + [&](auto &&s) { ea.Analyze(s); }, + [&](const parser::OmpObject::Invalid &invalid) {}, + }, + object.u); } void OmpStructureChecker::AnalyzeObjects(const parser::OmpObjectList &objects) { @@ -538,6 +544,7 @@ void OmpStructureChecker::CheckPredefinedAllocatorRestriction( [&](const parser::Name &name) { CheckPredefinedAllocatorRestriction(source, name); }, + [&](const parser::OmpObject::Invalid &invalid) {}, }, ompObject.u); } @@ -1302,7 +1309,11 @@ void OmpStructureChecker::CheckThreadprivateOrDeclareTargetVar( void OmpStructureChecker::CheckThreadprivateOrDeclareTargetVar( const parser::OmpObjectList &objList) { for (const auto &ompObject : objList.v) { - common::visit([&](auto &&s) { CheckThreadprivateOrDeclareTargetVar(s); }, + common::visit( // + common::visitors{ + [&](auto &&s) { CheckThreadprivateOrDeclareTargetVar(s); }, + [&](const parser::OmpObject::Invalid &invalid) {}, + }, ompObject.u); } } @@ -1434,8 +1445,14 @@ void OmpStructureChecker::Enter(const parser::OpenMPDepobjConstruct &x) { // refer to the same depend object as the depobj argument of the construct. if (clause.Id() == llvm::omp::Clause::OMPC_destroy) { auto getObjSymbol{[&](const parser::OmpObject &obj) { - return common::visit( - [&](auto &&s) { return GetLastName(s).symbol; }, obj.u); + return common::visit( // + common::visitors{ + [&](auto &&s) { return GetLastName(s).symbol; }, + [&](const parser::OmpObject::Invalid &invalid) { + return static_cast(nullptr); + }, + }, + obj.u); }}; auto getArgSymbol{[&](const parser::OmpArgument &arg) { if (auto *locator{std::get_if(&arg.u)}) { @@ -1450,9 +1467,7 @@ void OmpStructureChecker::Enter(const parser::OpenMPDepobjConstruct &x) { if (const std::optional &destroy{wrapper.v}) { const Symbol *constrSym{getArgSymbol(arguments.v.front())}; const Symbol *clauseSym{getObjSymbol(destroy->v)}; - assert(constrSym && "Unresolved depobj construct symbol"); - assert(clauseSym && "Unresolved destroy symbol on depobj construct"); - if (constrSym != clauseSym) { + if (constrSym && clauseSym && constrSym != clauseSym) { context_.Say(x.source, "The DESTROY clause must refer to the same object as the " "DEPOBJ construct"_err_en_US); @@ -1690,6 +1705,7 @@ void OmpStructureChecker::CheckSymbolNames( ContextDirectiveAsFortran()); } }, + [&](const parser::OmpObject::Invalid &invalid) {}, }, ompObject.u); } @@ -2710,6 +2726,7 @@ void OmpStructureChecker::Leave(const parser::OmpClauseList &) { } } }, + [&](const parser::OmpObject::Invalid &invalid) {}, }, ompObject.u); } @@ -3417,6 +3434,7 @@ void OmpStructureChecker::CheckVarIsNotPartOfAnotherVar( } }, [&](const parser::Name &name) {}, + [&](const parser::OmpObject::Invalid &invalid) {}, }, ompObject.u); } @@ -4102,11 +4120,11 @@ void OmpStructureChecker::CheckStructureComponent( }}; for (const auto &object : objects.v) { - common::visit( - common::visitors{ - CheckComponent, - [&](const parser::Name &name) {}, - }, + common::visit(common::visitors{ + CheckComponent, + [&](const parser::Name &name) {}, + [&](const parser::OmpObject::Invalid &invalid) {}, + }, object.u); } } diff --git a/flang/lib/Semantics/openmp-utils.cpp b/flang/lib/Semantics/openmp-utils.cpp index 2980f827d3ef3..e75149f21d117 100644 --- a/flang/lib/Semantics/openmp-utils.cpp +++ b/flang/lib/Semantics/openmp-utils.cpp @@ -225,7 +225,7 @@ struct ContiguousHelper { std::optional IsContiguous( SemanticsContext &semaCtx, const parser::OmpObject &object) { return common::visit( // - common::visitors{ + common::visitors{// [&](const parser::Name &x) { // Any member of a common block must be contiguous. return std::optional{true}; @@ -237,7 +237,7 @@ std::optional IsContiguous( } return std::optional{}; }, - }, + [&](const parser::OmpObject::Invalid &) { return std::nullopt; }}, object.u); } diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp index abb8f6430b29b..c2d1987f3ac91 100644 --- a/flang/lib/Semantics/resolve-directives.cpp +++ b/flang/lib/Semantics/resolve-directives.cpp @@ -3123,14 +3123,25 @@ void OmpAttributeVisitor::ResolveOmpCommonBlock( void OmpAttributeVisitor::ResolveOmpObject( const parser::OmpObject &ompObject, Symbol::Flag ompFlag) { - common::visit(common::visitors{ - [&](const parser::Designator &designator) { - ResolveOmpDesignator(designator, ompFlag); - }, - [&](const parser::Name &name) { // common block - ResolveOmpCommonBlock(name, ompFlag); - }, - }, + common::visit( // + common::visitors{ + [&](const parser::Designator &designator) { + ResolveOmpDesignator(designator, ompFlag); + }, + [&](const parser::Name &name) { // common block + ResolveOmpCommonBlock(name, ompFlag); + }, + [&](const parser::OmpObject::Invalid &invalid) { + switch (invalid.v) { + case parser::OmpObject::Invalid::Kind::BlankCommonBlock: + context_.Say(invalid.source, + "Blank common blocks are not allowed as directive or clause arguments"_err_en_US); + break; + default: + llvm_unreachable("Unexpected invalid object"); + } + }, + }, ompObject.u); } diff --git a/flang/lib/Semantics/resolve-names.cpp b/flang/lib/Semantics/resolve-names.cpp index cdd8d6ff2f60e..830891a222161 100644 --- a/flang/lib/Semantics/resolve-names.cpp +++ b/flang/lib/Semantics/resolve-names.cpp @@ -1625,25 +1625,34 @@ class OmpVisitor : public virtual DeclarationVisitor { void Post(const parser::OpenMPThreadprivate &) { SkipImplicitTyping(false); } bool Pre(const parser::OpenMPDeclareTargetConstruct &x) { const auto &spec{std::get(x.t)}; - auto populateDeclareTargetNames{ - [this](const parser::OmpObjectList &objectList) { - for (const auto &ompObject : objectList.v) { - common::visit( - common::visitors{ - [&](const parser::Designator &designator) { - if (const auto *name{ - semantics::getDesignatorNameIfDataRef( - designator)}) { - specPartState_.declareTargetNames.insert(name->source); - } - }, - [&](const parser::Name &name) { - specPartState_.declareTargetNames.insert(name.source); - }, + auto populateDeclareTargetNames{[this](const parser::OmpObjectList + &objectList) { + for (const auto &ompObject : objectList.v) { + common::visit( + common::visitors{ + [&](const parser::Designator &designator) { + if (const auto *name{ + semantics::getDesignatorNameIfDataRef(designator)}) { + specPartState_.declareTargetNames.insert(name->source); + } }, - ompObject.u); - } - }}; + [&](const parser::Name &name) { + specPartState_.declareTargetNames.insert(name.source); + }, + [&](const parser::OmpObject::Invalid &invalid) { + switch (invalid.v) { + case parser::OmpObject::Invalid::Kind::BlankCommonBlock: + context().Say(invalid.source, + "Blank common blocks are not allowed as directive or clause arguments"_err_en_US); + break; + default: + llvm_unreachable("Unexpected invalid object"); + } + }, + }, + ompObject.u); + } + }}; if (const auto *objectList{parser::Unwrap(spec.u)}) { populateDeclareTargetNames(*objectList); diff --git a/flang/test/Parser/OpenMP/threadprivate-blank-common-block.f90 b/flang/test/Parser/OpenMP/threadprivate-blank-common-block.f90 deleted file mode 100644 index 6317258e6ec8d..0000000000000 --- a/flang/test/Parser/OpenMP/threadprivate-blank-common-block.f90 +++ /dev/null @@ -1,9 +0,0 @@ -! RUN: not %flang_fc1 -fsyntax-only %s -fopenmp 2>&1 | FileCheck %s -! From Standard: A blank common block cannot appear in a threadprivate directive. - -program main - integer :: a - common//a - !CHECK: error: expected one of '$@ABCDEFGHIJKLMNOPQRSTUVWXYZ_' - !$omp threadprivate(//) - end diff --git a/flang/test/Semantics/OpenMP/blank-common-block.f90 b/flang/test/Semantics/OpenMP/blank-common-block.f90 new file mode 100644 index 0000000000000..4a217fced0ff7 --- /dev/null +++ b/flang/test/Semantics/OpenMP/blank-common-block.f90 @@ -0,0 +1,18 @@ +!RUN: %python %S/../test_errors.py %s %flang -fopenmp -fopenmp-version=60 + +module m + integer :: a + common // a + !ERROR: Blank common blocks are not allowed as directive or clause arguments + !$omp declare_target(//) + !ERROR: Blank common blocks are not allowed as directive or clause arguments + !$omp threadprivate(//) +end + +subroutine f00 + integer :: a + common // a + !ERROR: Blank common blocks are not allowed as directive or clause arguments + !$omp parallel shared(//) + !$omp end parallel +end From 105ca68c40f206a2cbeae669c4da9a4e9666abb4 Mon Sep 17 00:00:00 2001 From: Krzysztof Parzyszek Date: Thu, 18 Sep 2025 14:17:28 -0500 Subject: [PATCH 2/3] Handle switch-covers-all-cases errors --- flang/lib/Semantics/resolve-directives.cpp | 3 +-- flang/lib/Semantics/resolve-names.cpp | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp index c2d1987f3ac91..28c74b8c1908b 100644 --- a/flang/lib/Semantics/resolve-directives.cpp +++ b/flang/lib/Semantics/resolve-directives.cpp @@ -3133,12 +3133,11 @@ void OmpAttributeVisitor::ResolveOmpObject( }, [&](const parser::OmpObject::Invalid &invalid) { switch (invalid.v) { + SWITCH_COVERS_ALL_CASES case parser::OmpObject::Invalid::Kind::BlankCommonBlock: context_.Say(invalid.source, "Blank common blocks are not allowed as directive or clause arguments"_err_en_US); break; - default: - llvm_unreachable("Unexpected invalid object"); } }, }, diff --git a/flang/lib/Semantics/resolve-names.cpp b/flang/lib/Semantics/resolve-names.cpp index 830891a222161..e97f0bf02a515 100644 --- a/flang/lib/Semantics/resolve-names.cpp +++ b/flang/lib/Semantics/resolve-names.cpp @@ -1641,12 +1641,11 @@ class OmpVisitor : public virtual DeclarationVisitor { }, [&](const parser::OmpObject::Invalid &invalid) { switch (invalid.v) { + SWITCH_COVERS_ALL_CASES case parser::OmpObject::Invalid::Kind::BlankCommonBlock: context().Say(invalid.source, "Blank common blocks are not allowed as directive or clause arguments"_err_en_US); break; - default: - llvm_unreachable("Unexpected invalid object"); } }, }, From 991a38f2c0bc861f75d52d4c2d0cfa917ea3ef3f Mon Sep 17 00:00:00 2001 From: Krzysztof Parzyszek Date: Thu, 18 Sep 2025 14:42:10 -0500 Subject: [PATCH 3/3] fix MSVC build error --- flang/lib/Semantics/openmp-utils.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/flang/lib/Semantics/openmp-utils.cpp b/flang/lib/Semantics/openmp-utils.cpp index e75149f21d117..c62a1b33ed4e8 100644 --- a/flang/lib/Semantics/openmp-utils.cpp +++ b/flang/lib/Semantics/openmp-utils.cpp @@ -237,7 +237,9 @@ std::optional IsContiguous( } return std::optional{}; }, - [&](const parser::OmpObject::Invalid &) { return std::nullopt; }}, + [&](const parser::OmpObject::Invalid &) { + return std::optional{}; + }}, object.u); }