mgorny marked an inline comment as done. mgorny added a comment. In D125575#3586534 <https://reviews.llvm.org/D125575#3586534>, @labath wrote:
> I think this is a pretty good start. The main thing bugging me is the two > bool flags on the stop reply packet functions. I wonder if it would be more > readable to split this into two functions. Something like: > > SendStopReplyForThread(bool force_synchronous /*seems better than > allow_async, but that could just be me*/, bool enqueue /*enqueues just this > thread*/); > EnqueueStopReplyPackets(NativeThreadProtocol &thread_to_skip); > > perhaps? `force_synchronous` makes sense. I guess moving the loop into another function does as well. However, I think the `bool enqueue` change is going to be more confusing than helpful (note that the notification is always enqueued through `SendNotificationPacketNoLock()` unless `force_synchronous`, so `bool enqueue` only applies for that case), and I don't really see how to avoid `bool queue_all_threads` other than repeating the calls to `EnqueueStopReplyPackets()` all over the place — which ofc is possible but seems like a lot of duplication. The thing is, we generally have three cases to handle: 1. A function that uses asynchronous replies in non-stop mode — i.e. we always enqueue, and send async notification if one wasn't sent already. 2. A function that always uses synchronous replies — i.e. we send plain old reply and don't queue anything. 3. `?` — which in non-stop mode enqueues for all threads but sends the first one synchronous rather than asynchronous. Perhaps the best approach would be to eliminate `bool enqueue` entirely and call `PrepareStopReplyPacketForThread()` from `?` handler directly. I need to think about it more. ================ Comment at: lldb/test/API/tools/lldb-server/TestLldbGdbServer.py:1414 + + def test_QNonStop(self): + self.build() ---------------- labath wrote: > Could you put these into a new file? TestLldbGdbServer is already one of the > longest running tests... Yes, I agree. `TestNonStop.py` coming once! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125575/new/ https://reviews.llvm.org/D125575 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
