Re: `message' not outputting the newline "atomically"

[Paul Eggert <eggert@cs.ucla.edu>]

[-- Attachment #1: Type: text/plain, Size: 1342 bytes --]

Eli Zaretskii wrote:
> a version with a fixed-size automatic buffer and a loop using
> it to write the message in chunks, would be a much cleaner solution
> than allocating the buffer off the heap.

Yes, that would be an improvement. I see other issues with the current master. 
It doesn't fix output-line interleaving on AIX or Solaris in places where 
GNU/Linux works OK, as AIX and Solaris fprintf functions are less buffered 
(i.e., they commonly split Emacs output into 'write' calls that are smaller than 
a line), which means that common fprintf output can be interleaved on AIX and 
Solaris when it is not interleaved on GNU/Linux. Also, the current master 
mishandles the rare case where the string has INT_MAX or more bytes, and also 
mishandles line-interleaving if the string contains newlines.

Like Lars, I'm leery about temporarily trashing the representation of a Lisp 
string in order to do I/O. There will likely be other places where we need to do 
this sort of thing, but where the data are not in Lisp strings. It's not that 
hard to efficiently output even a very long Lisp string without modifying it, so 
let's do that.

Proposed patch attached. This defaults to line buffering on glibc as well as on 
AIX and Solaris, as I've verified that glibc buffers appropriately and have used 
it pretty extensively with Emacs.

[-- Attachment #2: 0001-Avoid-interleaving-stderr-lines.patch --]
[-- Type: text/x-patch, Size: 6114 bytes --]

From 9be73e5106262232a722cbe04fb8e387b1d3511d Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 26 Jun 2019 10:24:15 -0700
Subject: [PATCH] Avoid interleaving stderr lines

Do a better job of avoiding interleaved stderr lines.
The previous approach did not work on AIX or Solaris
because they issue multiple writes for common fprintf usages.
Also, it unnecessarily copied data for large strings,
unnecessarily used the heap, and mishandled the rare case
where the string contains INT_MAX or more bytes.
* src/sysdep.c (LINE_BUFFER_STDERR): New macro.
(init_standard_fds): Line-buffer stderr if LINE_BUFFER_STDERR says so.
(write_stderr): New function, that avoids interleaving stderr
lines when possible.
* src/pdumper.c (print_paths_to_root_1):
* src/xdisp.c (message_to_stderr, Ftrace_to_stderr): Use it.
---
 src/lisp.h    |  1 +
 src/pdumper.c |  3 +-
 src/sysdep.c  | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++
 src/xdisp.c   | 15 ++--------
 4 files changed, 81 insertions(+), 15 deletions(-)

diff --git a/src/lisp.h b/src/lisp.h
index 77fc22d118..836b0fcbb2 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4550,6 +4550,7 @@ maybe_disable_address_randomization (bool dumping, int argc, char **argv)
 extern ptrdiff_t emacs_write_sig (int, void const *, ptrdiff_t);
 extern ptrdiff_t emacs_write_quit (int, void const *, ptrdiff_t);
 extern void emacs_perror (char const *);
+extern void write_stderr (void const *, ptrdiff_t, bool);
 extern int renameat_noreplace (int, char const *, int, char const *);
 extern int str_collate (Lisp_Object, Lisp_Object, Lisp_Object, Lisp_Object);
 extern void syms_of_sysdep (void);
diff --git a/src/pdumper.c b/src/pdumper.c
index c00f8a0af5..68fe4d8ed4 100644
--- a/src/pdumper.c
+++ b/src/pdumper.c
@@ -1405,8 +1405,7 @@ print_paths_to_root_1 (struct dump_context *ctx,
       Lisp_Object repr = Fprin1_to_string (referrer, Qnil);
       for (int i = 0; i < level; ++i)
         fputc (' ', stderr);
-      fwrite (SDATA (repr), 1, SBYTES (repr), stderr);
-      fputc ('\n', stderr);
+      write_stderr (SDATA (repr), SBYTES (repr), true);
       print_paths_to_root_1 (ctx, referrer, level + 1);
     }
 }
diff --git a/src/sysdep.c b/src/sysdep.c
index 4f89e8aba1..54352022eb 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -231,6 +231,18 @@ force_open (int fd, int flags)
     }
 }
 
+/* Line-buffered stderr works with glibc.  It is required in AIX and
+   Solaris for short single-line printfs to be atomic.  It does not
+   work with MS-Windows.  When in doubt, assume it does not work.
+   Override the default by compiling with -D LINE_BUFFER_STDERR=[0|1].  */
+#ifndef LINE_BUFFER_STDERR
+# if defined __GLIBC__ || defined _AIX || defined __sun
+#  define LINE_BUFFER_STDERR 1
+# else
+#  define LINE_BUFFER_STDERR 0
+# endif
+#endif
+
 /* Make sure stdin, stdout, and stderr are open to something, so that
    their file descriptors are not hijacked by later system calls.  */
 void
@@ -243,6 +255,9 @@ init_standard_fds (void)
   force_open (STDIN_FILENO, O_WRONLY);
   force_open (STDOUT_FILENO, O_RDONLY);
   force_open (STDERR_FILENO, O_RDONLY);
+
+  if (LINE_BUFFER_STDERR)
+    setvbuf (stderr, NULL, _IOLBF, 0);
 }
 
 /* Return the current working directory.  The result should be freed
@@ -2714,6 +2729,68 @@ emacs_perror (char const *message)
     }
   errno = err;
 }
+
+/* Write BUFFER (of size BUFLEN bytes) to standard error.
+   Follow it with a newline if NLFLAG is true.
+   Avoid interleaving output lines with those of other processes,
+   if the output lines contain fewer than PIPE_BUF bytes.  */
+void
+write_stderr (void const *buffer, ptrdiff_t buflen, bool nlflag)
+{
+#if LINE_BUFFER_STDERR
+  fwrite_unlocked (buffer, 1, buflen, stderr);
+  if (nlflag)
+    fputc_unlocked ('\n', stderr);
+#else
+
+  /* This platform is not known to support line-buffering.
+     Line-buffer by hand.  */
+
+  fflush_unlocked (stderr);
+  if (buflen <= -nlflag)
+    return;
+
+  #ifndef PIPE_BUF
+  enum { PIPE_BUF = 512 };
+  #endif
+  verify (PIPE_BUF <= SSIZE_MAX);
+
+  /* Write chunks each containing as many lines as fit into PIPE_BUF bytes.
+     If a line is too long to fit in PIPE_BUF, write it out piecemeal;
+     it might be interleaved by the OS but this is the best we can do.
+     Append a newline to the last chunk if NLFLAG.  */
+
+  while (true)
+    {
+      char const *buf = buffer;
+      ptrdiff_t n = buflen;
+      bool appendnl = nlflag;
+      if (PIPE_BUF < n + appendnl)
+	{
+	  char const *nladdr = memrchr (buf, '\n', PIPE_BUF);
+	  n = nladdr ? nladdr - buf + 1 : PIPE_BUF;
+	  appendnl = false;
+	}
+
+      verify (PIPE_BUF <= MAX_ALLOCA);
+      char stackbuf[PIPE_BUF];
+      if (appendnl)
+	{
+	  memcpy (stackbuf, buf, n);
+	  stackbuf[n++] = '\n';
+	  buf = stackbuf;
+	}
+
+      n = write (STDERR_FILENO, buf, n);
+      if (n < 0)
+	break;
+      buflen -= n;
+      if (buflen <= -nlflag)
+	break;
+      buffer = buf + n;
+    }
+#endif
+}
 \f
 /* Set the access and modification time stamps of FD (a.k.a. FILE) to be
    ATIME and MTIME, respectively.
diff --git a/src/xdisp.c b/src/xdisp.c
index 9f63ef4b18..4329b48cb6 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -10705,18 +10705,7 @@ message_to_stderr (Lisp_Object m)
       else
 	s = m;
 
-      /* We want to write this out with a single fwrite call so that
-	 output doesn't interleave with other processes writing to
-	 stderr at the same time. */
-      {
-	int length = min (INT_MAX, SBYTES (s) + 1);
-	char *string = xmalloc (length);
-
-	memcpy (string, SSDATA (s), length - 1);
-	string[length - 1] = '\n';
-	fwrite (string, 1, length, stderr);
-	xfree (string);
-      }
+      write_stderr (SDATA (s), SBYTES (s), true);
     }
   else if (!cursor_in_echo_area)
     fputc ('\n', stderr);
@@ -19850,7 +19839,7 @@ DEFUN ("trace-to-stderr", Ftrace_to_stderr, Strace_to_stderr, 1, MANY, "",
   (ptrdiff_t nargs, Lisp_Object *args)
 {
   Lisp_Object s = Fformat (nargs, args);
-  fwrite (SDATA (s), 1, SBYTES (s), stderr);
+  write_stderr (SDATA (s), SBYTES (s), false);
   return Qnil;
 }
 
-- 
2.21.0