slydiman wrote: @labath
> For consistency, and to minimize the number of ifdefs, I believe windows and > non-windows paths should use as similar approaches as possible. That means I > think the end state should be that the posix path also uses a fork+exec > approach. fork+exec on non-Windows OS is redundant. It simply does not make sence. We can use multithreading or CreateProcess on Windows. It seems [the multithreading solution](https://github.com/llvm/llvm-project/pull/100670) faced a lot of issues and system's bugs on Linux. So CreateProcess is the only way. But it is the fork() workaround for Windows. > For me, it also seems natural that this should be the first step. While doing > that, we can make sure to create abstractions that can be used to implement > the windows behavior as well. I'm imagining some sort of a function or a > class, which takes the target binary, its arguments, and a socket, and > launches that binary with that socket. (we'll probably also need a similar > function/class on the other end to receive that socket). Look at GDBRemoteCommunication::StartDebugserverProcess(). I planed to extend it to use `pass_comm_fd` of Windows in the part 2. It is hard to create abstractions because of a lot of parameters. We can move most of SpawnProcessChild() code to `ConnectionFileDescriptor::ConnectFD` in ConnectionFileDescriptorPosix.cpp BTW, I don't like #ifdef _WIN32 in *Posix.cpp files. But currently the common ConnectionFileDescriptor.h contains `#include "lldb/Host/posix/ConnectionFileDescriptorPosix.h"` > I'm not sure if we really need the WriteWithTimeout functionality. The reason > we don't have it so far is that pipes have a buffer, and unless we're writing > a huge chunk of data, the write will never block anyway. There's nothing > wrong with it in principle though, and if you do want to have it, it'd be > best if it comes with in its own patch and with a test.) Before this patch the pipe read/write 1 byte in ConnectionFileDescriptor::CommandPipe. The maximal size was 6 bytes (the port value) in GDBRemoteCommunication::StartDebugserverProcess() and it is only the place where ReadWithTimeout() is used. But WSAPROTOCOL_INFO is big enough (628 bytes). Usually the pipe buffer is 4K. But we need an ability to avoid hangs anyway. I agree that all pipe related changes may be moved to another patch. And we need to add WriteWithTimeout() to PipePosix too. Note there is not any test for Pipe now. And I have no idea how to test it, especially async version. Running `lldb-server platform --server` on Windows is the best test. It must be tested with a high load with multiple connections. Otherwise any pipe tests are useles. https://github.com/llvm/llvm-project/pull/101283 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
