[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
----------------
But you *are* using "special values", harcoding the assumption that valid file descriptors are positive, and InvalidFd is negative.

This is not always so, e.g. on Windows fd_t is void*, so ">=0" check would always pass. It's better to use a named constant kInvalidFd than literal 0.

================
Comment at: lib/sanitizer_common/sanitizer_common.h:286
@@ +285,3 @@
+// streams would be redirect to the file. The files would always be closed
+// in parent process even in case of an error.
+int StartSubprocess(const char *filename, char *const argv[],
----------------
... but you call `FileExists` in the implementation? So, e.g. this wouldn't work: StartSubprocess("llvm-symbolizer", argv, ....);

================
Comment at: lib/sanitizer_common/sanitizer_common.h:768
@@ +767,3 @@
+ private:
+  Fn fn_;
+};
----------------
Ack.

================
Comment at: lib/sanitizer_common/sanitizer_common.h:774
@@ +773,3 @@
+template <typename Fn>
+AutoRunner<Fn> at_scope_exit(Fn fn) {
+  return AutoRunner<Fn>(fn);
----------------
... but you check for kInvalidFd three times in the implementation, though. Anyway, I don't have a strong opinion here.

================
Comment at: lib/sanitizer_common/sanitizer_posix_libcdep.cc:324
@@ -322,1 +323,3 @@
 
+int StartSubprocess(const char *program, char *const argv[], fd_t std_in_fd,
+                    fd_t std_out_fd, fd_t std_err_fd) {
----------------
Minor nit: we tend to catenate these to stdin, stdout etc.

================
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 */);
----------------
Yeah, let's do the latter for now.


http://reviews.llvm.org/D16546