amccarth marked an inline comment as done. amccarth added a comment. Thanks for the review.
In D60152#1452704 <https://reviews.llvm.org/D60152#1452704>, @labath wrote: > FTR, I believe using pipes here is wrong altogether because it can lead to > deadlock. The size of the pipe buffer is implementation-defined, and since > there's noone reading from it while we write it, we can block indefinitely if > we encounter a particularly long sequence of -o commands. I guess the fact > that we haven't hit this in practice means that the implementation-defined > size is sufficiently big on all OSs we care about. Agreed, pipes probably aren't the right way, but this was just a drive-by cleanup. On Windows, the necessary size is passed to the `_pipe` call, so if we have an unusually large buffer of commands it should either work or fail gracefully rather than deadlocking. ================ Comment at: lldb/tools/driver/Driver.cpp:492-496 #ifdef _WIN32 - _close(fds[WRITE]); - fds[WRITE] = -1; + _close(fd); #else - close(fds[WRITE]); - fds[WRITE] = -1; + close(fd); #endif ---------------- labath wrote: > Do you think it would be possible to use > `llvm::Process::SafelyCloseFileDescriptor` here? It looks like that still > uses close (without the `_`) on windows, which we are trying hard to avoid > here, but I'm not sure why. I know close is technically deprecated on > windows, but AFAIK the only thing deprecated about it is the name, and > otherwise it works correctly. > > (If that works, I'd even consider removing this function entirely, as the > only thing it does extra is clear the fd, but that does not seem necessary > since we're now calling it immediately before we return (and the fds go out > of scope anyway)). I'm happy to use `llvm::sys::Process::SafelyCloseFileDescriptor` and delete my little wrapper. I agree that resetting the df to -1 right before it goes out of scope isn't useful. I'll update the patch momentarily. If we're not that worried about the deprecated names on Windows, should I also just use `fdopen` and forget about wrapping that as well? Either way, I'm going to keep the OpenPipe wrapper since the Windows version takes additional parameters. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60152/new/ https://reviews.llvm.org/D60152 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
