Do not reset the mutex to 0 readers, 0 writers upon interp invocation.

But reset it to wherever the mutex was before entering a lock frame related to user code interpretation (which might in turn trigger a call to the interpreter).

(PR-1564)

Author: Axel-Naumann

core/base/inc/TVirtualMutex.h

@@ -34,10 +34,6 @@ R__EXTERN TVirtualMutex *gGlobalMutex;
 class TVirtualMutex {
 
 public:
-   struct State {
-      virtual ~State();
-   };
-
    TVirtualMutex(Bool_t /* recursive */ = kFALSE) { }
    virtual ~TVirtualMutex() { }
 
@@ -49,8 +45,6 @@ class TVirtualMutex {
    Int_t Release() { return UnLock(); }
 
    virtual TVirtualMutex *Factory(Bool_t /*recursive*/ = kFALSE) = 0;
-   virtual std::unique_ptr<TVirtualMutex::State> Reset() = 0;
-   virtual void Restore(std::unique_ptr<State> &&state) = 0;
 
    ClassDef(TVirtualMutex, 0)  // Virtual mutex lock class
 };
@@ -94,53 +88,6 @@ class TLockGuard {
    ClassDefNV(TLockGuard,0)  // Exception safe locking/unlocking of mutex
 };
 
-namespace ROOT {
-
-//////////////////////////////////////////////////////////////////////////
-//                                                                      //
-// TLockSuspend                                                         //
-//                                                                      //
-// This class provides mutex supension management in a guaranteed and   //
-// exception safe way. Use like this:                                   //
-// {                                                                    //
-//    TLockSuspend guard(mutex);                                        //
-//    ... // do something                                               //
-// }                                                                    //
-// when guard is created, 'suspend' the lock by calling Reset on the    //
-// on the mutex and when the guard goes out of scope the mutex is       //
-// restored in the TLockSuspend destructor.                             //
-// The exception mechanism takes care of calling the dtors              //
-// of local objects so it is exception safe.                            //
-//                                                                      //
-//////////////////////////////////////////////////////////////////////////
-
-class TLockSuspend {
-private:
-   TVirtualMutex *const fMutex;
-
-   std::unique_ptr<TVirtualMutex::State> fState;
-
-   TLockSuspend(const TLockSuspend &) = delete;
-   TLockSuspend &operator=(const TLockSuspend &) = delete;
-
-public:
-   TLockSuspend(TVirtualMutex *mutex) : fMutex(mutex)
-   {
-      if (fMutex)
-         fState = mutex->Reset();
-   }
-
-   ~TLockSuspend()
-   {
-      if (fMutex)
-         fMutex->Restore(std::move(fState));
-   }
-
-   ClassDefNV(TLockSuspend, 0) // Exception safe Reset/Restore of mutex
-};
-
-} // Namespace ROOT.
-
 // Zero overhead macros in case not compiled with thread support
 #if defined (_REENTRANT) || defined (WIN32)
 
@@ -155,13 +102,11 @@ class TLockSuspend {
    R__LOCKGUARD(mutex)
 #define R__LOCKGUARD_NAMED(name,mutex) TLockGuard _NAME2_(R__guard,name)(mutex)
 #define R__LOCKGUARD_UNLOCK(name) _NAME2_(R__guard,name).UnLock()
-#define R__LOCK_SUSPEND(mutex) TLockSuspend _R__UNIQUE_(R__guard)(mutex)
 #else
 #define R__LOCKGUARD(mutex)  (void)mutex; { }
 #define R__LOCKGUARD_NAMED(name,mutex) (void)mutex; { }
 #define R__LOCKGUARD2(mutex) (void)mutex; { }
 #define R__LOCKGUARD_UNLOCK(name) { }
-#define R__LOCK_SUSPEND(mutex) { }
 #endif
 
 #ifdef R__USE_IMT

core/base/inc/TVirtualRWMutex.h

@@ -44,6 +44,20 @@ class TVirtualRWMutex : public TVirtualMutex  {
    // distinct from these)
    class Hint_t;
 
+   /// \class State
+   /// Earlier lock state as returned by `GetState()` that can be passed to
+   /// `Restore()`
+   struct State {
+      virtual ~State(); // implemented in TVirtualMutex.cxx
+   };
+
+   /// \class StateDelta
+   /// State as returned by `GetStateDelta()` that can be passed to
+   /// `Restore()`
+   struct StateDelta {
+      virtual ~StateDelta(); // implemented in TVirtualMutex.cxx
+   };
+
    virtual Hint_t *ReadLock() = 0;
    virtual void ReadUnLock(Hint_t *) = 0;
    virtual Hint_t *WriteLock() = 0;
@@ -54,6 +68,10 @@ class TVirtualRWMutex : public TVirtualMutex  {
    Int_t UnLock() override { WriteUnLock(nullptr); return 1; }
    Int_t CleanUp() override { WriteUnLock(nullptr); return 1; }
 
+   virtual std::unique_ptr<State> GetStateBefore() = 0;
+   virtual std::unique_ptr<StateDelta> Rewind(const State& earlierState) = 0;
+   virtual void Apply(std::unique_ptr<StateDelta> &&delta) = 0;
+
    TVirtualRWMutex *Factory(Bool_t /*recursive*/ = kFALSE) override = 0;
 
    ClassDefOverride(TVirtualRWMutex, 0)  // Virtual mutex lock class

core/base/src/TVirtualMutex.cxx

@@ -33,6 +33,7 @@ of local objects so it is exception safe.
 */
 
 #include "TVirtualMutex.h"
+#include "TVirtualRWMutex.h"
 
 ClassImp(TVirtualMutex);
 ClassImp(TLockGuard);
@@ -43,5 +44,6 @@ ClassImp(TLockGuard);
 // lockup of the system (see TMutex::Factory)
 TVirtualMutex *gGlobalMutex = 0;
 
-// Pin that vtable.
-TVirtualMutex::State::~State() = default;
+// From TVirtualRWMutex.h:
+ROOT::TVirtualRWMutex::State::~State() = default;
+ROOT::TVirtualRWMutex::StateDelta::~StateDelta() = default;

core/meta/inc/TInterpreter.h

@@ -24,7 +24,7 @@
 
 #include "TDictionary.h"
 
-#include "TVirtualMutex.h"
+#include "TVirtualRWMutex.h"
 
 #include <map>
 #include <typeinfo>
@@ -41,6 +41,22 @@ class TListOfEnums;
 
 R__EXTERN TVirtualMutex *gInterpreterMutex;
 
+#if defined (_REENTRANT) || defined (WIN32)
+# define R__LOCKGUARD_CLING(mutex)  ::ROOT::Internal::InterpreterMutexRegistrationRAII _R__UNIQUE_(R__guard)(mutex); { }
+#else
+# define R__LOCKGUARD_CLING(mutex)  (void)mutex; { }
+#endif
+
+namespace ROOT {
+namespace Internal {
+struct InterpreterMutexRegistrationRAII {
+   TLockGuard fLockGuard;
+   InterpreterMutexRegistrationRAII(TVirtualMutex* mutex);
+   ~InterpreterMutexRegistrationRAII();
+};
+}
+}
+
 class TInterpreter : public TNamed {
 
 protected:
@@ -205,6 +221,9 @@ class TInterpreter : public TNamed {
    virtual void     SetProcessLineLock(Bool_t lock = kTRUE) = 0;
    virtual const char *TypeName(const char *s) = 0;
 
+   virtual void     SnapshotMutexState(ROOT::TVirtualRWMutex* mtx) = 0;
+   virtual void     ForgetMutexState() = 0;
+
    // All the functions below must be virtual with a dummy implementation
    // These functions are redefined in TCling.
    //The dummy implementation avoids an implementation in TGWin32InterpreterProxy
@@ -510,4 +529,16 @@ R__EXTERN TInterpreter* (*gPtr2Interpreter)();
 R__EXTERN TInterpreter* gCling;
 #endif
 
+inline ROOT::Internal::InterpreterMutexRegistrationRAII::InterpreterMutexRegistrationRAII(TVirtualMutex* mutex):
+   fLockGuard(mutex)
+{
+   if (gCoreMutex)
+      ::gCling->SnapshotMutexState(gCoreMutex);
+}
+inline ROOT::Internal::InterpreterMutexRegistrationRAII::~InterpreterMutexRegistrationRAII()
+{
+   if (gCoreMutex)
+      ::gCling->ForgetMutexState();
+}
+
 #endif

core/metacling/src/TCling.cxx

@@ -354,27 +354,20 @@ void TCling__PrintStackTrace() {
 }
 
 ////////////////////////////////////////////////////////////////////////////////
-/// Restore the interpreter lock.
+/// Re-apply the lock count delta that TCling__ResetInterpreterMutex() caused.
 
-extern "C" void TCling__RestoreInterpreterMutex(void *state)
+extern "C" void TCling__RestoreInterpreterMutex(void *delta)
 {
-   if (gInterpreterMutex && state) {
-      auto typedState = static_cast<TVirtualMutex::State *>(state);
-      std::unique_ptr<TVirtualMutex::State> uniqueP{typedState};
-      gInterpreterMutex->Restore(std::move(uniqueP));
-   }
+   ((TCling*)gCling)->ApplyToInterpreterMutex(delta);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
-/// Completely reset the interpreter lock.
+/// Reset the interpreter lock to the state it had before interpreter-related
+/// calls happened.
 
 extern "C" void *TCling__ResetInterpreterMutex()
 {
-   if (gInterpreterMutex) {
-      auto uniqueP = gInterpreterMutex->Reset();
-      return uniqueP.release();
-   }
-   return nullptr;
+   return ((TCling*)gCling)->RewindInterpreterMutex();
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -2120,7 +2113,7 @@ Long_t TCling::ProcessLine(const char* line, EErrorCode* error/*=0*/)
          gInterpreterMutex = gGlobalMutex->Factory(kTRUE);
       gGlobalMutex->UnLock();
    }
-   R__LOCKGUARD(fLockProcessLine ? gInterpreterMutex : 0);
+   R__LOCKGUARD_CLING(fLockProcessLine ? gInterpreterMutex : 0);
    gROOT->SetLineIsProcessing();
 
    struct InterpreterFlagsRAII {
@@ -2683,7 +2676,7 @@ void TCling::ClearStack()
 
 bool TCling::Declare(const char* code)
 {
-   R__LOCKGUARD(gInterpreterMutex);
+   R__LOCKGUARD_CLING(gInterpreterMutex);
 
    int oldload = SetClassAutoloading(0);
    SuspendAutoParsing autoParseRaii(this);
@@ -3000,7 +2993,7 @@ Int_t TCling::Load(const char* filename, Bool_t system)
    }
 
    // Used to return 0 on success, 1 on duplicate, -1 on failure, -2 on "fatal".
-   R__LOCKGUARD(gInterpreterMutex);
+   R__LOCKGUARD_CLING(gInterpreterMutex);
    cling::DynamicLibraryManager* DLM = fInterpreter->getDynamicLibraryManager();
    std::string canonLib = DLM->lookupLibrary(filename);
    cling::DynamicLibraryManager::LoadLibResult res
@@ -3051,7 +3044,7 @@ Long_t TCling::ProcessLineAsynch(const char* line, EErrorCode* error)
 
 Long_t TCling::ProcessLineSynch(const char* line, EErrorCode* error)
 {
-   R__LOCKGUARD(fLockProcessLine ? gInterpreterMutex : 0);
+   R__LOCKGUARD_CLING(fLockProcessLine ? gInterpreterMutex : 0);
    if (gApplication) {
       if (gApplication->IsCmdThread()) {
          return ProcessLine(line, error);
@@ -3079,7 +3072,7 @@ Long_t TCling::Calc(const char* line, EErrorCode* error)
       gROOT->SetLineIsProcessing();
    }
 #endif // R__WIN32
-   R__LOCKGUARD(gInterpreterMutex);
+   R__LOCKGUARD_CLING(gInterpreterMutex);
    if (error) {
       *error = TInterpreter::kNoError;
    }
@@ -4565,7 +4558,7 @@ void TCling::GetInterpreterTypeName(const char* name, std::string &output, Bool_
 
 void TCling::Execute(const char* function, const char* params, int* error)
 {
-   R__LOCKGUARD(gInterpreterMutex);
+   R__LOCKGUARD_CLING(gInterpreterMutex);
    if (error) {
       *error = TInterpreter::kNoError;
    }
@@ -4590,7 +4583,7 @@ void TCling::Execute(const char* function, const char* params, int* error)
 void TCling::Execute(TObject* obj, TClass* cl, const char* method,
                      const char* params, Bool_t objectIsConst, int* error)
 {
-   R__LOCKGUARD(gInterpreterMutex);
+   R__LOCKGUARD_CLING(gInterpreterMutex);
    if (error) {
       *error = TInterpreter::kNoError;
    }
@@ -4693,7 +4686,7 @@ void TCling::Execute(TObject* obj, TClass* cl, TMethod* method,
    }
 
    // And now execute it.
-   R__LOCKGUARD(gInterpreterMutex);
+   R__LOCKGUARD_CLING(gInterpreterMutex);
    if (error) {
       *error = TInterpreter::kNoError;
    }
@@ -4735,7 +4728,7 @@ void TCling::ExecuteWithArgsAndReturn(TMethod* method, void* address,
 
 Long_t TCling::ExecuteMacro(const char* filename, EErrorCode* error)
 {
-   R__LOCKGUARD(fLockProcessLine ? gInterpreterMutex : 0);
+   R__LOCKGUARD_CLING(fLockProcessLine ? gInterpreterMutex : 0);
    fCurExecutingMacros.push_back(filename);
    Long_t result = TApplication::ExecuteFile(filename, (int*)error);
    fCurExecutingMacros.pop_back();
@@ -8453,3 +8446,66 @@ const char* TCling::TypedefInfo_Title(TypedefInfo_t* tinfo) const
    TClingTypedefInfo* TClinginfo = (TClingTypedefInfo*) tinfo;
    return TClinginfo->Title();
 }
+
+////////////////////////////////////////////////////////////////////////////////
+
+void TCling::SnapshotMutexState(ROOT::TVirtualRWMutex* mtx)
+{
+   if (!fInitialMutex.back()) {
+      if (fInitialMutex.back().fRecurseCount) {
+         Error("SnapshotMutexState", "fRecurseCount != 0 even though initial mutex state is unset!");
+      }
+      fInitialMutex.back().fState = mtx->GetStateBefore();
+   }
+   // We will "forget" this lock once we backed out of all interpreter frames.
+   // Here we are entering one, so ++.
+   ++fInitialMutex.back().fRecurseCount;
+}
+
+////////////////////////////////////////////////////////////////////////////////
+
+void TCling::ForgetMutexState()
+{
+   if (!fInitialMutex.back())
+      return;
+   if (fInitialMutex.back().fRecurseCount == 0) {
+      Error("ForgetMutexState", "mutex state's recurse count already 0!");
+   }
+   else if (--fInitialMutex.back().fRecurseCount == 0) {
+      // We have returned from all interpreter frames. Reset the initial lock state.
+      fInitialMutex.back().fState.reset();
+   }
+}
+
+////////////////////////////////////////////////////////////////////////////////
+/// Re-apply the lock count delta that TCling__ResetInterpreterMutex() caused.
+
+void TCling::ApplyToInterpreterMutex(void *delta)
+{
+   R__ASSERT(!fInitialMutex.empty() && "Inconsistent state of fInitialMutex!");
+   if (gInterpreterMutex) {
+      if (delta) {
+         auto typedDelta = static_cast<TVirtualRWMutex::StateDelta *>(delta);
+         std::unique_ptr<TVirtualRWMutex::StateDelta> uniqueP{typedDelta};
+         gCoreMutex->Apply(std::move(uniqueP));
+      }
+   }
+   fInitialMutex.pop_back();
+}
+
+////////////////////////////////////////////////////////////////////////////////
+/// Reset the interpreter lock to the state it had before interpreter-related
+/// calls happened.
+
+void *TCling::RewindInterpreterMutex()
+{
+   if (fInitialMutex.back()) {
+      std::unique_ptr<TVirtualRWMutex::StateDelta> uniqueP = gCoreMutex->Rewind(*fInitialMutex.back().fState);
+      // Need to start a new recurse count.
+      fInitialMutex.emplace_back();
+      return uniqueP.release();
+   }
+   // Need to start a new recurse count.
+   fInitialMutex.emplace_back();
+   return nullptr;
+}