[PR,mi/18833] Provide unlimited redirection stack for MI

Message ID 047d7b86c3b0e4b459051d9c42da@google.com
State New, archived
Headers

Commit Message

Doug Evans Aug. 18, 2015, 9:04 p.m. UTC
  Pedro Alves writes:
  > On 08/17/2015 11:32 PM, Doug Evans wrote:
  >
  > > I wrote this patch up before noticing that someone had already done so.
  > > https://sourceware.org/ml/gdb-patches/2015-08/msg00438.html
  > >
  > > Adrian's email bounces, so not sure what to do with the attribution.
  >
  > Adrian's changes were done under Freescale's copyright assignment
  > at the time.  We should simply use the email address he had then,
  > just like we don't go over older ChangeLog entries and change
  > email addresses when we change jobs.  (tbc, I don't know if he
  > changed jobs).
  >
  > >
  > > The problem here is that if we invoke something from python
  > > with to_string=True with MI as the current interpreter,
  > > then that'll set up one level of redirection.
  > > Then if processing that python requires a second redirection,
  > > e.g. mi_command_param_changed,
  > > then MI's stack size of two will discard the original entry,
  > > and then when the python completes MI's uiout will be left pointing
  > > to freed memory from the redirection.
  > >
  > > The fix is to just copy what CLI does: use a VEC to record the stack.
  > >
  > > Adrian never completed his testcase.
  > > If someone wants to complete it and add it great.
  > > My testcase is sufficient to trigger the crash
  > > and is how I ran into this bug.
  > > I did copy over Pedro's suggestion for skip_python_tests_prompt.
  >
  > Thanks.  It looks good to me.  A couple minor comments below.
  >
  > > +/* Constructor for an `mi_out_data' object.  */
  > > +
  > > +static void
  > > +mi_out_data_ctor (int mi_version, mi_out_data *self, struct ui_file
  > > *stream)
  >
  > I find it a (tiny) bit surprising the 'self' isn't the first parameter.
  >
  > > diff --git a/gdb/testsuite/gdb.python/py-mi-objfile.exp
  > > b/gdb/testsuite/gdb.python/py-mi-objfile.exp
  > > new file mode 100644
  > > index 0000000..2f69453
  > > --- /dev/null
  > > +++ b/gdb/testsuite/gdb.python/py-mi-objfile.exp
  > > @@ -0,0 +1,58 @@
  >
  > > +# This file is part of the GDB testsuite.  It tests Python-based
  > > +# pretty-printing for MI.
  > > +
  >
  > This comment looks stale.

Thanks.
Committed with the suggested changes.

commit 4d6cceb4e40a057dbe4d9ad94b0641d5f4725c09
Author: Doug Evans <dje@google.com>
Date:   Tue Aug 18 14:02:03 2015 -0700

     PR mi/18833 gdb.execute ("set param value", to_string=True) will crash  
gdb if using MI

     gdb/ChangeLog:

     	* cli/cli-logging.c (pop_output_files): Don't restore redirection
     	if MI-like.
     	* mi/mi-out.c: #include "vec.h".
     	(ui_filep): New type.
     	(DEV_VEC_P (ui_filep)): New type.
     	(struct ui_out_data) <buffer, original_buffer>: Delete.
     	(struct ui_out_data) <streams>: New member.
     	(mi_ui_out_impl): Add data_destroy field.
     	(mi_field_string, mi_field_fmt): Update.
     	(mi_flush, mi_redirect, field_separator): Update.
     	(mi_open, mi_close): Update.
     	(mi_out_buffered, mi_out_rewind, mi_out_put): Update.
     	(mi_out_data_ctor, mi_out_data_dtor): New functions.
     	(mi_out_new): Call mi_out_data_ctor.

     testsuite/gdb/ChangeLog:

     	* lib/gdb.exp (skip_python_tests_prompt): Renamed from
     	skip_python_tests.  New arg prompt_regexp.
     	(skip_python_tests): New function.
     	* lib/mi-support.exp (mi_skip_python_tests): New function.
     	* gdb.python/py-mi-objfile-gdb.py: New file.
     	* gdb.python/py-mi-objfile.c: New file.
     	* gdb.python/py-mi-objfile.exp: New file.

+}
  

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 4af881e..ef8e493 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,22 @@ 
+2015-08-18  Doug Evans  <dje@google.com>
+	    Adrian Sendroiu <adrian.sendroiu@freescale.com>
+
+	PR mi/18833
+	* cli/cli-logging.c (pop_output_files): Don't restore redirection
+	if MI-like.
+	* mi/mi-out.c: #include "vec.h".
+	(ui_filep): New type.
+	(DEV_VEC_P (ui_filep)): New type.
+	(struct ui_out_data) <buffer, original_buffer>: Delete.
+	(struct ui_out_data) <streams>: New member.
+	(mi_ui_out_impl): Add data_destroy field.
+	(mi_field_string, mi_field_fmt): Update.
+	(mi_flush, mi_redirect, field_separator): Update.
+	(mi_open, mi_close): Update.
+	(mi_out_buffered, mi_out_rewind, mi_out_put): Update.
+	(mi_out_data_ctor, mi_out_data_dtor): New functions.
+	(mi_out_new): Call mi_out_data_ctor.
+
  2015-08-18  Sandra Loosemore  <sandra@codesourcery.com>

  	* remote.c (strprefix): New.
diff --git a/gdb/cli/cli-logging.c b/gdb/cli/cli-logging.c
index 7d340c1..c282260 100644
--- a/gdb/cli/cli-logging.c
+++ b/gdb/cli/cli-logging.c
@@ -177,7 +177,9 @@  pop_output_files (void)
    saved_output.targ = NULL;
    saved_output.targerr = NULL;

-  ui_out_redirect (current_uiout, NULL);
+  /* Stay consistent with handle_redirections.  */
+  if (!ui_out_is_mi_like_p (current_uiout))
+    ui_out_redirect (current_uiout, NULL);
  }

  /* This is a helper for the `set logging' command.  */
diff --git a/gdb/mi/mi-out.c b/gdb/mi/mi-out.c
index fa8282f..ef0a2df 100644
--- a/gdb/mi/mi-out.c
+++ b/gdb/mi/mi-out.c
@@ -22,19 +22,23 @@ 
  #include "defs.h"
  #include "ui-out.h"
  #include "mi-out.h"
+#include "vec.h"
+
+typedef struct ui_file *ui_filep;
+DEF_VEC_P (ui_filep);

  struct ui_out_data
    {
      int suppress_field_separator;
      int suppress_output;
      int mi_version;
-    struct ui_file *buffer;
-    struct ui_file *original_buffer;
+    VEC (ui_filep) *streams;
    };
  typedef struct ui_out_data mi_out_data;

  /* These are the MI output functions */

+static void mi_out_data_dtor (struct ui_out *ui_out);
  static void mi_table_begin (struct ui_out *uiout, int nbrofcols,
  			    int nr_rows, const char *tblid);
  static void mi_table_body (struct ui_out *uiout);
@@ -85,7 +89,7 @@  static const struct ui_out_impl mi_ui_out_impl =
    mi_wrap_hint,
    mi_flush,
    mi_redirect,
-  0,
+  mi_out_data_dtor,
    1, /* Needs MI hacks.  */
  };

@@ -215,17 +219,19 @@  mi_field_string (struct ui_out *uiout, int fldno, int  
width,
  		 enum ui_align align, const char *fldname, const char *string)
  {
    mi_out_data *data = ui_out_data (uiout);
+  struct ui_file *stream;

    if (data->suppress_output)
      return;

+  stream = VEC_last (ui_filep, data->streams);
    field_separator (uiout);
    if (fldname)
-    fprintf_unfiltered (data->buffer, "%s=", fldname);
-  fprintf_unfiltered (data->buffer, "\"");
+    fprintf_unfiltered (stream, "%s=", fldname);
+  fprintf_unfiltered (stream, "\"");
    if (string)
-    fputstr_unfiltered (string, '"', data->buffer);
-  fprintf_unfiltered (data->buffer, "\"");
+    fputstr_unfiltered (string, '"', stream);
+  fprintf_unfiltered (stream, "\"");
  }

  /* This is the only field function that does not align.  */
@@ -236,17 +242,19 @@  mi_field_fmt (struct ui_out *uiout, int fldno, int  
width,
  	      const char *format, va_list args)
  {
    mi_out_data *data = ui_out_data (uiout);
+  struct ui_file *stream;

    if (data->suppress_output)
      return;

+  stream = VEC_last (ui_filep, data->streams);
    field_separator (uiout);
    if (fldname)
-    fprintf_unfiltered (data->buffer, "%s=\"", fldname);
+    fprintf_unfiltered (stream, "%s=\"", fldname);
    else
-    fputs_unfiltered ("\"", data->buffer);
-  vfprintf_unfiltered (data->buffer, format, args);
-  fputs_unfiltered ("\"", data->buffer);
+    fputs_unfiltered ("\"", stream);
+  vfprintf_unfiltered (stream, format, args);
+  fputs_unfiltered ("\"", stream);
  }

  void
@@ -275,8 +283,9 @@  void
  mi_flush (struct ui_out *uiout)
  {
    mi_out_data *data = ui_out_data (uiout);
+  struct ui_file *stream = VEC_last (ui_filep, data->streams);

-  gdb_flush (data->buffer);
+  gdb_flush (stream);
  }

  int
@@ -285,15 +294,9 @@  mi_redirect (struct ui_out *uiout, struct ui_file  
*outstream)
    mi_out_data *data = ui_out_data (uiout);

    if (outstream != NULL)
-    {
-      data->original_buffer = data->buffer;
-      data->buffer = outstream;
-    }
-  else if (data->original_buffer != NULL)
-    {
-      data->buffer = data->original_buffer;
-      data->original_buffer = NULL;
-    }
+    VEC_safe_push (ui_filep, data->streams, outstream);
+  else
+    VEC_pop (ui_filep, data->streams);

    return 0;
  }
@@ -306,29 +309,31 @@  static void
  field_separator (struct ui_out *uiout)
  {
    mi_out_data *data = ui_out_data (uiout);
+  struct ui_file *stream = VEC_last (ui_filep, data->streams);

    if (data->suppress_field_separator)
      data->suppress_field_separator = 0;
    else
-    fputc_unfiltered (',', data->buffer);
+    fputc_unfiltered (',', stream);
  }

  static void
  mi_open (struct ui_out *uiout, const char *name, enum ui_out_type type)
  {
    mi_out_data *data = ui_out_data (uiout);
+  struct ui_file *stream = VEC_last (ui_filep, data->streams);

    field_separator (uiout);
    data->suppress_field_separator = 1;
    if (name)
-    fprintf_unfiltered (data->buffer, "%s=", name);
+    fprintf_unfiltered (stream, "%s=", name);
    switch (type)
      {
      case ui_out_type_tuple:
-      fputc_unfiltered ('{', data->buffer);
+      fputc_unfiltered ('{', stream);
        break;
      case ui_out_type_list:
-      fputc_unfiltered ('[', data->buffer);
+      fputc_unfiltered ('[', stream);
        break;
      default:
        internal_error (__FILE__, __LINE__, _("bad switch"));
@@ -339,14 +344,15 @@  static void
  mi_close (struct ui_out *uiout, enum ui_out_type type)
  {
    mi_out_data *data = ui_out_data (uiout);
+  struct ui_file *stream = VEC_last (ui_filep, data->streams);

    switch (type)
      {
      case ui_out_type_tuple:
-      fputc_unfiltered ('}', data->buffer);
+      fputc_unfiltered ('}', stream);
        break;
      case ui_out_type_list:
-      fputc_unfiltered (']', data->buffer);
+      fputc_unfiltered (']', stream);
        break;
      default:
        internal_error (__FILE__, __LINE__, _("bad switch"));
@@ -360,8 +366,9 @@  void
  mi_out_buffered (struct ui_out *uiout, char *string)
  {
    mi_out_data *data = ui_out_data (uiout);
+  struct ui_file *stream = VEC_last (ui_filep, data->streams);

-  fprintf_unfiltered (data->buffer, "%s", string);
+  fprintf_unfiltered (stream, "%s", string);
  }

  /* Clear the buffer.  */
@@ -370,8 +377,9 @@  void
  mi_out_rewind (struct ui_out *uiout)
  {
    mi_out_data *data = ui_out_data (uiout);
+  struct ui_file *stream = VEC_last (ui_filep, data->streams);

-  ui_file_rewind (data->buffer);
+  ui_file_rewind (stream);
  }

  /* Dump the buffer onto the specified stream.  */
@@ -380,9 +388,10 @@  void
  mi_out_put (struct ui_out *uiout, struct ui_file *stream)
  {
    mi_out_data *data = ui_out_data (uiout);
+  struct ui_file *outstream = VEC_last (ui_filep, data->streams);

-  ui_file_put (data->buffer, ui_file_write_for_put, stream);
-  ui_file_rewind (data->buffer);
+  ui_file_put (outstream, ui_file_write_for_put, stream);
+  ui_file_rewind (outstream);
  }

  /* Return the current MI version.  */
@@ -395,19 +404,41 @@  mi_version (struct ui_out *uiout)
    return data->mi_version;
  }

+/* Constructor for an `mi_out_data' object.  */
+
+static void
+mi_out_data_ctor (mi_out_data *self, int mi_version, struct ui_file  
*stream)
+{
+  gdb_assert (stream != NULL);
+
+  self->streams = NULL;
+  VEC_safe_push (ui_filep, self->streams, stream);
+
+  self->suppress_field_separator = 0;
+  self->suppress_output = 0;
+  self->mi_version = mi_version;
+}
+
+/* The destructor.  */
+
+static void
+mi_out_data_dtor (struct ui_out *ui_out)
+{
+  mi_out_data *data = ui_out_data (ui_out);
+
+  VEC_free (ui_filep, data->streams);
+  xfree (data);
+}
+
  /* Initialize private members at startup.  */

  struct ui_out *
  mi_out_new (int mi_version)
  {
    int flags = 0;
-
    mi_out_data *data = XNEW (mi_out_data);
-  data->suppress_field_separator = 0;
-  data->suppress_output = 0;
-  data->mi_version = mi_version;
-  /* FIXME: This code should be using a ``string_file'' and not the
-     TUI buffer hack. */
-  data->buffer = mem_fileopen ();
+  struct ui_file *stream = mem_fileopen ();
+
+  mi_out_data_ctor (data, mi_version, stream);
    return ui_out_new (&mi_ui_out_impl, data, flags);
  }
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 9a7cf88..eac803a 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,14 @@ 
+2015-08-18  Doug Evans  <dje@google.com>
+	    Adrian Sendroiu <adrian.sendroiu@freescale.com>
+
+	* lib/gdb.exp (skip_python_tests_prompt): Renamed from
+	skip_python_tests.  New arg prompt_regexp.
+	(skip_python_tests): New function.
+	* lib/mi-support.exp (mi_skip_python_tests): New function.
+	* gdb.python/py-mi-objfile-gdb.py: New file.
+	* gdb.python/py-mi-objfile.c: New file.
+	* gdb.python/py-mi-objfile.exp: New file.
+
  2015-08-17  Keith Seitz  <keiths@redhat.com>

  	* gdb.linespec/explicit.exp: Move strace test from here ...
diff --git a/gdb/testsuite/gdb.python/py-mi-objfile-gdb.py  
b/gdb/testsuite/gdb.python/py-mi-objfile-gdb.py
new file mode 100644
index 0000000..6e41773
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-mi-objfile-gdb.py
@@ -0,0 +1,27 @@ 
+# Copyright (C) 2015 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file is part of the GDB testsuite.
+
+import gdb
+
+# PR 18833
+# We want to have two levels of redirection while MI is current_uiout.
+# This will create one for to_string=True and then another for the
+# parameter change notification.
+gdb.execute("set width 101", to_string=True)
+# And finally a command that will use the original MI stream, which in a
+# buggy gdb will use just-freed data.
+gdb.execute("list")
diff --git a/gdb/testsuite/gdb.python/py-mi-objfile.c  
b/gdb/testsuite/gdb.python/py-mi-objfile.c
new file mode 100644
index 0000000..b92fefb
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-mi-objfile.c
@@ -0,0 +1,22 @@ 
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2015 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see  <http://www.gnu.org/licenses/>.   
*/
+
+int
+main (void)
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.python/py-mi-objfile.exp  
b/gdb/testsuite/gdb.python/py-mi-objfile.exp
new file mode 100644
index 0000000..5424cc1
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-mi-objfile.exp
@@ -0,0 +1,57 @@ 
+# Copyright (C) 2008-2015 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file is part of the GDB testsuite.  It tests exercises PR 18833.
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi2"
+
+gdb_exit
+if [mi_gdb_start] {
+    continue
+}
+
+standard_testfile
+set pyfile ${testfile}-gdb.py
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}"  
executable {debug}] != "" } {
+    untested ${testfile}.exp
+    return -1
+}
+
+if { [mi_skip_python_tests] } { continue }
+
+# Make the -gdb.py script available to gdb, it is automagically loaded by  
gdb.
+# Care is taken to put it in the same directory as the binary so that
+# gdb will find it.
+set remote_python_file [gdb_remote_download host  
${srcdir}/${subdir}/${pyfile}]
+
+mi_delete_breakpoints
+mi_gdb_reinitialize_dir $srcdir/$subdir
+mi_gdb_test "set auto-load safe-path ${remote_python_file}" \
+    {.*\^done} \
+    "set safe-path"
+
+if [is_remote host] {
+    set filename ${testfile}
+    remote_download host ${binfile} ${filename}
+} else {
+    set filename ${binfile}
+}
+
+# PR 18833.  This will cause an unpatched gdb to crash.
+mi_gdb_test "-file-exec-and-symbols ${filename}" \
+    ".*\\^done,line=.*${srcfile}\"" \
+    "file-exec-and-symbols operation"
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index c5b7a06..56cde7a 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -1734,35 +1734,35 @@  proc skip_d_tests {} {
  }

  # Return a 1 for configurations that do not support Python scripting.
+# PROMPT_REGEXP is the expected prompt.

-proc skip_python_tests {} {
-    global gdb_prompt
+proc skip_python_tests_prompt { prompt_regexp } {
      global gdb_py_is_py3k
      global gdb_py_is_py24

      gdb_test_multiple "python print ('test')" "verify python support" {
-	-re "not supported.*$gdb_prompt $"	{
+	-re "not supported.*$prompt_regexp" {
  	    unsupported "Python support is disabled."
  	    return 1
  	}
-	-re "$gdb_prompt $"	{}
+	-re "$prompt_regexp" {}
      }

      set gdb_py_is_py24 0
      gdb_test_multiple "python print (sys.version_info\[0\])" "check if  
python 3" {
-	-re "3.*$gdb_prompt $"	{
+	-re "3.*$prompt_regexp" {
              set gdb_py_is_py3k 1
          }
-	-re ".*$gdb_prompt $"	{
+	-re ".*$prompt_regexp" {
              set gdb_py_is_py3k 0
          }
      }
      if { $gdb_py_is_py3k == 0 } {
          gdb_test_multiple "python print (sys.version_info\[1\])" "check if  
python 2.4" {
-	    -re "\[45\].*$gdb_prompt $" {
+	    -re "\[45\].*$prompt_regexp" {
                  set gdb_py_is_py24 1
              }
-	    -re ".*$gdb_prompt $" {
+	    -re ".*$prompt_regexp" {
                  set gdb_py_is_py24 0
              }
          }
@@ -1771,6 +1771,15 @@  proc skip_python_tests {} {
      return 0
  }

+# Return a 1 for configurations that do not support Python scripting.
+# Note: This also sets various globals that specify which version of Python
+# is in use.  See skip_python_tests_prompt.
+
+proc skip_python_tests {} {
+    global gdb_prompt
+    return [skip_python_tests_prompt "$gdb_prompt $"]
+}
+
  # Return a 1 if we should skip shared library tests.

  proc skip_shlib_tests {} {
diff --git a/gdb/testsuite/lib/mi-support.exp  
b/gdb/testsuite/lib/mi-support.exp
index 0d17ecb..dd6c41a 100644
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -2501,3 +2501,12 @@  proc mi_make_breakpoint_table {bp_list} {
      # Assemble the final regexp.
       
return "BreakpointTable={nr_rows=\"$nr\",nr_cols=\"$nc\",$header,$body}"
  }
+
+# Return a 1 for configurations that do not support Python scripting.
+# Note: This also sets various globals that specify which version of Python
+# is in use.  See skip_python_tests_prompt.
+
+proc mi_skip_python_tests {} {
+    global mi_gdb_prompt
+    return [skip_python_tests_prompt "$mi_gdb_prompt$"]