[LLVMdev] PassRegistry locking scheme

[David Blaikie]

Oh, and here's an example patch that demonstrates the problem - by
making ManagedStatic enforce some reasonable constraints (that it
doesn't just get incidentally constructed after it's been destroyed),
with the current code, this asserts pretty liberally.

On Tue, Apr 15, 2014 at 9:05 PM, David Blaikie <dblaikie at gmail.com> wrote:
> So, long story short I came across some problematic locking:
>
> PassRegistry uses a ManagedStatic<sys::SmartRWMutex<true>> to handle
> safe registration, and, importantly, deregistration.
>
> The problem is that ManagedStatic's dtor isn't exactly safe: if the
> object is destroyed, then you try to access it again, ManagedStatic
> simply constructs a new object and returns it.
>
> This is relied upon to ensure safe/non-racy deregistration of
> passes... but what happens during destruction? The mutex is destroyed
> and replaced with another, completely different mutex... that doesn't
> seem like a safe locking scheme.
>
> Does any of this make sense? Am I missing something about how this is safe?
>
>
> Long story:
>
> This is how I came across this bug:
>
> * While removing manual memory management in PassRegistry I noticed a
> rather arcane piece of Pimpl usage
> * The pimpl used a void*, so I set about moving the declaration of the
> pimpl type into the outer type as is the usual case
> * Then I wondered why it was a pimpl at all - so I tried to remove it
> * (aside: to handle safe locking around destruction I added an
> Optional<SmartReadLock> as the first member, initialized that in the
> dtor, and let it hold the lock through the destruction of the other
> members... )
> * But then I realized that PassRegistry::removeRegistrationListener
> relied on testing that the pimpl was null to safely no-op after the
> PassRegistry had been destroyed (undefined order of destruction & all
> that jazz)
> * so I added a boolean to track destruction state - but this did not
> fix the issue
> * then, while talking it out, I realized what was really happening:
> the PassRegistry had been destroyed and the ManagedStatic around it,
> instead of returning the destroyed object, had simply built a new one
> in its place with a still null pimpl (the pimpl was lazy, clearly part
> of this cunning scheme) - and thus the removeRegistrationListener call
> was a no-op, except to construct and leave around an extra, empty,
> PassRegistry in the post-destroyed ManagedStatic...
> * which got me thinking: what about the lock, which was also a
> ManagedStatic... which is how I arrived at the above observation
-------------- next part --------------
commit 8947d680e44eb95c2461943733157c9b8ab19357
Author: David Blaikie <dblaikie at gmail.com>
Date:   Tue Apr 15 21:11:08 2014 -0700

    Prototype

diff --git include/llvm/Support/ManagedStatic.h include/llvm/Support/ManagedStatic.h
index 1bb8cea..ed95e21 100644
--- include/llvm/Support/ManagedStatic.h
+++ include/llvm/Support/ManagedStatic.h
@@ -18,6 +18,8 @@
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/Valgrind.h"
 
+#include <cassert>
+
 namespace llvm {
 
 /// object_creator - Helper method for ManagedStatic.
@@ -38,9 +40,11 @@ template<typename T, size_t N> struct object_deleter<T[N]> {
 /// ManagedStaticBase - Common base class for ManagedStatic instances.
 class ManagedStaticBase {
 protected:
+  enum State { Uninitialized, Constructed, Destroyed };
   // This should only be used as a static variable, which guarantees that this
   // will be zero initialized.
   mutable void *Ptr;
+  mutable State CurState;
   mutable void (*DeleterFn)(void*);
   mutable const ManagedStaticBase *Next;
 
@@ -48,6 +52,7 @@ protected:
 public:
   /// isConstructed - Return true if this object has not been created yet.
   bool isConstructed() const { return Ptr != nullptr; }
+  bool isDestroyed() const { return CurState == Destroyed; }
 
   void destroy() const;
 };
@@ -63,36 +68,22 @@ public:
 
   // Accessors.
   C &operator*() {
-    void* tmp = Ptr;
-    if (llvm_is_multithreaded()) sys::MemoryFence();
-    if (!tmp) RegisterManagedStatic(object_creator<C>, object_deleter<C>::call);
-    TsanHappensAfter(this);
-
-    return *static_cast<C*>(Ptr);
+    return const_cast<C&>(*static_cast<const ManagedStatic&>(*this));
   }
   C *operator->() {
-    void* tmp = Ptr;
-    if (llvm_is_multithreaded()) sys::MemoryFence();
-    if (!tmp) RegisterManagedStatic(object_creator<C>, object_deleter<C>::call);
-    TsanHappensAfter(this);
-
-    return static_cast<C*>(Ptr);
+    return &**this;
   }
   const C &operator*() const {
-    void* tmp = Ptr;
+    State tmp = CurState;
     if (llvm_is_multithreaded()) sys::MemoryFence();
-    if (!tmp) RegisterManagedStatic(object_creator<C>, object_deleter<C>::call);
+    assert(tmp != Destroyed);
+    if (tmp == Uninitialized) RegisterManagedStatic(object_creator<C>, object_deleter<C>::call);
     TsanHappensAfter(this);
 
     return *static_cast<C*>(Ptr);
   }
   const C *operator->() const {
-    void* tmp = Ptr;
-    if (llvm_is_multithreaded()) sys::MemoryFence();
-    if (!tmp) RegisterManagedStatic(object_creator<C>, object_deleter<C>::call);
-    TsanHappensAfter(this);
-
-    return static_cast<C*>(Ptr);
+    return &**this;
   }
 };
 
diff --git lib/Support/ManagedStatic.cpp lib/Support/ManagedStatic.cpp
index 6a1c2a5..e63d82c 100644
--- lib/Support/ManagedStatic.cpp
+++ lib/Support/ManagedStatic.cpp
@@ -25,7 +25,7 @@ void ManagedStaticBase::RegisterManagedStatic(void *(*Creator)(),
   if (llvm_is_multithreaded()) {
     llvm_acquire_global_lock();
 
-    if (!Ptr) {
+    if (CurState == Uninitialized) {
       void* tmp = Creator();
 
       TsanHappensBefore(this);
@@ -38,6 +38,7 @@ void ManagedStaticBase::RegisterManagedStatic(void *(*Creator)(),
       Ptr = tmp;
       TsanIgnoreWritesEnd();
       DeleterFn = Deleter;
+      CurState = Constructed;
       
       // Add to list of managed statics.
       Next = StaticList;
@@ -50,6 +51,7 @@ void ManagedStaticBase::RegisterManagedStatic(void *(*Creator)(),
            "Partially initialized ManagedStatic!?");
     Ptr = Creator();
     DeleterFn = Deleter;
+    CurState = Constructed;
   
     // Add to list of managed statics.
     Next = StaticList;
@@ -64,6 +66,8 @@ void ManagedStaticBase::destroy() const {
   // Unlink from list.
   StaticList = Next;
   Next = nullptr;
+  assert(CurState == Constructed);
+  CurState = Destroyed;
 
   // Destroy memory.
   DeleterFn(Ptr);