RFA: fix PR 9350

[Tom Tromey <tromey at redhat dot com>]

This patch fixes PR 9350, a memory leak in gdb.

I ran the test case from the PR under valgrind and then fixed all the
fixable memory leaks (valgrind reports a bunch of stuff from Python,
plus a non-serious leak when registering a gdb command).

I think most of the fixes are pretty obvious in context.  They are
largely cases of forgetting to run cleanups.

Built and regtested on x86-64 (compile farm).

Please review.

thanks,
Tom

2009-01-06  Tom Tromey  <tromey@redhat.com>

	PR breakpoints/9350:
	* varobj.c (varobj_invalidate): Unconditionally free
	all_rootvarobj.
	* symfile.c (syms_from_objfile): Free local_addr when returning
	normally.
	* exec.c (exec_file_attach): Do cleanups before returning.
	(exec_file_command): Likewise.
	* corefile.c (reopen_exec_file): Do cleanups before returning.
	* breakpoint.c (insert_breakpoint_locations): Do cleanups before
	returning.
	(do_vec_free): New function.
	(update_global_location_list): Make a cleanup for old_locations.
	Do cleanups before returning.  Remove unused variable 'e'.
	(find_condition_and_thread): Free result of parsing the
	expression.
	(print_it_typical): Do cleanups before returning.
	(breakpoint_re_set_one): Always free sals.sals.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 2e25490..fbd6159 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1282,7 +1282,7 @@ insert_breakpoint_locations (void)
   int process_warning = 0;
 
   struct ui_file *tmp_error_stream = mem_fileopen ();
-  make_cleanup_ui_file_delete (tmp_error_stream);
+  struct cleanup *cleanups = make_cleanup_ui_file_delete (tmp_error_stream);
   
   /* Explicitly mark the warning -- this will only be printed if
      there was an error.  */
@@ -1360,6 +1360,8 @@ You may have requested too many hardware breakpoints/watchpoints.\n");
       target_terminal_ours_for_output ();
       error_stream (tmp_error_stream);
     }
+
+  do_cleanups (cleanups);
 }
 
 int
@@ -2241,13 +2243,12 @@ watchpoint_value_print (struct value *val, struct ui_file *stream)
 static enum print_stop_action
 print_it_typical (bpstat bs)
 {
-  struct cleanup *old_chain, *ui_out_chain;
+  struct cleanup *old_chain;
   struct breakpoint *b;
   const struct bp_location *bl;
   struct ui_stream *stb;
   int bp_temp = 0;  
-  stb = ui_out_stream_new (uiout);
-  old_chain = make_cleanup_ui_out_stream_delete (stb);
+
   /* bs->breakpoint_at can be NULL if it was a momentary breakpoint
      which has since been deleted.  */
   if (bs->breakpoint_at == NULL)
@@ -2255,6 +2256,9 @@ print_it_typical (bpstat bs)
   bl = bs->breakpoint_at;
   b = bl->owner;
 
+  stb = ui_out_stream_new (uiout);
+  old_chain = make_cleanup_ui_out_stream_delete (stb);
+
   switch (b->type)
     {
     case bp_breakpoint:
@@ -2277,6 +2281,7 @@ print_it_typical (bpstat bs)
 	}
       ui_out_field_int (uiout, "bkptno", b->number);
       ui_out_text (uiout, ", ");
+      do_cleanups (old_chain);
       return PRINT_SRC_AND_LOC;
       break;
 
@@ -2285,6 +2290,7 @@ print_it_typical (bpstat bs)
 	 variable?  (If so, we report this as a generic, "Stopped due
 	 to shlib event" message.) */
       printf_filtered (_("Stopped due to shared library event\n"));
+      do_cleanups (old_chain);
       return PRINT_NOTHING;
       break;
 
@@ -2292,12 +2298,14 @@ print_it_typical (bpstat bs)
       /* Not sure how we will get here. 
 	 GDB should not stop for these breakpoints.  */
       printf_filtered (_("Thread Event Breakpoint: gdb should not stop!\n"));
+      do_cleanups (old_chain);
       return PRINT_NOTHING;
       break;
 
     case bp_overlay_event:
       /* By analogy with the thread event, GDB should not stop for these. */
       printf_filtered (_("Overlay Event Breakpoint: gdb should not stop!\n"));
+      do_cleanups (old_chain);
       return PRINT_NOTHING;
       break;
 
@@ -2309,14 +2317,14 @@ print_it_typical (bpstat bs)
 	  (uiout, "reason",
 	   async_reason_lookup (EXEC_ASYNC_WATCHPOINT_TRIGGER));
       mention (b);
-      ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, "value");
+      make_cleanup_ui_out_tuple_begin_end (uiout, "value");
       ui_out_text (uiout, "\nOld value = ");
       watchpoint_value_print (bs->old_val, stb->stream);
       ui_out_field_stream (uiout, "old", stb);
       ui_out_text (uiout, "\nNew value = ");
       watchpoint_value_print (b->val, stb->stream);
       ui_out_field_stream (uiout, "new", stb);
-      do_cleanups (ui_out_chain);
+      do_cleanups (old_chain);
       ui_out_text (uiout, "\n");
       /* More than one watchpoint may have been triggered.  */
       return PRINT_UNKNOWN;
@@ -2328,11 +2336,11 @@ print_it_typical (bpstat bs)
 	  (uiout, "reason",
 	   async_reason_lookup (EXEC_ASYNC_READ_WATCHPOINT_TRIGGER));
       mention (b);
-      ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, "value");
+      make_cleanup_ui_out_tuple_begin_end (uiout, "value");
       ui_out_text (uiout, "\nValue = ");
       watchpoint_value_print (b->val, stb->stream);
       ui_out_field_stream (uiout, "value", stb);
-      do_cleanups (ui_out_chain);
+      do_cleanups (old_chain);
       ui_out_text (uiout, "\n");
       return PRINT_UNKNOWN;
       break;
@@ -2346,7 +2354,7 @@ print_it_typical (bpstat bs)
 	      (uiout, "reason",
 	       async_reason_lookup (EXEC_ASYNC_ACCESS_WATCHPOINT_TRIGGER));
 	  mention (b);
-	  ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, "value");
+	  make_cleanup_ui_out_tuple_begin_end (uiout, "value");
 	  ui_out_text (uiout, "\nOld value = ");
 	  watchpoint_value_print (bs->old_val, stb->stream);
 	  ui_out_field_stream (uiout, "old", stb);
@@ -2359,12 +2367,12 @@ print_it_typical (bpstat bs)
 	    ui_out_field_string
 	      (uiout, "reason",
 	       async_reason_lookup (EXEC_ASYNC_ACCESS_WATCHPOINT_TRIGGER));
-	  ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, "value");
+	  make_cleanup_ui_out_tuple_begin_end (uiout, "value");
 	  ui_out_text (uiout, "\nValue = ");
 	}
       watchpoint_value_print (b->val, stb->stream);
       ui_out_field_stream (uiout, "new", stb);
-      do_cleanups (ui_out_chain);
+      do_cleanups (old_chain);
       ui_out_text (uiout, "\n");
       return PRINT_UNKNOWN;
       break;
@@ -2377,6 +2385,7 @@ print_it_typical (bpstat bs)
 	ui_out_field_string
 	  (uiout, "reason",
 	   async_reason_lookup (EXEC_ASYNC_FUNCTION_FINISHED));
+      do_cleanups (old_chain);
       return PRINT_UNKNOWN;
       break;
 
@@ -2385,6 +2394,7 @@ print_it_typical (bpstat bs)
 	ui_out_field_string
 	  (uiout, "reason",
 	   async_reason_lookup (EXEC_ASYNC_LOCATION_REACHED));
+      do_cleanups (old_chain);
       return PRINT_UNKNOWN;
       break;
 
@@ -2395,6 +2405,7 @@ print_it_typical (bpstat bs)
     case bp_watchpoint_scope:
     case bp_call_dummy:
     default:
+      do_cleanups (old_chain);
       return PRINT_UNKNOWN;
     }
 }
@@ -5441,8 +5452,10 @@ find_condition_and_thread (char *tok, CORE_ADDR pc,
       
       if (toklen >= 1 && strncmp (tok, "if", toklen) == 0)
 	{
+	  struct expression *expr;
 	  tok = cond_start = end_tok + 1;
-	  parse_exp_1 (&tok, block_for_pc (pc), 0);
+	  expr = parse_exp_1 (&tok, block_for_pc (pc), 0);
+	  xfree (expr);
 	  cond_end = tok;
 	  *cond_string = savestring (cond_start, 
 				     cond_end - cond_start);
@@ -6855,6 +6868,16 @@ breakpoint_auto_delete (bpstat bs)
   }
 }
 
+/* A cleanup function which destroys a vector.  */
+
+static void
+do_vec_free (void *p)
+{
+  VEC(bp_location_p) **vec = p;
+  if (*vec)
+    VEC_free (bp_location_p, *vec);
+}
+
 /* If SHOULD_INSERT is false, do not insert any breakpoint locations
    into the inferior, only remove already-inserted locations that no
    longer should be inserted.  Functions that delete a breakpoint or
@@ -6877,11 +6900,12 @@ update_global_location_list (int should_insert)
   struct bp_location **next = &bp_location_chain;
   struct bp_location *loc;
   struct bp_location *loc2;
-  struct gdb_exception e;
   VEC(bp_location_p) *old_locations = NULL;
   int ret;
   int ix;
-  
+  struct cleanup *cleanups;
+
+  cleanups = make_cleanup (do_vec_free, &old_locations);
   /* Store old locations for future reference.  */
   for (loc = bp_location_chain; loc; loc = loc->global_next)
     VEC_safe_push (bp_location_p, old_locations, loc);
@@ -7010,6 +7034,8 @@ update_global_location_list (int should_insert)
 	  || (gdbarch_has_global_solist (target_gdbarch)
 	      && target_supports_multi_process ())))
     insert_breakpoint_locations ();
+
+  do_cleanups (cleanups);
 }
 
 void
@@ -7369,7 +7395,7 @@ breakpoint_re_set_one (void *bint)
   char *s;
   enum enable_state save_enable;
   struct gdb_exception e;
-
+  struct cleanup *cleanups;
 
   switch (b->type)
     {
@@ -7439,9 +7465,9 @@ breakpoint_re_set_one (void *bint)
 	  b->condition_not_parsed = 0;
 	}
       expanded = expand_line_sal_maybe (sals.sals[0]);
+      cleanups = make_cleanup (xfree, sals.sals);
       update_breakpoint_locations (b, expanded);
-
-      xfree (sals.sals);
+      do_cleanups (cleanups);
       break;
 
     case bp_watchpoint:
diff --git a/gdb/corefile.c b/gdb/corefile.c
index 2566f9f..57a0cdf 100644
--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -153,6 +153,7 @@ reopen_exec_file (void)
   int res;
   struct stat st;
   long mtime;
+  struct cleanup *cleanups;
 
   /* Don't do anything if there isn't an exec file. */
   if (exec_bfd == NULL)
@@ -160,7 +161,7 @@ reopen_exec_file (void)
 
   /* If the timestamp of the exec file has changed, reopen it. */
   filename = xstrdup (bfd_get_filename (exec_bfd));
-  make_cleanup (xfree, filename);
+  cleanups = make_cleanup (xfree, filename);
   res = stat (filename, &st);
 
   if (exec_bfd_mtime && exec_bfd_mtime != st.st_mtime)
@@ -170,6 +171,8 @@ reopen_exec_file (void)
        this stops GDB from holding the executable open after it
        exits.  */
     bfd_cache_close_all ();
+
+  do_cleanups (cleanups);
 #endif
 }
 
diff --git a/gdb/exec.c b/gdb/exec.c
index 542af0e..8d8c1df 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -194,6 +194,7 @@ exec_file_attach (char *filename, int from_tty)
     }
   else
     {
+      struct cleanup *cleanups;
       char *scratch_pathname;
       int scratch_chan;
 
@@ -228,7 +229,7 @@ exec_file_attach (char *filename, int from_tty)
          via the exec_bfd->name pointer, so we need to make another copy and
          leave exec_bfd as the new owner of the original copy. */
       scratch_pathname = xstrdup (scratch_pathname);
-      make_cleanup (xfree, scratch_pathname);
+      cleanups = make_cleanup (xfree, scratch_pathname);
 
       if (!bfd_check_format (exec_bfd, bfd_object))
 	{
@@ -276,6 +277,8 @@ exec_file_attach (char *filename, int from_tty)
       /* Tell display code (if any) about the changed file name.  */
       if (deprecated_exec_file_display_hook)
 	(*deprecated_exec_file_display_hook) (filename);
+
+      do_cleanups (cleanups);
     }
   bfd_cache_close_all ();
   observer_notify_executable_changed ();
@@ -302,11 +305,13 @@ exec_file_command (char *args, int from_tty)
 
   if (args)
     {
+      struct cleanup *cleanups;
+
       /* Scan through the args and pick up the first non option arg
          as the filename.  */
 
       argv = gdb_buildargv (args);
-      make_cleanup_freeargv (argv);
+      cleanups = make_cleanup_freeargv (argv);
 
       for (; (*argv != NULL) && (**argv == '-'); argv++)
         {;
@@ -317,6 +322,8 @@ exec_file_command (char *args, int from_tty)
       filename = tilde_expand (*argv);
       make_cleanup (xfree, filename);
       exec_file_attach (filename, from_tty);
+
+      do_cleanups (cleanups);
     }
   else
     exec_file_attach (NULL, from_tty);
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 14cb7b8..21328b8 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -899,6 +899,7 @@ syms_from_objfile (struct objfile *objfile,
   /* Discard cleanups as symbol reading was successful.  */
 
   discard_cleanups (old_chain);
+  xfree (local_addr);
 }
 
 /* Perform required actions after either reading in the initial
diff --git a/gdb/value.c b/gdb/value.c
index 39df98e..1068f1d 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -931,7 +931,7 @@ set_internalvar (struct internalvar *var, struct value *val)
      something in the value chain (i.e., before release_value is
      called), because after the error free_all_values will get called before
      long.  */
-  xfree (var->value);
+  value_free (var->value);
   var->value = newval;
   var->endian = gdbarch_byte_order (current_gdbarch);
   release_value (newval);
diff --git a/gdb/varobj.c b/gdb/varobj.c
index 25cc207..5b2ed9c 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -2780,7 +2780,7 @@ varobj_invalidate (void)
 
         varp++;
       }
-    xfree (all_rootvarobj);
   }
+  xfree (all_rootvarobj);
   return;
 }