[Lldb-commits] m_private_run_lock relying on undefined pthread behaviour

[Ed Maste]

On 18 June 2013 18:45, Greg Clayton <gclayton at apple.com> wrote:
> Yes we do rely upon being able to lock on one thread and unlock on another. Good catch, let us know what you come up with.
>
> Greg

I finally had a chance to look at this, and have a proof of concept
change.  Here's the commit message from my tree:

| LLDB requires that the inferior process be stopped before, and remain
| stopped during, certain accesses to process state.
|
| Previously this was achieved with a wrlock that was held while the
| process was running, and released while the process was stopped.
| Any access toprocess state was performed with a read lock held.
|
| POSIX requires that pthread_rwlock_unlock() be called from the same
| thread as pthread_rwlock_wrlock(), but lldb needs to stop and start the
| process from different threads.
|
| All read lock consumers use ReadTryLock() and handle failure to obtain
| the lock (typically by logging an error "process is running").  Thus,
| instead of using the lock state itself to track the running state, add an
| explicit m_running flag.  ReadTryLock tests the flag, and keeps the read
| lock held if available.  WriteLock and WriteTryLock (if successful) set
| m_running with the lock held.  This way, read consumers can determine
| if the process is running and act appropriately, and write consumers are
| still held off from starting the process if read consumers are active.
|
| Note that with this change there are still some questionable access
| patterns, such as calling WriteUnlock twice in a row, and there's no
| protection from multiple threads trying to simultaneously start the
| process (i.e., take the write lock).  In practice this does not seem to be
| a problem, and was already exposing undefined POSIX behaviour.
|
| This is a proof of concept patch, and should be cleaned up further
| assuming the approach is sound -- at least, ReadLock / WriteLock
| should be renamed.

For now I wanted to avoid changing the Process class, so left the
interface unchanged.

I also removed the unused Process:RunLocker, WriteLocker class, and
unused Lock and ReadLocker functionality.

The commits are here (the last is the functional change):
https://github.com/fbsd/lldb/commit/b9471fe7e5c1e614ccfc73e7d451e4ed16ca391f
https://github.com/fbsd/lldb/commit/e89ccd72b42a3d572cb9924bdd0c256d7383129a
https://github.com/fbsd/lldb/commit/9566f404a35274d68cd29a183cff80cbe864bff7

-Ed