https://github.com/labath commented:
It's somewhat better, but it doesn't address my main concern, which is this mid-scope switching of lock types. It's an unusual setup, and that's usually not good thing when it comes to locking and threads. I don't think that the fact that these are all reader APIs (for clients of StackFrameList) has anything to do with this, as the locking scheme is an implementation detail of the StackFrameList class (evidenced by the fact that you can change it without modifying any of the clients). The interface we're concerned with here is the one between `m_frames` member (and anything else being protected) and the code accessing it. And there are definitely writers there. I still believe that for a code like this to be correct, each piece of code covered by a single type of a lock needs to have a clearly defined purpose. And if it has a purpose, then it can be written as a scope (block or a function). Let's take a look at `FetchOnlyConcreteFramesUpTo` and `FetchFramesUpTo`. Their only caller is `GetFramesUpTo` and their called close together, which means scoping them should be fairly easy. Now, let's see what happens before this "write" block. Inside GetNumFrames, we basically only have a "have the frames been computed already" check, which is amenable to be put into a separate function block. `GetFramesUpTo` is called from two places. One of them is `GetNumFrames` which does nothing besides locking the mutex and them calling `GetFramesUpTo`. The other is `GetFrameAtIndexNoLock` which does another "has the frame been computed check" (is there any difference between the two, and can they be merged?) and then calls `GetFramesUpTo`. It doesn't lock the mutex though, so lets look at where that happens. This happens either in `GetFrameAtIndex`, where the pattern is again "lock the mutex and call the next function"; in `GetFrameWithConcreteFrameIndex`, which AFAICT is not doing anything that needs protecting, as its only working with the frames returned from `GetFrameAtIndex`; or in `GetFrameWithStackID`, which is doing a slightly different form of "has this frame been computed" check (via a binary search), and then proceeds to generate new frames (it starts calling `GetFrameAtIndexNoLock` with frame zero, but I think it could start with m_frames.size(), since it has checked the previous frames already). So it seems to me that at least everything that happens before taking the write lock could be expressed as some sort of a function or a block. I haven't looked that closely at what happens after releasing the write lock, but I believe the same could be done there as well (with the exception of one block, which I think actually needs to be protected by the write lock as well -- see inline comment). I think the code would look better if we did that. https://github.com/llvm/llvm-project/pull/117252 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
