This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [continuation args 2/2] Make continuation args not leak
A Saturday 12 July 2008 19:54:48, Daniel Jacobowitz wrote:
> On Fri, Jul 11, 2008 at 09:38:55PM +0100, Pedro Alves wrote:
> > + struct cleanup **as_cleanup_p = (struct cleanup **) &cmd_continuation;
> > + make_cleanup_ftype *continuation_hook_ftype = continuation_hook;
>
> ftype stands for function type, and our convention is to use it for
> types only. So please don't use it in the name of a variable.
> continuation_fn?
Ok. I've fixed both instances.
> > +static void
> > +finish_command_continuation_free_arg (void *arg)
> > +{
> > + /* NOTE: See finish_command_continuation. This would go away, if
> > + this suppressing is made a thread property. */
> > + suppress_stop_observer = 0;
> > +}
>
> Doesn't it still need to call xfree?
>
Indeed.
> Otherwise OK.
For the archives, attached is what I checked in after another
round of testing.
--
Pedro Alves
2008-07-12 Pedro Alves <pedro@codesourcery.com>
Rewrite continuations internals on top of cleanups and plug
continuation arguments leaks.
* defs.h (struct continuation): Make it opaque.
(add_continuation, add_intermediate_continuation): Drop the int
argument of the continuation hook argument. Add
continuation_free_args argument.
(do_all_continuations, do_all_intermediate_continuations): Drop
the error_p argument.
* utils.c (add_continuation): Drop the int argument of the
continuation hook argument. Add continuation_free_args argument.
Reimplement on top of cleanups.
(do_all_continuations): Drop error argument. Reimplement on top
of cleanups.
(discard_all_continuations): Reimplement on top of cleanups.
(add_intermediate_continuation): Drop the int argument of the
continuation hook argument. Add continuation_free_args argument.
Reimplement on top of cleanups.
(do_all_intermediate_continuations): Drop error argument.
Reimplement on top of cleanups.
(discard_all_intermediate_continuations): Reimplement on top of
cleanups.
* breakpoint.c (until_break_command_continuation): Drop error
argument. Add xfree as continuation argument deleter.
* inf-loop.c (inferior_event_handler): On error, discard all
continuations. Adjust to new do_all_intermediate_continuations
and do_all_continuations interfaces.
* infcmd.c (step_1_continuation): Drop error_p argument. Adjust.
Pass xfree as continuation argument deleter.
(finish_command_continuation): Drop error_p argument. Adjust.
(finish_command_continuation_free_arg): New.
(finish_command): Pass finish_command_continuation_free_arg as
continuation argument deleter. Adjust to new do_all_continuations
interfaces.
(attach_command_continuation): Drop error_p argument.
(attach_command_continuation_free_args): New.
(attach_command): Pass attach_command_continuation_free_args as
continuation argument deleter.
* interps.c (interp_set): Adjust to new do_all_continuations
interfaces.
* event-top.c (stdin_event_handler): In error, also discard the
intermediate continuations.
---
gdb/breakpoint.c | 7 +---
gdb/defs.h | 17 ++++------
gdb/event-top.c | 1
gdb/inf-loop.c | 12 ++++---
gdb/infcmd.c | 81 ++++++++++++++++++++++++++++-------------------
gdb/interps.c | 2 -
gdb/utils.c | 93 ++++++++++++++++++++-----------------------------------
7 files changed, 102 insertions(+), 111 deletions(-)
Index: src/gdb/defs.h
===================================================================
--- src.orig/gdb/defs.h 2008-07-12 20:08:48.000000000 +0100
+++ src/gdb/defs.h 2008-07-12 20:08:56.000000000 +0100
@@ -677,12 +677,7 @@ extern void free_command_lines (struct c
used by the finish and until commands, and in the remote protocol
when opening an extended-remote connection. */
-struct continuation
- {
- void (*continuation_hook) (void *, int);
- void *args;
- struct continuation *next;
- };
+struct continuation;
/* In infrun.c. */
extern struct continuation *cmd_continuation;
@@ -690,12 +685,14 @@ extern struct continuation *cmd_continua
extern struct continuation *intermediate_continuation;
/* From utils.c */
-extern void add_continuation (void (*)(void *, int), void *);
-extern void do_all_continuations (int error);
+extern void add_continuation (void (*)(void *), void *,
+ void (*)(void *));
+extern void do_all_continuations (void);
extern void discard_all_continuations (void);
-extern void add_intermediate_continuation (void (*)(void *, int), void *);
-extern void do_all_intermediate_continuations (int error);
+extern void add_intermediate_continuation (void (*)(void *), void *,
+ void (*)(void *));
+extern void do_all_intermediate_continuations (void);
extern void discard_all_intermediate_continuations (void);
/* String containing the current directory (what getwd would return). */
Index: src/gdb/utils.c
===================================================================
--- src.orig/gdb/utils.c 2008-07-12 20:08:49.000000000 +0100
+++ src/gdb/utils.c 2008-07-12 20:12:41.000000000 +0100
@@ -473,16 +473,16 @@ null_cleanup (void *arg)
/* Add a continuation to the continuation list, the global list
cmd_continuation. The new continuation will be added at the front.*/
void
-add_continuation (void (*continuation_hook) (void *, int), void *args)
+add_continuation (void (*continuation_hook) (void *), void *args,
+ void (*continuation_free_args) (void *))
{
- struct continuation *continuation_ptr;
+ struct cleanup **as_cleanup_p = (struct cleanup **) &cmd_continuation;
+ make_cleanup_ftype *continuation_hook_fn = continuation_hook;
- continuation_ptr =
- (struct continuation *) xmalloc (sizeof (struct continuation));
- continuation_ptr->continuation_hook = continuation_hook;
- continuation_ptr->args = args;
- continuation_ptr->next = cmd_continuation;
- cmd_continuation = continuation_ptr;
+ make_my_cleanup2 (as_cleanup_p,
+ continuation_hook_fn,
+ args,
+ continuation_free_args);
}
/* Walk down the cmd_continuation list, and execute all the
@@ -494,26 +494,20 @@ add_continuation (void (*continuation_ho
and do the continuations from there on, instead of using the
global beginning of list as our iteration pointer. */
void
-do_all_continuations (int error)
+do_all_continuations (void)
{
- struct continuation *continuation_ptr;
- struct continuation *saved_continuation;
+ struct cleanup *continuation_ptr;
/* Copy the list header into another pointer, and set the global
list header to null, so that the global list can change as a side
- effect of invoking the continuations and the processing of
- the preexisting continuations will not be affected. */
- continuation_ptr = cmd_continuation;
+ effect of invoking the continuations and the processing of the
+ preexisting continuations will not be affected. */
+
+ continuation_ptr = (struct cleanup *) cmd_continuation;
cmd_continuation = NULL;
/* Work now on the list we have set aside. */
- while (continuation_ptr)
- {
- (continuation_ptr->continuation_hook) (continuation_ptr->args, error);
- saved_continuation = continuation_ptr;
- continuation_ptr = continuation_ptr->next;
- xfree (saved_continuation);
- }
+ do_my_cleanups (&continuation_ptr, NULL);
}
/* Walk down the cmd_continuation list, and get rid of all the
@@ -521,14 +515,8 @@ do_all_continuations (int error)
void
discard_all_continuations (void)
{
- struct continuation *continuation_ptr;
-
- while (cmd_continuation)
- {
- continuation_ptr = cmd_continuation;
- cmd_continuation = continuation_ptr->next;
- xfree (continuation_ptr);
- }
+ struct cleanup **continuation_ptr = (struct cleanup **) &cmd_continuation;
+ discard_my_cleanups (continuation_ptr, NULL);
}
/* Add a continuation to the continuation list, the global list
@@ -536,16 +524,16 @@ discard_all_continuations (void)
the front. */
void
add_intermediate_continuation (void (*continuation_hook)
- (void *, int), void *args)
+ (void *), void *args,
+ void (*continuation_free_args) (void *))
{
- struct continuation *continuation_ptr;
+ struct cleanup **as_cleanup_p = (struct cleanup **) &intermediate_continuation;
+ make_cleanup_ftype *continuation_hook_fn = continuation_hook;
- continuation_ptr =
- (struct continuation *) xmalloc (sizeof (struct continuation));
- continuation_ptr->continuation_hook = continuation_hook;
- continuation_ptr->args = args;
- continuation_ptr->next = intermediate_continuation;
- intermediate_continuation = continuation_ptr;
+ make_my_cleanup2 (as_cleanup_p,
+ continuation_hook_fn,
+ args,
+ continuation_free_args);
}
/* Walk down the cmd_continuation list, and execute all the
@@ -557,26 +545,20 @@ add_intermediate_continuation (void (*co
and do the continuations from there on, instead of using the
global beginning of list as our iteration pointer.*/
void
-do_all_intermediate_continuations (int error)
+do_all_intermediate_continuations (void)
{
- struct continuation *continuation_ptr;
- struct continuation *saved_continuation;
+ struct cleanup *continuation_ptr;
/* Copy the list header into another pointer, and set the global
list header to null, so that the global list can change as a side
- effect of invoking the continuations and the processing of
- the preexisting continuations will not be affected. */
- continuation_ptr = intermediate_continuation;
+ effect of invoking the continuations and the processing of the
+ preexisting continuations will not be affected. */
+
+ continuation_ptr = (struct cleanup *) intermediate_continuation;
intermediate_continuation = NULL;
/* Work now on the list we have set aside. */
- while (continuation_ptr)
- {
- (continuation_ptr->continuation_hook) (continuation_ptr->args, error);
- saved_continuation = continuation_ptr;
- continuation_ptr = continuation_ptr->next;
- xfree (saved_continuation);
- }
+ do_my_cleanups (&continuation_ptr, NULL);
}
/* Walk down the cmd_continuation list, and get rid of all the
@@ -584,14 +566,9 @@ do_all_intermediate_continuations (int e
void
discard_all_intermediate_continuations (void)
{
- struct continuation *continuation_ptr;
-
- while (intermediate_continuation)
- {
- continuation_ptr = intermediate_continuation;
- intermediate_continuation = continuation_ptr->next;
- xfree (continuation_ptr);
- }
+ struct cleanup **continuation_ptr
+ = (struct cleanup **) &intermediate_continuation;
+ discard_my_cleanups (continuation_ptr, NULL);
}
Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c 2008-07-12 20:08:48.000000000 +0100
+++ src/gdb/breakpoint.c 2008-07-12 20:08:56.000000000 +0100
@@ -62,8 +62,6 @@
/* Prototypes for local functions. */
-static void until_break_command_continuation (void *arg, int error);
-
static void catch_command_1 (char *, int, int);
static void enable_delete_command (char *, int);
@@ -6161,7 +6159,7 @@ struct until_break_command_continuation_
care of cleaning up the temporary breakpoints set up by the until
command. */
static void
-until_break_command_continuation (void *arg, int error)
+until_break_command_continuation (void *arg)
{
struct until_break_command_continuation_args *a = arg;
@@ -6243,7 +6241,8 @@ until_break_command (char *arg, int from
args->breakpoint2 = breakpoint2;
discard_cleanups (old_chain);
- add_continuation (until_break_command_continuation, args);
+ add_continuation (until_break_command_continuation, args,
+ xfree);
}
else
do_cleanups (old_chain);
Index: src/gdb/inf-loop.c
===================================================================
--- src.orig/gdb/inf-loop.c 2008-07-12 20:02:57.000000000 +0100
+++ src/gdb/inf-loop.c 2008-07-12 20:08:56.000000000 +0100
@@ -52,7 +52,8 @@ inferior_event_handler (enum inferior_ev
printf_unfiltered (_("error detected from target.\n"));
target_async (NULL, 0);
pop_target ();
- do_all_continuations (1);
+ discard_all_intermediate_continuations ();
+ discard_all_continuations ();
async_enable_stdin ();
break;
@@ -66,7 +67,8 @@ inferior_event_handler (enum inferior_ev
{
target_async (NULL, 0);
pop_target ();
- do_all_continuations (1);
+ discard_all_intermediate_continuations ();
+ discard_all_continuations ();
async_enable_stdin ();
display_gdb_prompt (0);
}
@@ -96,13 +98,13 @@ inferior_event_handler (enum inferior_ev
touch the inferior memory, e.g. to remove breakpoints, so run
them before running breakpoint commands, which may resume the
target. */
- do_all_intermediate_continuations (0);
+ do_all_intermediate_continuations ();
/* Always finish the previous command before running any
breakpoint commands. Any stop cancels the previous command.
E.g. a "finish" or "step-n" command interrupted by an
unrelated breakpoint is canceled. */
- do_all_continuations (0);
+ do_all_continuations ();
if (current_language != expected_language
&& language_mode == language_mode_auto)
@@ -123,7 +125,7 @@ inferior_event_handler (enum inferior_ev
case INF_EXEC_CONTINUE:
/* Is there anything left to do for the command issued to
complete? */
- do_all_intermediate_continuations (0);
+ do_all_intermediate_continuations ();
break;
case INF_QUIT_REQ:
Index: src/gdb/infcmd.c
===================================================================
--- src.orig/gdb/infcmd.c 2008-07-12 20:08:48.000000000 +0100
+++ src/gdb/infcmd.c 2008-07-12 20:10:20.000000000 +0100
@@ -73,8 +73,6 @@ static void nofp_registers_info (char *,
static void print_return_value (struct type *func_type,
struct type *value_type);
-static void finish_command_continuation (void *args, int error_p);
-
static void until_next_command (int);
static void until_command (char *, int);
@@ -107,7 +105,6 @@ static void jump_command (char *, int);
static void step_1 (int, int, char *);
static void step_once (int skip_subroutines, int single_inst, int count, int thread);
-static void step_1_continuation (void *args, int error_p);
static void next_command (char *, int);
@@ -877,15 +874,14 @@ struct step_1_continuation_args
proceed(), via step_once(). Basically it is like step_once and
step_1_continuation are co-recursive. */
static void
-step_1_continuation (void *args, int error_p)
+step_1_continuation (void *args)
{
struct step_1_continuation_args *a = args;
- if (error_p || !step_multi || !stop_step)
+ if (!step_multi || !stop_step)
{
- /* We either hit an error, or stopped for some reason
- that is not stepping, or there are no further steps
- to make. Cleanup. */
+ /* If we stopped for some reason that is not stepping there are
+ no further steps to make. Cleanup. */
if (!a->single_inst || a->skip_subroutines)
delete_longjmp_breakpoint (a->thread);
step_multi = 0;
@@ -960,7 +956,7 @@ which has no line number information.\n"
args->single_inst = single_inst;
args->count = count;
args->thread = thread;
- add_intermediate_continuation (step_1_continuation, args);
+ add_intermediate_continuation (step_1_continuation, args, xfree);
}
}
@@ -1325,35 +1321,44 @@ struct finish_command_continuation_args
};
static void
-finish_command_continuation (void *arg, int error_p)
+finish_command_continuation (void *arg)
{
struct finish_command_continuation_args *a = arg;
- if (!error_p)
+ if (bpstat_find_breakpoint (stop_bpstat, a->breakpoint) != NULL
+ && a->function != NULL)
{
- if (bpstat_find_breakpoint (stop_bpstat, a->breakpoint) != NULL
- && a->function != NULL)
- {
- struct type *value_type;
-
- value_type = TYPE_TARGET_TYPE (SYMBOL_TYPE (a->function));
- if (!value_type)
- internal_error (__FILE__, __LINE__,
- _("finish_command: function has no target type"));
-
- if (TYPE_CODE (value_type) != TYPE_CODE_VOID)
- print_return_value (SYMBOL_TYPE (a->function), value_type);
- }
-
- /* We suppress normal call of normal_stop observer and do it here so that
- that *stopped notification includes the return value. */
- observer_notify_normal_stop (stop_bpstat);
- }
+ struct type *value_type;
+ value_type = TYPE_TARGET_TYPE (SYMBOL_TYPE (a->function));
+ if (!value_type)
+ internal_error (__FILE__, __LINE__,
+ _("finish_command: function has no target type"));
+
+ if (TYPE_CODE (value_type) != TYPE_CODE_VOID)
+ print_return_value (SYMBOL_TYPE (a->function), value_type);
+ }
+
+ /* We suppress normal call of normal_stop observer and do it here so
+ that that *stopped notification includes the return value. */
+ /* NOTE: This is broken in non-stop mode. There is no guarantee the
+ next stop will be in the same thread that we started doing a
+ finish on. This suppressing (or some other replacement means)
+ should be a thread property. */
+ observer_notify_normal_stop (stop_bpstat);
suppress_stop_observer = 0;
delete_breakpoint (a->breakpoint);
}
+static void
+finish_command_continuation_free_arg (void *arg)
+{
+ /* NOTE: See finish_command_continuation. This would go away, if
+ this suppressing is made a thread property. */
+ suppress_stop_observer = 0;
+ xfree (arg);
+}
+
/* "finish": Set a temporary breakpoint at the place the selected
frame will return to, then continue. */
@@ -1425,11 +1430,12 @@ finish_command (char *arg, int from_tty)
cargs->breakpoint = breakpoint;
cargs->function = function;
- add_continuation (finish_command_continuation, cargs);
+ add_continuation (finish_command_continuation, cargs,
+ finish_command_continuation_free_arg);
discard_cleanups (old_chain);
if (!target_can_async_p ())
- do_all_continuations (0);
+ do_all_continuations ();
}
@@ -1982,12 +1988,20 @@ struct attach_command_continuation_args
};
static void
-attach_command_continuation (void *args, int error_p)
+attach_command_continuation (void *args)
{
struct attach_command_continuation_args *a = args;
attach_command_post_wait (a->args, a->from_tty, a->async_exec);
}
+static void
+attach_command_continuation_free_args (void *args)
+{
+ struct attach_command_continuation_args *a = args;
+ xfree (a->args);
+ xfree (a);
+}
+
void
attach_command (char *args, int from_tty)
{
@@ -2076,7 +2090,8 @@ attach_command (char *args, int from_tty
a->args = xstrdup (args);
a->from_tty = from_tty;
a->async_exec = async_exec;
- add_continuation (attach_command_continuation, a);
+ add_continuation (attach_command_continuation, a,
+ attach_command_continuation_free_args);
return;
}
Index: src/gdb/interps.c
===================================================================
--- src.orig/gdb/interps.c 2008-07-11 22:19:10.000000000 +0100
+++ src/gdb/interps.c 2008-07-12 20:08:56.000000000 +0100
@@ -148,7 +148,7 @@ interp_set (struct interp *interp, int t
if (current_interpreter != NULL)
{
- do_all_continuations (0);
+ do_all_continuations ();
ui_out_flush (uiout);
if (current_interpreter->procs->suspend_proc
&& !current_interpreter->procs->suspend_proc (current_interpreter->
Index: src/gdb/event-top.c
===================================================================
--- src.orig/gdb/event-top.c 2008-07-11 22:19:10.000000000 +0100
+++ src/gdb/event-top.c 2008-07-12 20:08:56.000000000 +0100
@@ -425,6 +425,7 @@ stdin_event_handler (int error, gdb_clie
printf_unfiltered (_("error detected on stdin\n"));
delete_file_handler (input_fd);
discard_all_continuations ();
+ discard_all_intermediate_continuations ();
/* If stdin died, we may as well kill gdb. */
quit_command ((char *) 0, stdin == instream);
}