[binutils-gdb] Implement write_async_safe for mi_console_file (PR 22299)

[Simon Marchi <simark at sourceware dot org>]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=7c4e78cf63f6436ae43e8289badba78d81e2eb2c

commit 7c4e78cf63f6436ae43e8289badba78d81e2eb2c
Author: Simon Marchi <simon.marchi@polymtl.ca>
Date:   Sat Apr 7 13:48:05 2018 -0400

    Implement write_async_safe for mi_console_file (PR 22299)
    
    Enabling "set debug lin-lwp 1" with the MI interpreter doesn't work.
    When the sigchld_handler function wants to print a debug output
    ("sigchld\n"), it uses ui_file_write_async_safe.  This ends up in the
    default implementation of ui_file::write_async_safe, which aborts GDB.
    
    This patch implements the write_async_safe method for mi_console_file.
    The "normal" MI output is line buffered, which means the output
    accumulates in m_buffer until a \n is written, at which point it's
    flushed in m_raw.  The implementation of write_async_safe provided by
    this patch bypasses this buffer and writes directly to m_raw.  There are
    two reasons for this:
    
    (1) Appending to m_buffer (therefore to an std::string) is probably not
        async-safe, as it may allocate memory.
    (2) We may have a partial output already in m_buffer, so that would lead
        to some nested MI output, not so great.
    
    There is probably still a chance to have bad MI output, if
    sigchld_handler is invoked in the middle of mi_console_file's flush, and
    the line being flushed is only partially sent to m_raw.  The solution
    would probably be to block signals during flushing.  Since this is only
    used for debug output, I don't know if it's worth the effort to do that.
    
    To implement write_async_safe, I needed to use the fputstrn_unfiltered,
    which does the necessary escaping (e.g. replace \n with \\n).  I started
    by adding printchar's callback parameters to fputstrn_unfiltered, to be
    able to pass async-safe versions of them.  It's not easy to provide an
    async-safe version of do_fprintf, but it turns out that we can easily
    replace printchar's callbacks with a single do_fputc quite easily.  The
    async-safe version of do_fputc simply calls the underlying ui_file's
    write_async_safe method.
    
    gdb/ChangeLog:
    
    	PR mi/22299
    	* mi/mi-console.c (do_fputc_async_safe): New.
    	(mi_console_file::write_async_safe): New.
    	(mi_console_file::flush): Adjust calls to fputstrn_unfiltered.
    	* mi/mi-console.h (class mi_console_file) <write_async_safe>:
    	New.
    	* ui-file.c (ui_file::putstrn): Adjust call to
    	fputstrn_unfiltered.
    	* utils.c (printchar): Replace do_fputs and do_fprintf
    	parameters by do_fputc.
    	(fputstr_filtered): Adjust call to printchar.
    	(fputstr_unfiltered): Likewise.
    	(fputstrn_filtered): Likewise.
    	(fputstrn_unfiltered): Add do_fputc parameter, pass to
    	printchar.
    	* utils.h (do_fputc_ftype): New typedef.
    	(fputstrn_unfiltered): Add do_fputc parameter.

Diff:
---
 gdb/ChangeLog       | 20 ++++++++++++++++++++
 gdb/mi/mi-console.c | 33 +++++++++++++++++++++++++++++++--
 gdb/mi/mi-console.h |  2 ++
 gdb/ui-file.c       |  2 +-
 gdb/utils.c         | 50 +++++++++++++++++++++++++-------------------------
 gdb/utils.h         |  3 +++
 6 files changed, 82 insertions(+), 28 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index e3b65e0..4ffa4e7 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,25 @@
 2018-04-07  Simon Marchi  <simon.marchi@polymtl.ca>
 
+	PR mi/22299
+	* mi/mi-console.c (do_fputc_async_safe): New.
+	(mi_console_file::write_async_safe): New.
+	(mi_console_file::flush): Adjust calls to fputstrn_unfiltered.
+	* mi/mi-console.h (class mi_console_file) <write_async_safe>:
+	New.
+	* ui-file.c (ui_file::putstrn): Adjust call to
+	fputstrn_unfiltered.
+	* utils.c (printchar): Replace do_fputs and do_fprintf
+	parameters by do_fputc.
+	(fputstr_filtered): Adjust call to printchar.
+	(fputstr_unfiltered): Likewise.
+	(fputstrn_filtered): Likewise.
+	(fputstrn_unfiltered): Add do_fputc parameter, pass to
+	printchar.
+	* utils.h (do_fputc_ftype): New typedef.
+	(fputstrn_unfiltered): Add do_fputc parameter.
+
+2018-04-07  Simon Marchi  <simon.marchi@polymtl.ca>
+
 	* regformats/i386/i386-avx.dat: Remove.
 
 2018-04-07  Simon Marchi  <simon.marchi@ericsson.com>
diff --git a/gdb/mi/mi-console.c b/gdb/mi/mi-console.c
index 43d5388..248d070 100644
--- a/gdb/mi/mi-console.c
+++ b/gdb/mi/mi-console.c
@@ -48,6 +48,34 @@ mi_console_file::write (const char *buf, long length_buf)
     this->flush ();
 }
 
+/* Write C to STREAM's in an async-safe way.  */
+
+static int
+do_fputc_async_safe (int c, ui_file *stream)
+{
+  char ch = c;
+  stream->write_async_safe (&ch, 1);
+  return c;
+}
+
+void
+mi_console_file::write_async_safe (const char *buf, long length_buf)
+{
+  m_raw->write_async_safe (m_prefix, strlen (m_prefix));
+  if (m_quote)
+    {
+      m_raw->write_async_safe (&m_quote, 1);
+      fputstrn_unfiltered (buf, length_buf, m_quote, do_fputc_async_safe,
+			   m_raw);
+      m_raw->write_async_safe (&m_quote, 1);
+    }
+  else
+    fputstrn_unfiltered (buf, length_buf, 0, do_fputc_async_safe, m_raw);
+
+  char nl = '\n';
+  m_raw->write_async_safe (&nl, 1);
+}
+
 void
 mi_console_file::flush ()
 {
@@ -63,13 +91,14 @@ mi_console_file::flush ()
       if (m_quote)
 	{
 	  fputc_unfiltered (m_quote, m_raw);
-	  fputstrn_unfiltered (buf, length_buf, m_quote, m_raw);
+	  fputstrn_unfiltered (buf, length_buf, m_quote, fputc_unfiltered,
+			       m_raw);
 	  fputc_unfiltered (m_quote, m_raw);
 	  fputc_unfiltered ('\n', m_raw);
 	}
       else
 	{
-	  fputstrn_unfiltered (buf, length_buf, 0, m_raw);
+	  fputstrn_unfiltered (buf, length_buf, 0, fputc_unfiltered, m_raw);
 	  fputc_unfiltered ('\n', m_raw);
 	}
       gdb_flush (m_raw);
diff --git a/gdb/mi/mi-console.h b/gdb/mi/mi-console.h
index fbfa03b..d98a0c8 100644
--- a/gdb/mi/mi-console.h
+++ b/gdb/mi/mi-console.h
@@ -39,6 +39,8 @@ public:
 
   void write (const char *buf, long length_buf) override;
 
+  void write_async_safe (const char *buf, long length_buf) override;
+
 private:
   /* The wrapped raw output stream.  */
   ui_file *m_raw;
diff --git a/gdb/ui-file.c b/gdb/ui-file.c
index 84ad603..1751509 100644
--- a/gdb/ui-file.c
+++ b/gdb/ui-file.c
@@ -52,7 +52,7 @@ ui_file::putstr (const char *str, int quoter)
 void
 ui_file::putstrn (const char *str, int n, int quoter)
 {
-  fputstrn_unfiltered (str, n, quoter, this);
+  fputstrn_unfiltered (str, n, quoter, fputc_unfiltered, this);
 }
 
 int
diff --git a/gdb/utils.c b/gdb/utils.c
index df0a2ce..b957b0d 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1170,9 +1170,7 @@ parse_escape (struct gdbarch *gdbarch, const char **string_ptr)
    character. */
 
 static void
-printchar (int c, void (*do_fputs) (const char *, struct ui_file *),
-	   void (*do_fprintf) (struct ui_file *, const char *, ...)
-	   ATTRIBUTE_FPTR_PRINTF_2, struct ui_file *stream, int quoter)
+printchar (int c, do_fputc_ftype do_fputc, ui_file *stream, int quoter)
 {
   c &= 0xFF;			/* Avoid sign bit follies */
 
@@ -1180,39 +1178,45 @@ printchar (int c, void (*do_fputs) (const char *, struct ui_file *),
       (c >= 0x7F && c < 0xA0) ||	/* DEL, High controls */
       (sevenbit_strings && c >= 0x80))
     {				/* high order bit set */
+      do_fputc ('\\', stream);
+
       switch (c)
 	{
 	case '\n':
-	  do_fputs ("\\n", stream);
+	  do_fputc ('n', stream);
 	  break;
 	case '\b':
-	  do_fputs ("\\b", stream);
+	  do_fputc ('b', stream);
 	  break;
 	case '\t':
-	  do_fputs ("\\t", stream);
+	  do_fputc ('t', stream);
 	  break;
 	case '\f':
-	  do_fputs ("\\f", stream);
+	  do_fputc ('f', stream);
 	  break;
 	case '\r':
-	  do_fputs ("\\r", stream);
+	  do_fputc ('r', stream);
 	  break;
 	case '\033':
-	  do_fputs ("\\e", stream);
+	  do_fputc ('e', stream);
 	  break;
 	case '\007':
-	  do_fputs ("\\a", stream);
+	  do_fputc ('a', stream);
 	  break;
 	default:
-	  do_fprintf (stream, "\\%.3o", (unsigned int) c);
-	  break;
+	  {
+	    do_fputc ('0' + ((c >> 6) & 0x7), stream);
+	    do_fputc ('0' + ((c >> 3) & 0x7), stream);
+	    do_fputc ('0' + ((c >> 0) & 0x7), stream);
+	    break;
+	  }
 	}
     }
   else
     {
       if (quoter != 0 && (c == '\\' || c == quoter))
-	do_fputs ("\\", stream);
-      do_fprintf (stream, "%c", c);
+	do_fputc ('\\', stream);
+      do_fputc (c, stream);
     }
 }
 
@@ -1225,34 +1229,30 @@ void
 fputstr_filtered (const char *str, int quoter, struct ui_file *stream)
 {
   while (*str)
-    printchar (*str++, fputs_filtered, fprintf_filtered, stream, quoter);
+    printchar (*str++, fputc_filtered, stream, quoter);
 }
 
 void
 fputstr_unfiltered (const char *str, int quoter, struct ui_file *stream)
 {
   while (*str)
-    printchar (*str++, fputs_unfiltered, fprintf_unfiltered, stream, quoter);
+    printchar (*str++, fputc_unfiltered, stream, quoter);
 }
 
 void
 fputstrn_filtered (const char *str, int n, int quoter,
 		   struct ui_file *stream)
 {
-  int i;
-
-  for (i = 0; i < n; i++)
-    printchar (str[i], fputs_filtered, fprintf_filtered, stream, quoter);
+  for (int i = 0; i < n; i++)
+    printchar (str[i], fputc_filtered, stream, quoter);
 }
 
 void
 fputstrn_unfiltered (const char *str, int n, int quoter,
-		     struct ui_file *stream)
+		     do_fputc_ftype do_fputc, struct ui_file *stream)
 {
-  int i;
-
-  for (i = 0; i < n; i++)
-    printchar (str[i], fputs_unfiltered, fprintf_unfiltered, stream, quoter);
+  for (int i = 0; i < n; i++)
+    printchar (str[i], do_fputc, stream, quoter);
 }
 
 
diff --git a/gdb/utils.h b/gdb/utils.h
index 9a1fb34..4dec889 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -418,7 +418,10 @@ extern void fputstr_unfiltered (const char *str, int quotr,
 extern void fputstrn_filtered (const char *str, int n, int quotr,
 			       struct ui_file * stream);
 
+typedef int (*do_fputc_ftype) (int c, ui_file *stream);
+
 extern void fputstrn_unfiltered (const char *str, int n, int quotr,
+				 do_fputc_ftype do_fputc,
 				 struct ui_file * stream);
 
 /* Return nonzero if filtered printing is initialized.  */