This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA] Use observers to report stop events.
- From: Vladimir Prus <vladimir at codesourcery dot com>
- To: Daniel Jacobowitz <drow at false dot org>
- Cc: Joel Brobecker <brobecker at adacore dot com>, gdb-patches at sources dot redhat dot com
- Date: Sun, 4 May 2008 12:25:54 +0400
- Subject: Re: [RFA] Use observers to report stop events.
- References: <200804112145.58456.vladimir@codesourcery.com> <200804292210.31614.vladimir@codesourcery.com> <20080501195758.GL22218@caradoc.them.org>
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