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

Mike Aizatsky via llvm-commits llvm-commits at lists.llvm.org
Mon Jan 25 17:10:35 PST 2016

aizatsky added a comment.

All done. 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
samsonov wrote:
> 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.
I didn't know that. Using kInvalidFd.

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[],
samsonov wrote:
> ... but you call `FileExists` in the implementation? So, e.g. this wouldn't work: StartSubprocess("llvm-symbolizer", argv, ....);
Thought about this. Removed FileExists. (I used it to debug things).

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);
samsonov wrote:
> ... but you check for kInvalidFd three times in the implementation, though. Anyway, I don't have a strong opinion here.
Let's see how this one would be used. If we end up using it only for files - I'll create another utility.


More information about the llvm-commits mailing list