diff mbox

[v2,2/2] mi-out: Implement mi redirection using a stack.

Message ID 1406819332-24242-1-git-send-email-adrian.sendroiu@freescale.com
State Superseded
Headers

Commit Message

Adrian Sendroiu July 31, 2014, 3:08 p.m. UTC 
  I implemented the test case. A couple of issues I encountered:

- skip_python_tests doesn't work with MI, because it searches for $gdb_prompt.
I implemented another one called mi_skip_python_tests which uses $mi_gdb_prompt.

- executing "python gdb.execute("break main", ... ) directly doesn't work,
because "python" is a cli command, and before running it the current_uiout will
be temporarily changed to cli_uiout, so the first redirection won't be done
on mi_uiout. To bypass this I implemented a small workaround: set a breakpoint,
then set the python command to be executed when the breakpoint hits.

- when running the test on a gdb without the redirection fix, the test will be
reported as UNRESOLVED, because gdb crashes.

--- 8< ---
Subject: [PATCH] mi-out: Implement mi redirection using a stack.

The current implementation doesn't support nested redirections because it only
has an "original_buffer" where it saves the current stream when a redirection is
done. To overcome this limitation, this patch reimplements it using a stack of
streams.

gdb/
2014-07-23  Adrian Sendroiu  <adrian.sendroiu@freescale.com>

	* mi/mi-out.c (ui_filep): New typedef.
	(DEF_VEC_P (ui_filep)): New type.
	(struct ui_out_data): <buffer> Remove field.
	<original_buffer> Remove field.
	<streams>: New field.
	(mi_field_string): Get the output stream from the top of the
	stack of streams.
	(mi_field_fmt): Likewise.
	(mi_flush): Likewise.
	(field_separator): Likewise.
	(mi_open): Likewise.
	(mi_close): Likewise.
	(mi_out_buffered): Likewise.
	(mi_out_rewind): Likewise.
	(mi_out_put): Likewise.
	(mi_out_new): Initialize the stack of streams. Push a newly
	created stream on the stack. Remove obsolete comment.
	(mi_redirect): Reimplement using push and pop operations.

gdb/testsuite/
2014-07-30  Adrian Sendroiu  <adrian.sendroiu@freescale.com>

	* gdb.mi/mi-logging.exp: Add test for nested mi redirection.
	* lib/mi-support.exp: (mi_skip_python_tests): New function.
---
 gdb/mi/mi-out.c                     |   77 ++++++++++++++++++++---------------
 gdb/testsuite/gdb.mi/mi-logging.exp |   14 +++++++
 gdb/testsuite/lib/mi-support.exp    |   37 +++++++++++++++++
 3 files changed, 95 insertions(+), 33 deletions(-)

Comments

Pedro Alves July 31, 2014, 4:23 p.m. UTC | #1 
Awesome!  Thanks for doing this.

This is close.  Some notes on the technicalities below.

On 07/31/2014 04:08 PM, Adrian Sendroiu wrote:
> +# This triggers a nested mi_ui_out redirection, by disabling a breakpoint
> +# inside a python command that has to_string = True.
> +if ![mi_skip_python_tests] {
> +    mi_gdb_test "-break-insert do_nothing" ".*"
> +    mi_gdb_test "-break-commands 2 \"python gdb.execute('disable 2', True, True)\"" ".*"

It's better to use $bpnum instead of hardcoding 2, as otherwise if someone adds a
test that adds another breakpoint before this, this test stops being
effective, silently.

> +
> +    mi_gdb_test "-exec-continue" ".*"

This should use mi_send_resuming_command/mi_expect_stop
or mi_execute_to, so that the test works when the whole MI
testsuite is run in async mode.

> +
> +    set s [string repeat "A" 100]
> +
> +    # This will crash gdb if redirection is not done properly.
> +    mi_gdb_test "echo $s" ".*\\^done" "mi nested redirect"

In addition to that, it'd be good to confirm the breakpoint did
end up disabled, which likewise confirms the breakpoint command
was set on the breakpoint we wanted.  (might not need the "echo"
if that itself already causes a crash.)

> +proc mi_skip_python_tests {} {
> +    global mi_gdb_prompt
> +    global gdb_py_is_py3k
> +    global gdb_py_is_py24
> +
> +    gdb_test_multiple "python print ('test')" "verify python support" {
> +	-re "not supported.*$mi_gdb_prompt$"	{
> +	    unsupported "Python support is disabled."
> +	    return 1
> +	}
> +	-re "$mi_gdb_prompt$"	{}
> +    }
> +
> +    set gdb_py_is_py24 0
> +    gdb_test_multiple "python print (sys.version_info\[0\])" "check if python 3" {
> +	-re "3.*$mi_gdb_prompt$"	{
> +            set gdb_py_is_py3k 1
> +        }
> +	-re ".*$mi_gdb_prompt$"	{
> +            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\].*$mi_gdb_prompt$" {
> +                set gdb_py_is_py24 1
> +            }
> +	    -re ".*$mi_gdb_prompt$" {
> +                set gdb_py_is_py24 0
> +            }
> +        }
> +    }
> +
> +    return 0
> +}

I think that we can avoid this duplication by renaming
skip_python_tests, adding it a prompt_re parameter, and
using that instead of $gdb_prompt.  Something like:

proc skip_python_tests {} {
   skip_python_tests_prompt "$gdb_prompt $"
}

proc mi_skip_python_tests {
   skip_python_tests_prompt "$mi_gdb_prompt$"
}

Did you try that?

Thanks,
Pedro Alves
diff mbox

Patch

diff --git a/gdb/mi/mi-out.c b/gdb/mi/mi-out.c
index 6ec41e6..6731509 100644
--- a/gdb/mi/mi-out.c
+++ b/gdb/mi/mi-out.c
@@ -22,14 +22,17 @@ 
 #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;
 
@@ -215,17 +218,20 @@  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,20 @@  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 +284,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 +295,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 +310,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 +345,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 +367,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 +378,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.  */
@@ -386,9 +395,10 @@  void
 mi_out_put (struct ui_out *uiout, struct ui_file *stream)
 {
   mi_out_data *data = ui_out_data (uiout);
+  struct ui_file *current_stream = VEC_last (ui_filep, data->streams);
 
-  ui_file_put (data->buffer, do_write, stream);
-  ui_file_rewind (data->buffer);
+  ui_file_put (current_stream, do_write, stream);
+  ui_file_rewind (current_stream);
 }
 
 /* Return the current MI version.  */
@@ -407,13 +417,14 @@  struct ui_out *
 mi_out_new (int mi_version)
 {
   int flags = 0;
+  struct ui_file *new_stream;
 
   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 ();
+  data->streams = NULL;
+  new_stream = mem_fileopen ();
+  VEC_safe_push (ui_filep, data->streams, new_stream);
   return ui_out_new (&mi_ui_out_impl, data, flags);
 }
diff --git a/gdb/testsuite/gdb.mi/mi-logging.exp b/gdb/testsuite/gdb.mi/mi-logging.exp
index d5e4193..063ea97 100644
--- a/gdb/testsuite/gdb.mi/mi-logging.exp
+++ b/gdb/testsuite/gdb.mi/mi-logging.exp
@@ -82,6 +82,20 @@  if [regexp "1001\\^done\[\r\n\]+$mi_log_prompt.*1002\\^running\[\r\n\]+\\*runnin
     fail "Redirect log file contents"
 }
 
+# This triggers a nested mi_ui_out redirection, by disabling a breakpoint
+# inside a python command that has to_string = True.
+if ![mi_skip_python_tests] {
+    mi_gdb_test "-break-insert do_nothing" ".*"
+    mi_gdb_test "-break-commands 2 \"python gdb.execute('disable 2', True, True)\"" ".*"
+
+    mi_gdb_test "-exec-continue" ".*"
+
+    set s [string repeat "A" 100]
+
+    # This will crash gdb if redirection is not done properly.
+    mi_gdb_test "echo $s" ".*\\^done" "mi nested redirect"
+}
+
 mi_gdb_exit
 
 remote_file host delete $milogfile
diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
index 204f1e8..23a5b80 100644
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -2491,3 +2491,40 @@  proc mi_make_breakpoint_table {bp_list} {
     # Assemble the final regexp.
     return "BreakpointTable={nr_rows=\"$nr\",nr_cols=\"$nc\",$header,$body}"
 }
+
+proc mi_skip_python_tests {} {
+    global mi_gdb_prompt
+    global gdb_py_is_py3k
+    global gdb_py_is_py24
+
+    gdb_test_multiple "python print ('test')" "verify python support" {
+	-re "not supported.*$mi_gdb_prompt$"	{
+	    unsupported "Python support is disabled."
+	    return 1
+	}
+	-re "$mi_gdb_prompt$"	{}
+    }
+
+    set gdb_py_is_py24 0
+    gdb_test_multiple "python print (sys.version_info\[0\])" "check if python 3" {
+	-re "3.*$mi_gdb_prompt$"	{
+            set gdb_py_is_py3k 1
+        }
+	-re ".*$mi_gdb_prompt$"	{
+            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\].*$mi_gdb_prompt$" {
+                set gdb_py_is_py24 1
+            }
+	    -re ".*$mi_gdb_prompt$" {
+                set gdb_py_is_py24 0
+            }
+        }
+    }
+
+    return 0
+}