Skip to content
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

[flang][runtime] Add a critical section for LookUpOrCreateAnonymous. #74468

Merged
merged 2 commits into from
Dec 6, 2023

Conversation

sihuan
Copy link
Contributor

@sihuan sihuan commented Dec 5, 2023

In parallel regions LookUpOrCreateAnonymous may return an anonymous unit that has not yet been opened, which may cause a runtime error.
This commit ensures that the returned anonymous unit has been opened.
For details see: #68856 (comment)

Fixes #68856

@llvmbot llvmbot added flang:runtime flang Flang issues not falling into any other category labels Dec 5, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Dec 5, 2023

@llvm/pr-subscribers-flang-runtime

Author: SiHuaN (sihuan)

Changes

In parallel regions LookUpOrCreateAnonymous may return an anonymous unit that has not yet been opened, which may cause a runtime error.
This commit ensures that the returned anonymous unit has been opened.
For details see: #68856 (comment)

Fixes #68856


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

1 Files Affected:

  • (modified) flang/runtime/unit.cpp (+4)
diff --git a/flang/runtime/unit.cpp b/flang/runtime/unit.cpp
index 995656b9480c4..fa9efee78127e 100644
--- a/flang/runtime/unit.cpp
+++ b/flang/runtime/unit.cpp
@@ -20,6 +20,7 @@ namespace Fortran::runtime::io {
 // The per-unit data structures are created on demand so that Fortran I/O
 // should work without a Fortran main program.
 static Lock unitMapLock;
+static Lock createOpenLock;
 static UnitMap *unitMap{nullptr};
 static ExternalFileUnit *defaultInput{nullptr}; // unit 5
 static ExternalFileUnit *defaultOutput{nullptr}; // unit 6
@@ -52,6 +53,9 @@ ExternalFileUnit *ExternalFileUnit::LookUpOrCreate(
 ExternalFileUnit *ExternalFileUnit::LookUpOrCreateAnonymous(int unit,
     Direction dir, std::optional<bool> isUnformatted,
     const Terminator &terminator) {
+  // Make sure that the returned anonymous unit has been opened
+  // not just created in the unitmap.
+  CriticalSection critical{createOpenLock};
   bool exists{false};
   ExternalFileUnit *result{
       GetUnitMap().LookUpOrCreate(unit, terminator, exists)};

flang/runtime/unit.cpp Outdated Show resolved Hide resolved
@@ -20,6 +20,7 @@ namespace Fortran::runtime::io {
// The per-unit data structures are created on demand so that Fortran I/O
// should work without a Fortran main program.
static Lock unitMapLock;
static Lock createOpenLock;
Copy link
Contributor

Choose a reason for hiding this comment

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

This lock can be declared static in the only function that uses it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I move this declaration into *ExternalFileUnit::LookUpOrCreateAnonymous, like the following:

ExternalFileUnit *ExternalFileUnit::LookUpOrCreateAnonymous(int unit,
    Direction dir, std::optional<bool> isUnformatted,
    const Terminator &terminator) {
  // Make sure that the returned anonymous unit has been opened
  // not just created in the unitMap.
  static Lock createOpenLock;
  CriticalSection critical{createOpenLock};
  bool exists{false};
  ExternalFileUnit *result{
      GetUnitMap().LookUpOrCreate(unit, terminator, exists)};
  if (result && !exists) {
    IoErrorHandler handler{terminator};
    result->OpenAnonymousUnit(
        dir == Direction::Input ? OpenStatus::Unknown : OpenStatus::Replace,
        Action::ReadWrite, Position::Rewind, Convert::Unknown, handler);
    result->isUnformatted = isUnformatted;
  }
  return result;
}

I will have to add the -lstdc++ flag when compiling with flang, or I will get a linker error:

$ cat test.f90
! test.f90
!$omp parallel
write(1,*) 'OK'
!$omp end parallel
end
$ ../../llvm-project/build/bin/flang-new test.f90 -fopenmp
/usr/bin/ld: /path/to/llvm-project/build/lib/libFortranRuntime.a(unit.cpp.o): in function `Fortran::runtime::io::ExternalFileUnit::LookUpOrCreateAnonymous(int, Fortran::runtime::io::Direction, std::optional<bool>, Fortran::runtime::Terminator const&)':
/path/to/llvm-project/flang/runtime/unit.cpp:57:(.text+0x32e): undefined reference to `__cxa_guard_acquire'
/usr/bin/ld: /path/to/llvm-project/flang/runtime/unit.cpp:57:(.text+0x369): undefined reference to `__cxa_guard_release'
flang-new: error: linker command failed with exit code 1 (use -v to see invocation)
$ ../../llvm-project/build/bin/flang-new test.f90 -fopenmp -lstdc++
$ OMP_NUM_THREADS=4 ./a.out  
$ cat fort.1
 OK
 OK
 OK    

Copy link
Contributor

@NimishMishra NimishMishra left a comment

Choose a reason for hiding this comment

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

Thanks for the patch. LGTM

@sihuan sihuan merged commit 62ece9c into llvm:main Dec 6, 2023
4 checks passed
@sihuan sihuan changed the title [flang][runtime] Ensure that the anonymous unit returned by LookUpOrCreateAnonymous has been opened. [flang][runtime] Add a critical section for LookUpOrCreateAnonymous. Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flang:runtime flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Flang][OpenMP] Execution error of a WRITE statement with an unopened unit in a parallel region
4 participants