Add stdout/stderr redirection to run_process

src/Util.cpp

@@ -9,6 +9,7 @@
 #include "Error.h"
 #include <atomic>
 #include <chrono>
+#include <cstdio>
 #include <fstream>
 #include <iomanip>
 #include <map>
@@ -17,10 +18,12 @@
 #include <string>
 
 #ifdef _MSC_VER
+#include <fcntl.h>
 #include <io.h>
 #include <process.h>  // For _spawnvp
 #else
 #include <cstdlib>
+#include <fcntl.h>
 #include <spawn.h>
 #include <sys/mman.h>  // For mmap
 #include <sys/wait.h>
@@ -509,6 +512,10 @@ void write_entire_file(const std::string &pathname, const void *source, size_t s
 }
 
 int run_process(std::vector<std::string> args) {
+    return run_process(std::move(args), "", "");
+}
+
+int run_process(std::vector<std::string> args, const std::string &stdout_path, const std::string &stderr_path) {
     internal_assert(!args.empty()) << "run_process called with empty args\n";
 
     std::vector<char *> argv;
@@ -521,12 +528,84 @@ int run_process(std::vector<std::string> args) {
     debug(2) << "Running process: " << PrintSpan(args) << "\n";
 
 #ifdef _WIN32
-    // Wait for completion; return the child's exit code.
+    // _spawnvp() has no redirection parameter, so temporarily point the
+    // calling process's own stdout/stderr at the requested files, spawn
+    // synchronously, then restore them. This is safe only because the
+    // call blocks (_P_WAIT) -- there's no window where another thread
+    // could observe the redirected fds.
+    int saved_stdout = -1, saved_stderr = -1;
+    if (!stdout_path.empty()) {
+        saved_stdout = _dup(_fileno(stdout));
+        int fd = _open(stdout_path.c_str(), _O_WRONLY | _O_CREAT | _O_TRUNC | _O_BINARY, _S_IWRITE);
+        if (fd == -1) {
+            if (saved_stdout != -1) {
+                _close(saved_stdout);
+            }
+            return -1;
+        }
+        _dup2(fd, _fileno(stdout));
+        _close(fd);
+    }
+    if (!stderr_path.empty()) {
+        saved_stderr = _dup(_fileno(stderr));
+        if (stderr_path == stdout_path) {
+            // Share the fd stdout was just redirected to, rather than
+            // opening the same path again: two independent opens would
+            // give stdout/stderr independent file offsets, so interleaved
+            // writes would clobber each other instead of concatenating.
+            _dup2(_fileno(stdout), _fileno(stderr));
+        } else {
+            int fd = _open(stderr_path.c_str(), _O_WRONLY | _O_CREAT | _O_TRUNC | _O_BINARY, _S_IWRITE);
+            if (fd == -1) {
+                if (saved_stdout != -1) {
+                    _dup2(saved_stdout, _fileno(stdout));
+                    _close(saved_stdout);
+                }
+                if (saved_stderr != -1) {
+                    _close(saved_stderr);
+                }
+                return -1;
+            }
+            _dup2(fd, _fileno(stderr));
+            _close(fd);
+        }
+    }
+
     int rc = _spawnvp(_P_WAIT, argv[0], argv.data());
+
+    if (saved_stdout != -1) {
+        _dup2(saved_stdout, _fileno(stdout));
+        _close(saved_stdout);
+    }
+    if (saved_stderr != -1) {
+        _dup2(saved_stderr, _fileno(stderr));
+        _close(saved_stderr);
+    }
+
     return (rc >= 0) ? rc : -1;
 #else
+    posix_spawn_file_actions_t actions;
+    posix_spawn_file_actions_init(&actions);
+    if (!stdout_path.empty()) {
+        posix_spawn_file_actions_addopen(&actions, STDOUT_FILENO, stdout_path.c_str(),
+                                         O_WRONLY | O_CREAT | O_TRUNC, 0644);
+    }
+    if (!stderr_path.empty()) {
+        if (stderr_path == stdout_path) {
+            // Share stdout's fd (as shell `2>&1` does) instead of opening
+            // the same path again: two independent opens would give
+            // stdout/stderr independent file offsets, so interleaved
+            // writes would clobber each other instead of concatenating.
+            posix_spawn_file_actions_adddup2(&actions, STDOUT_FILENO, STDERR_FILENO);
+        } else {
+            posix_spawn_file_actions_addopen(&actions, STDERR_FILENO, stderr_path.c_str(),
+                                             O_WRONLY | O_CREAT | O_TRUNC, 0644);
+        }
+    }
+
     pid_t pid = 0;
-    int status = posix_spawnp(&pid, argv[0], nullptr, nullptr, argv.data(), environ);
+    int status = posix_spawnp(&pid, argv[0], &actions, nullptr, argv.data(), environ);
+    posix_spawn_file_actions_destroy(&actions);
     if (status != 0 || waitpid(pid, &status, 0) == -1) {
         return -1;
     }

src/Util.h

@@ -398,6 +398,12 @@ class TemporaryFile final {
  * could not be started. */
 int run_process(std::vector<std::string> args);
 
+/** As above, but redirect the child's stdout and/or stderr to the given
+ * files instead of inheriting the caller's. Pass an empty string for
+ * either path to leave that stream untouched. Passing the same path for
+ * both combines them into a single file, as with the shell's `2>&1`. */
+int run_process(std::vector<std::string> args, const std::string &stdout_path, const std::string &stderr_path);
+
 /** Routines to test if math would overflow for signed integers with
  * the given number of bits. */
 // @{

test/correctness/CMakeLists.txt

@@ -306,6 +306,7 @@ tests(
     respect_input_constraint_in_bounds_inference.cpp
     reuse_stack_alloc.cpp
     round.cpp
+    run_process.cpp
     runtime_prefixes.cpp
     saturating_casts.cpp
     scatter.cpp

test/correctness/run_process.cpp

@@ -0,0 +1,107 @@
+// Exercises Internal::run_process()'s stdout/stderr redirection overload.
+// Each check re-execs this test binary with --child so that the process
+// being redirected is a real child process, not just an in-process write.
+
+#include "Halide.h"
+
+#include <cstdio>
+#include <cstdlib>
+#include <filesystem>
+#include <iostream>
+#include <string>
+
+#ifdef _MSC_VER
+#include <fcntl.h>
+#include <io.h>
+#endif
+
+using namespace Halide;
+
+namespace {
+
+namespace fs = std::filesystem;
+
+// Return 1 from main() on failure; the test harness treats that as a failure.
+// (assert() is compiled out in release builds, so we can't rely on it.)
+#define check(cond)                                                       \
+    do {                                                                  \
+        if (!(cond)) {                                                    \
+            std::cerr << "FAILED: " #cond " (line " << __LINE__ << ")\n"; \
+            return 1;                                                     \
+        }                                                                 \
+    } while (0)
+
+constexpr int kChildExitCode = 3;
+const std::string kStdoutMarker = "hello from stdout\n";
+const std::string kStderrMarker = "hello from stderr\n";
+
+// Child mode: write known markers to stdout and stderr, then exit with a
+// known nonzero code. stdout is flushed before writing to stderr (which is
+// unbuffered by default) so that when both streams share a single file, the
+// resulting order is deterministic.
+int run_child() {
+#ifdef _MSC_VER
+    // Markers are compared byte-for-byte against a Unix-style '\n'; without
+    // this, the CRT's default text-mode translation would write '\r\n'
+    // instead.
+    _setmode(_fileno(stdout), _O_BINARY);
+    _setmode(_fileno(stderr), _O_BINARY);
+#endif
+    std::fputs(kStdoutMarker.c_str(), stdout);
+    std::fflush(stdout);
+    std::fputs(kStderrMarker.c_str(), stderr);
+    return kChildExitCode;
+}
+
+std::string slurp(const std::string &path) {
+    std::vector<char> data = Internal::read_entire_file(path);
+    return {data.begin(), data.end()};
+}
+
+}  // namespace
+
+int main(int argc, char **argv) {
+    if (argc == 2 && std::string(argv[1]) == "--child") {
+        return run_child();
+    }
+
+    const std::string self = fs::absolute(argv[0]).string();
+
+    // Separate files for stdout and stderr: each should contain exactly its
+    // own marker, and the exit code should be preserved.
+    {
+        Internal::TemporaryFile out("run_process_out", ".txt");
+        Internal::TemporaryFile err("run_process_err", ".txt");
+
+        int rc = Internal::run_process({self, "--child"}, out.pathname(), err.pathname());
+        check(rc == kChildExitCode);
+        check(slurp(out.pathname()) == kStdoutMarker);
+        check(slurp(err.pathname()) == kStderrMarker);
+    }
+
+    // Same file for both streams: the two markers should be concatenated in
+    // write order, not corrupted or partially overwritten (which is what
+    // would happen if the same path were opened twice independently instead
+    // of sharing one fd).
+    {
+        Internal::TemporaryFile combined("run_process_combined", ".txt");
+
+        int rc = Internal::run_process({self, "--child"}, combined.pathname(), combined.pathname());
+        check(rc == kChildExitCode);
+        check(slurp(combined.pathname()) == kStdoutMarker + kStderrMarker);
+    }
+
+    // Empty paths (including the plain 1-arg overload, which forwards to
+    // this one) must still work exactly as before: no redirection, just the
+    // exit code.
+    {
+        int rc = Internal::run_process({self, "--child"}, "", "");
+        check(rc == kChildExitCode);
+
+        rc = Internal::run_process({self, "--child"});
+        check(rc == kChildExitCode);
+    }
+
+    std::cout << "Success!\n";
+    return 0;
+}