[libunwind] Abstract rwlocks into a class, provide a SRW lock implementation for windows

[mstorsjo]

Summary

This requires _WIN32_WINNT >= 0x0600.

If someone wants to spend effort on supporting earlier versions, one can easily add another fallback implementation based on critical sections, or try to load SRW lock functions dynamically.

This makes sure that the FDE cache is thread safe on windows.

[zturner]

I'm a little nervous about re-inventing a poor man's version of a reader writer lock. Can we not just copy LLVM's?

src/UnwindCursor.hpp

20 ↗

(On Diff #118266)

Maybe you want to check _MSC_VER here? I think MinGW compilers will have pthread support.

43–45 ↗

(On Diff #118266)

As a matter of principle, I'm not a huge fan of naming things with posix specific names if the implementation is not actually posix. For example, these functions always return success, but this is not true of the posix functions which can fail for various reasons. In general, if people are unaware that there is some abstraction behind the scenes for Windows, and they just see a function named posix_rwlock_rdlock, then they are going to assume that they can expect the semantics documented on the posix man pages, which is not true.

The alternative is to create a proper abstraction, which might be more work than you're interested in taking on, so consider this just an advisory suggestion.

50 ↗

(On Diff #118266)

This doesn't seem like what you want. a global static function / variable in a header file is going to be duplicated in every translation unit. i.e. two translation units will have different copies of lockedForWrite. Same goes for the rest of the functions.

61–64 ↗

(On Diff #118266)

Doesn't lockedForWrite need to be atomic? If it is not atomic, there is no guarantee that thread 2 will see the results of thread 1's modifications with any kind of reasonable order.

[mstorsjo]

In D38704#892479, @zturner wrote:

I'm a little nervous about re-inventing a poor man's version of a reader writer lock. Can we not just copy LLVM's?

I guess I could have a look to see how much extra either kitchen sink it would bring. Since it almost mapped 1:1 to the windows functions, I thought it wouldn't end up too large - unfortunately the return values and unlock functions made it a bit harder though.

src/UnwindCursor.hpp

20 ↗

(On Diff #118266)

MinGW compilers can have optional pthread support (it's not a feature of the compiler itself but an extra library that one can choose to build), but not everybody wants to use it and at least I wouldn't want to use it as mandatory dependency for any C++ support based on libunwind.

50 ↗

(On Diff #118266)

Oh, right - I would need to make it match the static class member _lock instead.

61–64 ↗

(On Diff #118266)

I don't think it needs to be atomic, although the rdlock function perhaps shouldn't touch it at all. It only ever gets set to true once we have an exclusive lock, and in those cases gets set back to false before the exclusive lock gets released. So without touching it in the rdlock function, we only ever write to it while holding the exclusive write lock.

[mstorsjo]

In D38704#892479, @zturner wrote:

I'm a little nervous about re-inventing a poor man's version of a reader writer lock. Can we not just copy LLVM's?

I had a closer look at this, and noticed the following:

• The LLVM RWMutex class on windows tries to load the SRW functions dynamically, and falls back on a normal critical section otherwise. This init procedure doesn't seem to be threadsafe though, but as long as the first RWMutex object is initialized before any extra threads are started, we should be fine. For libunwind, it would probably be done in a constructor on CRT startup.
• Importing this implementation verbatim would require including one header, 3 implementation files, and probably some CMake hookups.
• Until the LLVM relicensing is done, afaik we can't include code verbatim from LLVM into the runtime libs, or did I get this wrong?

If you prefer, it's pretty trivial to do a smaller but similar implementation for libunwind though. If we add a new RWMutex class instead of using the pthread API (and implementing the windows version behind that API), we can have separate unlock methods for read and write mode, getting rid of the ugly lockedForWrite variable I currently have. And one could have it either just use SRW, or try loading SRW dynamically if that's preferred (if one wants to spend extra effort on libunwind on pre-vista).

What do you think @zturner?

[majnemer]

I don't think we should depend on LLVM for the locking stuff. This sort of infrastructure is in the same bucket as the demangler which we haven't really solved.

I *do* find it weird to do it this way though. I think you should have some mutex functions which include read/write unlock. This way you don't need the weird state.

[mstorsjo]

In D38704#897935, @majnemer wrote:

I don't think we should depend on LLVM for the locking stuff. This sort of infrastructure is in the same bucket as the demangler which we haven't really solved.

I *do* find it weird to do it this way though. I think you should have some mutex functions which include read/write unlock. This way you don't need the weird state.

Ok, sure. I can add a new class with an API similar to the one in LLVM, but only the bare essentials (i.e. exactly what libunwind has got right now, plus SRW locks for windows) for now.

[mstorsjo]

Instead of trying to emulate the pthread rwlocks api with windows implementations, add a RWMutex class (with an interface similar to SmartRWMutex from LLVM) with inline implementations, which can either be no-ops (for no-threads configurations), implemented with SRW locks for windows or pthreads.

[mstorsjo]

Ping @zturner and others

[zturner]

This looks much better than the previous solution. Thanks!

[majnemer]

LGTM

[smeenai]

BTW LLVM requires a minimum of Windows 7 according to https://releases.llvm.org/3.8.0/docs/ReleaseNotes.html, so you can rely on SRW locks always being present. If LLVM still has a fallback path for when SRW locks are absent, that could be cleaned up.

libunwind/trunk/src/CMakeLists.txt

set(LIBUNWIND_HEADERS
     assembly.h
     CompactUnwinder.hpp
     config.h
     dwarf2.h
     DwarfInstructions.hpp
     DwarfParser.hpp
     libunwind_ext.h
     Registers.hpp
+    RWMutex.hpp
     UnwindCursor.hpp
     ${CMAKE_CURRENT_SOURCE_DIR}/../include/libunwind.h
     ${CMAKE_CURRENT_SOURCE_DIR}/../include/unwind.h)
 
 append_if(LIBUNWIND_HEADERS APPLE
           "${CMAKE_CURRENT_SOURCE_DIR}/../include/mach-o/compact_unwind_encoding.h")
 
 if (MSVC_IDE)

libunwind/trunk/src/RWMutex.hpp

+//===----------------------------- Registers.hpp --------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//
+// Abstract interface to shared reader/writer log, hiding platform and
+// configuration differences.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef __RWMUTEX_HPP__
+#define __RWMUTEX_HPP__
+
+#if defined(_WIN32)
+#include <windows.h>
+#elif !defined(_LIBUNWIND_HAS_NO_THREADS)
+#include <pthread.h>
+#endif
+
+namespace libunwind {
+
+#if defined(_LIBUNWIND_HAS_NO_THREADS)
+
+class _LIBUNWIND_HIDDEN RWMutex {
+public:
+  bool lock_shared() { return true; }
+  bool unlock_shared() { return true; }
+  bool lock() { return true; }
+  bool unlock() { return true; }
+};
+
+#elif defined(_WIN32)
+
+class _LIBUNWIND_HIDDEN RWMutex {
+public:
+  bool lock_shared() {
+    AcquireSRWLockShared(&_lock);
+    return true;
+  }
+  bool unlock_shared() {
+    ReleaseSRWLockShared(&_lock);
+    return true;
+  }
+  bool lock() {
+    AcquireSRWLockExclusive(&_lock);
+    return true;
+  }
+  bool unlock() {
+    ReleaseSRWLockExclusive(&_lock);
+    return true;
+  }
+
+private:
+  SRWLOCK _lock = SRWLOCK_INIT;
+};
+
+#else
+
+class _LIBUNWIND_HIDDEN RWMutex {
+public:
+  bool lock_shared() { return pthread_rwlock_rdlock(&_lock) == 0; }
+  bool unlock_shared() { return pthread_rwlock_unlock(&_lock) == 0; }
+  bool lock() { return pthread_rwlock_wrlock(&_lock) == 0; }
+  bool unlock() { return pthread_rwlock_unlock(&_lock) == 0; }
+
+private:
+  pthread_rwlock_t _lock = PTHREAD_RWLOCK_INITIALIZER;
+};
+
+#endif
+
+} // namespace libunwind
+
+#endif // __RWMUTEX_HPP__

libunwind/trunk/src/UnwindCursor.hpp

 
 #ifndef __UNWINDCURSOR_HPP__
 #define __UNWINDCURSOR_HPP__
 
 #include <algorithm>
 #include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
-#ifndef _LIBUNWIND_HAS_NO_THREADS
-  #include <pthread.h>
-#endif
 #include <unwind.h>
 
 #ifdef __APPLE__
   #include <mach-o/dyld.h>
 #endif
 
 #include "config.h"
 
 #include "AddressSpace.hpp"
 #include "CompactUnwinder.hpp"
 #include "config.h"
 #include "DwarfInstructions.hpp"
 #include "EHHeaderParser.hpp"
 #include "libunwind.h"
 #include "Registers.hpp"
+#include "RWMutex.hpp"
 #include "Unwind-EHABI.h"
 
 namespace libunwind {
 
 #if defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND)
 /// Cache of recently found FDEs.
 template <typename A>
 class _LIBUNWIND_HIDDEN DwarfFDECache {
struct entry {
     pint_t mh;
     pint_t ip_start;
     pint_t ip_end;
     pint_t fde;
   };
 
   // These fields are all static to avoid needing an initializer.
   // There is only one instance of this class per process.
-#ifndef _LIBUNWIND_HAS_NO_THREADS
-  static pthread_rwlock_t _lock;
-#endif
+  static RWMutex _lock;
 #ifdef __APPLE__
   static void dyldUnloadHook(const struct mach_header *mh, intptr_t slide);
   static bool _registeredForDyldUnloads;
 #endif
   // Can't use std::vector<> here because this code is below libc++.
   static entry *_buffer;
   static entry *_bufferUsed;
   static entry *_bufferEnd;

 
 template <typename A>
 typename DwarfFDECache<A>::entry *
 DwarfFDECache<A>::_bufferEnd = &_initialBuffer[64];
 
 template <typename A>
 typename DwarfFDECache<A>::entry DwarfFDECache<A>::_initialBuffer[64];
 
-#ifndef _LIBUNWIND_HAS_NO_THREADS
 template <typename A>
-pthread_rwlock_t DwarfFDECache<A>::_lock = PTHREAD_RWLOCK_INITIALIZER;
-#endif
+RWMutex DwarfFDECache<A>::_lock;
 
 #ifdef __APPLE__
 template <typename A>
 bool DwarfFDECache<A>::_registeredForDyldUnloads = false;
 #endif
 
 template <typename A>
 typename A::pint_t DwarfFDECache<A>::findFDE(pint_t mh, pint_t pc) {
   pint_t result = 0;
-  _LIBUNWIND_LOG_NON_ZERO(::pthread_rwlock_rdlock(&_lock));
+  _LIBUNWIND_LOG_IF_FALSE(_lock.lock_shared());
   for (entry *p = _buffer; p < _bufferUsed; ++p) {
     if ((mh == p->mh) || (mh == 0)) {
       if ((p->ip_start <= pc) && (pc < p->ip_end)) {
         result = p->fde;
         break;
       }
     }
   }
-  _LIBUNWIND_LOG_NON_ZERO(::pthread_rwlock_unlock(&_lock));
+  _LIBUNWIND_LOG_IF_FALSE(_lock.unlock_shared());
   return result;
 }
 
 template <typename A>
 void DwarfFDECache<A>::add(pint_t mh, pint_t ip_start, pint_t ip_end,
                            pint_t fde) {
 #if !defined(_LIBUNWIND_NO_HEAP)
-  _LIBUNWIND_LOG_NON_ZERO(::pthread_rwlock_wrlock(&_lock));
+  _LIBUNWIND_LOG_IF_FALSE(_lock.lock());
   if (_bufferUsed >= _bufferEnd) {
     size_t oldSize = (size_t)(_bufferEnd - _buffer);
     size_t newSize = oldSize * 4;
     // Can't use operator new (we are below it).
     entry *newBuffer = (entry *)malloc(newSize * sizeof(entry));
     memcpy(newBuffer, _buffer, oldSize * sizeof(entry));
     if (_buffer != _initialBuffer)
       free(_buffer);
     _buffer = newBuffer;
     _bufferUsed = &newBuffer[oldSize];
     _bufferEnd = &newBuffer[newSize];
   }
   _bufferUsed->mh = mh;
   _bufferUsed->ip_start = ip_start;
   _bufferUsed->ip_end = ip_end;
   _bufferUsed->fde = fde;
   ++_bufferUsed;
 #ifdef __APPLE__
   if (!_registeredForDyldUnloads) {
     _dyld_register_func_for_remove_image(&dyldUnloadHook);
     _registeredForDyldUnloads = true;
   }
 #endif
-  _LIBUNWIND_LOG_NON_ZERO(::pthread_rwlock_unlock(&_lock));
+  _LIBUNWIND_LOG_IF_FALSE(_lock.unlock());
 #endif
 }
 
 template <typename A>
 void DwarfFDECache<A>::removeAllIn(pint_t mh) {
-  _LIBUNWIND_LOG_NON_ZERO(::pthread_rwlock_wrlock(&_lock));
+  _LIBUNWIND_LOG_IF_FALSE(_lock.lock());
   entry *d = _buffer;
   for (const entry *s = _buffer; s < _bufferUsed; ++s) {
     if (s->mh != mh) {
       if (d != s)
         *d = *s;
       ++d;
     }
   }
   _bufferUsed = d;
-  _LIBUNWIND_LOG_NON_ZERO(::pthread_rwlock_unlock(&_lock));
+  _LIBUNWIND_LOG_IF_FALSE(_lock.unlock());
 }
 
 #ifdef __APPLE__
 template <typename A>
 void DwarfFDECache<A>::dyldUnloadHook(const struct mach_header *mh, intptr_t ) {
   removeAllIn((pint_t) mh);
 }
 #endif
 
 template <typename A>
 void DwarfFDECache<A>::iterateCacheEntries(void (*func)(
     unw_word_t ip_start, unw_word_t ip_end, unw_word_t fde, unw_word_t mh)) {
-  _LIBUNWIND_LOG_NON_ZERO(::pthread_rwlock_wrlock(&_lock));
+  _LIBUNWIND_LOG_IF_FALSE(_lock.lock());
   for (entry *p = _buffer; p < _bufferUsed; ++p) {
     (*func)(p->ip_start, p->ip_end, p->fde, p->mh);
   }
-  _LIBUNWIND_LOG_NON_ZERO(::pthread_rwlock_unlock(&_lock));
+  _LIBUNWIND_LOG_IF_FALSE(_lock.unlock());
 }
 #endif // defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND)
 
 
 #define arrayoffsetof(type, index, field) ((size_t)(&((type *)0)[index].field))
 
 #if defined(_LIBUNWIND_SUPPORT_COMPACT_UNWIND)
 template <typename A> class UnwindSectionHeader {

libunwind/trunk/src/config.h

 
 #if defined(NDEBUG) && defined(_LIBUNWIND_IS_BAREMETAL)
 #define _LIBUNWIND_LOG(msg, ...)
 #else
 #define _LIBUNWIND_LOG(msg, ...)                                               \
   fprintf(stderr, "libunwind: " msg "\n", __VA_ARGS__)
 #endif
 
-#if defined(_LIBUNWIND_HAS_NO_THREADS)
-  // only used with pthread calls, not needed for the single-threaded builds
-  #define _LIBUNWIND_LOG_NON_ZERO(x)
-#else
-  #if defined(NDEBUG)
-    #define _LIBUNWIND_LOG_NON_ZERO(x) x
-  #else
-    #define _LIBUNWIND_LOG_NON_ZERO(x)                                         \
-      do {                                                                     \
-        int _err = x;                                                          \
-        if (_err != 0)                                                         \
-          _LIBUNWIND_LOG("" #x "=%d in %s", _err, __FUNCTION__);               \
-      } while (0)
-  #endif
-#endif
+  #define _LIBUNWIND_LOG_IF_FALSE(x) x
+  #define _LIBUNWIND_LOG_IF_FALSE(x)                                           \
+    do {                                                                       \
+      bool _ret = x;                                                           \
+      if (!_ret)                                                               \
+        _LIBUNWIND_LOG("" #x " failed in %s", __FUNCTION__);                   \
 
 // Macros that define away in non-Debug builds
 #ifdef NDEBUG
   #define _LIBUNWIND_DEBUG_LOG(msg, ...)
   #define _LIBUNWIND_TRACE_API(msg, ...)
   #define _LIBUNWIND_TRACING_UNWINDING (0)
   #define _LIBUNWIND_TRACING_DWARF (0)
   #define _LIBUNWIND_TRACE_UNWINDING(msg, ...)