Re: [RFA] Fix segfault on Python convenience functions which call GDB commands

[Thiago Jung Bauermann <bauerman@br.ibm.com>]

On Thu, 2011-05-12 at 14:19 -0600, Tom Tromey wrote:
> >>>>> "Thiago" == Thiago Jung Bauermann <bauerman@br.ibm.com> writes:
> 
> Thiago> 2011-05-11  Thiago Jung Bauermann  <bauerman@br.ibm.com>
> Thiago> gdb/
> Thiago> 	* mi/mi-main.c (mi_cmd_execute): Use cleanup from
> Thiago> 	prepare_execute_command.
> Thiago> 	top.c (prepare_execute_command): Return cleanup.
> Thiago> 	(execute_command): Use cleanup from prepare_execute_command.
> Thiago> 	* top.h (prepare_execute_command): Change prototype to return
> Thiago> 	cleanup.
> Thiago> 	* defs.h (struct value): Add opaque declaration.
> Thiago> 	(make_cleanup_value_free_to_mark): Add prototype.
> Thiago> 	* utils.c (do_value_free_to_mark): New function.
> Thiago> 	(make_cleanup_value_free_to_mark): Likewise.
> 
> Looks good.
> Just one tiny nit.
> 
> Thiago> +extern struct cleanup * make_cleanup_value_free_to_mark (struct value *);
> 
> Extra space after the "*".
> 
> Ok with this change.

Thanks! I committed the following.
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


2011-05-13  Thiago Jung Bauermann  <bauerman@br.ibm.com>

gdb/
	* mi/mi-main.c (mi_cmd_execute): Use cleanup from
	prepare_execute_command.
	* top.c (prepare_execute_command): Return cleanup.
	(execute_command): Use cleanup from prepare_execute_command.
	* top.h (prepare_execute_command): Change prototype to return
	cleanup.
	* defs.h (struct value): Add opaque declaration.
	(make_cleanup_value_free_to_mark): Add prototype.
	* utils.c (do_value_free_to_mark): New function.
	(make_cleanup_value_free_to_mark): Likewise.

gdb/testsuite/
	* gdb.python/py-function.exp: Test setting a value from a function
	which executes a command.

Index: src/gdb/mi/mi-main.c
===================================================================
--- src.orig/gdb/mi/mi-main.c	2011-05-06 15:37:27.000000000 -0300
+++ src/gdb/mi/mi-main.c	2011-05-13 01:30:00.000000000 -0300
@@ -2025,9 +2025,7 @@ mi_cmd_execute (struct mi_parse *parse)
 {
   struct cleanup *cleanup;
 
-  prepare_execute_command ();
-
-  cleanup = make_cleanup (null_cleanup, NULL);
+  cleanup = prepare_execute_command ();
 
   if (parse->all && parse->thread_group != -1)
     error (_("Cannot specify --thread-group together with --all"));
Index: src/gdb/top.c
===================================================================
--- src.orig/gdb/top.c	2011-05-06 15:36:42.000000000 -0300
+++ src/gdb/top.c	2011-05-13 01:30:00.000000000 -0300
@@ -339,10 +339,14 @@ do_chdir_cleanup (void *old_dir)
 }
 #endif
 
-void
+struct cleanup *
 prepare_execute_command (void)
 {
-  free_all_values ();
+  struct value *mark;
+  struct cleanup *cleanup;
+
+  mark = value_mark ();
+  cleanup = make_cleanup_value_free_to_mark (mark);
 
   /* With multiple threads running while the one we're examining is
      stopped, the dcache can get stale without us being able to detect
@@ -350,6 +354,8 @@ prepare_execute_command (void)
      help things like backtrace.  */
   if (non_stop)
     target_dcache_invalidate ();
+
+  return cleanup;
 }
 
 /* Execute the line P as a command, in the current user context.
@@ -358,12 +364,13 @@ prepare_execute_command (void)
 void
 execute_command (char *p, int from_tty)
 {
+  struct cleanup *cleanup;
   struct cmd_list_element *c;
   enum language flang;
   static int warned = 0;
   char *line;
 
-  prepare_execute_command ();
+  cleanup = prepare_execute_command ();
 
   /* Force cleanup of any alloca areas if using C alloca instead of
      a builtin alloca.  */
@@ -462,6 +469,8 @@ execute_command (char *p, int from_tty)
 	  warned = 1;
 	}
     }
+
+    do_cleanups (cleanup);
 }
 
 /* Run execute_command for P and FROM_TTY.  Capture its output into the
Index: src/gdb/top.h
===================================================================
--- src.orig/gdb/top.h	2011-01-11 16:51:50.000000000 -0200
+++ src/gdb/top.h	2011-05-13 01:30:00.000000000 -0300
@@ -48,8 +48,9 @@ extern int quit_cover (void *);
 extern void execute_command (char *, int);
 
 /* Prepare for execution of a command.
-   Call this before every command, CLI or MI.  */
-extern void prepare_execute_command (void);
+   Call this before every command, CLI or MI.
+   Returns a cleanup to be run after the command is completed.  */
+extern struct cleanup *prepare_execute_command (void);
 
 /* This function returns a pointer to the string that is used
    by gdb for its command prompt.  */
Index: src/gdb/defs.h
===================================================================
--- src.orig/gdb/defs.h	2011-05-13 01:29:52.000000000 -0300
+++ src/gdb/defs.h	2011-05-13 01:31:58.000000000 -0300
@@ -281,6 +281,7 @@ struct symtab;
 struct breakpoint;
 struct frame_info;
 struct gdbarch;
+struct value;
 
 /* From main.c.  */
 
@@ -360,6 +361,8 @@ extern struct cleanup *make_cleanup_unpu
 extern struct cleanup *
   make_cleanup_restore_ui_file (struct ui_file **variable);
 
+extern struct cleanup *make_cleanup_value_free_to_mark (struct value *);
+
 extern struct cleanup *make_final_cleanup (make_cleanup_ftype *, void *);
 
 extern struct cleanup *make_my_cleanup (struct cleanup **,
Index: src/gdb/utils.c
===================================================================
--- src.orig/gdb/utils.c	2011-05-06 15:36:44.000000000 -0300
+++ src/gdb/utils.c	2011-05-13 01:30:00.000000000 -0300
@@ -431,6 +431,23 @@ make_cleanup_restore_ui_file (struct ui_
   return make_cleanup_dtor (do_restore_ui_file, (void *) c, xfree);
 }
 
+/* Helper for make_cleanup_value_free_to_mark.  */
+
+static void
+do_value_free_to_mark (void *value)
+{
+  value_free_to_mark ((struct value *) value);
+}
+
+/* Free all values allocated since MARK was obtained by value_mark
+   (except for those released) when the cleanup is run.  */
+
+struct cleanup *
+make_cleanup_value_free_to_mark (struct value *mark)
+{
+  return make_my_cleanup (&cleanup_chain, do_value_free_to_mark, mark);
+}
+
 struct cleanup *
 make_my_cleanup2 (struct cleanup **pmy_chain, make_cleanup_ftype *function,
 		  void *arg,  void (*free_arg) (void *))
Index: src/gdb/testsuite/gdb.python/py-function.exp
===================================================================
--- src.orig/gdb/testsuite/gdb.python/py-function.exp	2011-03-31 11:49:11.000000000 -0300
+++ src/gdb/testsuite/gdb.python/py-function.exp	2011-05-13 01:30:00.000000000 -0300
@@ -95,3 +95,17 @@ gdb_py_test_multiple "Test Normal Error"
 
 gdb_test "print \$normalerror()" "Traceback.*File.*line 5.*in invoke.*RuntimeError.*This is a Normal Error.*" \
     "Test a Runtime error.  There should be a stack trace."
+
+gdb_py_test_multiple "input command-calling function" \
+  "python" "" \
+  "class CallCommand(gdb.Function):" "" \
+  "    def __init__(self):" "" \
+  "        gdb.Function.__init__(self, 'call_command')" "" \
+  "    def invoke(self):" "" \
+  "        return gdb.execute('print 1', to_string=True)" "" \
+  "CallCommand ()" "" \
+  "end" ""
+
+gdb_test_no_output "set var \$foo = \$call_command()" "Setting a value from a function which executes a command."
+# There was a bug where GDB would segfault in the second call, so try calling again.
+gdb_test_no_output "set var \$foo = \$call_command()" "Setting a value from a function which executes a command, again."