This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFA] Use observers to report stop events.


On Thursday 01 May 2008 23:57:58 Daniel Jacobowitz wrote:
> On Tue, Apr 29, 2008 at 10:10:30PM +0400, Vladimir Prus wrote:
> > Here are 3 independent bits.
> > 
> > 1. Introduce the make_cleanup_restore_integer function. You're right
> > that it can lead to bad results if one discards this cleanup, but then
> > one should be careful with discarding cleanups anyway.
> 
> This patch looks fine except that ...
> 
> > 2. Modify the normal_stop observer not to fire in some cases. One
> > case is when doing function call -- we don't announce the stop in CLI
> > and for similar reason we don't have observer to be called. Also,
> > for the benefit of next patch, we want the call to observer to
> > be delayed until we print function return value, if we're doing finish.
> 
> ... unless I'm mistaken you have exactly the memory leak Joel warned
> about, since finish_command discards continuations.
> 
> Am I correct that the cleanup for finish_command is never supposed to
> survive the function returning?  It's run on error and discarded on
> normal return.  So you could put the closure in a local variable,
> maybe.
> 
>   struct foo_closure my_closure = { &my_global, my_global };
>   make_cleanup (restore_integer, &my_closure); 

This is somewhat limiting. Instead, I've implemented a mechanism that
allows a cleanup to well, "cleanup" its argument. I attach a patch
for that.

I also attach a patch stop the 'finish_command_continuation' from
accessing a cleanup is has no business with.

Are those two new patches, together with the previously posted one
(changing stop_observer not to always fire) OK?

- Volodya

From d855d879f515a4ac4a177a45a2b08fea48064b21 Mon Sep 17 00:00:00 2001
From: Vladimir Prus <vladimir@codesourcery.com>
Date: Tue, 29 Apr 2008 21:31:59 +0400
Subject: [RFA] Introduce common cleanup for restoring integers.
To: gdb-patches@sources.redhat.com
X-KMail-Transport: CodeSourcery
X-KMail-Identity: 901867920

	* defs.h (make_cleanup_restore_integer): New declaration.
	(struct cleanup): New field free_arg.
	(make_my_cleanup_2): New.
	* utils.c (restore_integer_closure, restore_integer)
	(make_cleanup_restore_integer): New.
	(make_my_cleanup): Initialize the free_arg field and
	renamed to make_my_cleanup_2.
	(do_my_cleanups): Call free_arg.
	(discard_cleanups): Call free_arg.
	* breakpoint.c (restore_always_inserted_mode): Remove.
	(update_breakpoints_after_exec): Use make_cleanup_restore_integer.
---
 gdb/breakpoint.c |   10 +---------
 gdb/defs.h       |   14 +++++++++++++-
 gdb/utils.c      |   43 +++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 55 insertions(+), 12 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 0648a4b..e5e5de0 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1452,12 +1452,6 @@ reattach_breakpoints (int pid)
   return 0;
 }
 
-static void
-restore_always_inserted_mode (void *p)
-{
-  always_inserted_mode = (uintptr_t) p;
-}
-
 void
 update_breakpoints_after_exec (void)
 {
@@ -1473,9 +1467,7 @@ update_breakpoints_after_exec (void)
   /* The binary we used to debug is now gone, and we're updating
      breakpoints for the new binary.  Until we're done, we should not
      try to insert breakpoints.  */
-  cleanup = make_cleanup (restore_always_inserted_mode, 
-			  (void *) (uintptr_t) always_inserted_mode);
-  always_inserted_mode = 0;
+  cleanup = make_cleanup_restore_integer (&always_inserted_mode, 0);
 
   ALL_BREAKPOINTS_SAFE (b, temp)
   {
diff --git a/gdb/defs.h b/gdb/defs.h
index 0fa0e6c..ce7e1b9 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -236,12 +236,18 @@ enum return_value_convention
    Use make_cleanup to add an element to the cleanup chain.
    Use do_cleanups to do all cleanup actions back to a given
    point in the chain.  Use discard_cleanups to remove cleanups
-   from the chain back to a given point, not doing them.  */
+   from the chain back to a given point, not doing them.  
+
+   If the argument is pointer to allocated memory, then you need to
+   to additionally set the 'free_arg' member to a function that will
+   free that memory.  This function will be called both when the cleanup
+   is executed and when it's discarded.  */
 
 struct cleanup
   {
     struct cleanup *next;
     void (*function) (void *);
+    void (*free_arg) (void *);
     void *arg;
   };
 
@@ -345,11 +351,17 @@ extern struct cleanup *make_cleanup_close (int fd);
 
 extern struct cleanup *make_cleanup_bfd_close (bfd *abfd);
 
+extern struct cleanup *make_cleanup_restore_integer (int *variable, int value);
+
 extern struct cleanup *make_final_cleanup (make_cleanup_ftype *, void *);
 
 extern struct cleanup *make_my_cleanup (struct cleanup **,
 					make_cleanup_ftype *, void *);
 
+extern struct cleanup *make_my_cleanup2 (struct cleanup **,
+					 make_cleanup_ftype *, void *,
+					 void (*free_arg) (void *));
+
 extern struct cleanup *save_cleanups (void);
 extern struct cleanup *save_final_cleanups (void);
 extern struct cleanup *save_my_cleanups (struct cleanup **);
diff --git a/gdb/utils.c b/gdb/utils.c
index d9953a0..9610bd0 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -277,10 +277,37 @@ make_cleanup_free_section_addr_info (struct section_addr_info *addrs)
   return make_my_cleanup (&cleanup_chain, do_free_section_addr_info, addrs);
 }
 
+struct restore_integer_closure
+{
+  int *variable;
+  int value;
+};
 
+static void
+restore_integer (void *p)
+{
+  struct restore_integer_closure *closure = p;
+  *(closure->variable) = closure->value;
+}
+
+/* Assign VALUE to *VARIABLE and arrange for the old value to
+   be restored via cleanup.  */
 struct cleanup *
-make_my_cleanup (struct cleanup **pmy_chain, make_cleanup_ftype *function,
-		 void *arg)
+make_cleanup_restore_integer (int *variable, int value)
+{
+  struct restore_integer_closure *c =
+    xmalloc (sizeof (struct restore_integer_closure));
+  struct cleanup *cleanup = make_cleanup (restore_integer, (void *) c);
+  c->variable = variable;
+  c->value = *variable;    
+  *variable = value;
+  cleanup_chain->free_arg = xfree;
+  return cleanup;
+}
+
+struct cleanup *
+make_my_cleanup2 (struct cleanup **pmy_chain, make_cleanup_ftype *function, 
+		  void *arg,  void (*free_arg) (void *))
 {
   struct cleanup *new
     = (struct cleanup *) xmalloc (sizeof (struct cleanup));
@@ -288,12 +315,20 @@ make_my_cleanup (struct cleanup **pmy_chain, make_cleanup_ftype *function,
 
   new->next = *pmy_chain;
   new->function = function;
+  new->free_arg = free_arg;
   new->arg = arg;
   *pmy_chain = new;
 
   return old_chain;
 }
 
+struct cleanup *
+make_my_cleanup (struct cleanup **pmy_chain, make_cleanup_ftype *function,
+		 void *arg)
+{
+  return make_my_cleanup2 (pmy_chain, function, arg, NULL);
+}
+
 /* Discard cleanups and do the actions they describe
    until we get back to the point OLD_CHAIN in the cleanup_chain.  */
 
@@ -318,6 +353,8 @@ do_my_cleanups (struct cleanup **pmy_chain,
     {
       *pmy_chain = ptr->next;	/* Do this first incase recursion */
       (*ptr->function) (ptr->arg);
+      if (ptr->free_arg)
+	(*ptr->free_arg) (ptr->arg);
       xfree (ptr);
     }
 }
@@ -345,6 +382,8 @@ discard_my_cleanups (struct cleanup **pmy_chain,
   while ((ptr = *pmy_chain) != old_chain)
     {
       *pmy_chain = ptr->next;
+      if (ptr->free_arg)
+	(*ptr->free_arg) (ptr->arg);
       xfree (ptr);
     }
 }
-- 
1.5.3.5

From 962b24aa709c684a616070453b7f9d31175a2d61 Mon Sep 17 00:00:00 2001
From: Vladimir Prus <vladimir@codesourcery.com>
Date: Sun, 4 May 2008 12:09:45 +0400
Subject: [RFA] Remove stale code.
To: gdb-patches@sources.redhat.com
X-KMail-Transport: CodeSourcery
X-KMail-Identity: 901867920

	* infrun.c (finish_command): Don't pass cleanup
	to continuation.
	(finish_command_continuation): Don't grab cleanup from
	the passed data, as we don't use, and cannot, use it anyway.
---
 gdb/infcmd.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index abf1354..66c8a05 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1263,7 +1263,6 @@ finish_command_continuation (struct continuation_arg *arg, int error_p)
 
   breakpoint = (struct breakpoint *) arg->data.pointer;
   function = (struct symbol *) arg->next->data.pointer;
-  cleanups = (struct cleanup *) arg->next->next->data.pointer;
 
   if (!error_p)
     {
@@ -1354,14 +1353,10 @@ finish_command (char *arg, int from_tty)
     (struct continuation_arg *) xmalloc (sizeof (struct continuation_arg));
   arg2 =
     (struct continuation_arg *) xmalloc (sizeof (struct continuation_arg));
-  arg3 =
-    (struct continuation_arg *) xmalloc (sizeof (struct continuation_arg));
   arg1->next = arg2;
-  arg2->next = arg3;
-  arg3->next = NULL;
+  arg2->next = NULL;
   arg1->data.pointer = breakpoint;
   arg2->data.pointer = function;
-  arg3->data.pointer = old_chain;
   add_continuation (finish_command_continuation, arg1);
 
   /* Do this only if not running asynchronously or if the target
-- 
1.5.3.5


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]