[Lldb-commits] [PATCH] D23802: gdb-remote: Make the sequence mutex non-recursive

[Pavel Labath via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D23802#523244, @clayborg wrote:

> The old mutex was there so that you could send a packet and get a result without worrying about other threads getting your response. It was recursive for the case where we needed to send two packets as one (set thread ID, then send packet), and for reading all registers (as you have found), so your patch does work, but it now only for these special cases. Now there is no way to easily take the mutex and send 5 different packets without going and adding new NoLock variants of each call that you might want to send as a single stream of packets. Can we think slightly cleaner way to do this? Before you could stake the sequence mutex in your main function that wanted to make N calls, and each call would then recursively lock the mutex and everything just worked. Now you will deadlock if you do things incorrectly (granted you will need to do a "GDBRemoteClientBase::Lock lock(gdb_comm, false);" followed by a "m_gdb_comm.SendSomePacket()".


I see what you mean. I have considered a couple of other options, but none of them struck me as obviously best. Alternatives I see are:

- have all functions (or just those that we need right now) take an optional `lock` boolean parameter (probably defaulting to true). If it is set the function takes a lock, if not, it doesn't. The main downside I see here is that then the locking behaviour would depend on the run-time parameter, which is not completely ideal. OTOH, it would sort of sit along nicely with the "send_async" parameter, which also controls behavior in a similar way.
- just make all functions `NoLock` and require the user to lock manually. The tricky part here is that unlike a normal lock, locking the packet lock can fail, so you end up with code like `Lock lock; if(!lock) {if(log) log->Printf("getting registers failed");} else { ... }`, which doesn't look nice but we could make it better. If we move the logging part into the `Lock` object, then this would end up with: `Lock lock(gdb_comm, false, "getting registers"); if (!lock) return; ...` which I don't think is too bad.
- like before, but only do it for functions which are used from locked and unlocked contexts. Then the policy would be that if you need a NoLock version of a function, instead of creating a new one, you rename the current one and change all callers (I don't expect that there will be so many of them) to lock explicitly. This does not require any major rewrite, but it is a bit messy because you have two kinds of functions. (This is not really an alternative, just an extension of the current approach.)
- write our own implementation of a **recursive** read/write lock. This may not be so difficult even (although I can see how some edge cases might be tricky -- e.g. we have a read lock (so we can share the connection with other threads), but then we need to send a packet which wants exclusive access), and it would nicely compartmentalize all the ugliness. However, I am kinda against inventing locking primitives (even though the Lock class is such a thing, but there I think it was justified).

What do you think?


https://reviews.llvm.org/D23802