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

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

Nice cleanup, thanks. I have one suggestion inline you can implement if you think it's a good idea.

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.



================
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
----------------
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)).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60152/new/

https://reviews.llvm.org/D60152