Upcoming change with how libc++, libc++abi and libunwind are being built

Project Infrastructure LLVM Dev List Archives
21

Well, the example I gave doesn't match that, I believe. This is a value only written on Thread 1, and read on Thread 1 and on other threads. The requirements for locking would be the lock around all writes (on thread 1), and lock around all reads from threads *other* than thread 1. Reads from Thread 1 don't need to lock, since they're on the only thread that ever writes to the value. This is conceptually GUARDED_BY(mMutex || OnThread1).

The case you're referencing sounds more like "value is freely written and read on thread 1, then at some point later access to it is provided to other threads, and after that happens access is controlled by a mutex" - where the protection regime changes over time (and may also correlate with specific methods, like an Init() method).

We have another pattern that's interesting, where a method may use Maybe<MutexAutoLock> lock(std::in_place, mMutex); (Maybe<> is effectively std::optional). This allows us to lock.reset() instead of using MutexAutoUnlock lock(mLock), which will lead to an extra lock/unlock pair compared to Maybe<> and lock.reset().

We also have an RAII MutexAutoTryLock class, which is like MutexAutoLock but does a trylock operation, and also you can convert it to bool to see if it obtained the lock. I don't think this is easily representable in the current system. (I'll note that we use it 4 times in our codebase, and 3 of those are in Mutext test code, so I don't care that much :wink: .)

Randell Jesup, Mozilla

22

Thanks for letting me know. (I barely follow the list.)

One way to possibly handle the reader/writer vs readers case (where reads on the writing thread don't need to lock) would be to be able to say "guarded by this or that", in this case something like GUARDED_BY(mMutex, MainThread) (GUARDED_BY(mMutex || MainThread) ??).

There has been some work on logical expressions in capability attributes in rG7c192b452fa2, but I believe it's not functional yet. I thought about the semantics of this a bit, but don't have a good understanding yet.

If you want accesses to a resource to be protected, it's in general not sufficient to have one of a set of capabilities. There can only be one. What happens in this scenario is that there is a certain period of time where the resource is exclusive to the main thread, and another where it's only accessible via mutex (even if the main thread were to access it). So maybe logical expressions aren't the right way to understand this situation, but rather there should be different types, one that has GUARDED_BY(MainThread) and another that has GUARDED_BY(mMutex), and at some point we convert between them. In some sense the protection regime changes throughout the lifetime, and for static analysis that means we need a new (static) type.

Well, the example I gave doesn't match that, I believe. This is a value only written on Thread 1, and read on Thread 1 and on other threads. The requirements for locking would be the lock around all writes (on thread 1), and lock around all reads from threads *other* than thread 1. Reads from Thread 1 don't need to lock, since they're on the only thread that ever writes to the value. This is conceptually GUARDED_BY(mMutex || OnThread1).

The case you're referencing sounds more like "value is freely written and read on thread 1, then at some point later access to it is provided to other threads, and after that happens access is controlled by a mutex" - where the protection regime changes over time (and may also correlate with specific methods, like an Init() method).

You're absolutely right, I was misreading your case.

What you're describing sounds in fact like thread 1 always has a shared lock, which it sometimes promotes to exclusive lock, then demotes to a shared lock again. The other threads can only acquire shared locks, because thread 1 doesn't ever release its shared lock. Now there are multiple ways to write that down:

  * Use an actual “multiple readers / single writer” lock, also known as
    read/write lock. Thread 1 never releases it. Then we can always
    read, and we block other threads from ever acquiring a write lock.
    They can still get a read lock though if we don't have the write lock.
  * If that's not desirable, for example because multiple readers aren't
    needed, or a read/write lock would be overkill, build a “fake
    read/write lock” that looks like a read/write lock to the thread
    safety analysis, but is actually implemented via mutex. It would
    have different entry points for the different threads:
      o LockMain acquires the mutex and is annotated RELEASE_SHARED()
        ACQUIRE(), perhaps REQUIRES(main).
      o UnlockMain releases the mutex and is annotated RELEASE()
        ACQUIRE_SHARED(), perhaps REQUIRES(main).
      o LockOther acquires the mutex and is annotated ACQUIRE_SHARED().
      o UnlockOther releases the mutex and is annotated RELEASE_SHARED().

There might be more options, but you get the idea. Conceptually you don't have multiple capabilities, you have just one. The key is which modes (shared/exclusive) they are held in, and who can acquire that capability in which mode.

The problem with booleans is that they mess with exclusivity: mMutex || OnThread1 seems to imply that either of them is fine. But the other threads can't get write access even if they have the mutex. In fact the mutex gives different threads different levels of access. That's why I think you actually have a (possibly specialized) read/write lock here. And note that the second option should be a zero-cost abstraction around a mutex.

We have another pattern that's interesting, where a method may use Maybe<MutexAutoLock> lock(std::in_place, mMutex); (Maybe<> is effectively std::optional). This allows us to lock.reset() instead of using MutexAutoUnlock lock(mLock), which will lead to an extra lock/unlock pair compared to Maybe<> and lock.reset().

Supporting that would be hard, because we'd need to look into the Maybe type, and then the analysis isn't local anymore. However, the analysis supports “premature unlocking” of scopes, i.e. you can write lock.Unlock(). I've even added support for “relockable scopes”, where you can lock.Lock() again. (Have a look at the MutexLocker in Thread Safety Analysis — Clang 18.0.0git documentation to see what is possible.)

The philosophy that we follow with scope types is that they should work as local variables whose lifetime is determined by a scope. Anything else is “out of scope”, which includes types like optional that invoke the destructor manually. Also the scopes (at least for now) need to be bound to a fixed set of mutexes for their entire lifetime.

We also have an RAII MutexAutoTryLock class, which is like MutexAutoLock but does a trylock operation, and also you can convert it to bool to see if it obtained the lock. I don't think this is easily representable in the current system. (I'll note that we use it 4 times in our codebase, and 3 of those are in Mutext test code, so I don't care that much :wink: .)

Same problem. We support a scope with deferred locking and then try-acquiring that. To stay in the language of mutex.h:

MutexLocker lock(&mu, defer_lock);
if (lock.TryLock())
...

If the try-acquire is hidden in the constructor, and we obtain the value only later, that seems hard to support. (For starters, we'd need additional attributes because TRY_ACQUIRE wants a return value.)

If you're worried about scope, you can with C++17 write

if (MutexLocker lock(&mu, defer_lock); lock.TryLock())
...

which I guess looks almost like what you can do with MutexAutoTryLock.

Aaron

23

Well, the example I gave doesn't match that, I believe. This is a value only written on Thread 1, and read on Thread 1 and on other threads. The requirements for locking would be the lock around all writes (on thread 1), and lock around all reads from threads *other* than thread 1. Reads from Thread 1 don't need to lock, since they're on the only thread that ever writes to the value. This is conceptually GUARDED_BY(mMutex || OnThread1).

The case you're referencing sounds more like "value is freely written and read on thread 1, then at some point later access to it is provided to other threads, and after that happens access is controlled by a mutex" - where the protection regime changes over time (and may also correlate with specific methods, like an Init() method).

You're absolutely right, I was misreading your case.

What you're describing sounds in fact like thread 1 always has a shared lock, which it sometimes promotes to exclusive lock, then demotes to a shared lock again. The other threads can only acquire shared locks, because thread 1 doesn't ever release its shared lock. Now there are multiple ways to write that down:

You're correct, that's a reasonable way to model this pattern using locks for everything. The pattern I mentioned is data-race safe (it shouldn't trigger tsan alerts), but doesn't allow for validation of safety. It depends on the developers following the rules for that variable.

We have another pattern that's interesting, where a method may use Maybe<MutexAutoLock> lock(std::in_place, mMutex); (Maybe<> is effectively std::optional). This allows us to lock.reset() instead of using MutexAutoUnlock lock(mLock), which will lead to an extra lock/unlock pair compared to Maybe<> and lock.reset().

Supporting that would be hard, because we'd need to look into the Maybe type, and then the analysis isn't local anymore. However, the analysis supports “premature unlocking” of scopes, i.e. you can write lock.Unlock(). I've even added support for “relockable scopes”, where you can lock.Lock() again. (Have a look at the MutexLocker in Thread Safety Analysis — Clang 18.0.0git documentation to see what is possible.)

I've checked, and the reason for this pattern is basically as I mentioned; to allow for unlocking of an RAII lock without an added lock/unlock pair on exit of the scope. If we can do "MutexAutoLock lock(mMutex); ...; if (foo) { lock.Unlock(); MethodThatWeCantHoldLockIn(); return; } ..." then we don't need the Maybe<> stuff.

I presume support for an RAII unlocker isn't likely soon.

Thanks again, this has been very helpful.

Randell Jesup, Mozilla

24

Well, the example I gave doesn't match that, I believe. This is a value only written on Thread 1, and read on Thread 1 and on other threads. The requirements for locking would be the lock around all writes (on thread 1), and lock around all reads from threads *other* than thread 1. Reads from Thread 1 don't need to lock, since they're on the only thread that ever writes to the value. This is conceptually GUARDED_BY(mMutex || OnThread1).

The case you're referencing sounds more like "value is freely written and read on thread 1, then at some point later access to it is provided to other threads, and after that happens access is controlled by a mutex" - where the protection regime changes over time (and may also correlate with specific methods, like an Init() method).

You're absolutely right, I was misreading your case.

What you're describing sounds in fact like thread 1 always has a shared lock, which it sometimes promotes to exclusive lock, then demotes to a shared lock again. The other threads can only acquire shared locks, because thread 1 doesn't ever release its shared lock. Now there are multiple ways to write that down:

You're correct, that's a reasonable way to model this pattern using locks for everything. The pattern I mentioned is data-race safe (it shouldn't trigger tsan alerts), but doesn't allow for validation of safety. It depends on the developers following the rules for that variable.

The downside of using a rwlock is performance and fairness; c++ doesn't guarantee any specific fairness to rwlocks, and they have lower performance/higher complexity than normal locks/mutexes. In particular, an implementation could let readers starve writers... though if the same thread holds a read lock, can it obtain a write lock? Only I think in rwlocks that allow upgrades. Otherwise it must release the read lock, grab the write lock, then release it and get the read lock back.

The other alternatives are to lock exclusively around each access (including on the writing thread), or ignore static analysis warnings if we're on the writing thread, or annotate with an assertion that quiets the warnings, such as AssertOnWritingThread(), which has ASSERT_EXCLUSIVE_LOCK/ASSERT_CAPABILITY (perhaps only in DEBUG mode).

We have another pattern that's interesting, where a method may use Maybe<MutexAutoLock> lock(std::in_place, mMutex); (Maybe<> is effectively std::optional). This allows us to lock.reset() instead of using MutexAutoUnlock lock(mLock), which will lead to an extra lock/unlock pair compared to Maybe<> and lock.reset().

Supporting that would be hard, because we'd need to look into the Maybe type, and then the analysis isn't local anymore. However, the analysis supports “premature unlocking” of scopes, i.e. you can write lock.Unlock(). I've even added support for “relockable scopes”, where you can lock.Lock() again. (Have a look at the MutexLocker in Thread Safety Analysis — Clang 18.0.0git documentation to see what is possible.)

I've checked, and the reason for this pattern is basically as I mentioned; to allow for unlocking of an RAII lock without an added lock/unlock pair on exit of the scope. If we can do "MutexAutoLock lock(mMutex); ...; if (foo) { lock.Unlock(); MethodThatWeCantHoldLockIn(); return; } ..." then we don't need the Maybe<> stuff.

Writing a MaybeMutexAutoLock class actually works fairly well for this case, and avoids Maybe<>/std::optional, and lets us avoid the extra bool for all the MutexAutoLocks (which appear ~50-100x+ the frequency of MaybeMutexAutoLock).

Randell Jesup, Mozilla

25

We support that since ⚙ D52578 Thread safety analysis: Allow scoped releasing of capabilities. (See the tests for how to use it.)

Interestingly that was actually requested by your friendly competitors at Chromium (https://bugs.llvm.org/show_bug.cgi?id=36162), but unless I'm missing something, they're not using it yet.

Aaron

26

The downside of using a rwlock is performance and fairness; c++ doesn't guarantee any specific fairness to rwlocks, and they have lower performance/higher complexity than normal locks/mutexes. In particular, an implementation could let readers starve writers... though if the same thread holds a read lock, can it obtain a write lock? Only I think in rwlocks that allow upgrades. Otherwise it must release the read lock, grab the write lock, then release it and get the read lock back.

The other alternatives are to lock exclusively around each access (including on the writing thread), or ignore static analysis warnings if we're on the writing thread, or annotate with an assertion that quiets the warnings, such as AssertOnWritingThread(), which has ASSERT_EXCLUSIVE_LOCK/ASSERT_CAPABILITY (perhaps only in DEBUG mode).

Yes, true read/write locks might not work for you. But as I wrote, a capability doesn't have to be an actual mutex, it's just some kind of mechanism that regulates read/write accesses. If you could wrap your existing mechanism into a class that gives the main thread perpetual read access (perhaps via ASSERT_SHARED_CAPABILITY, as you suggested) and write access on acquisition, and the other threads read access on acquisition, that's fine. It needs different entry points (because we're doing static analysis), but that's it.

As an unrelated example, I work in a code base where we have "speculative locks" that work with hardware transactional memory. They actually allow multiple threads write access at the same time. But that's no problem for the analysis, because exclusivity (or perhaps rather the avoidance of races/conflicts) is the capability's job. The analysis only checks that the capability is acquired for the access.

Writing a MaybeMutexAutoLock class actually works fairly well for this case, and avoids Maybe<>/std::optional, and lets us avoid the extra bool for all the MutexAutoLocks (which appear ~50-100x+ the frequency of MaybeMutexAutoLock).

That's good to hear! For what it's worth, we've made pretty good experiences with more powerful lock scopes (they allow unlocking, relocking, promotion and demotion). Sometimes for example there are loops where the lock has to be temporarily released. Since we're using exceptions, we can't use an RAII unlocker. (And we probably wouldn't want that because of possibly unnecessary relocking on scope exits.) So we just call scope.Unlock(), then later scope.Lock(), and the lock will be released by the destructor if it's still held, but not if it isn't.

Aaron

27

I've tried this.... it works better than without it, but there are some oddities:

In the test code (that's the only documentation for this?) both MutexAutoUnLock() and ~MutexAutoUnlock() are marked as EXCLUSIVE_UNLOCK_FUNCTION()?? But if I make ~MutexAutoUnlock() EXCLUSIVE_LOCK_FUNCTION() (which would make sense), I get more errors. This is at minimum confusing. Perhaps specific attributes for RAII unlockers? (even if the #define to the same thing... though personally I wouldn't)

I still get a warning that "mutex 'mMutex' is still held at the end of function" if I have:

void foo() {

mMutex\.AssertCurrentThreadOwns\(\);  // where AssertCurrentThreadOwns\(\) has ASSERT\_EXCLUSIVE\_LOCK\(this\)

\.\.\.\.

\{

    MutexAutoUnlock unlock\(mMutex\);

    \.\.\.\.

\}

\.\.\.\.

}

Thanks for any help,

Randell Jesup, Mozilla

}

28

I've tried this.... it works better than without it, but there are some oddities:

In the test code (that's the only documentation for this?) [...]

Yes, for now it's not documented. The pattern is a bit weird, and we're not sure if and how we should support it.

[...] both MutexAutoUnLock() and ~MutexAutoUnlock() are marked as EXCLUSIVE_UNLOCK_FUNCTION()??

Almost. The constructor will mention the lock argument, so it's something like MutexAutoUnlock(Mutex &mutex) EXCLUSIVE_UNLOCK_FUNCTION(mutex), while the destructor's attribute will have no argument: ~MutexAutoUnlock() EXCLUSIVE_UNLOCK_FUNCTION(). Omitting the argument implies an implicit 'this', so the destructor is releasing the scoped capability.

When I designed this I extrapolated the handling of regular scoped locks, see 36162 – Unlocking equivalent to scoped_lockable for Thread Safety Analysis and 33504 – Thread Safety Analysis [-Wthread-safety-analysis] work wrong with shared lock. I guess this isn't ideal, and occasionally I have thoughts about changing that, but it's not easy without breaking existing code. (I don't think anybody is using scoped unlocking yet, since it's undocumented, but for consistency we'd also need to change scoped locking.)

But if I make ~MutexAutoUnlock() EXCLUSIVE_LOCK_FUNCTION() (which would make sense), I get more errors.

Exactly, that won't work.

I still get a warning that "mutex 'mMutex' is still held at the end of function" if I have:

void foo() {
mMutex.AssertCurrentThreadOwns(); // where AssertCurrentThreadOwns() has ASSERT_EXCLUSIVE_LOCK(this)
....
{
MutexAutoUnlock unlock(mMutex);
....
}
....
}

Seems like a bug, though not a surprising one. Asserted locks behave a bit strangely, because they don't have to (in fact should not) be released, and so we don't want in general allow them to join with acquired locks (which you have at the end of this scope). In this example it's fine though. We discussed about asserted locks a bit in ⚙ D102026 Thread safety analysis: Allow exlusive/shared joins for managed and asserted capabilities, and probably other places, but didn't develop a full understanding yet. Perhaps you can just file a bug? Otherwise I can also do it. There are some places where we should warn and others where we shouldn't, and perhaps they all have the same root cause.

Best regards,
Aaron

29

@ldionne One thing I am confused on is how much is the intent here to just merge libc++, libc++, and/or libunwind?

For context, I am a bit partial to the standalone builds; in my view the standalone build and the runtimes build should be very similar — as indeed the old HandleOutOfTreeLLVM.cmake and new runtimes/CMakeLists.txt are very similar. I am also optimistic that by making snippets of CMake factored out in in cmake/ idempotent, we could support both a runtimes combined build and standalone builds without the huge if(${PROJECT}_STANDALONE) blocks that D125561 removed, and in fact have largely unconditional code per project (with the conditions instead being the idempotency tricks).

I think that this technique to avoid project-side conditionals would basically solve the maintainability problem — and indeed, I wouldn’t suggest putting back standalone builds unless it for sure wouldn’t reintroduce maintainability problems.

Also, if indeed the goal is to require libc++, libc++, and/or libunwind to always be built together, should they just be merged into the libcxx/ top-level directory? That would even more strongly indicate that the days of treating them as separate things are over — and would have led me not bother attempting D132411 for example.

I feel a little sheepish suggesting things after I screwed up breaking a bunch of builds and didn’t immediately revert. But this is a broader discussion I was hoping to have the whole time, and I suppose the disruption has “paged in” the topic at hand to more people’s brains, so I might as post it now. Cheers.

30

I don’t think there was an intent of merging the three libraries, since some people want to build only one of them. I am also not in favour of supporting standalone builds (which we just spent years removing!) unless there are significant benefits to those over the runtimes build we have right now.

That being said, if we managed to refactor the junction points between libcxx, libcxxabi and libunwind (e.g. similarly to what we did in HandleLibCXXABI) to the point where they could build separately, I would not be opposed to it just out of principle.

In other words, I am against:

  1. Introducing yet another way to build the runtimes when we are managing just now to streamline that after years of work. That would confuse users and do more bad than good IMO.
  2. Adding complexity to the build system.
  3. Duplicating code in the build system.

However, I am not against the idea of building projects as standalone, in principle. It just so happens that I don’t think it’s possible to satisfy (1), (2) and (3) while reintroducing standalone builds. That being said, if someone can make an argument for why standalone builds offer functionality that the runtimes build doesn’t achieve, perhaps these (1-3) criteria need to be re-evaluated. Also, that’s just my .02.

Please don’t. You didn’t screw anything up. 95% of what happened when ⚙ D132324 [RFC] Remove support for building libc++ with `LLVM_ENABLE_PROJECTS` first landed would have happened no matter how and when it landed, and that was just necessary to get an understanding of what would break when we made the change. It’s just how these things are.

1 Like
31

Thanks for the nice reply, @ldionne.

If (1) is satisfied by being still net fewer entry points, or or net more de-duplication (really good attack on 3), I think those are fine criteria, and I think what I have in mind ought to satisfy them if it is any good.

I guess the big challenge remaining regardless is wrangling compiler-rt to be like the other. IIUC, the “projects” build doesn’t bootstrap libc++ and friends (as one would expect, but my some pile hacks it does in fact bootstrap compiler-rt? Once that is a regular library I think everything will be a little less weird and that will free up some mental bandwith for dealing with other things.

That being said, if we managed to refactor the junction points between libcxx, libcxxabi and libunwind (e.g. similarly to what we did in HandleLibCXXABI) to the point where they could build separately, I would not be opposed to it just out of principle.

Woo!

I don’t think there was an intent of merging the three libraries, since some people want to build only one of them.

Ah OK. Then in that case maybe it is good to fix building libc++abi alone after all? I have a new version of that I’ll post in a second that doesn’t bother with any IN_LIST annoyances, and also tries to mimic the HandleLibCXXABI design.

32

Yes, indeed compiler-rt is a weird beast. Ideally, it would behave like the other runtimes, but someone basically needs to jump in and do the work. I’m not super familiar with the details of how its build is set up, so I don’t know how much work that represents.

My original reply was not super precise. I know that:

  • people want to build just libc++ against a custom ABI/unwinding library.
  • people want to build libunwind and libunwind only

I don’t think anyone tries to build libc++abi in isolation. I could be wrong, but I think libc++abi is always built against libc++, and in fact I think it might be impossible to do otherwise. In that sense, I guess one could technically merge libcxxabi/ into libcxx/ and make that some kind of optional target. When desired, one would build the ABI library as part of the libcxx project. If one wants to link e.g. against a system ABI library, one would disable that optional part of libcxx.

Anyways, I’m not sure this is worth pursuing just for the sake of it – I don’t see a lot of benefits for being able to do that. IMO, if you want to tackle some clean up in our build, I would really go for compiler-rt. That’s by far the biggest can of worms and also the one with the fewest domain experts AFAICT.

1 Like