[Lldb-commits] [PATCH] D125575: [lldb] [llgs] Implement non-stop style stop notification packets

[Michał Górny via Phabricator via lldb-commits]

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