[Flang][OpenMP] Prevent ICE for certain constructs in unnamed programs

[skatrak]

This patch fixes #72748 by modifying the processing of program units to search for a symbol to which OpenMP REQUIRES clauses can bind to. Rather than picking up the first PFT node with a source reference and getting its associated scope, it picks up the last one.

This avoids using the source from the first specification construct of an nameless program, which can sometimes not be associated to any scope, causing an ICE due to an invalid source location.

[llvmbot]

@llvm/pr-subscribers-flang-semantics

Author: Sergio Afonso (skatrak)

Changes

This patch fixes #72748 by modifying the processing of program units to search for a symbol to which OpenMP REQUIRES clauses can bind to. Rather than picking up the first PFT node with a source reference and getting its associated scope, it picks up the last one.

This avoids using the source from the first specification construct of an nameless program, which can sometimes not be associated to any scope, causing an ICE due to an invalid source location.

---

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

2 Files Affected:

• (modified) flang/lib/Semantics/resolve-directives.cpp (+1-1)
• (added) flang/test/Semantics/OpenMP/struct.f90 (+7)

diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index da6c865ad56a3b1..9084bffc7139869 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -25,7 +25,7 @@
 template <typename T>
 static Fortran::semantics::Scope *GetScope(
     Fortran::semantics::SemanticsContext &context, const T &x) {
-  std::optional<Fortran::parser::CharBlock> source{GetSource(x)};
+  std::optional<Fortran::parser::CharBlock> source{GetLastSource(x)};
   return source ? &context.FindScope(*source) : nullptr;
 }
 
diff --git a/flang/test/Semantics/OpenMP/struct.f90 b/flang/test/Semantics/OpenMP/struct.f90
new file mode 100644
index 000000000000000..8ae1fbe4da86f93
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/struct.f90
@@ -0,0 +1,7 @@
+! RUN: %python %S/../test_errors.py %s %flang_fc1 -fopenmp
+! Check OpenMP compatibility with the DEC STRUCTURE extension
+
+structure /s/
+end structure
+
+end

[llvmbot]

@llvm/pr-subscribers-flang-openmp

Author: Sergio Afonso (skatrak)

Changes

This patch fixes #72748 by modifying the processing of program units to search for a symbol to which OpenMP REQUIRES clauses can bind to. Rather than picking up the first PFT node with a source reference and getting its associated scope, it picks up the last one.

This avoids using the source from the first specification construct of an nameless program, which can sometimes not be associated to any scope, causing an ICE due to an invalid source location.

---

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

2 Files Affected:

• (modified) flang/lib/Semantics/resolve-directives.cpp (+1-1)
• (added) flang/test/Semantics/OpenMP/struct.f90 (+7)

diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index da6c865ad56a3b1..9084bffc7139869 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -25,7 +25,7 @@
 template <typename T>
 static Fortran::semantics::Scope *GetScope(
     Fortran::semantics::SemanticsContext &context, const T &x) {
-  std::optional<Fortran::parser::CharBlock> source{GetSource(x)};
+  std::optional<Fortran::parser::CharBlock> source{GetLastSource(x)};
   return source ? &context.FindScope(*source) : nullptr;
 }
 
diff --git a/flang/test/Semantics/OpenMP/struct.f90 b/flang/test/Semantics/OpenMP/struct.f90
new file mode 100644
index 000000000000000..8ae1fbe4da86f93
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/struct.f90
@@ -0,0 +1,7 @@
+! RUN: %python %S/../test_errors.py %s %flang_fc1 -fopenmp
+! Check OpenMP compatibility with the DEC STRUCTURE extension
+
+structure /s/
+end structure
+
+end

[kiranchandramohan]

This looks OK. But GetLastSource does not seem to be used elsewhere. Would be good to check with @klausler.

[klausler]

Why does this fix the bug? Wouldn't it still fail when presented with a list or vector or tuple of parse tree nodes with a single element that would fail without this change?

[skatrak]

This function is only called for MainProgram, FunctionSubprogram, SubroutineSubprogram and BlockData (see ResolveOmpTopLevelParts in the same file). It tries to get the scope associated to these nodes and crashes while doing so in certain situations when it is a nameless MainProgram. As far as I can tell, the issue is that, when the optional ProgramStmt is not present (nameless program) the call to GetSource will return the source object of the first sourced PFT node that appears in the program, which may not have a scope associated.

In my opinion, it is incorrect to return the scope associated to any inner constructs when querying the program itself. This change makes it so the END statement of the program/function/subroutine/block, which is always present, is picked up every time, preventing this problem and stopping the crash from happening.

Maybe it'd be good to possibly restrict the GetScope function so it fails to compile if we pass any unexpected PFT node type, or just add a comment to describe its intended use. I'll gladly do it if that helps, or if there are any other suggestions as to how to deal with this case.

[skatrak]

Does this sound like a reasonable solution? Thanks!

[skatrak]

Ping @klausler, I'd appreciate it if you could let me know whether my previous explanation addresses your concerns. Thanks for your time!