This is an archive of the discontinued LLVM Phabricator instance.

[lldb] [llgs] Improve stdio forwarding in multiprocess+nonstop
ClosedPublic

Authored by mgorny on Jun 30 2022, 11:26 AM.

Details

Summary

Enable stdio forwarding when nonstop mode is enabled, and disable it
once it is disabled. This makes it possible to cleanly handle stdio
forwarding while running multiple processes in non-stop mode.

Sponsored by: The FreeBSD Foundation

Diff Detail

Event Timeline

mgorny created this revision.Jun 30 2022, 11:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2022, 11:26 AM
Herald added a subscriber: arichardson. · View Herald Transcript
mgorny requested review of this revision.Jun 30 2022, 11:26 AM
mgorny added inline comments.Jun 30 2022, 11:27 AM
lldb/test/API/tools/lldb-server/TestGdbRemoteForkNonStop.py
119

I really dislike these sleeps but I can't think of a better way of doing it (short of using IPC for synchronization). The goal is to 1) ensure that both processes start before they start outputting, and 2) ensure that both output before the first stop reason comes.

mgorny updated this revision to Diff 441624.Jul 1 2022, 12:35 AM

Use semaphores to sync instead of relying on ugly sleeps.

lldb/test/API/tools/lldb-server/TestGdbRemoteForkNonStop.py
119

Ok, semaphores are not scary after all, and I suppose we can expect them to work if we expect fork() to work.

mgorny updated this revision to Diff 441664.Jul 1 2022, 4:39 AM

Fix sem_destroy() assertions.

labath added a comment.Jul 6 2022, 1:35 AM

I have a feeling the semaphores will not work (compile) on darwin. I didn't find any sem_init call there -- just sem_open. Maybe instead of semaphores we could use files for the synchronization? Something similar to the wait_for_file_on_target function, just in the other direction. Then the test could create a file to move the processes forward? (This might be easier to achieve with a dedicated inferior, instead of trying to fit it into the universal LLGS inferior.)

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
1116–1117

In the multiprocess all-stop mode, if one process stops, we are supposed to stop all of them, right? I take it that's not something we do right now, is it?

lldb/test/API/tools/lldb-server/TestGdbRemoteForkNonStop.py
119

ensure that both processes start before they start outputting

But we should be able to see the output from the first process (if it had any) even it is the only process running. Do we have a test for that? Could you add a step where the first process outputs something before it waits to synchronize with the second process?

mgorny added a comment.Jul 6 2022, 2:28 AM

I have a feeling the semaphores will not work (compile) on darwin. I didn't find any sem_init call there -- just sem_open. Maybe instead of semaphores we could use files for the synchronization? Something similar to the wait_for_file_on_target function, just in the other direction. Then the test could create a file to move the processes forward? (This might be easier to achieve with a dedicated inferior, instead of trying to fit it into the universal LLGS inferior.)

Damn, and I thought POSIX actually means something to Darwin. Yeah, I'll look into using some other synchronization mechanism.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
1116–1117

We don't support resuming multiple processes in all-stop mode. vCont returns an error if you try to do that, and other continue packets are blocking, so it can't happen ;-).

lldb/test/API/tools/lldb-server/TestGdbRemoteForkNonStop.py
119

Yes, I suppose covering that with a test also makes sense.

mgorny added inline comments.Jul 6 2022, 7:00 AM
lldb/test/API/tools/lldb-server/TestGdbRemoteForkNonStop.py
119

Actually, this is already covered by TestNonStop.py::test_stdio.

labath added inline comments.Jul 6 2022, 7:10 AM
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
1116–1117

ok, got it.

mgorny updated this revision to Diff 442602.Jul 6 2022, 9:05 AM

Replace semaphores with completely non-fancy file-based locking.

labath accepted this revision.Jul 7 2022, 4:00 AM

Ok, let's give this a shot.

lldb/test/API/tools/lldb-server/main.cpp
351

I'm not convinced this will be enough. How about 125 * (1<<i) ?

This revision is now accepted and ready to land.Jul 7 2022, 4:00 AM
mgorny added inline comments.Jul 7 2022, 9:10 AM
lldb/test/API/tools/lldb-server/main.cpp
351

Sure, I just tried to keep it simple ;-).

mgorny updated this revision to Diff 444296.Jul 13 2022, 9:06 AM

Rebased.

This revision was landed with ongoing or failed builds.Jul 15 2022, 1:12 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2022, 1:12 PM
mgorny added a comment.Aug 1 2022, 8:16 AM

Flake here: https://lab.llvm.org/buildbot/#/builders/68/builds/36967.

Presumably the same problem that cd18e2ea3f4e87f8804a7d6661d5596ef1f07b81 fixed for TestNonStop.

Thanks for the ping and the suggestion. I'm testing a fix right now and will push if it works ;-).