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

[Mike Aizatsky via llvm-commits]

aizatsky marked an inline comment as done.
aizatsky added a comment.

Thanks for reviewing. PTAL.


================
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
----------------
samsonov wrote:
> Why do you check if they are negative, instead of checking if they are specified (i.e. not kInvalidFd)?
Why not? Valid descriptors are >=0 and I really don't like special values. 

================
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,
----------------
samsonov wrote:
> It's unclear that `program` should be absolute path. Either add a comment, or rename variable (e.g. "filename").
It shouldn't. It could be relative to current dir which is perfectly fine. Renamed to "filename" though I can't say I like it.

================
Comment at: lib/sanitizer_common/sanitizer_common.h:767
@@ +766,3 @@
+
+// A simple scope guard. Usage:
+// auto cleanup = at_scope_exit([]{ do_cleanup; });
----------------
samsonov wrote:
> Why do you need this?
Because otherwise it is impossible to use AutoRunner with lambdas.

================
Comment at: lib/sanitizer_common/sanitizer_common.h:773
@@ -751,1 +772,3 @@
+}
+
 }  // namespace __sanitizer
----------------
samsonov wrote:
> 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.
Code size overhead was Kostya's primary concern. I don't think AutoFileCloser is generic enough. Plus I'd hate checking for kInvalidFd in it.

================
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 */);
----------------
samsonov wrote:
> Looks like it makes more sense to do const_cast in the `StartSubprocess` function.
I'd actually like a little advice here.

Here's how execve is defined in standard:

int execv(const char *path, char *const argv[]);

So argv points to mutable strings. I don't actually understand why. Is it a standard oversight? WDYT?

I'd be happy to define argv in StartSubprocess as "const char *const argv[];" if you thinks it is ok to cast const off later.




http://reviews.llvm.org/D16546