labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.
Ok, looks good. Some extra suggestions inline.
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:490
+ response.GetString());
+ else
+ return SendPacketNoLock(response.GetString());
----------------
no else after return
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1592-1602
Status error = m_continue_process->Resume(actions);
if (error.Fail()) {
LLDB_LOG(log, "c failed for process {0}: {1}", m_continue_process->GetID(),
error);
return SendErrorResponse(GDBRemoteServerError::eErrorResume);
}
----------------
Could we have a helper function for this, and make all the continue-like
actions go through that?
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1753
+ // notifications.
+ for (auto it = m_stop_notification_queue.begin();
+ it != m_stop_notification_queue.end();) {
----------------
`llvm::erase_if(m_stop_notification_queue, []{ return !begins_with_W_or_X;});`
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:3264
// Notify we attached by sending a stop packet.
- return SendStopReasonForState(m_current_process->GetState());
+ return SendStopReasonForState(m_current_process->GetState(), false);
}
----------------
add `/*force_synchronous=*/` here and elsewhere.
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:3749
+ return SendErrorResponse(Status("No pending notification to ack"));
+ m_stop_notification_queue.pop_front();
+ if (!m_stop_notification_queue.empty())
----------------
mgorny wrote:
> labath wrote:
> > Is this correct? I would expect to this to first read (`front()`) the
> > packet before popping it.
> Yes, it is. I suppose it could be a bit misleading. The first message is sent
> as asynchronous notification but we keep it on the queue (and technically,
> the protocol assumes we may resend the asynchronous notification if we think
> client may have not gotten it). `vStopped` means client has processed it and
> is ready for the next one, so we pop the one that's been sent already and
> send the next one, wait for the next ACK and so on.
Yeah, it is confusing. Having a comment (with the explanation you just made)
would help a lot. However, if you don't see any use case for the resending, we
could also delete it, and use the standard queue pattern. Up to you...
================
Comment at: lldb/test/API/tools/lldb-server/TestLldbGdbServer.py:1443
+ m = self.expect_gdbremote_sequence()
+ del m["O_content"]
+ threads = [m]
----------------
mgorny wrote:
> labath wrote:
> > Is this necessary? (I get that it's not useful, but it seems weird...)
> It's convenient because it lets us compare the matches from `threads` and
> `threads_verify`.
Ok, I suppose I can live with that.
================
Comment at: lldb/test/API/tools/lldb-server/TestLldbGdbServer.py:1525
+ procs = self.prep_debug_monitor_and_inferior(
+ inferior_args=["thread:new"])
+ self.test_sequence.add_log_lines(
----------------
mgorny wrote:
> labath wrote:
> > Are you sure this is not racy (like, can the inferior terminate before we
> > get a chance to interrupt it)?
> Well, if I'm reading the code right, the new thread waits 60 seconds, so yes,
> technically it's racy ;-). Do you have a better idea? Some new argument that
> puts the process in an infinite sleep loop?
That's ok, a 60s wait is fine. I forgot there was a default sleep action -- I
though the process would quit immediately...
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125575/new/
https://reviews.llvm.org/D125575
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
