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

[Thiago Jung Bauermann <bauerman at br dot ibm dot com>]

Hi,

If you have the following convenience function:

class Segfault(gdb.Function):
	"""Calls a GDB command to make it segfault."""

	def __init__(self):
		super (Segfault, self).__init__ ("segfault")

	def invoke(self):
		return gdb.execute("print 1", to_string=True)

Segfault ()

and call it from GDB, it will live up to its name:

hactar% ./gdb -q
(gdb) source /tmp/segfault.py
(gdb) set var $foo = $segfault()
Left operand of assignment is not a modifiable lvalue.
(gdb) set var $foo = $segfault()
zsh: segmentation fault (core dumped)  ./gdb -q

When evaluate_subexp_standard evaluates "$foo = $segfault()", it will
first evaluate the left hand side, generate a value, then evaluate the
right hand side. The problem is that evaluating the RHS involves
executing a GDB command. The first thing execute_command does is call
prepare_execute_command which in turn calls free_all_values. This nukes
the value that evaluate_subexp_standard found for the LHS, leading to a
segmentation fault further down the road. The error message about the
left operand when first calling the convenience function in the output
above is simply because the LHS value is garbage and value->modifiable
is 0. We aren't so lucky in the second call and segfault in
value_assign.

The call to free_all_values in the beginning of execute_command is there
since the first version of GDB in the repository. What it does is to
free the values created by the command which ran before. This patch
fixes the bug by making execute_command (and also mi_cmd_execute, which
also calls prepare_execute_command) free all the values created during
the execution of the command it just ran instead of leaving them to be
cleaned up later.

I verified that this patch introduces no memory leaks by running a few
testcases under Valgrind's memcheck and massif: break.exp, demangle.exp,
funcargs.exp and mi2-var-child.exp. These tests were chosen because they
run a fair number of tests against GDB.

There are no regressions on ppc-linux, ppc64-linux and x86-linux.
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


2011-05-11  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: gdb.git/gdb/mi/mi-main.c
===================================================================
--- gdb.git.orig/gdb/mi/mi-main.c	2011-05-11 00:43:29.000000000 -0300
+++ gdb.git/gdb/mi/mi-main.c	2011-05-11 00:43:32.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: gdb.git/gdb/top.c
===================================================================
--- gdb.git.orig/gdb/top.c	2011-05-11 00:43:29.000000000 -0300
+++ gdb.git/gdb/top.c	2011-05-11 00:43:32.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: gdb.git/gdb/top.h
===================================================================
--- gdb.git.orig/gdb/top.h	2011-05-11 00:43:29.000000000 -0300
+++ gdb.git/gdb/top.h	2011-05-11 00:43:32.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: gdb.git/gdb/defs.h
===================================================================
--- gdb.git.orig/gdb/defs.h	2011-05-11 00:43:29.000000000 -0300
+++ gdb.git/gdb/defs.h	2011-05-11 00:43:32.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: gdb.git/gdb/utils.c
===================================================================
--- gdb.git.orig/gdb/utils.c	2011-05-11 00:43:29.000000000 -0300
+++ gdb.git/gdb/utils.c	2011-05-11 00:45:32.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: gdb.git/gdb/testsuite/gdb.python/py-function.exp
===================================================================
--- gdb.git.orig/gdb/testsuite/gdb.python/py-function.exp	2011-05-11 00:43:29.000000000 -0300
+++ gdb.git/gdb/testsuite/gdb.python/py-function.exp	2011-05-11 00:43:32.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."