Initial work on child process diagnostics buffering

Currently this is implemented for C/C++ compile and link rules.

10 files changed, 578 insertions, 87 deletions

diff --git a/libbuild2/bin/def-rule.cxx b/libbuild2/bin/def-rule.cxx
index c0e82fb..421074c 100644
--- a/libbuild2/bin/def-rule.cxx
+++ b/libbuild2/bin/def-rule.cxx
@@ -751,6 +751,9 @@ namespace build2
       return target_state::changed;
     }
 
+    // @@ TODO: split into rule_name (string) and rule_version (integer)
+    //    on next increment.
+    //
     const string def_rule::rule_id_ {"bin.def 2"};
   }
 }
diff --git a/libbuild2/cc/compile-rule.cxx b/libbuild2/cc/compile-rule.cxx
index fda97a0..8d507e7 100644
--- a/libbuild2/cc/compile-rule.cxx
+++ b/libbuild2/cc/compile-rule.cxx
@@ -264,6 +264,10 @@ namespace build2
     compile_rule::
     compile_rule (data&& d, const scope& rs)
         : common (move (d)),
+          //
+          // @@ TODO: split into rule_name (string) and rule_version (integer)
+          //    on next increment.
+          //
           rule_id (string (x) += ".compile 6")
     {
       // Locate the header cache (see enter_header() for details).
@@ -3961,6 +3965,8 @@ namespace build2
               // diagnostics (if things go badly we will restart with this
               // support).
               //
+              // @@ TODO: diag_buffer
+              //
               if (drmp == nullptr) // Dependency info goes to stdout.
               {
                 assert (!sense_diag); // Note: could support with fdselect().
@@ -3970,7 +3976,7 @@ namespace build2
                 //
                 pr = process (
                   cpath,
-                  args.data (),
+                  args,
                   0,
                   -1,
                   cclass == compiler_class::msvc ? 1 : gen ? 2 : -2,
@@ -3980,7 +3986,7 @@ namespace build2
               else // Dependency info goes to a temporary file.
               {
                 pr = process (cpath,
-                              args.data (),
+                              args,
                               mod_mapper ? -1 : 0,
                               mod_mapper ? -1 : 2, // Send stdout to stderr.
                               gen ? 2 : sense_diag ? -1 : -2,
@@ -4890,10 +4896,10 @@ namespace build2
               print_process (args);
 
             // We don't want to see warnings multiple times so ignore all
-            // diagnostics.
+            // diagnostics (thus no need for diag_buffer).
             //
             pr = process (cpath,
-                          args.data (),
+                          args,
                           0, -1, -2,
                           nullptr, // CWD
                           env.empty () ? nullptr : env.data ());
@@ -6327,7 +6333,7 @@ namespace build2
     // Filter cl.exe noise (msvc.cxx).
     //
     void
-    msvc_filter_cl (ifdstream&, const path& src);
+    msvc_filter_cl (diag_buffer&, const path& src);
 
     // Append header unit-related options.
     //
@@ -6674,7 +6680,7 @@ namespace build2
       small_vector<string, 2> module_args; // Module options storage.
 
       size_t out_i (0);  // Index of the -o option.
-      size_t lang_n (0); // Number of lang options.
+      //size_t lang_n (0); // Number of lang options. @@ TMP
 
       switch (cclass)
       {
@@ -7025,7 +7031,7 @@ namespace build2
             args.push_back ("-c");
           }
 
-          lang_n = append_lang_options (args, md);
+          /*lang_n = */append_lang_options (args, md); // @@ TMP
 
           if (md.pp == preprocessed::all)
           {
@@ -7074,6 +7080,8 @@ namespace build2
       // the source file, not its preprocessed version (so that it's easy to
       // copy and re-run, etc). Only at level 3 and above print the real deal.
       //
+      // @@ TODO: why don't we print env (here and/or below)? Also link rule.
+      //
       if (verb == 1)
         text << x_name << ' ' << s;
       else if (verb == 2)
@@ -7081,12 +7089,17 @@ namespace build2
 
       // If we have the (partially) preprocessed output, switch to that.
       //
+      // But we remember the original source/position to restore later.
+      //
       bool psrc (md.psrc);
       bool ptmp (psrc && md.psrc.temporary);
+      pair<size_t, const char*> osrc;
       if (psrc)
       {
         args.pop_back (); // nullptr
+        osrc.second = args.back ();
         args.pop_back (); // sp
+        osrc.first = args.size ();
 
         sp = &md.psrc.path ();
 
@@ -7096,6 +7109,8 @@ namespace build2
         {
         case compiler_type::gcc:
           {
+            // @@ TMP
+#if 0
             // The -fpreprocessed is implied by .i/.ii. But not when compiling
             // a header unit (there is no .hi/.hii).
             //
@@ -7109,6 +7124,15 @@ namespace build2
               //
               for (; lang_n != 0; --lang_n)
                 args.pop_back ();
+#else
+            // -fpreprocessed is implied by .i/.ii unless compiling a header
+            // unit (there is no .hi/.hii). Also, we would need to pop -x
+            // since it takes precedence over the extension, which would mess
+            // up our osrc logic. So in the end it feels like always passing
+            // explicit -fpreprocessed is the way to go.
+            //
+            args.push_back ("-fpreprocessed");
+#endif
 
             args.push_back ("-fdirectives-only");
             break;
@@ -7145,6 +7169,8 @@ 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
@@ -7163,45 +7189,36 @@ namespace build2
           file_cache::read psrcr (psrc ? md.psrc.open () : file_cache::read ());
 
           // VC cl.exe sends diagnostics to stdout. It also prints the file
-          // name being compiled as the first line. So for cl.exe we redirect
-          // stdout to a pipe, filter that noise out, and send the rest to
-          // stderr.
+          // name being compiled as the first line. So for cl.exe we filter
+          // that noise out.
           //
-          // For other compilers redirect stdout to stderr, in case any of
-          // them tries to pull off something similar. For sane compilers this
-          // should be harmless.
+          // For other compilers also redirect stdout to stderr, in case any
+          // of them tries to pull off something similar. For sane compilers
+          // this should be harmless.
           //
           bool filter (ctype == compiler_type::msvc);
 
           process pr (cpath,
-                      args.data (),
-                      0, (filter ? -1 : 2), 2,
+                      args,
+                      0, 2, dbuf.open (args[0], filter),
                       nullptr, // CWD
                       env.empty () ? nullptr : env.data ());
 
           if (filter)
-          {
-            try
-            {
-              ifdstream is (
-                move (pr.in_ofd), fdstream_mode::text, ifdstream::badbit);
-
-              msvc_filter_cl (is, *sp);
+            msvc_filter_cl (dbuf, *sp);
 
-              // If anything remains in the stream, send it all to stderr.
-              // Note that the eof check is important: if the stream is at
-              // eof, this and all subsequent writes to the diagnostics stream
-              // will fail (and you won't see a thing).
-              //
-              if (is.peek () != ifdstream::traits_type::eof ())
-                diag_stream_lock () << is.rdbuf ();
+          dbuf.read ();
 
-              is.close ();
-            }
-            catch (const io_error&) {} // Assume exits with error.
+          // Restore the original source if we switched to preprocessed.
+          //
+          if (psrc)
+          {
+            args.resize (osrc.first);
+            args.push_back (osrc.second);
+            args.push_back (nullptr);
           }
 
-          run_finish (args, pr);
+          dbuf.finish (args, pr);
         }
         catch (const process_error& e)
         {
@@ -7252,12 +7269,14 @@ namespace build2
           try
           {
             process pr (cpath,
-                        args.data (),
-                        0, 2, 2,
+                        args,
+                        0, 2, dbuf.open (args[0]),
                         nullptr, // CWD
                         env.empty () ? nullptr : env.data ());
 
-            run_finish (args, pr);
+            dbuf.read ();
+
+            dbuf.finish (args, pr);
           }
           catch (const process_error& e)
           {
diff --git a/libbuild2/cc/link-rule.cxx b/libbuild2/cc/link-rule.cxx
index a5fa7bc..5d212e6 100644
--- a/libbuild2/cc/link-rule.cxx
+++ b/libbuild2/cc/link-rule.cxx
@@ -254,6 +254,10 @@ namespace build2
     link_rule::
     link_rule (data&& d)
         : common (move (d)),
+          //
+          // @@ TODO: split into rule_name (string) and rule_version (integer)
+          //    on next increment.
+          //
           rule_id (string (x) += ".link 3")
     {
     }
@@ -2671,7 +2675,7 @@ namespace build2
     // Filter link.exe noise (msvc.cxx).
     //
     void
-    msvc_filter_link (ifdstream&, const file&, otype);
+    msvc_filter_link (diag_buffer&, const file&, otype);
 
     // Translate target CPU to the link.exe/lib.exe /MACHINE option.
     //
@@ -2879,6 +2883,8 @@ 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.
@@ -2929,12 +2935,15 @@ namespace build2
 
               try
               {
+                // We assume that what we write to stdin is small enough to
+                // fit into the pipe's buffer without blocking.
+                //
                 process pr (rc,
                             args,
-                            -1      /* stdin  */,
-                            1       /* stdout */,
-                            2       /* stderr */,
-                            nullptr /* cwd    */,
+                            -1                  /* stdin  */,
+                            1                   /* stdout */,
+                            dbuf.open (args[0]) /* stderr */,
+                            nullptr             /* cwd    */,
                             env_ptrs.empty () ? nullptr : env_ptrs.data ());
 
                 try
@@ -2960,7 +2969,8 @@ namespace build2
                   // was caused by that and let run_finish() deal with it.
                 }
 
-                run_finish (args, pr);
+                dbuf.read ();
+                dbuf.finish (args, pr, 2 /* verbosity */);
               }
               catch (const process_error& e)
               {
@@ -3823,8 +3833,21 @@ namespace build2
       else if (verb == 2)
         print_process (args);
 
+      // Do any necessary fixups to the command line to make it runnable.
+      //
+      // Notice the split in the diagnostics: at verbosity level 1 we print
+      // the "logical" command line while at level 2 and above -- what we are
+      // actually executing.
+      //
+      // We also need to save the original for the diag_buffer::close() call
+      // below if at verbosity level 1.
+      //
+      cstrings oargs;
+
       // Adjust linker parallelism.
       //
+      // Note that we are not going to bother with oargs for this.
+      //
       string jobs_arg;
       scheduler::alloc_guard jobs_extra;
 
@@ -3875,12 +3898,6 @@ namespace build2
         }
       }
 
-      // Do any necessary fixups to the command line to make it runnable.
-      //
-      // Notice the split in the diagnostics: at verbosity level 1 we print
-      // the "logical" command line while at level 2 and above -- what we are
-      // actually executing.
-      //
       // On Windows we need to deal with the command line length limit. The
       // best workaround seems to be passing (part of) the command line in an
       // "options file" ("response file" in Microsoft's terminology). Both
@@ -3966,14 +3983,15 @@ namespace build2
             fail << "unable to write to " << f << ": " << e;
           }
 
+          if (verb == 1)
+            oargs = args;
+
           // Replace input arguments with @file.
           //
           targ = '@' + f.string ();
           args.resize (args_input);
           args.push_back (targ.c_str());
           args.push_back (nullptr);
-
-          //@@ TODO: leave .t file if linker failed and verb > 2?
         }
       }
 #endif
@@ -3996,51 +4014,48 @@ namespace build2
         {
           // VC tools (both lib.exe and link.exe) send diagnostics to stdout.
           // Also, link.exe likes to print various gratuitous messages. So for
-          // link.exe we redirect stdout to a pipe, filter that noise out, and
-          // send the rest to stderr.
+          // link.exe we filter that noise out.
           //
           // For lib.exe (and any other insane linker that may try to pull off
           // something like this) we are going to redirect stdout to stderr.
           // For sane compilers this should be harmless.
           //
           // Note that we don't need this for LLD's link.exe replacement which
-          // is quiet.
+          // is thankfully quiet.
           //
           bool filter (tsys == "win32-msvc"  &&
                        !lt.static_library () &&
                        cast<string> (rs["bin.ld.id"]) != "msvc-lld");
 
           process pr (*ld,
-                      args.data (),
-                      0                  /* stdin  */,
-                      (filter ? -1 : 2)  /* stdout */,
-                      2                  /* stderr */,
-                      nullptr            /* cwd    */,
+                      args,
+                      0                           /* stdin  */,
+                      2                           /* stdout */,
+                      dbuf.open (args[0], filter) /* stderr */,
+                      nullptr                     /* cwd    */,
                       env_ptrs.empty () ? nullptr : env_ptrs.data ());
 
           if (filter)
+            msvc_filter_link (dbuf, t, ot);
+
+          dbuf.read ();
+
           {
-            try
-            {
-              ifdstream is (
-                move (pr.in_ofd), fdstream_mode::text, ifdstream::badbit);
+            bool e (pr.wait ());
 
-              msvc_filter_link (is, t, ot);
+#ifdef _WIN32
+            // Keep the options file if we have shown it.
+            //
+            if (!e && verb > 2)
+              trm.cancel ();
+#endif
 
-              // If anything remains in the stream, send it all to stderr.
-              // Note that the eof check is important: if the stream is at
-              // eof, this and all subsequent writes to the diagnostics stream
-              // will fail (and you won't see a thing).
-              //
-              if (is.peek () != ifdstream::traits_type::eof ())
-                diag_stream_lock () << is.rdbuf ();
+            dbuf.close (oargs.empty () ? args : oargs, *pr.exit);
 
-              is.close ();
-            }
-            catch (const io_error&) {} // Assume exits with error.
+            if (!e)
+              throw failed ();
           }
 
-          run_finish (args, pr);
           jobs_extra.deallocate ();
         }
         catch (const process_error& e)
@@ -4100,10 +4115,25 @@ namespace build2
           print_process (args);
 
         if (!ctx.dry_run)
+        {
+#if 0
           run (rl,
                args,
                dir_path () /* cwd */,
                env_ptrs.empty () ? nullptr : env_ptrs.data ());
+#else
+          process pr (rl,
+                      args,
+                      0                   /* stdin  */,
+                      1                   /* stdout */,
+                      dbuf.open (args[0]) /* stderr */,
+                      nullptr             /* cwd    */,
+                      env_ptrs.empty () ? nullptr : env_ptrs.data ());
+
+          dbuf.read ();
+          dbuf.finish (args, pr);
+#endif
+        }
       }
 
       // For Windows generate (or clean up) rpath-emulating assembly.
diff --git a/libbuild2/cc/msvc.cxx b/libbuild2/cc/msvc.cxx
index f95cab0..9aff347 100644
--- a/libbuild2/cc/msvc.cxx
+++ b/libbuild2/cc/msvc.cxx
@@ -164,18 +164,21 @@ namespace build2
 
     // Filter cl.exe and link.exe noise.
     //
+    // Note: must be followed with the dbuf.read() call.
+    //
     void
-    msvc_filter_cl (ifdstream& is, const path& src)
+    msvc_filter_cl (diag_buffer& dbuf, const path& src)
+    try
     {
       // While it appears VC always prints the source name (event if the
       // file does not exist), let's do a sanity check. Also handle the
       // command line errors/warnings which come before the file name.
       //
-      for (string l; !eof (getline (is, l)); )
+      for (string l; !eof (getline (dbuf.is, l)); )
       {
         if (l != src.leaf ().string ())
         {
-          diag_stream_lock () << l << endl;
+          dbuf.write (l, true /* newline */);
 
           if (msvc_sense_diag (l, 'D').first != string::npos)
             continue;
@@ -184,14 +187,19 @@ namespace build2
         break;
       }
     }
+    catch (const io_error&)
+    {
+      // Let the following diag_buffer::read() call deal with this.
+    }
 
     void
-    msvc_filter_link (ifdstream& is, const file& t, otype lt)
+    msvc_filter_link (diag_buffer& dbuf, const file& t, otype lt)
+    try
     {
       // Filter lines until we encounter something we don't recognize. We also
       // have to assume the messages can be translated.
       //
-      for (string l; getline (is, l); )
+      for (string l; getline (dbuf.is, l); )
       {
         // "   Creating library foo\foo.dll.lib and object foo\foo.dll.exp"
         //
@@ -216,12 +224,15 @@ namespace build2
 
         // /INCREMENTAL causes linker to sometimes issue messages but now I
         // can't quite reproduce it.
-        //
 
-        diag_stream_lock () << l << endl;
+        dbuf.write (l, true /* newline */);
         break;
       }
     }
+    catch (const io_error&)
+    {
+      // Let the following diag_buffer::read() call deal with this.
+    }
 
     void
     msvc_extract_header_search_dirs (const strings& v, dir_paths& r)
diff --git a/libbuild2/diagnostics.cxx b/libbuild2/diagnostics.cxx
index a2a8444..7bdfea2 100644
--- a/libbuild2/diagnostics.cxx
+++ b/libbuild2/diagnostics.cxx
@@ -3,7 +3,7 @@
 
 #include <libbuild2/diagnostics.hxx>
 
-#include <cstring>  // strchr()
+#include <cstring>  // strchr(), memcpy()
 
 #include <libbutl/process-io.hxx>
 
@@ -13,6 +13,7 @@
 #include <libbuild2/context.hxx>
 
 using namespace std;
+using namespace butl;
 
 namespace build2
 {
@@ -46,6 +47,259 @@ namespace build2
   //
   const int stream_verb_index = ostream::xalloc ();
 
+  // diag_buffer
+  //
+  process::pipe diag_buffer::
+  open (const char* args0, bool force, bool blocking)
+  {
+    assert (state_ == state::closed && args0 != nullptr);
+
+    assert (blocking); // @@ TODO (also in read() and close () below).
+
+    serial = ctx_.sched.serial ();
+
+    process::pipe r;
+    if (!serial || force)
+    {
+      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));
+
+        is.open (move (p.in), fdstream_mode::text, ifdstream::badbit);
+      }
+      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;
+  }
+
+  bool diag_buffer::
+  read ()
+  {
+    assert (state_ == state::opened);
+
+    bool r;
+    if (is.is_open ())
+    {
+      try
+      {
+        if (is.blocking ())
+        {
+          if (serial)
+          {
+            // This is the case where we are called after custom processing.
+            //
+            assert (buf.empty ());
+
+            // Note that the eof check is important: if the stream is at eof,
+            // this and all subsequent writes to the diagnostics stream will
+            // fail (and you won't see a thing).
+            //
+            if (is.peek () != ifdstream::traits_type::eof ())
+            {
+              // Holding the diag lock while waiting for diagnostics from the
+              // child process would be a bad idea in the parallel build. But
+              // it should be harmless in serial.
+              //
+              diag_stream_lock l;
+              *diag_stream << is.rdbuf ();
+            }
+          }
+          else
+          {
+            fdstreambuf& sb (*static_cast<fdstreambuf*> (is.rdbuf ()));
+
+            while (is.peek () != istream::traits_type::eof ())
+            {
+              const char* p (sb.gptr ());
+              size_t n (sb.egptr () - p);
+
+              // Allocate at least fdstreambuf::buffer_size to reduce
+              // reallocations and memory fragmentation.
+              //
+              size_t i (buf.size ());
+              if (i == 0 && n < fdstreambuf::buffer_size)
+                buf.reserve (fdstreambuf::buffer_size);
+
+              buf.resize (i + n);
+              memcpy (buf.data () + i, p, n);
+
+              sb.gbump (static_cast<int> (n));
+            }
+          }
+
+          r = false;
+        }
+        else
+        {
+          // @@ TODO (maybe we can unify the two?)
+          r = true;
+        }
+
+        if (!r)
+          is.close ();
+      }
+      catch (const io_error& e)
+      {
+        // For now we assume (here and pretty much everywhere else) that the
+        // output can't fail.
+        //
+        fail << "unable to read from " << args0 << " stderr:" << e;
+      }
+    }
+    else
+      r = false;
+
+    if (!r)
+      state_ = state::eof;
+
+    return r;
+  }
+
+  void diag_buffer::
+  write (const string& s, bool nl)
+  {
+    // Similar logic to read() above.
+    //
+    if (serial)
+    {
+      assert (buf.empty ());
+
+      diag_stream_lock l;
+      *diag_stream << s;
+      if (nl)
+        *diag_stream << '\n';
+    }
+    else
+    {
+      size_t n (s.size () + (nl ? 1 : 0));
+
+      size_t i (buf.size ());
+      if (i == 0 && n < fdstreambuf::buffer_size)
+        buf.reserve (fdstreambuf::buffer_size);
+
+      buf.resize (i + n);
+      memcpy (buf.data () + i, s.c_str (), s.size ());
+
+      if (nl)
+        buf.back () = '\n';
+    }
+  }
+
+  void diag_buffer::
+  close (const char* const* args, size_t args_size,
+         const process_exit& pe,
+         uint16_t v,
+         const location& loc)
+  {
+    assert (state_ != state::closed);
+
+    // We may still be in the open state in case of custom processing.
+    //
+    if (state_ == state::opened)
+    {
+      if (is.is_open ())
+      {
+        try
+        {
+          // @@ TODO: is it ok to call peek() in non-blocking?
+          //
+          assert (is.peek () == ifdstream::traits_type::eof ());
+          is.close ();
+        }
+        catch (const io_error& e)
+        {
+          fail << "unable to read from " << args0 << " stderr:" << e;
+        }
+      }
+
+      state_ = state::eof;
+    }
+
+    // We need to make sure the command line we print on the unsuccessful exit
+    // is inseparable from any buffered diagnostics. So we prepare the record
+    // first and then write both while holding the diagnostics stream lock.
+    //
+    diag_record dr;
+    if (!pe)
+    {
+      // Note: see similar code in run_finish_impl().
+
+      // It's unclear whether we should print this only if printing the
+      // command line (we could also do things differently for normal/abnormal
+      // exit). Let's print this always and see how it wears.
+      //
+      dr << error (loc) << "process " << args[0] << " " << pe;
+
+      if (verb >= 1 && verb <= v)
+      {
+        dr << info << "command line: ";
+        print_process (dr, args, args_size);
+      }
+    }
+
+    if (!buf.empty () || !dr.empty ())
+    {
+      diag_stream_lock l;
+
+      if (!buf.empty ())
+        diag_stream->write (buf.data (), static_cast<streamsize> (buf.size ()));
+
+      if (!dr.empty ())
+        dr.flush ([] (const butl::diag_record& r)
+                  {
+                    // Similar to default_writer().
+                    //
+                    *diag_stream << r.os.str () << '\n';
+                  });
+
+      diag_stream->flush ();
+    }
+
+    buf.clear ();
+    args0 = nullptr;
+    state_ = state::closed;
+  }
+
+  void diag_buffer::
+  finish (const char* const* args, size_t args_size,
+          process& pr,
+          uint16_t v,
+          const location& loc)
+  {
+    // Note: see similar code in run_finish_impl().
+    //
+    try
+    {
+      pr.wait ();
+    }
+    catch (const process_error& e)
+    {
+      fail (loc) << "unable to execute " << args[0] << ": " << e << endf;
+    }
+
+    const process_exit& pe (*pr.exit);
+
+    close (args, args_size, pe, v, loc);
+
+    if (!pe)
+      throw failed ();
+  }
+
+  // print_process()
+  //
   void
   print_process (const char* const* args, size_t n)
   {
diff --git a/libbuild2/diagnostics.hxx b/libbuild2/diagnostics.hxx
index 179b5c9..a56fd64 100644
--- a/libbuild2/diagnostics.hxx
+++ b/libbuild2/diagnostics.hxx
@@ -21,10 +21,177 @@ namespace build2
   //
   class failed: public std::exception {};
 
-  // Print process commmand line. If the number of elements is specified
-  // (or the second version is used), then it will print the piped multi-
-  // process command line, if present. In this case, the expected format
-  // is as follows:
+  // Diagnostics buffer.
+  //
+  // The purpose of this class is to handle diagnostics from child processes,
+  // where handle can mean:
+  //
+  // - Buffer it (to avoid interleaving in parallel builds).
+  //
+  // - Stream it (if the input can be split into diagnostic records).
+  //
+  // - Do nothing (in serial builds).
+  //
+  // This class is also responsible for converting the diagnostics into the
+  // structured form (which means it may need to buffer even in serial
+  // builds).
+  //
+  class LIBBUILD2_SYMEXPORT diag_buffer
+  {
+  public:
+    explicit
+    diag_buffer (context& c): ctx_ (c) {}
+
+  public:
+    // If buffering is necessary or force is true, open a pipe and return the
+    // child process end of it. Otherwise, return stderr. If blocking is
+    // false, 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 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.
+    //
+    // All the below functions handle io errors, issue suitable diagnostics,
+    // and throw failed. If an exception is thrown from any of them, then the
+    // instance should not be used any further.
+    //
+    process::pipe
+    open (const char* args0, bool force = false, bool blocking = true);
+
+    // Read the diagnostics from the parent end of the pipe if one was opened
+    // and buffer/stream it as necessary. Return true if there could be more
+    // diagnostics to read (only possible in the non-blocking mode).
+    //
+    // Instead of calling this function you can perform custom reading and, if
+    // necessary, buffering of the diagnostics by accessing the input stream
+    // (is) and underlying buffer (buf) directly. This can be used to filter,
+    // split the diagnostics into records according to a certain format, etc.
+    // Note that such custom processing implementation should maintain the
+    // overall semantics of diagnostics buffering in that it may only omit
+    // buffering in the serial case or if the diagnostics can be streamed in
+    // atomic records. See also write() below.
+    //
+    // The input stream is opened in the text mode and has the badbit but not
+    // failbit exception mask. The custom processing should also be compatible
+    // with the stream mode (blocking or non). If buffering is performed, then
+    // depending on the expected diagnostics the custom processing may want to
+    // reserve an appropriate initial buffer size to avoid unnecessary
+    // reallocation. As a convenience, in the blocking mode, if the stream
+    // still contains some diagnostics, then it can be handled by calling
+    // read(). This is helpful when needing to process only the inital part of
+    // the diagnostics.
+    //
+    bool
+    read ();
+
+    // Close the parent end of the pipe if one was opened and write out any
+    // buffered diagnostics.
+    //
+    // If the verbosity level is between 1 and the specified value and the
+    // child process exited with non-0 code, then print the command line after
+    // the diagnostics. Normally the specified verbosity will be 1 and the
+    // command line args represent the verbosity level 2 (logical) command
+    // line. The semantics os the args/args_size arguments is the same as
+    // in print_process() below.
+    //
+    // If the diag_buffer instance is destroyed before calling close(), then
+    // any buffered diagnostics is discarded.
+    //
+    // @@ TODO: need overload with process_env (see print_process).
+    //
+    void
+    close (const cstrings& args,
+           const process_exit& pe,
+           uint16_t verbosity = 1,
+           const location& loc = {})
+    {
+      close (args.data (), args.size (), pe, verbosity, loc);
+    }
+
+    void
+    close (const char* const* args,
+           const process_exit& pe,
+           uint16_t verbosity = 1,
+           const location& loc = {})
+    {
+      close (args, 0, pe, verbosity, loc);
+    }
+
+    void
+    close (const char* const* args, size_t args_size,
+           const process_exit& pe,
+           uint16_t verbosity = 1,
+           const location& loc = {});
+
+
+    // This version calls close() plus it first waits for the process to
+    // finish and later throws failed if it didn't exit with 0 code (so
+    // similar to run_finish ()).
+    //
+    void
+    finish (const cstrings& args,
+            process& pr,
+            uint16_t verbosity = 1,
+            const location& loc = {})
+    {
+      finish (args.data (), args.size (), pr, verbosity, loc);
+    }
+
+    void
+    finish (const char* const* args,
+            process& pr,
+            uint16_t verbosity = 1,
+            const location& loc = {})
+    {
+      finish (args, 0, pr, verbosity, loc);
+    }
+
+    void
+    finish (const char* const* args, size_t args_size,
+            process&,
+            uint16_t verbosity = 1,
+            const location& = {});
+
+    // Direct access to the underlying stream and buffer for custom processing
+    // (see read() above for details).
+    //
+  public:
+    ifdstream    is;
+    vector<char> buf;
+    const char*  args0;
+    bool         serial;
+
+    // Buffer or stream a fragment of diagnostics as necessary. If newline
+    // is true, also add a trailing newline.
+    //
+    // This function is normally called from a custom diagnostics processing
+    // implementation (see read() above for details).
+    //
+    void
+    write (const string&, bool newline);
+
+  private:
+    // Note that we don't seem to need a custom destructor to achieve the
+    // desired semantics: we can assume the process has exited before we are
+    // destroyed (because we supply stderr to its constructor) which means
+    // closing fdstream without reading any futher should be ok.
+    //
+    enum class state {closed, opened, eof};
+
+    context& ctx_;
+    state    state_ = state::closed;
+  };
+
+  // Print process commmand line. If the number of elements is specified (or
+  // the const cstrings& version is used), then it will print the piped multi-
+  // process command line, if present. In this case, the expected format is as
+  // follows:
   //
   // name1 arg arg ... nullptr
   // name2 arg arg ... nullptr
diff --git a/libbuild2/in/rule.cxx b/libbuild2/in/rule.cxx
index d07adfc..761a882 100644
--- a/libbuild2/in/rule.cxx
+++ b/libbuild2/in/rule.cxx
@@ -147,6 +147,9 @@ namespace build2
 
       // First should come the rule name/version.
       //
+      // @@ TODO: split into rule_name (string) and rule_version (integer)
+      //    on next increment.
+      //
       if (dd.expect (rule_id_ + " 1") != nullptr)
         l4 ([&]{trace << "rule mismatch forcing update of " << t;});
 
diff --git a/libbuild2/script/run.cxx b/libbuild2/script/run.cxx
index ca04443..1971bd1 100644
--- a/libbuild2/script/run.cxx
+++ b/libbuild2/script/run.cxx
@@ -1329,7 +1329,7 @@ namespace build2
           }
           catch (const io_error& e)
           {
-            fail (ll) << "unable to read from " << cmd_path (c) << " output: "
+            fail (ll) << "unable to read from " << cmd_path (c) << " stdout: "
                       << e;
           }
         }
diff --git a/libbuild2/types.hxx b/libbuild2/types.hxx
index 6b4022d..d93da6d 100644
--- a/libbuild2/types.hxx
+++ b/libbuild2/types.hxx
@@ -373,8 +373,10 @@ namespace build2
   using butl::sha256;
 
   // <libbutl/process.hxx>
+  //
   using butl::process;
   using butl::process_env;
+  using butl::process_exit;
   using butl::process_path;
   using butl::process_error;
 
diff --git a/libbuild2/utility.cxx b/libbuild2/utility.cxx
index 3eeeeaa..e73cd61 100644
--- a/libbuild2/utility.cxx
+++ b/libbuild2/utility.cxx
@@ -281,6 +281,8 @@ namespace build2
     if (pr.wait ())
       return true;
 
+    // Note: see similar code in diag_buffer::close/finish().
+    //
     const process_exit& e (*pr.exit);
 
     if (!e.normal ())