[PATCH] D16546: [sanitizers] extracted process management functions

[Alexey Samsonov via llvm-commits]

samsonov added inline comments.

================
Comment at: lib/sanitizer_common/sanitizer_common.h:283
@@ +282,3 @@
+// Starts a subprocess and returs its pid.
+// If *_fd parameters are >=0 their corresponding input/output
+// streams would be redirect to the file. The files would always be closed
----------------
Why do you check if they are negative, instead of checking if they are specified (i.e. not kInvalidFd)?

================
Comment at: lib/sanitizer_common/sanitizer_common.h:286
@@ +285,3 @@
+// in parent process even in case of an error.
+int StartSubprocess(const char *program, char *const argv[],
+                    fd_t std_in_fd = kInvalidFd, fd_t std_out_fd = kInvalidFd,
----------------
It's unclear that `program` should be absolute path. Either add a comment, or rename variable (e.g. "filename").

================
Comment at: lib/sanitizer_common/sanitizer_common.h:764
@@ +763,3 @@
+  ~AutoRunner() { fn_(); }
+  Fn fn_;
+};
----------------
Why is fn_ public?

================
Comment at: lib/sanitizer_common/sanitizer_common.h:767
@@ +766,3 @@
+
+// A simple scope guard. Usage:
+// auto cleanup = at_scope_exit([]{ do_cleanup; });
----------------
Why do you need this?

================
Comment at: lib/sanitizer_common/sanitizer_common.h:773
@@ -751,1 +772,3 @@
+}
+
 }  // namespace __sanitizer
----------------
You can add a AutoFileCloser as well (takes fd in a constructor, and close file in dtor if fd is valid).
FTR there is `ScopedHandle` in `lib/sanitizer_common/sanitizer_symbolizer_win.cc` you might want to generalize on.

================
Comment at: lib/sanitizer_common/sanitizer_linux_libcdep.cc:550
@@ -548,1 +549,3 @@
 
+int StartSubprocess(const char *program, char *const argv[], fd_t std_in_fd,
+                    fd_t std_out_fd, fd_t std_err_fd) {
----------------
This function must work on all POSIX platforms, as `SymbolizerProcess::StartSymbolizerSubprocess` from `sanitizer_symbolizer_posix_libcdep.cc` is used there. You might need to move this function (and `IsProcessRunning` below)
to sanitizer_posix_libcdep.cc instead.

================
Comment at: lib/sanitizer_common/sanitizer_mac.cc:673
@@ -672,1 +672,3 @@
 
+int StartSubprocess(const char *program, char *const argv[], fd_t std_in_fd,
+                    fd_t std_out_fd, fd_t std_err_fd) {
----------------
See above: delete this.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:142
@@ +141,3 @@
+    GetArgV(path_, argv);
+    pid = StartSubprocess(path_, const_cast<char **>(&argv[0]),
+                          outfd[0] /* stdin */, infd[1] /* stdout */);
----------------
Looks like it makes more sense to do const_cast in the `StartSubprocess` function.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:146
@@ -144,7 +145,3 @@
       internal_close(infd[0]);
-      internal_close(infd[1]);
       internal_close(outfd[0]);
       return false;
----------------
Shouldn't this be `outfd[1]`? (`outfd[0]` will be closed by StartSubprocess).

================
Comment at: lib/sanitizer_common/sanitizer_win.cc:780
@@ +779,3 @@
+                    fd_t std_out_fd, fd_t std_err_fd) {
+  // FIXME: implement on this platform.
+  return -1;
----------------
Please note that this should be implemented based on `SymbolizerProcess::StartSymbolizerSubprocess` from `lib/sanitizer_common/sanitizer_symbolizer_win.cc`.


http://reviews.llvm.org/D16546