[flang] Restore old unit locking behavior

[klausler]

Summary

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.

[peixin]

LGTM. Thanks for this support. You may need rebase before landing this. The patch application is not successful now.

[kiranchandramohan]

LGTM. Also tested with a simple program.

The only question I had is regarding setting pthread_t holder_ to 0, is that a reasonable value and will there be a type mismatch if pthread_t is a struct. I did not face any issues on two compilers I tried.

[klausler]

Avoid assuming that pthread_t can be assigned a zero; rebase.

[kiranchandramohan]

Thanks Peter.

flang/runtime/lock.h

 class Lock {
 public:
 #if USE_PTHREADS
   Lock() { pthread_mutex_init(&mutex_, nullptr); }
   ~Lock() { pthread_mutex_destroy(&mutex_); }
   void Take() {
     while (pthread_mutex_lock(&mutex_)) {
     }
+    holder_ = pthread_self();
+    isBusy_ = true;
+  }
+  bool TakeIfNoDeadlock() {
+    if (isBusy_) {
+      auto thisThread{pthread_self()};
+      if (pthread_equal(thisThread, holder_)) {
+        return false;
+      }
+    }
+    Take();
+    return true;
   }
   bool Try() { return pthread_mutex_trylock(&mutex_) == 0; }
-  void Drop() { pthread_mutex_unlock(&mutex_); }
+  void Drop() {
+    isBusy_ = false;
+    pthread_mutex_unlock(&mutex_);
+  }
 #elif defined(_WIN32)
   Lock() { InitializeCriticalSection(&cs_); }
   ~Lock() { DeleteCriticalSection(&cs_); }
   void Take() { EnterCriticalSection(&cs_); }
   bool Try() { return TryEnterCriticalSection(&cs_); }
   void Drop() { LeaveCriticalSection(&cs_); }
 #else
   void Take() { mutex_.lock(); }
   bool Try() { return mutex_.try_lock(); }
   void Drop() { mutex_.unlock(); }
 #endif
 
   void CheckLocked(const Terminator &terminator) {
     if (Try()) {
       Drop();
       terminator.Crash("Lock::CheckLocked() failed");
     }
   }
 
 private:
 #if USE_PTHREADS
   pthread_mutex_t mutex_{};
+  volatile bool isBusy_{false};
+  volatile pthread_t holder_;
 #elif defined(_WIN32)
   CRITICAL_SECTION cs_;
 #else
   std::mutex mutex_;
 #endif
 };
 
 class CriticalSection {

flang/runtime/unit.h

void OpenAnonymousUnit(std::optional<OpenStatus>, std::optional<Action>,
       Position, Convert, IoErrorHandler &);
   void CloseUnit(CloseStatus, IoErrorHandler &);
   void DestroyClosed();
 
   Iostat SetDirection(Direction);
 
   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;
     }
     directAccessRecWasSet_ = false;
     io_.emplace(state);
     return *io_;
   }
private:
   void DoImpliedEndfile(IoErrorHandler &);
   void DoEndfile(IoErrorHandler &);
   void CommitWrites();
   bool CheckDirectAccess(IoErrorHandler &);
   void HitEndOnRead(IoErrorHandler &);
   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};
   bool impliedEndfile_{false}; // sequential/stream output has taken place
   bool beganReadingRecord_{false};
   bool directAccessRecWasSet_{false}; // REC= appeared
   // Subtle: The beginning of the frame can't be allowed to advance
   // during a single list-directed READ due to the possibility of a

flang/runtime/unit.cpp

bool ExternalFileUnit::SetDirectRec(
   currentRecordNumber = oneBasedRec;
   SetPosition((oneBasedRec - 1) * *openRecl, handler);
   return true;
 }
 
 void ExternalFileUnit::EndIoStatement() {
   io_.reset();
   u_.emplace<std::monostate>();
-  CriticalSection critical{lock_};
-  isBusy_ = false;
+  lock_.Drop();
 }
 
 void ExternalFileUnit::BeginSequentialVariableUnformattedInputRecord(
     IoErrorHandler &handler) {
   std::int32_t header{0}, footer{0};
   std::size_t need{recordOffsetInFrame_ + sizeof header};
   std::size_t got{ReadFrame(frameOffsetInFile_, need, handler)};
   // Try to emit informative errors to help debug corrupted files.