[PATCH] Add non-blocking Wait() for launched processes

[Tareq A. Siraj]

  @rafael: We need this to implement process based parallelism in the clang-modernizer project. We are in the process of implementing this feature into the modernizer, so expect to see it being used soon. See https://cpp11-migrate.atlassian.net/browse/CM-107 for details.

  Let me know if you have more comments. Thanks.


================
Comment at: include/llvm/Support/Program.h:43
@@ +42,3 @@
+  // to pass around the process handle and the last object needs to free it.
+  friend class llvm::RefCountedBase<ProcessInfo>;
+
----------------
Rafael Ávila de Espíndola wrote:
> How is this different from other OS resources? A file must be closed for example, but having all files be refcounted is probably not a good design.
> 
This is needed for the Windows implementation. CreateProcess() gives you a process id and a handle to the process and someone needs to release the handle. When we make copies of the ProcessInfo objects, question becomes who calls CloseHandle(). Alternatively we can close the handle and not store the handle but in that case we will need to call OpenProcess() every time we call Wait (non-blocking).

Let me know if you have any better implementation in mind.

================
Comment at: include/llvm/Support/Program.h:67
@@ +66,3 @@
+  /// The path to the executable this process is running.
+  llvm::SmallString<256> Program;
+
----------------
Rafael Ávila de Espíndola wrote:
> This is just for the sys::fs::exists check in Wait, right? Does any code depend on it? If not, just delete it. In the unlikely case that we do depend on it, can you move the check to Execute and avoid having this member variable?
Yes, its the sys::fs:exists() check that needs it. We need to store it somewhere because Wait() on Linux/posix_spawn implementation returns 127 for any kind of error (including file-not-found. See comment on lib/Support/Unix/Program.inc:396 in this patch).

Since Wait() is a public API function now, we need to store it somewhere. Alternatively we can ask the user to pass it when calling Wait() but that will be annoying for cases where you pass a vector<ProcessInfo> to some function and you'll need to pass vector<pair<ProcessInfo,string>>.

================
Comment at: include/llvm/Support/Program.h:157
@@ +156,3 @@
+      ///< until child has terminated.
+      std::string *ErrMsg = 0 ///< If non-zero, provides a pointer to a string
+      ///< instance in which error messages will be returned. If the string
----------------
Rafael Ávila de Espíndola wrote:
> This is the old style of error handling. The new one would be for this function to return an error_code.
> 
Its consistent with the current interface of Execute functions. I'll suggest we improve the interface for all three in a separate patch.

================
Comment at: lib/Support/Program.cpp:26
@@ +25,3 @@
+bool operator==(const ProcessInfo &LHS, const ProcessInfo &RHS) {
+  return (LHS.Pid == RHS.Pid && LHS.Program == RHS.Program);
+}
----------------
Rafael Ávila de Espíndola wrote:
> We cannot have two running process with the same pid. You don't need to compare the Program field.
Good point. Will remove it in the next update.


http://llvm-reviews.chandlerc.com/D1728