[flang-commits] [PATCH] D139477: [flang] Restore old unit locking behavior

[Peter Klausler via Phabricator via flang-commits]

klausler created this revision.
klausler added reviewers: peixin, kiranchandramohan.
klausler added a project: Flang.
Herald added a subscriber: jdoerfert.
Herald added a project: All.
klausler requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added a subscriber: sstefan1.

Rework  the recursive I/O error check on I/O units so that
threads again hold a lock on a unit throughout an I/O statement.  
Add an API to the runtime's Lock class implementation for pthreads
to allow detection of solf-deadlock without depending on EDEADLK
or recursive mutexes.

This should fix I/O from OpenMP threads.


https://reviews.llvm.org/D139477

Files:
  flang/runtime/lock.h
  flang/runtime/unit.cpp
  flang/runtime/unit.h


Index: flang/runtime/unit.h
===================================================================
--- flang/runtime/unit.h
+++ flang/runtime/unit.h
@@ -71,17 +71,14 @@
 
   template <typename A, typename... X>
   IoStatementState &BeginIoStatement(const Terminator &terminator, X &&...xs) {
-    bool alreadyBusy{false};
-    {
-      CriticalSection critical{lock_};
-      alreadyBusy = isBusy_;
-      isBusy_ = true; // cleared in EndIoStatement()
-    }
-    if (alreadyBusy) {
-      terminator.Crash("Could not acquire exclusive lock on unit %d, perhaps "
-                       "due to an attempt to perform recursive I/O",
-          unitNumber_);
+    // Take lock_ and hold it until EndIoStatement().
+#if USE_PTHREADS
+    if (!lock_.TakeIfNoDeadlock()) {
+      terminator.Crash("Recursive I/O attempted on unit %d", unitNumber_);
     }
+#else
+    lock_.Take();
+#endif
     A &state{u_.emplace<A>(std::forward<X>(xs)...)};
     if constexpr (!std::is_same_v<A, OpenStatementState>) {
       state.mutableModes() = ConnectionState::modes;
@@ -137,8 +134,6 @@
   std::int32_t ReadHeaderOrFooter(std::int64_t frameOffset);
 
   Lock lock_;
-  // TODO: replace with a thread ID
-  bool isBusy_{false}; // under lock_
 
   int unitNumber_{-1};
   Direction direction_{Direction::Output};
Index: flang/runtime/unit.cpp
===================================================================
--- flang/runtime/unit.cpp
+++ flang/runtime/unit.cpp
@@ -714,8 +714,7 @@
 void ExternalFileUnit::EndIoStatement() {
   io_.reset();
   u_.emplace<std::monostate>();
-  CriticalSection critical{lock_};
-  isBusy_ = false;
+  lock_.Drop();
 }
 
 void ExternalFileUnit::BeginSequentialVariableUnformattedInputRecord(
Index: flang/runtime/lock.h
===================================================================
--- flang/runtime/lock.h
+++ flang/runtime/lock.h
@@ -41,9 +41,22 @@
   void Take() {
     while (pthread_mutex_lock(&mutex_)) {
     }
+    holder_ = pthread_self();
+  }
+  bool TakeIfNoDeadlock() {
+    auto thisThread{pthread_self()};
+    if (pthread_equal(thisThread, holder_)) {
+      return false;
+    } else {
+      Take();
+      return true;
+    }
   }
   bool Try() { return pthread_mutex_trylock(&mutex_) == 0; }
-  void Drop() { pthread_mutex_unlock(&mutex_); }
+  void Drop() {
+    holder_ = 0;
+    pthread_mutex_unlock(&mutex_);
+  }
 #elif defined(_WIN32)
   Lock() { InitializeCriticalSection(&cs_); }
   ~Lock() { DeleteCriticalSection(&cs_); }
@@ -66,6 +79,7 @@
 private:
 #if USE_PTHREADS
   pthread_mutex_t mutex_{};
+  volatile pthread_t holder_{0};
 #elif defined(_WIN32)
   CRITICAL_SECTION cs_;
 #else


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D139477.480658.patch
Type: text/x-patch
Size: 2641 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/flang-commits/attachments/20221206/aadac77d/attachment.bin>