This is an archive of the discontinued LLVM Phabricator instance.

[lldb] [gdb-remote] Send interrupt packets from async thread
AcceptedPublic

Authored by mgorny on Aug 3 2022, 9:13 AM.

Details

Summary

Refactor the mechanism for sending interrupt packets to send them
from async thread (i.e. the same thread that sends the continue packet
preceding them and that waits for the response), rather than from
the thread requesting the interrupt. This is going to become especially
important when using the vCtrlC packet as part of the non-stop protocol,
as -- unlike the simple ^c sent in the all-stop mode -- this packet
involves an explicit reply.

Suggested by Pavel Labath in D126614.

Sponsored by: The FreeBSD Foundation

Diff Detail

Event Timeline

mgorny created this revision.Aug 3 2022, 9:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 3 2022, 9:13 AM
Herald added a subscriber: arichardson. · View Herald Transcript
mgorny requested review of this revision.Aug 3 2022, 9:13 AM
labath accepted this revision.Aug 3 2022, 9:30 AM

Looks good. Please pay special attention to the windows bot when landing.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp
55–63

I think this could be simplified now (to avoid the intermittent wakeups), but those wakeups could be the only thing that makes windows work...

This revision is now accepted and ready to land.Aug 3 2022, 9:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 3 2022, 9:40 AM
mgorny added a comment.Aug 3 2022, 9:41 AM

Looks good. Please pay special attention to the windows bot when landing.

Thanks. Will do.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp
55–63

If Windows relies on this, we'll soon find out by seeing a few tests failing due to interrupts taking too long (I've seen this myself prior to adding the InterruptRead() call).

mgorny reopened this revision.Aug 3 2022, 10:11 AM

Yep, you were right. It broke on Windows ;-).

https://lab.llvm.org/buildbot/#/builders/83/builds/22075

Failed Tests (6):
  lldb-unit :: Process/gdb-remote/./ProcessGdbRemoteTests.exe/GDBRemoteClientBaseTest/InterruptNoResponse
  lldb-unit :: Process/gdb-remote/./ProcessGdbRemoteTests.exe/GDBRemoteClientBaseTest/SendContinueAndAsyncPacket
  lldb-unit :: Process/gdb-remote/./ProcessGdbRemoteTests.exe/GDBRemoteClientBaseTest/SendContinueAndAsyncSignal
  lldb-unit :: Process/gdb-remote/./ProcessGdbRemoteTests.exe/GDBRemoteClientBaseTest/SendContinueAndInterrupt
  lldb-unit :: Process/gdb-remote/./ProcessGdbRemoteTests.exe/GDBRemoteClientBaseTest/SendContinueAndInterrupt2PacketBug
  lldb-unit :: Process/gdb-remote/./ProcessGdbRemoteTests.exe/GDBRemoteClientBaseTest/SendContinueAndLateInterrupt
This revision is now accepted and ready to land.Aug 3 2022, 10:11 AM

Yeah, I can't say I'm surprised by that. It's fairly annoying that such basic functionality is missing there. Let me see what I can do about that.

labath added a comment.Aug 4 2022, 5:41 AM

Ok, I was able to concoct D131159 and D131160 this morning. They need some more cleanup, but I tried them on windows, and it worked. The general idea is to add a platform-neutral way to wait on "events" and FDs (unfortunately, this is still limited to sockets on windows) at the same time. This could then be used instead of the pipe+select interruption implementation in the Connection class.

With D131159 and D131160 in, where does that leave us wrt this change?