Optionally separate Tasks` stderr from stdout.

scallanan · scallanan · commit 399709ccb87f · 2017-01-11T11:11:23.000-08:00
Fixes a serious problem where spurious output from xcrun breaks
swift's discovery of libarclite.

&lt;rdar://problem/28573949&gt;
diff --git a/include/swift/Basic/TaskQueue.h b/include/swift/Basic/TaskQueue.h
@@ -72,12 +72,15 @@ class TaskQueue {
   /// \param ReturnCode the return code of the task which finished execution.
   /// \param Output the output from the task which finished execution,
   /// if available. (This may not be available on all platforms.)
+  /// \param Errors the errors from the task which finished execution, if
+  /// available and SeparateErrors was true.  (This may not be available on all
+  /// platforms.)
   /// \param Context the context which was passed when the task was added
   ///
   /// \returns true if further execution of tasks should stop,
   /// false if execution should continue
   typedef std::function<TaskFinishedResponse(ProcessId Pid, int ReturnCode,
-                                             StringRef Output, void *Context)>
+                                             StringRef Output, StringRef Errors, void *Context)>
     TaskFinishedCallback;
 
   /// \brief A callback which will be executed if a task exited abnormally due
@@ -88,12 +91,15 @@ class TaskQueue {
   /// no reason could be deduced, this may be empty.
   /// \param Output the output from the task which exited abnormally, if
   /// available. (This may not be available on all platforms.)
+  /// \param Errors the errors from the task which exited abnormally, if
+  /// available and SeparateErrors was true.  (This may not be available on all
+  /// platforms.)
   /// \param Context the context which was passed when the task was added
   ///
   /// \returns a TaskFinishedResponse indicating whether or not execution
   /// should proceed
   typedef std::function<TaskFinishedResponse(ProcessId Pid, StringRef ErrorMsg,
-                                             StringRef Output, void *Context)>
+                                             StringRef Output, StringRef Errors, void *Context)>
     TaskSignalledCallback;
 #pragma clang diagnostic pop
 
@@ -120,9 +126,10 @@ class TaskQueue {
   /// \param Env the environment which should be used for the task;
   /// must be null-terminated. If empty, inherits the parent's environment.
   /// \param Context an optional context which will be associated with the task
+  /// \param SeparateErrors Controls whether error output is reported separately
   virtual void addTask(const char *ExecPath, ArrayRef<const char *> Args,
                        ArrayRef<const char *> Env = llvm::None,
-                       void *Context = nullptr);
+                       void *Context = nullptr, bool SeparateErrors = false);
 
   /// \brief Synchronously executes the tasks in the TaskQueue.
   ///
@@ -153,10 +160,13 @@ class DummyTaskQueue : public TaskQueue {
     ArrayRef<const char *> Args;
     ArrayRef<const char *> Env;
     void *Context;
+    bool SeparateErrors;
 
     DummyTask(const char *ExecPath, ArrayRef<const char *> Args,
-              ArrayRef<const char *> Env = llvm::None, void *Context = nullptr)
-      : ExecPath(ExecPath), Args(Args), Env(Env), Context(Context) {}
+              ArrayRef<const char *> Env = llvm::None, void *Context = nullptr,
+              bool SeparateErrors = false)
+        : ExecPath(ExecPath), Args(Args), Env(Env), Context(Context),
+          SeparateErrors(SeparateErrors) {}
   };
 
   std::queue<std::unique_ptr<DummyTask>> QueuedTasks;
@@ -168,7 +178,7 @@ class DummyTaskQueue : public TaskQueue {
 
   virtual void addTask(const char *ExecPath, ArrayRef<const char *> Args,
                        ArrayRef<const char *> Env = llvm::None,
-                       void *Context = nullptr);
+                       void *Context = nullptr, bool SeparateErrors = false);
 
   virtual bool
   execute(TaskBeganCallback Began = TaskBeganCallback(),
diff --git a/lib/Basic/Default/TaskQueue.inc b/lib/Basic/Default/TaskQueue.inc
@@ -112,8 +112,8 @@ bool TaskQueue::execute(TaskBeganCallback Began, TaskFinishedCallback Finished,
       // Wait() returning a return code of -2 indicates the process received
       // a signal during execution.
       if (Signalled) {
-        TaskFinishedResponse Response = Signalled(PI.Pid, ErrMsg, StringRef(),
-                                                  T->Context);
+        TaskFinishedResponse Response =
+            Signalled(PI.Pid, ErrMsg, StringRef(), StringRef(), T->Context);
         ContinueExecution = Response != TaskFinishedResponse::StopExecution;
       } else {
         // If we don't have a Signalled callback, unconditionally stop.
@@ -124,7 +124,7 @@ bool TaskQueue::execute(TaskBeganCallback Began, TaskFinishedCallback Finished,
       // finished.
       if (Finished) {
         TaskFinishedResponse Response = Finished(PI.Pid, PI.ReturnCode,
-        StringRef(), T->Context);
+        StringRef(), StringRef(), T->Context);
         ContinueExecution = Response != TaskFinishedResponse::StopExecution;
       } else if (PI.ReturnCode != 0) {
         ContinueExecution = false;
diff --git a/lib/Basic/TaskQueue.cpp b/lib/Basic/TaskQueue.cpp
@@ -42,9 +42,10 @@ DummyTaskQueue::DummyTaskQueue(unsigned NumberOfParallelTasks)
 DummyTaskQueue::~DummyTaskQueue() = default;
 
 void DummyTaskQueue::addTask(const char *ExecPath, ArrayRef<const char *> Args,
-                             ArrayRef<const char *> Env, void *Context) {
-  QueuedTasks.emplace(
-    std::unique_ptr<DummyTask>(new DummyTask(ExecPath, Args, Env, Context)));
+                             ArrayRef<const char *> Env, void *Context,
+                             bool SeparateErrors) {
+  QueuedTasks.emplace(std::unique_ptr<DummyTask>(
+      new DummyTask(ExecPath, Args, Env, Context, SeparateErrors)));
 }
 
 bool DummyTaskQueue::execute(TaskQueue::TaskBeganCallback Began,
@@ -80,9 +81,11 @@ bool DummyTaskQueue::execute(TaskQueue::TaskBeganCallback Began,
 
     if (Finished) {
       std::string Output = "Output placeholder\n";
-        if (Finished(P.first, 0, Output, P.second->Context) ==
-            TaskFinishedResponse::StopExecution)
-          SubtaskFailed = true;
+      std::string Errors =
+          P.second->SeparateErrors ? "Error placeholder\n" : "";
+      if (Finished(P.first, 0, Output, Errors, P.second->Context) ==
+          TaskFinishedResponse::StopExecution)
+        SubtaskFailed = true;
     }
   }
 
diff --git a/lib/Basic/Unix/TaskQueue.inc b/lib/Basic/Unix/TaskQueue.inc
@@ -58,12 +58,18 @@ class Task {
   /// Context which should be associated with this task.
   void *Context;
 
+  /// True if the errors of the Task should be stored in Errors instead of Output.
+  bool SeparateErrors;
+
   /// The pid of this Task when executing.
   pid_t Pid;
 
   /// A pipe for reading output from the child process.
   int Pipe;
 
+  /// A pipe for reading errors from the child prcess, if SeparateErrors is true.
+  int ErrorPipe;
+
   /// The current state of the Task.
   enum {
     Preparing,
@@ -74,29 +80,36 @@ class Task {
   /// Once the Task has finished, this contains the buffered output of the Task.
   std::string Output;
 
+  /// Once the Task has finished, if SeparateErrors is true, this contains the errors
+  /// from the Task.
+  std::string Errors;
+
 public:
   Task(const char *ExecPath, ArrayRef<const char *> Args,
-       ArrayRef<const char *> Env, void *Context)
+       ArrayRef<const char *> Env, void *Context, bool SeparateErrors)
       : ExecPath(ExecPath), Args(Args), Env(Env), Context(Context),
-        Pid(-1), Pipe(-1), State(Preparing) {
+        SeparateErrors(SeparateErrors), Pid(-1), Pipe(-1), ErrorPipe(-1),
+        State(Preparing) {
     assert((Env.empty() || Env.back() == nullptr) &&
            "Env must either be empty or null-terminated!");
   }
 
   const char *getExecPath() const { return ExecPath; }
   ArrayRef<const char *> getArgs() const { return Args; }
   StringRef getOutput() const { return Output; }
+  StringRef getErrors() const { return Errors; }
   void *getContext() const { return Context; }
   pid_t getPid() const { return Pid; }
   int getPipe() const { return Pipe; }
+  int getErrorPipe() const { return ErrorPipe; }
 
   /// \brief Begins execution of this Task.
   /// \returns true on error, false on success
   bool execute();
 
-  /// \brief Reads data from the pipe, if any is available.
+  /// \brief Reads data from the pipes, if any is available.
   /// \returns true on error, false on success
-  bool readFromPipe();
+  bool readFromPipes();
 
   /// \brief Performs any post-execution work for this Task, such as reading
   /// piped output and closing the pipe.
@@ -121,6 +134,12 @@ bool Task::execute() {
   pipe(FullPipe);
   Pipe = FullPipe[0];
 
+  int FullErrorPipe[2];
+  if (SeparateErrors) {
+    pipe(FullErrorPipe);
+    ErrorPipe = FullErrorPipe[0];
+  }
+
   // Get the environment to pass down to the subtask.
   const char *const *envp = Env.empty() ? nullptr : Env.data();
   if (!envp) {
@@ -138,8 +157,19 @@ bool Task::execute() {
   posix_spawn_file_actions_init(&FileActions);
 
   posix_spawn_file_actions_adddup2(&FileActions, FullPipe[1], STDOUT_FILENO);
-  posix_spawn_file_actions_adddup2(&FileActions, STDOUT_FILENO, STDERR_FILENO);
+
+  if (SeparateErrors) {
+    posix_spawn_file_actions_adddup2(&FileActions, FullErrorPipe[1],
+                                     STDERR_FILENO);
+  } else {
+    posix_spawn_file_actions_adddup2(&FileActions, STDOUT_FILENO,
+                                     STDERR_FILENO);
+  }
+
   posix_spawn_file_actions_addclose(&FileActions, FullPipe[0]);
+  if (SeparateErrors) {
+    posix_spawn_file_actions_addclose(&FileActions, FullErrorPipe[0]);
+  }
 
   // Spawn the subtask.
   int spawnErr = posix_spawn(&Pid, ExecPath, &FileActions, nullptr,
@@ -148,9 +178,15 @@ bool Task::execute() {
 
   posix_spawn_file_actions_destroy(&FileActions);
   close(FullPipe[1]);
+  if (SeparateErrors) {
+    close(FullErrorPipe[1]);
+  }
 
   if (spawnErr != 0 || Pid == 0) {
     close(FullPipe[0]);
+    if (SeparateErrors) {
+      close(FullErrorPipe[0]);
+    }
     State = Finished;
     return true;
   }
@@ -159,15 +195,25 @@ bool Task::execute() {
   switch (Pid) {
   case -1: {
     close(FullPipe[0]);
+    if (SeparateErrors) {
+      close(FullErrorPipe[0]);
+    }
     State = Finished;
     Pid = 0;
     break;
   }
   case 0: {
     // Child process: Execute the program.
     dup2(FullPipe[1], STDOUT_FILENO);
-    dup2(STDOUT_FILENO, STDERR_FILENO);
+    if (SeparateErrors) {
+      dup2(FullErrorPipe[1], STDERR_FILENO);
+    } else {
+      dup2(STDOUT_FILENO, STDERR_FILENO);
+    }
     close(FullPipe[0]);
+    if (SeparateErrors) {
+      close(FullErrorPipe[0]);
+    }
     execve(ExecPath, const_cast<char **>(argvp), const_cast<char **>(envp));
 
     // If the execve() failed, we should exit. Follow Unix protocol and
@@ -184,6 +230,9 @@ bool Task::execute() {
   }
 
   close(FullPipe[1]);
+  if (SeparateErrors) {
+    close(FullErrorPipe[1]);
+  }
 
   if (Pid == 0)
     return true;
@@ -192,7 +241,7 @@ bool Task::execute() {
   return false;
 }
 
-bool Task::readFromPipe() {
+static bool readFromAPipe(int Pipe, std::string &Output) {
   char outputBuffer[1024];
   ssize_t readBytes = 0;
   while ((readBytes = read(Pipe, outputBuffer, sizeof(outputBuffer))) != 0) {
@@ -209,16 +258,27 @@ bool Task::readFromPipe() {
   return false;
 }
 
+bool Task::readFromPipes() {
+  bool Ret = readFromAPipe(Pipe, Output);
+  if (SeparateErrors) {
+    Ret |= readFromAPipe(ErrorPipe, Errors);
+  }
+  return Ret;
+}
+
 void Task::finishExecution() {
   assert(State == Executing &&
          "This Task must be executing to finish execution!");
 
   State = Finished;
 
   // Read the output of the command, so we can use it later.
-  readFromPipe();
+  readFromPipes();
 
   close(Pipe);
+  if (SeparateErrors) {
+    close(ErrorPipe);
+  }
 }
 
 bool TaskQueue::supportsBufferingOutput() {
@@ -239,8 +299,10 @@ unsigned TaskQueue::getNumberOfParallelTasks() const {
 }
 
 void TaskQueue::addTask(const char *ExecPath, ArrayRef<const char *> Args,
-                        ArrayRef<const char *> Env, void *Context) {
-  std::unique_ptr<Task> T(new Task(ExecPath, Args, Env, Context));
+                        ArrayRef<const char *> Env, void *Context,
+                        bool SeparateErrors) {
+  std::unique_ptr<Task> T(
+      new Task(ExecPath, Args, Env, Context, SeparateErrors));
   QueuedTasks.push(std::move(T));
 }
 
@@ -279,6 +341,8 @@ bool TaskQueue::execute(TaskBeganCallback Began, TaskFinishedCallback Finished,
       }
 
       PollFds.push_back({ T->getPipe(), POLLIN | POLLPRI | POLLHUP, 0 });
+      // We should also poll T->getErrorPipe(), but this intrroduces timing
+      // issues with shutting down the task after reading getPipe().
       ExecutingTasks[Pid] = std::move(T);
     }
 
@@ -299,7 +363,7 @@ bool TaskQueue::execute(TaskBeganCallback Began, TaskFinishedCallback Finished,
       if (fd.revents & POLLIN || fd.revents & POLLPRI || fd.revents & POLLHUP ||
           fd.revents & POLLERR) {
         // An event which we care about occurred. Find the appropriate Task.
-        auto predicate = [&fd] (PidToTaskMap::value_type &value) -> bool {
+        auto predicate = [&fd](PidToTaskMap::value_type &value) -> bool {
           return value.second->getPipe() == fd.fd;
         };
 
@@ -310,7 +374,7 @@ bool TaskQueue::execute(TaskBeganCallback Began, TaskFinishedCallback Finished,
         Task &T = *iter->second;
         if (fd.revents & POLLIN || fd.revents & POLLPRI) {
           // There's data available to read.
-          T.readFromPipe();
+          T.readFromPipes();
         }
 
         if (fd.revents & POLLHUP || fd.revents & POLLERR) {
@@ -339,7 +403,7 @@ bool TaskQueue::execute(TaskBeganCallback Began, TaskFinishedCallback Finished,
               // If we have a TaskFinishedCallback, only set SubtaskFailed to
               // true if the callback returns StopExecution.
               SubtaskFailed = Finished(T.getPid(), Result, T.getOutput(),
-                                       T.getContext()) ==
+                                       T.getErrors(), T.getContext()) ==
                   TaskFinishedResponse::StopExecution;
             } else if (Result != 0) {
               // Since we don't have a TaskFinishedCallback, treat a subtask
@@ -353,9 +417,9 @@ bool TaskQueue::execute(TaskBeganCallback Began, TaskFinishedCallback Finished,
             StringRef ErrorMsg = strsignal(Signal);
 
             if (Signalled) {
-              TaskFinishedResponse Response = Signalled(T.getPid(), ErrorMsg,
-                                                        T.getOutput(),
-                                                        T.getContext());
+              TaskFinishedResponse Response =
+                  Signalled(T.getPid(), ErrorMsg, T.getOutput(), T.getErrors(),
+                            T.getContext());
               if (Response == TaskFinishedResponse::StopExecution)
                 // If we have a TaskCrashedCallback, only set SubtaskFailed to
                 // true if the callback returns StopExecution.
diff --git a/lib/Driver/Compilation.cpp b/lib/Driver/Compilation.cpp
@@ -481,8 +481,9 @@ int Compilation::performJobsImpl() {
   // continue (if execution should stop, this callback should return true), and
   // it should also schedule any additional commands which we now know need
   // to run.
-  auto taskFinished = [&] (ProcessId Pid, int ReturnCode, StringRef Output,
-                           void *Context) -> TaskFinishedResponse {
+  auto taskFinished = [&](ProcessId Pid, int ReturnCode, StringRef Output,
+                          StringRef Errors,
+                          void *Context) -> TaskFinishedResponse {
     const Job *FinishedCmd = (const Job *)Context;
 
     if (ShowDriverTimeCompilation) {
@@ -612,8 +613,9 @@ int Compilation::performJobsImpl() {
     return TaskFinishedResponse::ContinueExecution;
   };
 
-  auto taskSignalled = [&] (ProcessId Pid, StringRef ErrorMsg, StringRef Output,
-                            void *Context) -> TaskFinishedResponse {
+  auto taskSignalled = [&](ProcessId Pid, StringRef ErrorMsg, StringRef Output,
+                           StringRef Errors,
+                           void *Context) -> TaskFinishedResponse {
     const Job *SignalledCmd = (const Job *)Context;
 
     if (ShowDriverTimeCompilation) {
diff --git a/lib/Driver/Driver.cpp b/lib/Driver/Driver.cpp
@@ -1204,6 +1204,7 @@ void Driver::buildOutputInfo(const ToolChain &TC, const DerivedArgList &Args,
                         [&OI](sys::ProcessId PID,
                               int returnCode,
                               StringRef output,
+                              StringRef errors,
                               void *unused) -> sys::TaskFinishedResponse {
             if (returnCode == 0) {
               output = output.rtrim();
diff --git a/lib/Driver/ToolChains.cpp b/lib/Driver/ToolChains.cpp
