This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by mstorsjo on Oct 9 2017, 1:32 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

mstorsjo created this revision.Oct 9 2017, 1:32 PM
zturner added a subscriber: zturner.Oct 9 2017, 2:03 PM

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.

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.

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?

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.

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 updated this revision to Diff 119094.Oct 15 2017, 12:50 PM
mstorsjo retitled this revision from [libunwind] Emulate pthread rwlocks via SRW locks for windows to [libunwind] Abstract rwlocks into a class, provide a SRW lock implementation for windows.
mstorsjo edited the summary of this revision. (Show Details)
mstorsjo added reviewers: majnemer, zturner.
mstorsjo removed subscribers: majnemer, zturner.

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.

zturner accepted this revision.Oct 23 2017, 9:17 AM

This looks much better than the previous solution. Thanks!

This revision is now accepted and ready to land.Oct 23 2017, 9:17 AM

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.

This revision was automatically updated to reflect the committed changes.