[Lldb-commits] [PATCH] D60152: Fix and simplify PrepareCommandsForSourcing

[Adrian McCarthy via Phabricator via lldb-commits]

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