From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6454 invoked by alias); 4 May 2008 08:26:53 -0000 Received: (qmail 6443 invoked by uid 22791); 4 May 2008 08:26:52 -0000 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.31) with ESMTP; Sun, 04 May 2008 08:26:34 +0000 Received: (qmail 12930 invoked from network); 4 May 2008 08:26:31 -0000 Received: from unknown (HELO 172.16.unknown.plus.ru) (vladimir@127.0.0.2) by mail.codesourcery.com with ESMTPA; 4 May 2008 08:26:31 -0000 From: Vladimir Prus To: Daniel Jacobowitz Subject: Re: [RFA] Use observers to report stop events. Date: Sun, 04 May 2008 09:05:00 -0000 User-Agent: KMail/1.9.6 (enterprise 0.20070907.709405) Cc: Joel Brobecker , gdb-patches@sources.redhat.com References: <200804112145.58456.vladimir@codesourcery.com> <200804292210.31614.vladimir@codesourcery.com> <20080501195758.GL22218@caradoc.them.org> In-Reply-To: <20080501195758.GL22218@caradoc.them.org> MIME-Version: 1.0 Content-Type: Multipart/Mixed; boundary="Boundary-00=_SMXHIpwV4W1MmK0" Message-Id: <200805041225.54416.vladimir@codesourcery.com> Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-05/txt/msg00162.txt.bz2 --Boundary-00=_SMXHIpwV4W1MmK0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Content-length: 1625 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 --Boundary-00=_SMXHIpwV4W1MmK0 Content-Type: text/x-diff; charset="iso-8859-1"; name="0001-Introduce-common-cleanup-for-restoring-integers.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename="0001-Introduce-common-cleanup-for-restoring-integers.patch" Content-length: 5854 =46rom d855d879f515a4ac4a177a45a2b08fea48064b21 Mon Sep 17 00:00:00 2001 From: Vladimir Prus 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; } =20 -static void -restore_always_inserted_mode (void *p) -{ - always_inserted_mode =3D (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 =3D make_cleanup (restore_always_inserted_mode,=20 - (void *) (uintptr_t) always_inserted_mode); - always_inserted_mode =3D 0; + cleanup =3D make_cleanup_restore_integer (&always_inserted_mode, 0); =20 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.=20=20 + + 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. */ =20 struct cleanup { struct cleanup *next; void (*function) (void *); + void (*free_arg) (void *); void *arg; }; =20 @@ -345,11 +351,17 @@ extern struct cleanup *make_cleanup_close (int fd); =20 extern struct cleanup *make_cleanup_bfd_close (bfd *abfd); =20 +extern struct cleanup *make_cleanup_restore_integer (int *variable, int va= lue); + extern struct cleanup *make_final_cleanup (make_cleanup_ftype *, void *); =20 extern struct cleanup *make_my_cleanup (struct cleanup **, make_cleanup_ftype *, void *); =20 +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_a= ddr_info *addrs) return make_my_cleanup (&cleanup_chain, do_free_section_addr_info, addrs= ); } =20 +struct restore_integer_closure +{ + int *variable; + int value; +}; =20 +static void +restore_integer (void *p) +{ + struct restore_integer_closure *closure =3D p; + *(closure->variable) =3D 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 =3D + xmalloc (sizeof (struct restore_integer_closure)); + struct cleanup *cleanup =3D make_cleanup (restore_integer, (void *) c); + c->variable =3D variable; + c->value =3D *variable;=20=20=20=20 + *variable =3D value; + cleanup_chain->free_arg =3D xfree; + return cleanup; +} + +struct cleanup * +make_my_cleanup2 (struct cleanup **pmy_chain, make_cleanup_ftype *function= ,=20 + void *arg, void (*free_arg) (void *)) { struct cleanup *new =3D (struct cleanup *) xmalloc (sizeof (struct cleanup)); @@ -288,12 +315,20 @@ make_my_cleanup (struct cleanup **pmy_chain, make_cle= anup_ftype *function, =20 new->next =3D *pmy_chain; new->function =3D function; + new->free_arg =3D free_arg; new->arg =3D arg; *pmy_chain =3D new; =20 return old_chain; } =20 +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. */ =20 @@ -318,6 +353,8 @@ do_my_cleanups (struct cleanup **pmy_chain, { *pmy_chain =3D 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 =3D *pmy_chain) !=3D old_chain) { *pmy_chain =3D ptr->next; + if (ptr->free_arg) + (*ptr->free_arg) (ptr->arg); xfree (ptr); } } --=20 1.5.3.5 --Boundary-00=_SMXHIpwV4W1MmK0 Content-Type: text/x-diff; charset="iso-8859-1"; name="0002-Remove-stale-code.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename="0002-Remove-stale-code.patch" Content-length: 1647 =46rom 962b24aa709c684a616070453b7f9d31175a2d61 Mon Sep 17 00:00:00 2001 From: Vladimir Prus 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) =20 breakpoint =3D (struct breakpoint *) arg->data.pointer; function =3D (struct symbol *) arg->next->data.pointer; - cleanups =3D (struct cleanup *) arg->next->next->data.pointer; =20 if (!error_p) { @@ -1354,14 +1353,10 @@ finish_command (char *arg, int from_tty) (struct continuation_arg *) xmalloc (sizeof (struct continuation_arg)); arg2 =3D (struct continuation_arg *) xmalloc (sizeof (struct continuation_arg)); - arg3 =3D - (struct continuation_arg *) xmalloc (sizeof (struct continuation_arg)); arg1->next =3D arg2; - arg2->next =3D arg3; - arg3->next =3D NULL; + arg2->next =3D NULL; arg1->data.pointer =3D breakpoint; arg2->data.pointer =3D function; - arg3->data.pointer =3D old_chain; add_continuation (finish_command_continuation, arg1); =20 /* Do this only if not running asynchronously or if the target --=20 1.5.3.5 --Boundary-00=_SMXHIpwV4W1MmK0--