Rework diag_buffer interface to facilitate correct destruction order

17 files changed, 444 insertions, 470 deletions

diff --git a/libbuild2/bin/def-rule.cxx b/libbuild2/bin/def-rule.cxx
index 2879ca9..0998c89 100644
--- a/libbuild2/bin/def-rule.cxx
+++ b/libbuild2/bin/def-rule.cxx
@@ -727,8 +727,6 @@ namespace build2
       if (verb == 1)
         print_diag ("def", t);
 
-      diag_buffer dbuf (ctx);
-
       // Extract symbols from each object file.
       //
       symbols syms;
@@ -752,24 +750,24 @@ namespace build2
         process pr (
           run_start (nm,
                      args,
-                     0                                /* stdin */,
-                     -1                               /* stdout */,
-                     dbuf.open (args[0],
-                                false /* force */,
-                                fdstream_mode::non_blocking |
-                                fdstream_mode::skip)  /* stderr */));
+                     0                       /* stdin */,
+                     -1                      /* stdout */,
+                     diag_buffer::pipe (ctx) /* stderr */));
+
+        // Note that while we read both streams until eof in the normal
+        // circumstances, we cannot use fdstream_mode::skip for the exception
+        // case on both of them: we may end up being blocked trying to read
+        // one stream while the process may be blocked writing to the other.
+        // So in case of an exception we only skip the diagnostics and close
+        // stdout hard. The latter should happen first so the order of the
+        // dbuf/is variables is important.
+        //
+        diag_buffer dbuf (ctx, args[0], pr, (fdstream_mode::non_blocking |
+                                             fdstream_mode::skip));
 
         bool io (false);
         try
         {
-          // Note that while we read both streams until eof in the normal
-          // circumstances, we cannot use fdstream_mode::skip for the
-          // exception case on both of them: we may end up being blocked
-          // trying to read one stream while the process may be blocked
-          // writing to the other. So in case of an exception we only skip the
-          // diagnostics and close stdout hard. The latter should happen first
-          // so the order of the dbuf/is variables is important.
-          //
           ifdstream is (move (pr.in_ofd),
                         fdstream_mode::non_blocking,
                         ifdstream::badbit);
diff --git a/libbuild2/cc/compile-rule.cxx b/libbuild2/cc/compile-rule.cxx
index 6ca1f0a..874674d 100644
--- a/libbuild2/cc/compile-rule.cxx
+++ b/libbuild2/cc/compile-rule.cxx
@@ -4009,13 +4009,14 @@ namespace build2
                               args,
                               -1,
                               -1,
-                              dbuf.open (args[0],
-                                         false /* force */,
-                                         fdstream_mode::non_blocking |
-                                         fdstream_mode::skip),
+                              diag_buffer::pipe (ctx),
                               nullptr, // CWD
                               env.empty () ? nullptr : env.data ());
 
+                dbuf.open (args[0],
+                           move (pr.in_efd),
+                           fdstream_mode::non_blocking |
+                           fdstream_mode::skip);
                 try
                 {
                   gcc_module_mapper_state mm_state (skip_count, imports);
@@ -4126,8 +4127,6 @@ namespace build2
                 // diagnostics (if things go badly we will restart with this
                 // support).
                 //
-                using pipe = process::pipe;
-
                 if (drmp == nullptr) // Dependency info goes to stdout.
                 {
                   assert (!sense_diag); // Note: could support if necessary.
@@ -4135,39 +4134,42 @@ namespace build2
                   // For VC with /P the dependency info and diagnostics all go
                   // to stderr so redirect it to stdout.
                   //
-                  pipe err (
-                    cclass == compiler_class::msvc ? pipe {-1,  1} : // stdout
-                    !gen                           ? pipe {-1, -2} : // null
-                    dbuf.open (args[0],
-                               sense_diag /* force */,
-                               fdstream_mode::non_blocking));
+                  int err (
+                    cclass == compiler_class::msvc ?  1 : // stdout
+                    !gen                           ? -2 : // /dev/null
+                    diag_buffer::pipe (ctx, sense_diag /* force */));
 
                   pr = process (
                     cpath,
                     args,
                     0,
                     -1,
-                    move (err),
+                    err,
                     nullptr, // CWD
                     env.empty () ? nullptr : env.data ());
+
+                  dbuf.open (args[0],
+                             move (pr.in_efd),
+                             fdstream_mode::non_blocking); // Skip on stdout.
                 }
                 else // Dependency info goes to temporary file.
                 {
                   // Since we only need to read from one stream (dbuf) let's
                   // use the simpler blocking setup.
                   //
-                  pipe err (
-                    !gen && !sense_diag ? pipe {-1, -2} : // null
-                    dbuf.open (args[0], sense_diag /* force */));
+                  int err (
+                    !gen && !sense_diag ? -2 : // /dev/null
+                    diag_buffer::pipe (ctx, sense_diag /* force */));
 
                   pr = process (cpath,
                                 args,
                                 0,
                                 2, // Send stdout to stderr.
-                                move (err),
+                                err,
                                 nullptr, // CWD
                                 env.empty () ? nullptr : env.data ());
 
+                  dbuf.open (args[0], move (pr.in_efd));
                   dbuf.read (sense_diag /* force */);
 
                   if (sense_diag)
@@ -7343,8 +7345,6 @@ namespace build2
       if (verb >= 3)
         print_process (args);
 
-      diag_buffer dbuf (ctx);
-
       // @@ DRYRUN: Currently we discard the (partially) preprocessed file on
       // dry-run which is a waste. Even if we keep the file around (like we do
       // for the error case; see above), we currently have no support for
@@ -7374,10 +7374,12 @@ namespace build2
 
           process pr (cpath,
                       args,
-                      0, 2, dbuf.open (args[0], filter),
+                      0, 2, diag_buffer::pipe (ctx, filter /* force */),
                       nullptr, // CWD
                       env.empty () ? nullptr : env.data ());
 
+          diag_buffer dbuf (ctx, args[0], pr);
+
           if (filter)
             msvc_filter_cl (dbuf, *sp);
 
@@ -7444,10 +7446,11 @@ namespace build2
           {
             process pr (cpath,
                         args,
-                        0, 2, dbuf.open (args[0]),
+                        0, 2, diag_buffer::pipe (ctx),
                         nullptr, // CWD
                         env.empty () ? nullptr : env.data ());
 
+            diag_buffer dbuf (ctx, args[0], pr);
             dbuf.read ();
             run_finish (dbuf, args, pr, 1 /* verbosity */);
           }
diff --git a/libbuild2/cc/gcc.cxx b/libbuild2/cc/gcc.cxx
index 0045c27..755b0d8 100644
--- a/libbuild2/cc/gcc.cxx
+++ b/libbuild2/cc/gcc.cxx
@@ -137,9 +137,9 @@ namespace build2
       process pr (run_start (
                     env,
                     args,
-                    -2,      /* stdin */
-                    -2,      /* stdout */
-                    {-1, -1} /* stderr */));
+                    -2, /* stdin */
+                    -2, /* stdout */
+                    -1  /* stderr */));
       try
       {
         ifdstream is (
diff --git a/libbuild2/cc/guess.cxx b/libbuild2/cc/guess.cxx
index a0d7ee4..7a2ede9 100644
--- a/libbuild2/cc/guess.cxx
+++ b/libbuild2/cc/guess.cxx
@@ -186,7 +186,7 @@ namespace build2
                              args,
                              -1      /* stdin */,
                              -1      /* stdout */,
-                             {-1, 1} /* stderr (to stdout) */));
+                             1       /* stderr (to stdout) */));
       string l, r;
       try
       {
@@ -2179,7 +2179,7 @@ namespace build2
                              args,
                              -2      /* stdin  (to /dev/null) */,
                              -1      /* stdout                */,
-                             {-1, 1} /* stderr (to stdout)    */));
+                             1       /* stderr (to stdout)    */));
 
       clang_msvc_info r;
 
diff --git a/libbuild2/cc/link-rule.cxx b/libbuild2/cc/link-rule.cxx
index 9bf86e6..20c6c62 100644
--- a/libbuild2/cc/link-rule.cxx
+++ b/libbuild2/cc/link-rule.cxx
@@ -2892,8 +2892,6 @@ namespace build2
         env_ptrs.push_back (nullptr);
       }
 
-      diag_buffer dbuf (ctx);
-
       // If targeting Windows, take care of the manifest.
       //
       path manifest; // Manifest itself (msvc) or compiled object file.
@@ -2949,12 +2947,14 @@ namespace build2
                 //
                 process pr (rc,
                             args,
-                            -1                  /* stdin  */,
-                            1                   /* stdout */,
-                            dbuf.open (args[0]) /* stderr */,
-                            nullptr             /* cwd    */,
+                            -1                      /* stdin  */,
+                            1                       /* stdout */,
+                            diag_buffer::pipe (ctx) /* stderr */,
+                            nullptr                 /* cwd    */,
                             env_ptrs.empty () ? nullptr : env_ptrs.data ());
 
+                diag_buffer dbuf (ctx, args[0], pr);
+
                 try
                 {
                   ofdstream os (move (pr.out_fd));
@@ -4038,12 +4038,14 @@ namespace build2
 
           process pr (*ld,
                       args,
-                      0                           /* stdin  */,
-                      2                           /* stdout */,
-                      dbuf.open (args[0], filter) /* stderr */,
-                      nullptr                     /* cwd    */,
+                      0                                           /* stdin  */,
+                      2                                           /* stdout */,
+                      diag_buffer::pipe (ctx, filter /* force */) /* stderr */,
+                      nullptr                                     /* cwd    */,
                       env_ptrs.empty () ? nullptr : env_ptrs.data ());
 
+          diag_buffer dbuf (ctx, args[0], pr);
+
           if (filter)
             msvc_filter_link (dbuf, t, ot);
 
@@ -4127,7 +4129,7 @@ namespace build2
 
         if (!ctx.dry_run)
         {
-          run (dbuf,
+          run (ctx,
                rl,
                args,
                1 /* finish_verbosity */,
diff --git a/libbuild2/cc/msvc.cxx b/libbuild2/cc/msvc.cxx
index 8b9c05b..8fcbb0b 100644
--- a/libbuild2/cc/msvc.cxx
+++ b/libbuild2/cc/msvc.cxx
@@ -433,9 +433,9 @@ namespace build2
       //
       process pr (run_start (ld,
                              args,
-                             0       /* stdin */,
-                             -1      /* stdout */,
-                             {-1, 1} /* stderr (to stdout) */));
+                             0  /* stdin */,
+                             -1 /* stdout */,
+                             1  /* stderr (to stdout) */));
 
       bool obj (false), dll (false);
       string s;
diff --git a/libbuild2/diagnostics.cxx b/libbuild2/diagnostics.cxx
index 3246cae..bc74db3 100644
--- a/libbuild2/diagnostics.cxx
+++ b/libbuild2/diagnostics.cxx
@@ -498,41 +498,35 @@ namespace build2
 
   // diag_buffer
   //
-  process::pipe diag_buffer::
-  open (const char* args0, bool force, fdstream_mode m)
+
+  int diag_buffer::
+  pipe (context& ctx, bool force)
+  {
+    return (ctx.sched.serial () || ctx.no_diag_buffer) && !force ? 2 : -1;
+  }
+
+  void diag_buffer::
+  open (const char* args0, auto_fd&& fd, fdstream_mode m)
   {
     assert (state_ == state::closed && args0 != nullptr);
 
     serial = ctx_.sched.serial ();
     nobuf = !serial && ctx_.no_diag_buffer;
 
-    process::pipe r;
-    if (!(serial || nobuf) || force)
+    if (fd != nullfd)
     {
       try
       {
-        fdpipe p (fdopen_pipe ());
-
-        // Note that we must return non-owning fd to our end of the pipe (see
-        // the process class for details).
-        //
-        r = process::pipe (p.in.get (), move (p.out));
-
-        m |= fdstream_mode::text;
-
-        is.open (move (p.in), m);
+        is.open (move (fd), m | fdstream_mode::text);
       }
       catch (const io_error& e)
       {
         fail << "unable to read from " << args0 << " stderr: " << e;
       }
     }
-    else
-      r = process::pipe (-1, 2);
 
     this->args0 = args0;
     state_ = state::opened;
-    return r;
   }
 
   void diag_buffer::
diff --git a/libbuild2/diagnostics.hxx b/libbuild2/diagnostics.hxx
index 9b9f6d8..397d3c0 100644
--- a/libbuild2/diagnostics.hxx
+++ b/libbuild2/diagnostics.hxx
@@ -730,29 +730,59 @@ namespace build2
   //
   // - Do nothing (in serial builds or if requested not to buffer).
   //
-  // In the future this class will also be responsible for converting the
+  // In the future this class may also be responsible for converting the
   // diagnostics into the structured form (which means it may need to buffer
   // even in serial builds).
   //
+  // The typical usage is as follows:
+  //
+  // process pr (..., diag_buffer::pipe (ctx));
+  // diag_buffer dbuf (ctx, args[0], pr);  // Skip.
+  // ifdstream is (move (pr.in_ofd));      // No skip.
+  // ofdstream os (move (pr.out_fd));
+  //
+  // The reason for this somewhat roundabout setup is to make sure the
+  // diag_buffer instance is destroyed before the process instance. This is
+  // important in case an exception is thrown where we want to make sure all
+  // our pipe ends are closed before we wait for the process exit (which
+  // happens in the process destructor).
+  //
+  // And speaking of the destruction order, another thing to keep in mind is
+  // that only one stream can use the skip mode (fdstream_mode::skip; because
+  // skipping is performed in the blocking mode) and the stream that skips
+  // should come first so that all other streams are destroyed/closed before
+  // it (failed that, we may end up in a deadlock). For example:
+  //
+  // process pr (..., diag_buffer::pipe (ctx));
+  // ifdstream is (move (pr.in_ofd), fdstream_mode::skip);      // Skip.
+  // diag_buffer dbuf (ctx, args[0], pr, fdstream_mode::none);  // No skip.
+  // ofdstream os (move (pr.out_fd));
+  //
   class LIBBUILD2_SYMEXPORT diag_buffer
   {
   public:
-    explicit
-    diag_buffer (context& c): is (ifdstream::badbit), ctx_ (c) {}
+    // If buffering is necessary or force is true, return an "instruction"
+    // (-1) to the process class constructor to open a pipe and redirect
+    // stderr to it. Otherwise, return an "instruction" to inherit stderr (2).
+    //
+    // The force flag is normally used if custom diagnostics processing is
+    // required (filter, split, etc; see read() below).
+    //
+    // Note that the diagnostics buffer must be opened (see below) regardless
+    // of the pipe() result.
+    //
+    static int
+    pipe (context&, bool force = false);
 
-  public:
-    // If buffering is necessary or force is true, open a pipe and return the
-    // child process end of it. Otherwise, return stderr. If mode is
-    // non_blocking, then make reading from the parent end of the pipe
-    // non-blocking.
+    // Open the diagnostics buffer given the parent end of the pipe (normally
+    // process:in_efd). If it is nullfd, then assume no buffering is
+    // necessary. If mode is non_blocking, then make reading from the parent
+    // end of the pipe non-blocking.
     //
     // The args0 argument is the child process program name for diagnostics.
     // It is expected to remain valid until the call to close() and should
     // normally be the same as args[0] passed to close().
     //
-    // The force flag is normally used if custom diagnostics processing is
-    // required (filter, split, etc; see read() below).
-    //
     // Note that the same buffer can go through multiple open-read-close
     // sequences, for example, to execute multiple commands.
     //
@@ -764,10 +794,68 @@ namespace build2
     // only the last stream to be destroyed can normally have the skip mode
     // since in case of an exception, skipping will be blocking.
     //
-    process::pipe
+    diag_buffer (context&,
+                 const char* args0,
+                 auto_fd&&,
+                 fdstream_mode = fdstream_mode::skip);
+
+    // As above, but the parrent end of the pipe (process:in_efd) is passed
+    // via a process instance.
+    //
+    diag_buffer (context&,
+                 const char* args0,
+                 process&,
+                 fdstream_mode = fdstream_mode::skip);
+
+    // As above but with support for the underlying buffer reuse.
+    //
+    // Note that in most cases reusing the buffer is probably not worth the
+    // trouble because we normally don't expect any diagnostics in the common
+    // case. However, if needed, it can be arranged, for example:
+    //
+    // vector<char> buf;
+    //
+    // {
+    //   process pr (...);
+    //   diag_buffer dbuf (ctx, move (buf), args[0], pr);
+    //   dbuf.read ();
+    //   dbuf.close ();
+    //   buf = move (dbuf.buf);
+    // }
+    //
+    // {
+    //   ...
+    // }
+    //
+    // Note also that while there is no guarantee the underlying buffer is
+    // moved when, say, the vector is empty, all the main implementations
+    // always steal the buffer.
+    //
+    diag_buffer (context&,
+                 vector<char>&& buf,
+                 const char* args0,
+                 auto_fd&&,
+                 fdstream_mode = fdstream_mode::skip);
+
+    diag_buffer (context&,
+                 vector<char>&& buf,
+                 const char* args0,
+                 process&,
+                 fdstream_mode = fdstream_mode::skip);
+
+    // Separate construction and opening.
+    //
+    // Note: be careful with the destruction order (see above for details).
+    //
+    explicit
+    diag_buffer (context&);
+
+    diag_buffer (context&, vector<char>&& buf);
+
+    void
     open (const char* args0,
-          bool force = false,
-          fdstream_mode mode = fdstream_mode::skip);
+          auto_fd&&,
+          fdstream_mode = fdstream_mode::skip);
 
     // Open the buffer in the state as if after read() returned false, that
     // is, the stream corresponding to the parent's end of the pipe reached
@@ -849,20 +937,17 @@ namespace build2
     //
     void
     close (const cstrings& args,
-           const process_exit& pe,
+           const process_exit&,
            uint16_t verbosity,
            bool omit_normal = false,
-           const location& loc = {})
-    {
-      close (args.data (), pe, verbosity, omit_normal, loc);
-    }
+           const location& = {});
 
     void
     close (const char* const* args,
-           const process_exit& pe,
+           const process_exit&,
            uint16_t verbosity,
            bool omit_normal = false,
-           const location& loc = {});
+           const location& = {});
 
     // As above but with a custom diag record for the child exit diagnostics,
     // if any. Note that if the diag record has the fail epilogue, then this
diff --git a/libbuild2/diagnostics.ixx b/libbuild2/diagnostics.ixx
index a082290..273dfad 100644
--- a/libbuild2/diagnostics.ixx
+++ b/libbuild2/diagnostics.ixx
@@ -3,6 +3,8 @@
 
 namespace build2
 {
+  // print_diag()
+  //
   LIBBUILD2_SYMEXPORT void
   print_diag_impl (const char*, target_key*, target_key&&, const char*);
 
@@ -60,4 +62,65 @@ namespace build2
   {
     print_diag (p, l, path_name (&r), c);
   }
+
+  // diag_buffer
+  //
+  inline diag_buffer::
+  diag_buffer (context& ctx)
+      : is (ifdstream::badbit), ctx_ (ctx)
+  {
+  }
+
+  inline diag_buffer::
+  diag_buffer (context& ctx, vector<char>&& b)
+      : is (ifdstream::badbit), buf (move (b)), ctx_ (ctx)
+  {
+    buf.clear ();
+  }
+
+  inline diag_buffer::
+  diag_buffer (context& ctx, const char* args0, auto_fd&& fd, fdstream_mode m)
+      : diag_buffer (ctx)
+  {
+    open (args0, move (fd), m);
+  }
+
+  inline diag_buffer::
+  diag_buffer (context& ctx, const char* args0, process& pr, fdstream_mode m)
+      : diag_buffer (ctx)
+  {
+    open (args0, move (pr.in_efd), m);
+  }
+
+  inline diag_buffer::
+  diag_buffer (context& ctx,
+               vector<char>&& b,
+               const char* args0,
+               auto_fd&& fd,
+               fdstream_mode m)
+      : diag_buffer (ctx, move (b))
+  {
+    open (args0, move (fd), m);
+  }
+
+  inline diag_buffer::
+  diag_buffer (context& ctx,
+               vector<char>&& b,
+               const char* args0,
+               process& pr,
+               fdstream_mode m)
+      : diag_buffer (ctx, move (b))
+  {
+    open (args0, move (pr.in_efd), m);
+  }
+
+  inline void diag_buffer::
+  close (const cstrings& args,
+         const process_exit& pe,
+         uint16_t verbosity,
+         bool omit_normal,
+         const location& loc)
+  {
+    close (args.data (), pe, verbosity, omit_normal, loc);
+  }
 }
diff --git a/libbuild2/install/rule.cxx b/libbuild2/install/rule.cxx
index 36a06fa..8818ea3 100644
--- a/libbuild2/install/rule.cxx
+++ b/libbuild2/install/rule.cxx
@@ -1198,11 +1198,11 @@ namespace build2
               print_diag ("uninstall -d", chd);
           }
 
-          diag_buffer dbuf (rs.ctx);
           process pr (run_start (pp, args,
-                                 0                   /* stdin */,
-                                 1                   /* stdout */,
-                                 dbuf.open (args[0]) /* stderr */));
+                                 0                          /* stdin */,
+                                 1                          /* stdout */,
+                                 diag_buffer::pipe (rs.ctx) /* stderr */));
+          diag_buffer dbuf (rs.ctx, args[0], pr);
           dbuf.read ();
           r = run_finish_code (
             dbuf,
diff --git a/libbuild2/parser.cxx b/libbuild2/parser.cxx
index c761d09..c8f66de 100644
--- a/libbuild2/parser.cxx
+++ b/libbuild2/parser.cxx
@@ -3133,7 +3133,7 @@ namespace build2
                            cargs,
                            0            /* stdin  */,
                            -1           /* stdout */,
-                           {-1, 2}      /* stderr */,
+                           2            /* stderr */,
                            nullptr      /* env    */,
                            dir_path ()  /* cwd    */,
                            l));
diff --git a/libbuild2/script/run.cxx b/libbuild2/script/run.cxx
index cbd750f..550fdc1 100644
--- a/libbuild2/script/run.cxx
+++ b/libbuild2/script/run.cxx
@@ -2059,31 +2059,45 @@ namespace build2
         {
         case redirect_type::pass:
           {
-            if (dfd == 2) // stderr?
+            try
             {
-              // Deduce the args0 argument similar to cmd_path().
-              //
-              process::pipe ep (
+              if (dfd == 2) // stderr?
+              {
+                fdpipe p;
+                if (diag_buffer::pipe (env.context) == -1) // Are we buffering?
+                  p = fdopen_pipe ();
+
+                // @@ TODO: perhaps this lambda should return process::pipe?
+                //          (see the simple test rule implementatio for an
+                //           example).
+                //
+                // Note that we must return non-owning fd to our end of the
+                // pipe (see the process class for details).
+                //
+                // process::pipe r (p.in.get (), move (p.out));
+
+                // Deduce the args0 argument similar to cmd_path().
+                //
+                // Note that we must open the diag buffer regardless of the
+                // diag_buffer::pipe() result.
+                //
                 pc.dbuf.open ((c.program.initial == nullptr
                                ? c.program.recall.string ().c_str ()
                                : c.program.recall_string ()),
-                              false /* force */,
-                              fdstream_mode::non_blocking));
+                              move (p.in),
+                              fdstream_mode::non_blocking);
 
-              if (ep.out != 2) // Are we buffering stderr?
-              {
-                ep.own_out = false;
-                return auto_fd (ep.out);
+                if (p.out != nullfd)
+                  return move (p.out);
+
+                // Fall through.
               }
-            }
 
-            try
-            {
               return fddup (dfd);
             }
             catch (const io_error& e)
             {
-              fail (ll) << "unable to duplicate " << what << ": " << e;
+              fail (ll) << "unable to redirect " << what << ": " << e;
             }
           }
 
diff --git a/libbuild2/test/rule.cxx b/libbuild2/test/rule.cxx
index 5cdd8fb..5992414 100644
--- a/libbuild2/test/rule.cxx
+++ b/libbuild2/test/rule.cxx
@@ -958,13 +958,37 @@ namespace build2
 
         process p;
         {
-          process::pipe ep (pp.dbuf.open (args[0],
-                                          pp.force_dbuf,
-                                          fdstream_mode::non_blocking));
+          process::pipe ep;
+          {
+            fdpipe p;
+            if (diag_buffer::pipe (t.ctx, pp.force_dbuf) == -1) // Buffering?
+            {
+              try
+              {
+                p = fdopen_pipe ();
+              }
+              catch (const io_error& e)
+              {
+                fail << "unable to redirect stderr: " << e;
+              }
+
+              // Note that we must return non-owning fd to our end of the pipe
+              // (see the process class for details).
+              //
+              ep = process::pipe (p.in.get (), move (p.out));
+            }
+            else
+              ep = process::pipe (-1, 2);
+
+            // Note that we must open the diag buffer regardless of the
+            // diag_buffer::pipe() result.
+            //
+            pp.dbuf.open (args[0], move (p.in), fdstream_mode::non_blocking);
+          }
 
           p = (prev == nullptr
-               ? process (args, 0, out, ep.out)             // First process.
-               : process (args, *prev->proc, out, ep.out)); // Next process.
+               ? process (args, 0, out, move (ep))             // First process.
+               : process (args, *prev->proc, out, move (ep))); // Next process.
         }
 
         pp.proc = &p;
diff --git a/libbuild2/utility.cxx b/libbuild2/utility.cxx
index 3ce0026..5a58287 100644
--- a/libbuild2/utility.cxx
+++ b/libbuild2/utility.cxx
@@ -218,7 +218,7 @@ namespace build2
              const char* const* args,
              int in,
              int out,
-             process::pipe err,
+             int err,
              const location& l)
   try
   {
@@ -232,7 +232,7 @@ namespace build2
       args,
       in,
       out,
-      move (err),
+      err,
       pe.cwd != nullptr ? pe.cwd->string ().c_str () : nullptr,
       pe.vars);
   }
@@ -379,26 +379,17 @@ namespace build2
     }
     else
     {
-      diag_buffer dbuf (ctx);
-      run (dbuf, pe, args, v);
+      process pr (run_start (pe,
+                             args,
+                             0                       /* stdin  */,
+                             1                       /* stdout */,
+                             diag_buffer::pipe (ctx) /* stderr */));
+      diag_buffer dbuf (ctx, args[0], pr);
+      dbuf.read ();
+      run_finish (dbuf, args, pr, v);
     }
   }
 
-  void
-  run (diag_buffer& dbuf,
-       const process_env& pe,
-       const char* const* args,
-       uint16_t v)
-  {
-    process pr (run_start (pe,
-                           args,
-                           0                   /* stdin  */,
-                           1                   /* stdout */,
-                           dbuf.open (args[0]) /* stderr */));
-    dbuf.read ();
-    run_finish (dbuf, args, pr, v);
-  }
-
   bool
   run (context& ctx,
        uint16_t verbosity,
@@ -413,101 +404,73 @@ namespace build2
   {
     assert (!err || !ignore_exit);
 
-    if (err && ctx.phase != run_phase::load)
+    if (!err || ctx.phase == run_phase::load)
     {
-      diag_buffer dbuf (ctx);
-      run (dbuf,
-           verbosity,
-           pe, args,
-           finish_verbosity,
-           f,
-           tr,
-           checksum);
-      return true;
-    }
+      process pr (run_start (verbosity,
+                             pe,
+                             args,
+                             0           /* stdin */,
+                             -1          /* stdout */,
+                             err ? 2 : 1 /* stderr */));
+
+      string l; // Last line of output.
+      try
+      {
+        ifdstream is (move (pr.in_ofd), fdstream_mode::skip);
 
-    process pr (run_start (verbosity,
-                           pe,
-                           args,
-                           0  /* stdin */,
-                           -1 /* stdout */,
-                           {-1, err ? 2 : 1}));
+        bool empty (true);
 
-    string l; // Last line of output.
-    try
-    {
-      ifdstream is (move (pr.in_ofd), fdstream_mode::skip);
+        // Make sure we keep the last line.
+        //
+        for (bool last (is.peek () == ifdstream::traits_type::eof ());
+             !last && getline (is, l); )
+        {
+          last = (is.peek () == ifdstream::traits_type::eof ());
 
-      bool empty (true);
+          if (tr)
+            trim (l);
 
-      // Make sure we keep the last line.
-      //
-      for (bool last (is.peek () == ifdstream::traits_type::eof ());
-           !last && getline (is, l); )
-      {
-        last = (is.peek () == ifdstream::traits_type::eof ());
+          if (checksum != nullptr)
+            checksum->append (l);
 
-        if (tr)
-          trim (l);
+          if (empty)
+          {
+            empty = f (l, last);
 
-        if (checksum != nullptr)
-          checksum->append (l);
+            if (!empty && checksum == nullptr)
+              break;
+          }
+        }
 
-        if (empty)
-        {
-          empty = f (l, last);
+        is.close ();
+      }
+      catch (const io_error& e)
+      {
+        if (run_wait (args, pr))
+          fail << "io error reading " << args[0] << " output: " << e << endf;
 
-          if (!empty && checksum == nullptr)
-            break;
-        }
+        // If the child process has failed then assume the io error was
+        // caused by that and let run_finish() deal with it.
       }
 
-      is.close ();
+      // Omit normal exit code diagnostics if err is false.
+      //
+      if (!(run_finish_impl (args, pr, err, l, finish_verbosity, !err) ||
+            ignore_exit))
+        return false;
     }
-    catch (const io_error& e)
+    else
     {
-      if (run_wait (args, pr))
-        fail << "io error reading " << args[0] << " output: " << e << endf;
-
-      // If the child process has failed then assume the io error was
-      // caused by that and let run_finish() deal with it.
-    }
-
-    // Omit normal exit code diagnostics if err is false.
-    //
-    if (!(run_finish_impl (args, pr, err, l, finish_verbosity, !err) ||
-          ignore_exit))
-      return false;
-
-    return true;
-  }
-
-  void
-  run (diag_buffer& dbuf,
-       uint16_t verbosity,
-       const process_env& pe,
-       const char* const* args,
-       uint16_t finish_verbosity,
-       const function<bool (string&, bool)>& f,
-       bool tr,
-       sha256* checksum)
-  {
-    // We have to use the non-blocking setup since we have to read from stdout
-    // and stderr simultaneously.
-    //
-    process pr (
-      run_start (verbosity,
-                 pe,
-                 args,
-                 0                                /* stdin */,
-                 -1                               /* stdout */,
-                 dbuf.open (args[0],
-                            false /* force */,
-                            fdstream_mode::non_blocking |
-                            fdstream_mode::skip)  /* stderr */));
+      // We have to use the non-blocking setup since we have to read from stdout
+      // and stderr simultaneously.
+      //
+      process pr (run_start (verbosity,
+                             pe,
+                             args,
+                             0                       /* stdin */,
+                             -1                      /* stdout */,
+                             diag_buffer::pipe (ctx) /* stderr */));
 
-    try
-    {
       // Note that while we read both streams until eof in the normal
       // circumstances, we cannot use fdstream_mode::skip for the exception
       // case on both of them: we may end up being blocked trying to read one
@@ -516,95 +479,102 @@ namespace build2
       // hard. The latter should happen first so the order of the dbuf/is
       // variables is important.
       //
-      ifdstream is (move (pr.in_ofd),
-                    fdstream_mode::non_blocking,
-                    ifdstream::badbit);
+      diag_buffer dbuf (ctx, args[0], pr, (fdstream_mode::non_blocking |
+                                           fdstream_mode::skip));
+      try
+      {
+        ifdstream is (move (pr.in_ofd),
+                      fdstream_mode::non_blocking,
+                      ifdstream::badbit);
 
-      bool empty (true);
+        bool empty (true);
 
-      // Read until we reach EOF on all streams.
-      //
-      // Note that if dbuf is not opened, then we automatically get an
-      // inactive nullfd entry.
-      //
-      fdselect_set fds {is.fd (), dbuf.is.fd ()};
-      fdselect_state& ist (fds[0]);
-      fdselect_state& dst (fds[1]);
+        // Read until we reach EOF on all streams.
+        //
+        // Note that if dbuf is not opened, then we automatically get an
+        // inactive nullfd entry.
+        //
+        fdselect_set fds {is.fd (), dbuf.is.fd ()};
+        fdselect_state& ist (fds[0]);
+        fdselect_state& dst (fds[1]);
 
-      // To detect the last line we are going keep the previous line and
-      // only call the function once we've read the next.
-      //
-      optional<string> pl;
+        // To detect the last line we are going keep the previous line and
+        // only call the function once we've read the next.
+        //
+        optional<string> pl;
 
-      for (string l; ist.fd != nullfd || dst.fd != nullfd; )
-      {
-        if (ist.fd != nullfd && getline_non_blocking (is, l))
+        for (string l; ist.fd != nullfd || dst.fd != nullfd; )
         {
-          if (eof (is))
+          if (ist.fd != nullfd && getline_non_blocking (is, l))
           {
-            if (pl && empty)
-              f (*pl, true /* last */);
+            if (eof (is))
+            {
+              if (pl && empty)
+                f (*pl, true /* last */);
 
-            ist.fd = nullfd;
-          }
-          else
-          {
-            if (checksum != nullptr || empty)
+              ist.fd = nullfd;
+            }
+            else
             {
-              if (tr)
-                trim (l);
+              if (checksum != nullptr || empty)
+              {
+                if (tr)
+                  trim (l);
 
-              if (checksum != nullptr)
-                checksum->append (l);
+                if (checksum != nullptr)
+                  checksum->append (l);
 
-              if (empty)
-              {
-                if (pl)
+                if (empty)
                 {
-                  if ((empty = f (*pl, false /* last */)))
+                  if (pl)
+                  {
+                    if ((empty = f (*pl, false /* last */)))
                       swap (l, *pl);
 
-                  // Note that we cannot bail out like in the other version
-                  // since we don't have the skip mode on is. Plus, we might
-                  // still have the diagnostics.
+                    // Note that we cannot bail out like in the other version
+                    // since we don't have the skip mode on is. Plus, we might
+                    // still have the diagnostics.
+                  }
+                  else
+                    pl = move (l);
                 }
-                else
-                  pl = move (l);
               }
+
+              l.clear ();
             }
 
-            l.clear ();
+            continue;
           }
 
-          continue;
-        }
+          ifdselect (fds);
 
-        ifdselect (fds);
+          if (dst.ready)
+          {
+            if (!dbuf.read ())
+              dst.fd = nullfd;
+          }
+        }
 
-        if (dst.ready)
+        is.close ();
+      }
+      catch (const io_error& e)
+      {
+        if (run_wait (args, pr))
         {
-          if (!dbuf.read ())
-            dst.fd = nullfd;
+          // Note that we will drop the diagnostics in this case since reading
+          // it could have been the cause of this error.
+          //
+          fail << "io error reading " << args[0] << " output: " << e << endf;
         }
-      }
 
-      is.close ();
-    }
-    catch (const io_error& e)
-    {
-      if (run_wait (args, pr))
-      {
-        // Note that we will drop the diagnostics in this case since reading
-        // it could have been the cause of this error.
-        //
-        fail << "io error reading " << args[0] << " output: " << e << endf;
+        // If the child process has failed then assume the io error was caused
+        // by that and let run_finish() deal with it.
       }
 
-      // If the child process has failed then assume the io error was
-      // caused by that and let run_finish() deal with it.
+      run_finish_impl (dbuf, args, pr, true /* fail */, finish_verbosity);
     }
 
-    run_finish_impl (dbuf, args, pr, true /* fail */, finish_verbosity);
+    return true;
   }
 
   cstrings
diff --git a/libbuild2/utility.hxx b/libbuild2/utility.hxx
index 5fffc8c..c12fae7 100644
--- a/libbuild2/utility.hxx
+++ b/libbuild2/utility.hxx
@@ -276,7 +276,7 @@ namespace build2
              const char* const* args,
              int in = 0,
              int out = 1,
-             process::pipe = {-1, 2},
+             int err = 2,
              const location& = {});
 
   inline process
@@ -285,10 +285,10 @@ namespace build2
              const cstrings& args,
              int in = 0,
              int out = 1,
-             process::pipe err = {-1, 2},
+             int err = 2,
              const location& l = {})
   {
-    return run_start (verbosity, pe, args.data (), in, out, move (err), l);
+    return run_start (verbosity, pe, args.data (), in, out, err, l);
   }
 
   inline process
@@ -296,10 +296,10 @@ namespace build2
              const char* const* args,
              int in = 0,
              int out = 1,
-             process::pipe err = {-1, 2},
+             int err = 2,
              const location& l = {})
   {
-    return run_start (verb_never, pe, args, in, out, move (err), l);
+    return run_start (verb_never, pe, args, in, out, err, l);
   }
 
   inline process
@@ -307,10 +307,10 @@ namespace build2
              const cstrings& args,
              int in = 0,
              int out = 1,
-             process::pipe err = {-1, 2},
+             int err = 2,
              const location& l = {})
   {
-    return run_start (pe, args.data (), in, out, move (err), l);
+    return run_start (pe, args.data (), in, out, err, l);
   }
 
   // As above, but search for the process (including updating args[0]) and
@@ -321,7 +321,7 @@ namespace build2
              const char* args[],
              int in = 0,
              int out = 1,
-             process::pipe err = {-1, 2},
+             int err = 2,
              const char* const* env = nullptr,
              const dir_path& cwd = {},
              const location& l = {})
@@ -329,7 +329,7 @@ namespace build2
     process_path pp (run_search (args[0], l));
     return run_start (verbosity,
                       process_env (pp, cwd, env), args,
-                      in, out, move (err),
+                      in, out, err,
                       l);
   }
 
@@ -338,15 +338,12 @@ namespace build2
              cstrings& args,
              int in = 0,
              int out = 1,
-             process::pipe err = {-1, 2},
+             int err = 2,
              const char* const* env = nullptr,
              const dir_path& cwd = {},
              const location& l = {})
   {
-    return run_start (verbosity,
-                      args.data (),
-                      in, out, move (err),
-                      env, cwd, l);
+    return run_start (verbosity, args.data (), in, out, err, env, cwd, l);
   }
 
   // Wait for process termination returning true if the process exited
@@ -485,12 +482,6 @@ namespace build2
        const char* const* args,
        uint16_t finish_verbosity);
 
-  LIBBUILD2_SYMEXPORT void
-  run (diag_buffer&,
-       const process_env& pe,
-       const char* const* args,
-       uint16_t finish_verbosity);
-
   inline void
   run (context& ctx,
        const process_env& pe,
@@ -500,15 +491,6 @@ namespace build2
     run (ctx, pe, args.data (), finish_verbosity);
   }
 
-  inline void
-  run (diag_buffer& dbuf,
-       const process_env& pe,
-       const cstrings& args,
-       uint16_t finish_verbosity)
-  {
-    run (dbuf, pe, args.data (), finish_verbosity);
-  }
-
   // As above but pass cwd/env vars as arguments rather than as part of
   // process_env.
   //
@@ -524,17 +506,6 @@ namespace build2
   }
 
   inline void
-  run (diag_buffer& dbuf,
-       const process_path& p,
-       const char* const* args,
-       uint16_t finish_verbosity,
-       const char* const* env,
-       const dir_path& cwd = {})
-  {
-    run (dbuf, process_env (p, cwd, env), args, finish_verbosity);
-  }
-
-  inline void
   run (context& ctx,
        const process_path& p,
        const cstrings& args,
@@ -545,17 +516,6 @@ namespace build2
     run (ctx, p, args.data (), finish_verbosity, env, cwd);
   }
 
-  inline void
-  run (diag_buffer& dbuf,
-       const process_path& p,
-       const cstrings& args,
-       uint16_t finish_verbosity,
-       const char* const* env,
-       const dir_path& cwd = {})
-  {
-    run (dbuf, p, args.data (), finish_verbosity, env, cwd);
-  }
-
   // Start the process as above and then call the specified function on each
   // trimmed line of the output until it returns a non-empty object T (tested
   // with T::empty()) which is then returned to the caller.
@@ -611,31 +571,6 @@ namespace build2
   }
 
   template <typename T, typename F>
-  T
-  run (diag_buffer&,
-       uint16_t verbosity,
-       const process_env&,
-       const char* const* args,
-       F&&,
-       sha256* checksum = nullptr);
-
-  template <typename T, typename F>
-  inline T
-  run (diag_buffer& dbuf,
-       uint16_t verbosity,
-       const process_env& pe,
-       const cstrings& args,
-       F&& f,
-       sha256* checksum = nullptr)
-  {
-    return run<T> (dbuf,
-                   verbosity,
-                   pe, args.data (),
-                   forward<F> (f),
-                   checksum);
-  }
-
-  template <typename T, typename F>
   inline T
   run (context&,
        const process_env&,
@@ -666,31 +601,6 @@ namespace build2
 
   template <typename T, typename F>
   inline T
-  run (diag_buffer&,
-       const process_env&,
-       const char* const* args,
-       uint16_t finish_verbosity,
-       F&&,
-       sha256* checksum = nullptr);
-
-  template <typename T, typename F>
-  inline T
-  run (diag_buffer& dbuf,
-       const process_env& pe,
-       const cstrings& args,
-       uint16_t finish_verbosity,
-       F&& f,
-       sha256* checksum = nullptr)
-  {
-    return run<T> (dbuf,
-                   pe, args.data (),
-                   finish_verbosity,
-                   forward<F> (f),
-                   checksum);
-  }
-
-  template <typename T, typename F>
-  inline T
   run (context& ctx,
        uint16_t verbosity,
        const char* args[],
@@ -724,37 +634,6 @@ namespace build2
                    error, ignore_exit, checksum);
   }
 
-  template <typename T, typename F>
-  inline T
-  run (diag_buffer& dbuf,
-       uint16_t verbosity,
-       const char* args[],
-       F&& f,
-       sha256* checksum = nullptr)
-  {
-    process_path pp (run_search (args[0]));
-    return run<T> (dbuf,
-                   verbosity,
-                   pp, args,
-                   forward<F> (f),
-                   checksum);
-  }
-
-  template <typename T, typename F>
-  inline T
-  run (diag_buffer& dbuf,
-       uint16_t verbosity,
-       cstrings& args,
-       F&& f,
-       sha256* checksum = nullptr)
-  {
-    return run<T> (dbuf,
-                   verbosity,
-                   args.data (),
-                   forward<F> (f),
-                   checksum);
-  }
-
   // As above but run a program without any arguments or with one argument.
   //
   // run <prog>
@@ -862,16 +741,6 @@ namespace build2
        bool ignore_exit = false,
        sha256* checksum = nullptr);
 
-  LIBBUILD2_SYMEXPORT void
-  run (diag_buffer& dbuf,
-       uint16_t verbosity,
-       const process_env&,
-       const char* const* args,
-       uint16_t finish_verbosity,
-       const function<bool (string& line, bool last)>&,
-       bool trim = true,
-       sha256* checksum = nullptr);
-
   // Concatenate the program path and arguments into a shallow NULL-terminated
   // vector of C-strings.
   //
diff --git a/libbuild2/utility.ixx b/libbuild2/utility.ixx
index 3948623..58ea8db 100644
--- a/libbuild2/utility.ixx
+++ b/libbuild2/utility.ixx
@@ -169,30 +169,6 @@ namespace build2
 
   template <typename T, typename F>
   inline T
-  run (diag_buffer& dbuf,
-       uint16_t verbosity,
-       const process_env& pe,
-       const char* const* args,
-       F&& f,
-       sha256* checksum)
-  {
-    T r;
-    run (dbuf,
-         verbosity,
-         pe, args,
-         verbosity - 1,
-         [&r, &f] (string& l, bool last) // Small function optimmization.
-         {
-           r = f (l, last);
-           return r.empty ();
-         },
-         true /* trim */,
-         checksum);
-    return r;
-  }
-
-  template <typename T, typename F>
-  inline T
   run (context& ctx,
        const process_env& pe,
        const char* const* args,
@@ -221,30 +197,6 @@ namespace build2
     return r;
   }
 
-  template <typename T, typename F>
-  inline T
-  run (diag_buffer& dbuf,
-       const process_env& pe,
-       const char* const* args,
-       uint16_t finish_verbosity,
-       F&& f,
-       sha256* checksum)
-  {
-    T r;
-    run (dbuf,
-         verb_never,
-         pe, args,
-         finish_verbosity,
-         [&r, &f] (string& l, bool last)
-         {
-           r = f (l, last);
-           return r.empty ();
-         },
-         true /* trim */,
-         checksum);
-    return r;
-  }
-
   inline void
   hash_path (sha256& cs, const path& p, const dir_path& prefix)
   {
diff --git a/libbuild2/version/snapshot-git.cxx b/libbuild2/version/snapshot-git.cxx
index ad12c9f..ab0224a 100644
--- a/libbuild2/version/snapshot-git.cxx
+++ b/libbuild2/version/snapshot-git.cxx
@@ -127,7 +127,7 @@ namespace build2
                              args,
                              0       /* stdin  */,
                              -1      /* stdout */,
-                             {-1, 1} /* stderr (to stdout) */));
+                             1       /* stderr (to stdout) */));
 
       string l;
       try