* [PATCH 4/6] infcall: stop_registers -> register_dummy_frame_dtor
2015-05-08 20:21 [PATCH 1/6] dummy_frame_dtor_ftype vs. call_function_by_hand_dummy_dtor_ftype cleanup Jan Kratochvil
@ 2015-05-08 20:21 ` Jan Kratochvil
2015-05-13 13:28 ` Pedro Alves
2015-05-08 20:21 ` [PATCH 3/6] register_dummy_frame_dtor: Permit multiple dtors Jan Kratochvil
` (4 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Jan Kratochvil @ 2015-05-08 20:21 UTC (permalink / raw)
To: gdb-patches
Hi,
with dummy_frame destructors GDB no longer has to use global stop_registers.
dummy_frame's registers can be now stored associated with their specific
dummy_frame.
stop_registers was questioned by Pedro in:
Re: [PATCH v4 11/11] RFC only: compile: Use also inferior munmap
https://sourceware.org/ml/gdb-patches/2015-05/msg00107.html
I am not completely sure what is right for bpfinishpy_pre_stop_hook but the
testsuite passes. I can think more about it if it gets otherwise approved.
Jan
gdb/ChangeLog
2015-05-08 Jan Kratochvil <jan.kratochvil@redhat.com>
* infcall.c (struct call_function_by_hand_dtor)
(call_function_by_hand_dtor_data_free, call_function_by_hand_dtor)
(call_function_by_hand_dtor_drop, call_function_by_hand_dtor_cleanup)
(call_function_by_hand_dtor_get, call_function_by_hand_dtor_setup): New.
(call_function_by_hand_dummy): Move discard_cleanups of
inf_status_cleanup before dummy_frame_push. Call
call_function_by_hand_dtor_setup and prepare our_dtor_cleanup.
Use call_function_by_hand_dtor_get instead of stop_registers.
* infcall.h (struct call_function_by_hand_dtor)
(call_function_by_hand_dtor_drop, call_function_by_hand_dtor_cleanup)
(call_function_by_hand_dtor_get, call_function_by_hand_dtor_setup): New
declarations.
* infcmd.c: Include infcall.h.
(get_return_value): Add parameter dtor_data, use it instead of
stop_registers.
(print_return_value): Add parameter dtor_data, pass it.
(struct finish_command_continuation_args): Add field dtor_data.
(finish_command_continuation): Update print_return_value caller.
(finish_command_continuation_free_arg): Free also dtor_data.
(finish_forward): Call call_function_by_hand_dtor_setup.
* inferior.h (struct call_function_by_hand_dtor): New declaration.
(get_return_value): Add parameter dtor_data.
* python/py-finishbreakpoint.c (bpfinishpy_pre_stop_hook): Update
get_return_value caller.
---
gdb/infcall.c | 118 ++++++++++++++++++++++++++++++++++----
gdb/infcall.h | 10 +++
gdb/infcmd.c | 48 ++++++++++++---
gdb/inferior.h | 6 +-
gdb/python/py-finishbreakpoint.c | 2 -
5 files changed, 159 insertions(+), 25 deletions(-)
diff --git a/gdb/infcall.c b/gdb/infcall.c
index cef6b91..9707861 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -469,6 +469,97 @@ call_function_by_hand (struct value *function, int nargs, struct value **args)
return call_function_by_hand_dummy (function, nargs, args, NULL, NULL);
}
+/* Data for call_function_by_hand_dtor. Structure can be freed only
+ after both call_function_by_hand_dtor and
+ call_function_by_hand_dtor_drop have been called for it. */
+
+struct call_function_by_hand_dtor
+{
+ /* Inferior registers fetched before associated dummy_frame got freed
+ and before any other destructors of associated dummy_frame got called.
+ It is initialized to NULL. */
+ struct regcache *retbuf;
+
+ /* It is 1 if this call_function_by_hand_dtor_drop has been already
+ called. */
+ int drop_done;
+};
+
+/* Free struct call_function_by_hand_dtor. */
+
+static void
+call_function_by_hand_dtor_data_free (struct call_function_by_hand_dtor *data)
+{
+ regcache_xfree (data->retbuf);
+ xfree (data);
+}
+
+/* Destructor for associated dummy_frame. */
+
+static dummy_frame_dtor_ftype call_function_by_hand_dtor;
+static void
+call_function_by_hand_dtor (void *data_voidp, int registers_valid)
+{
+ struct call_function_by_hand_dtor *data = data_voidp;
+
+ gdb_assert (data->retbuf == NULL);
+
+ if (data->drop_done)
+ call_function_by_hand_dtor_data_free (data);
+ else if (registers_valid)
+ data->retbuf = regcache_dup (get_current_regcache ());
+}
+
+/* Caller is no longer interested in this
+ struct call_function_by_hand_dtor. After its associated dummy_frame
+ gets freed struct call_function_by_hand_dtor can be also freed. */
+
+void
+call_function_by_hand_dtor_drop (struct call_function_by_hand_dtor *data)
+{
+ data->drop_done = 1;
+
+ if (!find_dummy_frame_dtor (call_function_by_hand_dtor, data))
+ call_function_by_hand_dtor_data_free (data);
+}
+
+/* Stub for call_function_by_hand_dtor_drop compatible with make_cleanup. */
+
+void
+call_function_by_hand_dtor_cleanup (void *data_voidp)
+{
+ struct call_function_by_hand_dtor *data = data_voidp;
+
+ call_function_by_hand_dtor_drop (data);
+}
+
+/* Fetch RETBUF field of possibly opaque DTOR_DATA.
+ RETBUF must not be NULL. */
+
+struct regcache *
+call_function_by_hand_dtor_get (struct call_function_by_hand_dtor *dtor_data)
+{
+ gdb_assert (dtor_data->retbuf != NULL);
+ return dtor_data->retbuf;
+}
+
+/* Register provider of inferior registers at the time DUMMY_ID frame of
+ PTID gets freed (before inferior registers get restored to those
+ before dummy_frame). */
+
+struct call_function_by_hand_dtor *
+call_function_by_hand_dtor_setup (struct frame_id dummy_id, ptid_t ptid)
+{
+ struct call_function_by_hand_dtor *our_dtor_data;
+
+ our_dtor_data = xmalloc (sizeof (*our_dtor_data));
+ our_dtor_data->retbuf = NULL;
+ our_dtor_data->drop_done = 0;
+ register_dummy_frame_dtor (dummy_id, inferior_ptid,
+ call_function_by_hand_dtor, our_dtor_data);
+ return our_dtor_data;
+}
+
/* All this stuff with a dummy frame may seem unnecessarily complicated
(why not just save registers in GDB?). The purpose of pushing a dummy
frame which looks just like a real frame is so that if you call a
@@ -513,6 +604,8 @@ call_function_by_hand_dummy (struct value *function,
struct gdb_exception e;
char name_buf[RAW_FUNCTION_ADDRESS_SIZE];
int stack_temporaries = thread_stack_temporaries_enabled_p (inferior_ptid);
+ struct call_function_by_hand_dtor *our_dtor_data;
+ struct cleanup *our_dtor_cleanup;
if (TYPE_CODE (ftype) == TYPE_CODE_PTR)
ftype = check_typedef (TYPE_TARGET_TYPE (ftype));
@@ -886,6 +979,11 @@ call_function_by_hand_dummy (struct value *function,
if (unwind_on_terminating_exception_p)
set_std_terminate_breakpoint ();
+ /* Discard both inf_status and caller_state cleanups.
+ From this point on we explicitly restore the associated state
+ or discard it. */
+ discard_cleanups (inf_status_cleanup);
+
/* Everything's ready, push all the info needed to restore the
caller (and identify the dummy-frame) onto the dummy-frame
stack. */
@@ -894,10 +992,12 @@ call_function_by_hand_dummy (struct value *function,
register_dummy_frame_dtor (dummy_id, inferior_ptid,
dummy_dtor, dummy_dtor_data);
- /* Discard both inf_status and caller_state cleanups.
- From this point on we explicitly restore the associated state
- or discard it. */
- discard_cleanups (inf_status_cleanup);
+ /* call_function_by_hand_dtor_setup should be called last so that its
+ saving of inferior registers gets called first (before possible
+ DUMMY_DTOR destructor). */
+ our_dtor_data = call_function_by_hand_dtor_setup (dummy_id, inferior_ptid);
+ our_dtor_cleanup = make_cleanup (call_function_by_hand_dtor_cleanup,
+ our_dtor_data);
/* Register a clean-up for unwind_on_terminating_exception_breakpoint. */
terminate_bp_cleanup = make_cleanup (cleanup_delete_std_terminate_breakpoint,
@@ -1112,13 +1212,8 @@ When the function is done executing, GDB will silently stop."),
and the dummy frame has already been popped. */
{
- struct address_space *aspace = get_regcache_aspace (stop_registers);
- struct regcache *retbuf = regcache_xmalloc (gdbarch, aspace);
- struct cleanup *retbuf_cleanup = make_cleanup_regcache_xfree (retbuf);
struct value *retval = NULL;
- regcache_cpy_no_passthrough (retbuf, stop_registers);
-
/* Inferior call is successful. Restore the inferior status.
At this stage, leave the RETBUF alone. */
restore_infcall_control_state (inf_status);
@@ -1145,7 +1240,8 @@ When the function is done executing, GDB will silently stop."),
{
retval = allocate_value (values_type);
gdbarch_return_value (gdbarch, function, values_type,
- retbuf, value_contents_raw (retval), NULL);
+ call_function_by_hand_dtor_get (our_dtor_data),
+ value_contents_raw (retval), NULL);
if (stack_temporaries && class_or_union_p (values_type))
{
/* Values of class type returned in registers are copied onto
@@ -1160,7 +1256,7 @@ When the function is done executing, GDB will silently stop."),
}
}
- do_cleanups (retbuf_cleanup);
+ do_cleanups (our_dtor_cleanup);
gdb_assert (retval);
return retval;
diff --git a/gdb/infcall.h b/gdb/infcall.h
index 77c5101..9ec8617 100644
--- a/gdb/infcall.h
+++ b/gdb/infcall.h
@@ -50,4 +50,14 @@ extern struct value *
dummy_frame_dtor_ftype *dummy_dtor,
void *dummy_dtor_data);
+struct call_function_by_hand_dtor;
+extern void call_function_by_hand_dtor_drop
+ (struct call_function_by_hand_dtor *data);
+extern void call_function_by_hand_dtor_cleanup
+ (void *data_voidp);
+extern struct regcache *call_function_by_hand_dtor_get
+ (struct call_function_by_hand_dtor *dtor_data);
+extern struct call_function_by_hand_dtor *call_function_by_hand_dtor_setup
+ (struct frame_id dummy_id, ptid_t ptid);
+
#endif
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 11d5310..f8874d3 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -54,6 +54,7 @@
#include "continuations.h"
#include "linespec.h"
#include "cli/cli-utils.h"
+#include "infcall.h"
/* Local functions: */
@@ -1506,18 +1507,22 @@ advance_command (char *arg, int from_tty)
}
\f
/* Return the value of the result of a function at the end of a 'finish'
- command/BP. */
+ command/BP. DTOR_DATA (if not NULL) can represent inferior registers
+ right after an inferior call has finished. */
struct value *
-get_return_value (struct value *function, struct type *value_type)
+get_return_value (struct value *function, struct type *value_type,
+ struct call_function_by_hand_dtor *dtor_data)
{
- struct regcache *stop_regs = stop_registers;
+ struct regcache *stop_regs = NULL;
struct gdbarch *gdbarch;
struct value *value;
struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
/* If stop_registers were not saved, use the current registers. */
- if (!stop_regs)
+ if (dtor_data != NULL)
+ stop_regs = call_function_by_hand_dtor_get (dtor_data);
+ else
{
stop_regs = regcache_dup (get_current_regcache ());
make_cleanup_regcache_xfree (stop_regs);
@@ -1557,12 +1562,15 @@ get_return_value (struct value *function, struct type *value_type)
return value;
}
-/* Print the result of a function at the end of a 'finish' command. */
+/* Print the result of a function at the end of a 'finish' command.
+ DTOR_DATA (if not NULL) can represent inferior registers right after
+ an inferior call has finished. */
static void
-print_return_value (struct value *function, struct type *value_type)
+print_return_value (struct value *function, struct type *value_type,
+ struct call_function_by_hand_dtor *dtor_data)
{
- struct value *value = get_return_value (function, value_type);
+ struct value *value = get_return_value (function, value_type, dtor_data);
struct ui_out *uiout = current_uiout;
if (value)
@@ -1613,6 +1621,11 @@ struct finish_command_continuation_args
int thread;
struct breakpoint *breakpoint;
struct symbol *function;
+
+ /* Inferior registers stored right before dummy_frame has been freed
+ after an inferior call. It can be NULL if no inferior call was
+ involved, GDB will then use current inferior registers. */
+ struct call_function_by_hand_dtor *dtor_data;
};
static void
@@ -1653,7 +1666,7 @@ finish_command_continuation (void *arg, int err)
/* print_return_value can throw an exception in some
circumstances. We need to catch this so that we still
delete the breakpoint. */
- print_return_value (func, value_type);
+ print_return_value (func, value_type, a->dtor_data);
}
CATCH (ex, RETURN_MASK_ALL)
{
@@ -1677,7 +1690,11 @@ finish_command_continuation (void *arg, int err)
static void
finish_command_continuation_free_arg (void *arg)
{
- xfree (arg);
+ struct finish_command_continuation_args *cargs = arg;
+
+ if (cargs->dtor_data != NULL)
+ call_function_by_hand_dtor_drop (cargs->dtor_data);
+ xfree (cargs);
}
/* finish_backward -- helper function for finish_command. */
@@ -1742,13 +1759,21 @@ finish_forward (struct symbol *function, struct frame_info *frame)
struct symtab_and_line sal;
struct thread_info *tp = inferior_thread ();
struct breakpoint *breakpoint;
- struct cleanup *old_chain;
+ struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
struct finish_command_continuation_args *cargs;
int thread = tp->num;
+ struct call_function_by_hand_dtor *dtor_data = NULL;
sal = find_pc_line (get_frame_pc (frame), 0);
sal.pc = get_frame_pc (frame);
+ if (get_frame_type (frame) == DUMMY_FRAME)
+ {
+ dtor_data = call_function_by_hand_dtor_setup (get_stack_frame_id (frame),
+ inferior_ptid);
+ make_cleanup (call_function_by_hand_dtor_cleanup, dtor_data);
+ }
+
breakpoint = set_momentary_breakpoint (gdbarch, sal,
get_stack_frame_id (frame),
bp_finish);
@@ -1756,7 +1781,7 @@ finish_forward (struct symbol *function, struct frame_info *frame)
/* set_momentary_breakpoint invalidates FRAME. */
frame = NULL;
- old_chain = make_cleanup_delete_breakpoint (breakpoint);
+ make_cleanup_delete_breakpoint (breakpoint);
set_longjmp_breakpoint (tp, frame_id);
make_cleanup (delete_longjmp_breakpoint_cleanup, &thread);
@@ -1768,6 +1793,7 @@ finish_forward (struct symbol *function, struct frame_info *frame)
cargs->thread = thread;
cargs->breakpoint = breakpoint;
cargs->function = function;
+ cargs->dtor_data = dtor_data;
add_continuation (tp, finish_command_continuation, cargs,
finish_command_continuation_free_arg);
proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 2530777..f305e79 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -165,8 +165,10 @@ extern void detach_command (char *, int);
extern void notice_new_inferior (ptid_t, int, int);
-extern struct value *get_return_value (struct value *function,
- struct type *value_type);
+struct call_function_by_hand_dtor;
+extern struct value *get_return_value
+ (struct value *function, struct type *value_type,
+ struct call_function_by_hand_dtor *dtor_data);
/* Prepare for execution command. TARGET is the target that will run
the command. BACKGROUND determines whether this is a foreground
diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
index 34e9643..098d2ee 100644
--- a/gdb/python/py-finishbreakpoint.c
+++ b/gdb/python/py-finishbreakpoint.c
@@ -106,7 +106,7 @@ bpfinishpy_pre_stop_hook (struct gdbpy_breakpoint_object *bp_obj)
value_object_to_value (self_finishbp->function_value);
struct type *value_type =
type_object_to_type (self_finishbp->return_type);
- struct value *ret = get_return_value (function, value_type);
+ struct value *ret = get_return_value (function, value_type, NULL);
if (ret)
{
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 4/6] infcall: stop_registers -> register_dummy_frame_dtor
2015-05-08 20:21 ` [PATCH 4/6] infcall: stop_registers -> register_dummy_frame_dtor Jan Kratochvil
@ 2015-05-13 13:28 ` Pedro Alves
2015-05-13 18:54 ` [commit] " Jan Kratochvil
0 siblings, 1 reply; 19+ messages in thread
From: Pedro Alves @ 2015-05-13 13:28 UTC (permalink / raw)
To: Jan Kratochvil, gdb-patches
On 05/08/2015 09:21 PM, Jan Kratochvil wrote:
> Hi,
>
> with dummy_frame destructors GDB no longer has to use global stop_registers.
> dummy_frame's registers can be now stored associated with their specific
> dummy_frame.
>
> stop_registers was questioned by Pedro in:
> Re: [PATCH v4 11/11] RFC only: compile: Use also inferior munmap
> https://sourceware.org/ml/gdb-patches/2015-05/msg00107.html
>
> I am not completely sure what is right for bpfinishpy_pre_stop_hook but the
> testsuite passes. I can think more about it if it gets otherwise approved.
Yeah, if the Python API around that gives access the return value,
then something needs to be done there, otherwise, probably nothing else
is needed.
> +/* Free struct call_function_by_hand_dtor. */
> +
> +static void
> +call_function_by_hand_dtor_data_free (struct call_function_by_hand_dtor *data)
> +{
> + regcache_xfree (data->retbuf);
> + xfree (data);
> +}
> +
> +/* Destructor for associated dummy_frame. */
> +
> +static dummy_frame_dtor_ftype call_function_by_hand_dtor;
This forward declaration is not really necessary, right?
A wrong prototype mismismatch would be caught here:
if (!find_dummy_frame_dtor (call_function_by_hand_dtor, data))
for example.
>
> +static void
> +call_function_by_hand_dtor (void *data_voidp, int registers_valid)
> +{
> + struct call_function_by_hand_dtor *data = data_voidp;
> +
> + gdb_assert (data->retbuf == NULL);
> +
> + if (data->drop_done)
> + call_function_by_hand_dtor_data_free (data);
> + else if (registers_valid)
> + data->retbuf = regcache_dup (get_current_regcache ());
> +}
OK, I know why it took me a while to understand all this. I think
it was due to the function/symbol naming chosen.
call_function_by_hand_dtor is too easy to confuse with the
dummy frame destructor function itself, and with the
"call_function_by_hand" function itself, instead of the
dummy frame dtor. It doesn't really hint to me what this is all
doing, which is saving the context of the dummy frame, before that
frame is destroyed.
How about we rename things, to something like in the patch below?
Once I did that locally, things were much clearer to me.
I can push through with the rename afterwards if it makes it
easier for you.
Otherwise this looks good to me.
Thanks for doing this.
From 9f11bb8611cd86f07a5f54aacf1594ee4227178c Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 13 May 2015 12:38:11 +0100
Subject: [PATCH] Rename call_function_by_hand_dtor -> dummy_frame_context_saver
---
gdb/infcall.c | 83 +++++++++++++++++++++++++++++-----------------------------
gdb/infcall.h | 15 +++++------
gdb/infcmd.c | 28 ++++++++++----------
gdb/inferior.h | 4 +--
4 files changed, 64 insertions(+), 66 deletions(-)
diff --git a/gdb/infcall.c b/gdb/infcall.c
index 849f500..5dd908d 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -469,95 +469,94 @@ call_function_by_hand (struct value *function, int nargs, struct value **args)
return call_function_by_hand_dummy (function, nargs, args, NULL, NULL);
}
-/* Data for call_function_by_hand_dtor. Structure can be freed only
- after both call_function_by_hand_dtor and
- call_function_by_hand_dtor_drop have been called for it. */
+/* Data for dummy_frame_context_saver. Structure can be freed only
+ after both dummy_frame_context_saver_dtor and
+ dummy_frame_context_saver_drop have been called for it. */
-struct call_function_by_hand_dtor
+struct dummy_frame_context_saver
{
/* Inferior registers fetched before associated dummy_frame got freed
and before any other destructors of associated dummy_frame got called.
It is initialized to NULL. */
struct regcache *retbuf;
- /* It is 1 if this call_function_by_hand_dtor_drop has been already
+ /* It is 1 if this dummy_frame_context_saver_drop has been already
called. */
int drop_done;
};
-/* Free struct call_function_by_hand_dtor. */
+/* Free struct dummy_frame_context_saver. */
static void
-call_function_by_hand_dtor_data_free (struct call_function_by_hand_dtor *data)
+dummy_frame_context_saver_free (struct dummy_frame_context_saver *saver)
{
- regcache_xfree (data->retbuf);
- xfree (data);
+ regcache_xfree (saver->retbuf);
+ xfree (saver);
}
/* Destructor for associated dummy_frame. */
-static dummy_frame_dtor_ftype call_function_by_hand_dtor;
static void
-call_function_by_hand_dtor (void *data_voidp, int registers_valid)
+dummy_frame_context_saver_dtor (void *data_voidp, int registers_valid)
{
- struct call_function_by_hand_dtor *data = data_voidp;
+ struct dummy_frame_context_saver *data = data_voidp;
gdb_assert (data->retbuf == NULL);
if (data->drop_done)
- call_function_by_hand_dtor_data_free (data);
+ dummy_frame_context_saver_free (data);
else if (registers_valid)
data->retbuf = regcache_dup (get_current_regcache ());
}
/* Caller is no longer interested in this
- struct call_function_by_hand_dtor. After its associated dummy_frame
- gets freed struct call_function_by_hand_dtor can be also freed. */
+ struct dummy_frame_context_saver. After its associated dummy_frame
+ gets freed struct dummy_frame_context_saver can be also freed. */
void
-call_function_by_hand_dtor_drop (struct call_function_by_hand_dtor *data)
+dummy_frame_context_saver_drop (struct dummy_frame_context_saver *saver)
{
- data->drop_done = 1;
+ saver->drop_done = 1;
- if (!find_dummy_frame_dtor (call_function_by_hand_dtor, data))
- call_function_by_hand_dtor_data_free (data);
+ if (!find_dummy_frame_dtor (dummy_frame_context_saver_dtor, saver))
+ dummy_frame_context_saver_free (saver);
}
-/* Stub for call_function_by_hand_dtor_drop compatible with make_cleanup. */
+/* Stub dummy_frame_context_saver_drop compatible with make_cleanup. */
void
-call_function_by_hand_dtor_cleanup (void *data_voidp)
+dummy_frame_context_saver_cleanup (void *data)
{
- struct call_function_by_hand_dtor *data = data_voidp;
+ struct dummy_frame_context_saver *saver = data;
- call_function_by_hand_dtor_drop (data);
+ dummy_frame_context_saver_drop (saver);
}
/* Fetch RETBUF field of possibly opaque DTOR_DATA.
RETBUF must not be NULL. */
struct regcache *
-call_function_by_hand_dtor_get (struct call_function_by_hand_dtor *dtor_data)
+dummy_frame_context_saver_get_regs (struct dummy_frame_context_saver *saver)
{
- gdb_assert (dtor_data->retbuf != NULL);
- return dtor_data->retbuf;
+ gdb_assert (saver->retbuf != NULL);
+ return saver->retbuf;
}
/* Register provider of inferior registers at the time DUMMY_ID frame of
PTID gets freed (before inferior registers get restored to those
before dummy_frame). */
-struct call_function_by_hand_dtor *
-call_function_by_hand_dtor_setup (struct frame_id dummy_id, ptid_t ptid)
+struct dummy_frame_context_saver *
+dummy_frame_context_saver_setup (struct frame_id dummy_id, ptid_t ptid)
{
- struct call_function_by_hand_dtor *our_dtor_data;
+ struct dummy_frame_context_saver *saver;
- our_dtor_data = xmalloc (sizeof (*our_dtor_data));
- our_dtor_data->retbuf = NULL;
- our_dtor_data->drop_done = 0;
+ saver = xmalloc (sizeof (*saver));
+ saver->retbuf = NULL;
+ saver->drop_done = 0;
register_dummy_frame_dtor (dummy_id, inferior_ptid,
- call_function_by_hand_dtor, our_dtor_data);
- return our_dtor_data;
+ dummy_frame_context_saver_dtor, saver);
+ return saver;
}
/* All this stuff with a dummy frame may seem unnecessarily complicated
@@ -604,8 +603,8 @@ call_function_by_hand_dummy (struct value *function,
struct gdb_exception e;
char name_buf[RAW_FUNCTION_ADDRESS_SIZE];
int stack_temporaries = thread_stack_temporaries_enabled_p (inferior_ptid);
- struct call_function_by_hand_dtor *our_dtor_data;
- struct cleanup *our_dtor_cleanup;
+ struct dummy_frame_context_saver *context_saver;
+ struct cleanup *context_saver_cleanup;
if (TYPE_CODE (ftype) == TYPE_CODE_PTR)
ftype = check_typedef (TYPE_TARGET_TYPE (ftype));
@@ -992,12 +991,12 @@ call_function_by_hand_dummy (struct value *function,
register_dummy_frame_dtor (dummy_id, inferior_ptid,
dummy_dtor, dummy_dtor_data);
- /* call_function_by_hand_dtor_setup should be called last so that its
+ /* dummy_frame_context_saver_setup must be called last so that its
saving of inferior registers gets called first (before possible
DUMMY_DTOR destructor). */
- our_dtor_data = call_function_by_hand_dtor_setup (dummy_id, inferior_ptid);
- our_dtor_cleanup = make_cleanup (call_function_by_hand_dtor_cleanup,
- our_dtor_data);
+ context_saver = dummy_frame_context_saver_setup (dummy_id, inferior_ptid);
+ context_saver_cleanup = make_cleanup (dummy_frame_context_saver_cleanup,
+ context_saver);
/* Register a clean-up for unwind_on_terminating_exception_breakpoint. */
terminate_bp_cleanup = make_cleanup (cleanup_delete_std_terminate_breakpoint,
@@ -1240,7 +1239,7 @@ When the function is done executing, GDB will silently stop."),
{
retval = allocate_value (values_type);
gdbarch_return_value (gdbarch, function, values_type,
- call_function_by_hand_dtor_get (our_dtor_data),
+ dummy_frame_context_saver_get_regs (context_saver),
value_contents_raw (retval), NULL);
if (stack_temporaries && class_or_union_p (values_type))
{
@@ -1256,7 +1255,7 @@ When the function is done executing, GDB will silently stop."),
}
}
- do_cleanups (our_dtor_cleanup);
+ do_cleanups (context_saver_cleanup);
gdb_assert (retval);
return retval;
diff --git a/gdb/infcall.h b/gdb/infcall.h
index 9ec8617..43b5f66 100644
--- a/gdb/infcall.h
+++ b/gdb/infcall.h
@@ -50,14 +50,13 @@ extern struct value *
dummy_frame_dtor_ftype *dummy_dtor,
void *dummy_dtor_data);
-struct call_function_by_hand_dtor;
-extern void call_function_by_hand_dtor_drop
- (struct call_function_by_hand_dtor *data);
-extern void call_function_by_hand_dtor_cleanup
- (void *data_voidp);
-extern struct regcache *call_function_by_hand_dtor_get
- (struct call_function_by_hand_dtor *dtor_data);
-extern struct call_function_by_hand_dtor *call_function_by_hand_dtor_setup
+struct dummy_frame_context_saver;
+extern void dummy_frame_context_saver_drop
+ (struct dummy_frame_context_saver *data);
+extern void dummy_frame_context_saver_cleanup (void *data_voidp);
+extern struct regcache *dummy_frame_context_saver_get_regs
+ (struct dummy_frame_context_saver *saver);
+extern struct dummy_frame_context_saver *dummy_frame_context_saver_setup
(struct frame_id dummy_id, ptid_t ptid);
#endif
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 5df8e0f..3bcab77 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1512,7 +1512,7 @@ advance_command (char *arg, int from_tty)
struct value *
get_return_value (struct value *function, struct type *value_type,
- struct call_function_by_hand_dtor *dtor_data)
+ struct dummy_frame_context_saver *ctx_saver)
{
struct regcache *stop_regs = NULL;
struct gdbarch *gdbarch;
@@ -1520,8 +1520,8 @@ get_return_value (struct value *function, struct type *value_type,
struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
/* If were not saved, use the current registers. */
- if (dtor_data != NULL)
- stop_regs = call_function_by_hand_dtor_get (dtor_data);
+ if (ctx_saver != NULL)
+ stop_regs = dummy_frame_context_saver_get_regs (ctx_saver);
else
{
stop_regs = regcache_dup (get_current_regcache ());
@@ -1568,9 +1568,9 @@ get_return_value (struct value *function, struct type *value_type,
static void
print_return_value (struct value *function, struct type *value_type,
- struct call_function_by_hand_dtor *dtor_data)
+ struct dummy_frame_context_saver *ctx_saver)
{
- struct value *value = get_return_value (function, value_type, dtor_data);
+ struct value *value = get_return_value (function, value_type, ctx_saver);
struct ui_out *uiout = current_uiout;
if (value)
@@ -1625,7 +1625,7 @@ struct finish_command_continuation_args
/* Inferior registers stored right before dummy_frame has been freed
after an inferior call. It can be NULL if no inferior call was
involved, GDB will then use current inferior registers. */
- struct call_function_by_hand_dtor *dtor_data;
+ struct dummy_frame_context_saver *ctx_saver;
};
static void
@@ -1666,7 +1666,7 @@ finish_command_continuation (void *arg, int err)
/* print_return_value can throw an exception in some
circumstances. We need to catch this so that we still
delete the breakpoint. */
- print_return_value (func, value_type, a->dtor_data);
+ print_return_value (func, value_type, a->ctx_saver);
}
CATCH (ex, RETURN_MASK_ALL)
{
@@ -1692,8 +1692,8 @@ finish_command_continuation_free_arg (void *arg)
{
struct finish_command_continuation_args *cargs = arg;
- if (cargs->dtor_data != NULL)
- call_function_by_hand_dtor_drop (cargs->dtor_data);
+ if (cargs->ctx_saver != NULL)
+ dummy_frame_context_saver_drop (cargs->ctx_saver);
xfree (cargs);
}
@@ -1762,16 +1762,16 @@ finish_forward (struct symbol *function, struct frame_info *frame)
struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
struct finish_command_continuation_args *cargs;
int thread = tp->num;
- struct call_function_by_hand_dtor *dtor_data = NULL;
+ struct dummy_frame_context_saver *saver = NULL;
sal = find_pc_line (get_frame_pc (frame), 0);
sal.pc = get_frame_pc (frame);
if (get_frame_type (frame) == DUMMY_FRAME)
{
- dtor_data = call_function_by_hand_dtor_setup (get_stack_frame_id (frame),
- inferior_ptid);
- make_cleanup (call_function_by_hand_dtor_cleanup, dtor_data);
+ saver = dummy_frame_context_saver_setup (get_stack_frame_id (frame),
+ inferior_ptid);
+ make_cleanup (dummy_frame_context_saver_cleanup, saver);
}
breakpoint = set_momentary_breakpoint (gdbarch, sal,
@@ -1793,7 +1793,7 @@ finish_forward (struct symbol *function, struct frame_info *frame)
cargs->thread = thread;
cargs->breakpoint = breakpoint;
cargs->function = function;
- cargs->dtor_data = dtor_data;
+ cargs->ctx_saver = saver;
add_continuation (tp, finish_command_continuation, cargs,
finish_command_continuation_free_arg);
proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
diff --git a/gdb/inferior.h b/gdb/inferior.h
index f305e79..f0c5b52 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -165,10 +165,10 @@ extern void detach_command (char *, int);
extern void notice_new_inferior (ptid_t, int, int);
-struct call_function_by_hand_dtor;
+struct dummy_frame_context_saver;
extern struct value *get_return_value
(struct value *function, struct type *value_type,
- struct call_function_by_hand_dtor *dtor_data);
+ struct dummy_frame_context_saver *dtor_data);
/* Prepare for execution command. TARGET is the target that will run
the command. BACKGROUND determines whether this is a foreground
--
1.9.3
^ permalink raw reply [flat|nested] 19+ messages in thread* [commit] [PATCH 4/6] infcall: stop_registers -> register_dummy_frame_dtor
2015-05-13 13:28 ` Pedro Alves
@ 2015-05-13 18:54 ` Jan Kratochvil
0 siblings, 0 replies; 19+ messages in thread
From: Jan Kratochvil @ 2015-05-13 18:54 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On Wed, 13 May 2015 15:28:40 +0200, Pedro Alves wrote:
> On 05/08/2015 09:21 PM, Jan Kratochvil wrote:
> > I am not completely sure what is right for bpfinishpy_pre_stop_hook but the
> > testsuite passes. I can think more about it if it gets otherwise approved.
>
> Yeah, if the Python API around that gives access the return value,
> then something needs to be done there, otherwise, probably nothing else
> is needed.
It does not, see bottom of the mail, so I have just put there:
/* bpfinishpy_init cannot finish into DUMMY_FRAME (throws an error
in such case) so it is OK to always pass CTX_SAVER as NULL. */
struct value *ret = get_return_value (function, value_type, NULL);
> > +/* Destructor for associated dummy_frame. */
> > +
> > +static dummy_frame_dtor_ftype call_function_by_hand_dtor;
>
> This forward declaration is not really necessary, right?
> A wrong prototype mismismatch would be caught here:
Yes, it would. I think it could be there but I have removed it.
> How about we rename things,
I have renamed it by your patch, I agree the new naming is better.
Checked in:
8a6c40311297f60ad13827650fdde13da301b505
Thanks,
Jan
------------------------------------------------------------------------------
(gdb) l
1 long f(void) { return 1234567890123456789; }
2 int main(void) { return 0; }
(gdb) start
(gdb) b f
Breakpoint 2 at 0x4004fa: file 1.c, line 1.
(gdb) bt
#0 main () at 1.c:2
(gdb) p f()
Breakpoint 2, f () at 1.c:1
1 long f(void) { return 1234567890123456789; }
The program being debugged stopped while in a function called from GDB.
Evaluation of the expression containing the function
(f) will be abandoned.
When the function is done executing, GDB will silently stop.
(gdb) bt
#0 f () at 1.c:1
#1 <function called from gdb>
#2 main () at 1.c:2
(gdb) fini
Run till exit from #0 f () at 1.c:1
Value returned is $1 = 1234567890123456789
(gdb) bt
#0 main () at 1.c:2
(gdb) p f()
Breakpoint 2, f () at 1.c:1
1 long f(void) { return 1234567890123456789; }
The program being debugged stopped while in a function called from GDB.
Evaluation of the expression containing the function
(f) will be abandoned.
When the function is done executing, GDB will silently stop.
(gdb) bt
#0 f () at 1.c:1
#1 <function called from gdb>
#2 main () at 1.c:2
(gdb) python gdb.FinishBreakpoint()
Traceback (most recent call last):
File "<string>", line 1, in <module>
ValueError: "FinishBreakpoint" cannot be set on a dummy frame.
Error while executing Python code.
(gdb) _
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/6] register_dummy_frame_dtor: Permit multiple dtors
2015-05-08 20:21 [PATCH 1/6] dummy_frame_dtor_ftype vs. call_function_by_hand_dummy_dtor_ftype cleanup Jan Kratochvil
2015-05-08 20:21 ` [PATCH 4/6] infcall: stop_registers -> register_dummy_frame_dtor Jan Kratochvil
@ 2015-05-08 20:21 ` Jan Kratochvil
2015-05-13 13:00 ` Pedro Alves
2015-05-08 20:21 ` [PATCH 5/6] Remove stop_registers Jan Kratochvil
` (3 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Jan Kratochvil @ 2015-05-08 20:21 UTC (permalink / raw)
To: gdb-patches
Hi,
later patch needs two independent destructors for the same dummy_frame.
Therefore the registrar has been extended to an arbitrary number of
destructors.
Jan
gdb/ChangeLog
2015-05-08 Jan Kratochvil <jan.kratochvil@redhat.com>
* dummy-frame.c (struct dummy_frame_dtor_list): New.
(struct dummy_frame): Replace dtor and dtor_data by dtor_list.
(remove_dummy_frame): Process dtor_list.
(pop_dummy_frame): Process dtor_list.
(register_dummy_frame_dtor): Maintain dtor_list.
(find_dummy_frame_dtor): Handle dtor_list.
* dummy-frame.h (register_dummy_frame_dtor, find_dummy_frame_dtor):
Update comments.
---
gdb/dummy-frame.c | 60 +++++++++++++++++++++++++++++++++++++++++------------
gdb/dummy-frame.h | 9 ++++----
2 files changed, 51 insertions(+), 18 deletions(-)
diff --git a/gdb/dummy-frame.c b/gdb/dummy-frame.c
index 6c4fb4c..f1d3de8 100644
--- a/gdb/dummy-frame.c
+++ b/gdb/dummy-frame.c
@@ -49,6 +49,20 @@ dummy_frame_id_eq (struct dummy_frame_id *id1,
return frame_id_eq (id1->id, id2->id) && ptid_equal (id1->ptid, id2->ptid);
}
+/* List of dummy_frame destructors. */
+
+struct dummy_frame_dtor_list
+{
+ /* Next element in the list or NULL if this is the last element. */
+ struct dummy_frame_dtor_list *next;
+
+ /* If non-NULL, a destructor that is run when this dummy frame is freed. */
+ dummy_frame_dtor_ftype *dtor;
+
+ /* Arbitrary data that is passed to DTOR. */
+ void *dtor_data;
+};
+
/* Dummy frame. This saves the processor state just prior to setting
up the inferior function call. Older targets save the registers
on the target stack (but that really slows down function calls). */
@@ -63,11 +77,9 @@ struct dummy_frame
/* The caller's state prior to the call. */
struct infcall_suspend_state *caller_state;
- /* If non-NULL, a destructor that is run when this dummy frame is freed. */
- dummy_frame_dtor_ftype *dtor;
-
- /* Arbitrary data that is passed to DTOR. */
- void *dtor_data;
+ /* First element of destructors list or NULL if there are no
+ destructors registered for this dummy_frame. */
+ struct dummy_frame_dtor_list *dtor_list;
};
static struct dummy_frame *dummy_frame_stack = NULL;
@@ -96,8 +108,14 @@ remove_dummy_frame (struct dummy_frame **dummy_ptr)
{
struct dummy_frame *dummy = *dummy_ptr;
- if (dummy->dtor != NULL)
- dummy->dtor (dummy->dtor_data, 0);
+ while (dummy->dtor_list != NULL)
+ {
+ struct dummy_frame_dtor_list *list = dummy->dtor_list;
+
+ dummy->dtor_list = list->next;
+ list->dtor (list->dtor_data, 0);
+ xfree (list);
+ }
*dummy_ptr = dummy->next;
discard_infcall_suspend_state (dummy->caller_state);
@@ -138,8 +156,14 @@ pop_dummy_frame (struct dummy_frame **dummy_ptr)
gdb_assert (ptid_equal (dummy->id.ptid, inferior_ptid));
- if (dummy->dtor != NULL)
- dummy->dtor (dummy->dtor_data, 1);
+ while (dummy->dtor_list != NULL)
+ {
+ struct dummy_frame_dtor_list *list = dummy->dtor_list;
+
+ dummy->dtor_list = list->next;
+ list->dtor (list->dtor_data, 1);
+ xfree (list);
+ }
restore_infcall_suspend_state (dummy->caller_state);
@@ -212,13 +236,16 @@ register_dummy_frame_dtor (struct frame_id dummy_id, ptid_t ptid,
{
struct dummy_frame_id id = { dummy_id, ptid };
struct dummy_frame **dp, *d;
+ struct dummy_frame_dtor_list *list;
dp = lookup_dummy_frame (&id);
gdb_assert (dp != NULL);
d = *dp;
- gdb_assert (d->dtor == NULL);
- d->dtor = dtor;
- d->dtor_data = dtor_data;
+ list = xmalloc (sizeof (*list));
+ list->next = d->dtor_list;
+ d->dtor_list = list;
+ list->dtor = dtor;
+ list->dtor_data = dtor_data;
}
/* See dummy-frame.h. */
@@ -229,8 +256,13 @@ find_dummy_frame_dtor (dummy_frame_dtor_ftype *dtor, void *dtor_data)
struct dummy_frame *d;
for (d = dummy_frame_stack; d != NULL; d = d->next)
- if (d->dtor == dtor && d->dtor_data == dtor_data)
- return 1;
+ {
+ struct dummy_frame_dtor_list *list;
+
+ for (list = d->dtor_list; list != NULL; list = list->next)
+ if (list->dtor == dtor && list->dtor_data == dtor_data)
+ return 1;
+ }
return 0;
}
diff --git a/gdb/dummy-frame.h b/gdb/dummy-frame.h
index c156b81..9b5aff5 100644
--- a/gdb/dummy-frame.h
+++ b/gdb/dummy-frame.h
@@ -59,14 +59,15 @@ extern const struct frame_unwind dummy_frame_unwind;
typedef void (dummy_frame_dtor_ftype) (void *data, int registers_valid);
/* Call DTOR with DTOR_DATA when DUMMY_ID frame of thread PTID gets discarded.
- Dummy frame with DUMMY_ID must exist. There must be no other call of
- register_dummy_frame_dtor for that dummy frame. */
+ Dummy frame with DUMMY_ID must exist. Multiple destructors may be
+ registered, they will be called in the reverse order of registrations
+ (LIFO). */
extern void register_dummy_frame_dtor (struct frame_id dummy_id, ptid_t ptid,
dummy_frame_dtor_ftype *dtor,
void *dtor_data);
-/* Return 1 if there exists dummy frame with registered DTOR and DTOR_DATA.
- Return 0 otherwise. */
+/* Return 1 if there exists any dummy frame with any of its registered
+ destructors equal to both DTOR and DTOR_DATA. Return 0 otherwise. */
extern int find_dummy_frame_dtor (dummy_frame_dtor_ftype *dtor,
void *dtor_data);
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH 5/6] Remove stop_registers
2015-05-08 20:21 [PATCH 1/6] dummy_frame_dtor_ftype vs. call_function_by_hand_dummy_dtor_ftype cleanup Jan Kratochvil
2015-05-08 20:21 ` [PATCH 4/6] infcall: stop_registers -> register_dummy_frame_dtor Jan Kratochvil
2015-05-08 20:21 ` [PATCH 3/6] register_dummy_frame_dtor: Permit multiple dtors Jan Kratochvil
@ 2015-05-08 20:21 ` Jan Kratochvil
2015-05-13 13:10 ` Pedro Alves
2015-05-08 20:21 ` [PATCH 2/6] Call dummy_frame_dtor_ftype also from remove_dummy_frame Jan Kratochvil
` (2 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Jan Kratochvil @ 2015-05-08 20:21 UTC (permalink / raw)
To: gdb-patches
Hi,
now stop_registers are no longer used and it can be removed.
I am not much sure what 'proceed_to_finish' really means now so I make a wild
guess while updating comments about it.
Jan
gdb/ChangeLog
2015-05-08 Jan Kratochvil <jan.kratochvil@redhat.com>
* gdbthread.h (struct thread_control_state): Update comment for
proceed_to_finish.
* infcall.c (run_inferior_call): Update comment about
proceed_to_finish.
* infcmd.c (get_return_value): Update comment about stop_registers.
(finish_forward): Update comment about proceed_to_finish.
* infrun.c (stop_registers): Remove.
(clear_proceed_status, normal_stop): Remove stop_registers handling.
* infrun.h (stop_registers): Remove.
---
gdb/gdbthread.h | 2 +-
gdb/infcall.c | 2 +-
gdb/infcmd.c | 4 ++--
gdb/infrun.c | 27 ---------------------------
gdb/infrun.h | 7 -------
5 files changed, 4 insertions(+), 38 deletions(-)
diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index ff7cec2..0926f5f 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -118,7 +118,7 @@ struct thread_control_state
int trap_expected;
/* Nonzero if the thread is being proceeded for a "finish" command
- or a similar situation when stop_registers should be saved. */
+ or a similar situation when return value should be printed. */
int proceed_to_finish;
/* Nonzero if the thread is being proceeded for an inferior function
diff --git a/gdb/infcall.c b/gdb/infcall.c
index 9707861..849f500 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -398,7 +398,7 @@ run_inferior_call (struct thread_info *call_thread, CORE_ADDR real_pc)
disable_watchpoints_before_interactive_call_start ();
- /* We want stop_registers, please... */
+ /* We want to print return value, please... */
call_thread->control.proceed_to_finish = 1;
TRY
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index f8874d3..5df8e0f 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1519,7 +1519,7 @@ get_return_value (struct value *function, struct type *value_type,
struct value *value;
struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
- /* If stop_registers were not saved, use the current registers. */
+ /* If were not saved, use the current registers. */
if (dtor_data != NULL)
stop_regs = call_function_by_hand_dtor_get (dtor_data);
else
@@ -1786,7 +1786,7 @@ finish_forward (struct symbol *function, struct frame_info *frame)
set_longjmp_breakpoint (tp, frame_id);
make_cleanup (delete_longjmp_breakpoint_cleanup, &thread);
- /* We want stop_registers, please... */
+ /* We want to print return value, please... */
tp->control.proceed_to_finish = 1;
cargs = xmalloc (sizeof (*cargs));
diff --git a/gdb/infrun.c b/gdb/infrun.c
index a4f0b9f..71cf208 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -353,13 +353,6 @@ show_stop_on_solib_events (struct ui_file *file, int from_tty,
int stop_after_trap;
-/* Save register contents here when executing a "finish" command or are
- about to pop a stack dummy frame, if-and-only-if proceed_to_finish is set.
- Thus this contains the return value from the called function (assuming
- values are returned in a register). */
-
-struct regcache *stop_registers;
-
/* Nonzero after stop if current stack frame should be printed. */
static int stop_print_frame;
@@ -2505,12 +2498,6 @@ clear_proceed_status (int step)
clear_step_over_info ();
observer_notify_about_to_proceed ();
-
- if (stop_registers)
- {
- regcache_xfree (stop_registers);
- stop_registers = NULL;
- }
}
/* Returns true if TP is still stopped at a breakpoint that needs
@@ -6697,20 +6684,6 @@ normal_stop (void)
print_stop_event (&last);
}
- /* Save the function value return registers, if we care.
- We might be about to restore their previous contents. */
- if (inferior_thread ()->control.proceed_to_finish
- && execution_direction != EXEC_REVERSE)
- {
- /* This should not be necessary. */
- if (stop_registers)
- regcache_xfree (stop_registers);
-
- /* NB: The copy goes through to the target picking up the value of
- all the registers. */
- stop_registers = regcache_dup (get_current_regcache ());
- }
-
if (stop_stack_dummy == STOP_STACK_DUMMY)
{
/* Pop the empty frame that contains the stack dummy.
diff --git a/gdb/infrun.h b/gdb/infrun.h
index 1f09e41..75bb075 100644
--- a/gdb/infrun.h
+++ b/gdb/infrun.h
@@ -73,13 +73,6 @@ enum exec_direction_kind
compatible with make_cleanup_restore_integer. */
extern int execution_direction;
-/* Save register contents here when executing a "finish" command or
- are about to pop a stack dummy frame, if-and-only-if
- proceed_to_finish is set. Thus this contains the return value from
- the called function (assuming values are returned in a
- register). */
-extern struct regcache *stop_registers;
-
extern void start_remote (int from_tty);
/* Clear out all variables saying what to do when inferior is
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 5/6] Remove stop_registers
2015-05-08 20:21 ` [PATCH 5/6] Remove stop_registers Jan Kratochvil
@ 2015-05-13 13:10 ` Pedro Alves
2015-05-13 18:55 ` [commit] " Jan Kratochvil
0 siblings, 1 reply; 19+ messages in thread
From: Pedro Alves @ 2015-05-13 13:10 UTC (permalink / raw)
To: Jan Kratochvil, gdb-patches
I'll comment on patch #4 once I grok it fully, but meanwhile...
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -1519,7 +1519,7 @@ get_return_value (struct value *function, struct type *value_type,
> struct value *value;
> struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
>
> - /* If stop_registers were not saved, use the current registers. */
> + /* If were not saved, use the current registers. */
A word after "If" is missing, I guess:
"If registers were not saved, ..."
Otherwise looks fine.
This variable should probably be renamed and its comments
further clarified, but what you have is good already. We can
do that separately.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 19+ messages in thread
* [commit] [PATCH 5/6] Remove stop_registers
2015-05-13 13:10 ` Pedro Alves
@ 2015-05-13 18:55 ` Jan Kratochvil
0 siblings, 0 replies; 19+ messages in thread
From: Jan Kratochvil @ 2015-05-13 18:55 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On Wed, 13 May 2015 15:10:15 +0200, Pedro Alves wrote:
> > - /* If stop_registers were not saved, use the current registers. */
> > + /* If were not saved, use the current registers. */
>
> A word after "If" is missing, I guess:
>
> "If registers were not saved, ..."
Yes, done.
> Otherwise looks fine.
Checked in:
46c03469b37d2ccb6a1eaa3ea4e21c57d07246fc
Jan
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/6] Call dummy_frame_dtor_ftype also from remove_dummy_frame
2015-05-08 20:21 [PATCH 1/6] dummy_frame_dtor_ftype vs. call_function_by_hand_dummy_dtor_ftype cleanup Jan Kratochvil
` (2 preceding siblings ...)
2015-05-08 20:21 ` [PATCH 5/6] Remove stop_registers Jan Kratochvil
@ 2015-05-08 20:21 ` Jan Kratochvil
2015-05-13 12:57 ` Pedro Alves
2015-05-13 18:53 ` [commit] " Jan Kratochvil
2015-05-08 20:22 ` [PATCH 6/6] Make regcache_cpy_no_passthrough static Jan Kratochvil
2015-05-13 12:55 ` [PATCH 1/6] dummy_frame_dtor_ftype vs. call_function_by_hand_dummy_dtor_ftype cleanup Pedro Alves
5 siblings, 2 replies; 19+ messages in thread
From: Jan Kratochvil @ 2015-05-08 20:21 UTC (permalink / raw)
To: gdb-patches
Hi,
there was now a leak-like bug that if dummy_frame "disappeared" by
remove_dummy_frame then its destructor was not called. For example in the case
of 'compile code' dummy frames the injected objfile would never get freed after
some inferior longjmp out of the injected code.
Jan
gdb/ChangeLog
2015-05-08 Jan Kratochvil <jan.kratochvil@redhat.com>
* compile/compile-object-run.c (do_module_cleanup): Add parameter
registers_valid.
(compile_object_run): Update do_module_cleanup caller.
* dummy-frame.c: Include infcall.h.
(struct dummy_frame): Update dtor comment.
(remove_dummy_frame): Call dtor.
(pop_dummy_frame): Update dtor caller.
* dummy-frame.h (dummy_frame_dtor_ftype): Add parameter
registers_valid.
---
gdb/compile/compile-object-run.c | 4 ++--
gdb/dummy-frame.c | 9 ++++++---
gdb/dummy-frame.h | 5 +++--
3 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/gdb/compile/compile-object-run.c b/gdb/compile/compile-object-run.c
index 6738aad..422693b 100644
--- a/gdb/compile/compile-object-run.c
+++ b/gdb/compile/compile-object-run.c
@@ -45,7 +45,7 @@ struct do_module_cleanup
static dummy_frame_dtor_ftype do_module_cleanup;
static void
-do_module_cleanup (void *arg)
+do_module_cleanup (void *arg, int registers_valid)
{
struct do_module_cleanup *data = arg;
struct objfile *objfile;
@@ -129,7 +129,7 @@ compile_object_run (struct compile_module *module)
data->executedp = NULL;
gdb_assert (!(dtor_found && executed));
if (!dtor_found && !executed)
- do_module_cleanup (data);
+ do_module_cleanup (data, 0);
throw_exception (ex);
}
END_CATCH
diff --git a/gdb/dummy-frame.c b/gdb/dummy-frame.c
index f193289..6c4fb4c 100644
--- a/gdb/dummy-frame.c
+++ b/gdb/dummy-frame.c
@@ -28,6 +28,7 @@
#include "gdbcmd.h"
#include "observer.h"
#include "gdbthread.h"
+#include "infcall.h"
struct dummy_frame_id
{
@@ -62,8 +63,7 @@ struct dummy_frame
/* The caller's state prior to the call. */
struct infcall_suspend_state *caller_state;
- /* If non-NULL, a destructor that is run when this dummy frame is
- popped. */
+ /* If non-NULL, a destructor that is run when this dummy frame is freed. */
dummy_frame_dtor_ftype *dtor;
/* Arbitrary data that is passed to DTOR. */
@@ -96,6 +96,9 @@ remove_dummy_frame (struct dummy_frame **dummy_ptr)
{
struct dummy_frame *dummy = *dummy_ptr;
+ if (dummy->dtor != NULL)
+ dummy->dtor (dummy->dtor_data, 0);
+
*dummy_ptr = dummy->next;
discard_infcall_suspend_state (dummy->caller_state);
xfree (dummy);
@@ -136,7 +139,7 @@ pop_dummy_frame (struct dummy_frame **dummy_ptr)
gdb_assert (ptid_equal (dummy->id.ptid, inferior_ptid));
if (dummy->dtor != NULL)
- dummy->dtor (dummy->dtor_data);
+ dummy->dtor (dummy->dtor_data, 1);
restore_infcall_suspend_state (dummy->caller_state);
diff --git a/gdb/dummy-frame.h b/gdb/dummy-frame.h
index ffd3b0a..c156b81 100644
--- a/gdb/dummy-frame.h
+++ b/gdb/dummy-frame.h
@@ -54,8 +54,9 @@ extern void dummy_frame_discard (struct frame_id dummy_id, ptid_t ptid);
extern const struct frame_unwind dummy_frame_unwind;
-/* Destructor for dummy_frame. DATA is supplied by registrant. */
-typedef void (dummy_frame_dtor_ftype) (void *data);
+/* Destructor for dummy_frame. DATA is supplied by registrant.
+ REGISTERS_VALID is 1 for dummy_frame_pop, 0 for dummy_frame_discard. */
+typedef void (dummy_frame_dtor_ftype) (void *data, int registers_valid);
/* Call DTOR with DTOR_DATA when DUMMY_ID frame of thread PTID gets discarded.
Dummy frame with DUMMY_ID must exist. There must be no other call of
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/6] Call dummy_frame_dtor_ftype also from remove_dummy_frame
2015-05-08 20:21 ` [PATCH 2/6] Call dummy_frame_dtor_ftype also from remove_dummy_frame Jan Kratochvil
@ 2015-05-13 12:57 ` Pedro Alves
2015-05-13 13:52 ` Jan Kratochvil
2015-05-13 18:53 ` [commit] " Jan Kratochvil
1 sibling, 1 reply; 19+ messages in thread
From: Pedro Alves @ 2015-05-13 12:57 UTC (permalink / raw)
To: Jan Kratochvil, gdb-patches
On 05/08/2015 09:21 PM, Jan Kratochvil wrote:
> Hi,
>
> there was now a leak-like bug that if dummy_frame "disappeared" by
> remove_dummy_frame then its destructor was not called. For example in the case
> of 'compile code' dummy frames the injected objfile would never get freed after
> some inferior longjmp out of the injected code.
So the real fix here is this bit, right?
> /* Arbitrary data that is passed to DTOR. */
> @@ -96,6 +96,9 @@ remove_dummy_frame (struct dummy_frame **dummy_ptr)
> {
> struct dummy_frame *dummy = *dummy_ptr;
>
> + if (dummy->dtor != NULL)
> + dummy->dtor (dummy->dtor_data, 0);
> +
I'm not seeing where registers_valid is used anywhere in this patch.
Guess it'll be used in follow up patches.
(A clearer patch split would have added the missing dummy->dtor call
in this patch, and left the registers_valid addition to a separate
patch that needs that.)
/me goes try to understand the rest of the series.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/6] Call dummy_frame_dtor_ftype also from remove_dummy_frame
2015-05-13 12:57 ` Pedro Alves
@ 2015-05-13 13:52 ` Jan Kratochvil
0 siblings, 0 replies; 19+ messages in thread
From: Jan Kratochvil @ 2015-05-13 13:52 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On Wed, 13 May 2015 14:57:18 +0200, Pedro Alves wrote:
> On 05/08/2015 09:21 PM, Jan Kratochvil wrote:
> > Hi,
> >
> > there was now a leak-like bug that if dummy_frame "disappeared" by
> > remove_dummy_frame then its destructor was not called. For example in the case
> > of 'compile code' dummy frames the injected objfile would never get freed after
> > some inferior longjmp out of the injected code.
>
> So the real fix here is this bit, right?
This fix was just a side-effect, the original goal of this patch was to add
'registers_valid'.
> > /* Arbitrary data that is passed to DTOR. */
> > @@ -96,6 +96,9 @@ remove_dummy_frame (struct dummy_frame **dummy_ptr)
> > {
> > struct dummy_frame *dummy = *dummy_ptr;
> >
> > + if (dummy->dtor != NULL)
> > + dummy->dtor (dummy->dtor_data, 0);
> > +
>
> I'm not seeing where registers_valid is used anywhere in this patch.
> Guess it'll be used in follow up patches.
Yes.
> (A clearer patch split would have added the missing dummy->dtor call
> in this patch, and left the registers_valid addition to a separate
> patch that needs that.)
I haven't found the fix too much significant to separate it into a new patch
part but you are right, it should have been separated.
Thanks,
Jan
^ permalink raw reply [flat|nested] 19+ messages in thread
* [commit] [PATCH 2/6] Call dummy_frame_dtor_ftype also from remove_dummy_frame
2015-05-08 20:21 ` [PATCH 2/6] Call dummy_frame_dtor_ftype also from remove_dummy_frame Jan Kratochvil
2015-05-13 12:57 ` Pedro Alves
@ 2015-05-13 18:53 ` Jan Kratochvil
1 sibling, 0 replies; 19+ messages in thread
From: Jan Kratochvil @ 2015-05-13 18:53 UTC (permalink / raw)
To: gdb-patches
On Fri, 08 May 2015 22:21:29 +0200, Jan Kratochvil wrote:
> gdb/ChangeLog
> 2015-05-08 Jan Kratochvil <jan.kratochvil@redhat.com>
>
> * compile/compile-object-run.c (do_module_cleanup): Add parameter
> registers_valid.
> (compile_object_run): Update do_module_cleanup caller.
> * dummy-frame.c: Include infcall.h.
> (struct dummy_frame): Update dtor comment.
> (remove_dummy_frame): Call dtor.
> (pop_dummy_frame): Update dtor caller.
> * dummy-frame.h (dummy_frame_dtor_ftype): Add parameter
> registers_valid.
Checked in:
5e9705017f5b257421136b8d7752b9c793335ace
Jan
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 6/6] Make regcache_cpy_no_passthrough static
2015-05-08 20:21 [PATCH 1/6] dummy_frame_dtor_ftype vs. call_function_by_hand_dummy_dtor_ftype cleanup Jan Kratochvil
` (3 preceding siblings ...)
2015-05-08 20:21 ` [PATCH 2/6] Call dummy_frame_dtor_ftype also from remove_dummy_frame Jan Kratochvil
@ 2015-05-08 20:22 ` Jan Kratochvil
2015-05-13 13:16 ` Pedro Alves
2015-05-13 12:55 ` [PATCH 1/6] dummy_frame_dtor_ftype vs. call_function_by_hand_dummy_dtor_ftype cleanup Pedro Alves
5 siblings, 1 reply; 19+ messages in thread
From: Jan Kratochvil @ 2015-05-08 20:22 UTC (permalink / raw)
To: gdb-patches
Hi,
regcache_cpy_no_passthrough is no longer used for a standalone call.
Jan
gdb/ChangeLog
2015-05-08 Jan Kratochvil <jan.kratochvil@redhat.com>
* regcache.c (regcache_cpy_no_passthrough): New declaration.
(regcache_cpy_no_passthrough): Make it static, add function comment.
* regcache.h (regcache_dup, regcache_cpy): Reduce/update their comment.
(regcache_cpy_no_passthrough): Remove declaration.
---
gdb/regcache.c | 14 +++++++++++++-
gdb/regcache.h | 9 ++-------
2 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 366eba0..d363a12 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -391,6 +391,9 @@ do_cooked_read (void *src, int regnum, gdb_byte *buf)
return regcache_cooked_read (regcache, regnum, buf);
}
+static void regcache_cpy_no_passthrough (struct regcache *dst,
+ struct regcache *src);
+
void
regcache_cpy (struct regcache *dst, struct regcache *src)
{
@@ -407,7 +410,16 @@ regcache_cpy (struct regcache *dst, struct regcache *src)
regcache_cpy_no_passthrough (dst, src);
}
-void
+/* Copy/duplicate the contents of a register cache. By default, the
+ operation is pass-through. Writes to DST and reads from SRC will
+ go through to the target. See also regcache_cpy.
+
+ It can not have overlapping SRC and DST buffers.
+
+ It does not go through to the target. It only transfer values
+ already in the cache. */
+
+static void
regcache_cpy_no_passthrough (struct regcache *dst, struct regcache *src)
{
gdb_assert (src != NULL && dst != NULL);
diff --git a/gdb/regcache.h b/gdb/regcache.h
index 394506a..a9fb44b 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -224,17 +224,12 @@ extern void regcache_save (struct regcache *dst,
/* Copy/duplicate the contents of a register cache. By default, the
operation is pass-through. Writes to DST and reads from SRC will
- go through to the target.
+ go through to the target. See also regcache_cpy_no_passthrough.
- The ``cpy'' functions can not have overlapping SRC and DST buffers.
-
- ``no passthrough'' versions do not go through to the target. They
- only transfer values already in the cache. */
+ regcache_cpy can not have overlapping SRC and DST buffers. */
extern struct regcache *regcache_dup (struct regcache *regcache);
extern void regcache_cpy (struct regcache *dest, struct regcache *src);
-extern void regcache_cpy_no_passthrough (struct regcache *dest,
- struct regcache *src);
extern void registers_changed (void);
extern void registers_changed_ptid (ptid_t);
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 6/6] Make regcache_cpy_no_passthrough static
2015-05-08 20:22 ` [PATCH 6/6] Make regcache_cpy_no_passthrough static Jan Kratochvil
@ 2015-05-13 13:16 ` Pedro Alves
2015-05-13 19:49 ` [commit] " Jan Kratochvil
0 siblings, 1 reply; 19+ messages in thread
From: Pedro Alves @ 2015-05-13 13:16 UTC (permalink / raw)
To: Jan Kratochvil, gdb-patches
On 05/08/2015 09:21 PM, Jan Kratochvil wrote:
> +/* Copy/duplicate the contents of a register cache. By default, the
> + operation is pass-through. Writes to DST and reads from SRC will
> + go through to the target. See also regcache_cpy.
> +
> + It can not have overlapping SRC and DST buffers.
> +
> + It does not go through to the target. It only transfer values
> + already in the cache. */
I think this would be clearer:
/* Copy/duplicate the contents of a register cache. Unlike regcache_cpy,
which is pass-through, this does not go through to the target.
Only values values already in the cache are transferred. The SRC and DST
buffers must not overlap. */
static void
regcache_cpy_no_passthrough (struct regcache *dst, struct regcache *src)
{
Otherwise looks fine.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 19+ messages in thread* [commit] [PATCH 6/6] Make regcache_cpy_no_passthrough static
2015-05-13 13:16 ` Pedro Alves
@ 2015-05-13 19:49 ` Jan Kratochvil
0 siblings, 0 replies; 19+ messages in thread
From: Jan Kratochvil @ 2015-05-13 19:49 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On Wed, 13 May 2015 15:16:42 +0200, Pedro Alves wrote:
> I think this would be clearer:
>
> /* Copy/duplicate the contents of a register cache. Unlike regcache_cpy,
> which is pass-through, this does not go through to the target.
> Only values values already in the cache are transferred. The SRC and DST
> buffers must not overlap. */
Done.
> Otherwise looks fine.
Checked in:
bd49952bd7fbe616e2cb488e8080502f2338aaa2
Jan
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/6] dummy_frame_dtor_ftype vs. call_function_by_hand_dummy_dtor_ftype cleanup
2015-05-08 20:21 [PATCH 1/6] dummy_frame_dtor_ftype vs. call_function_by_hand_dummy_dtor_ftype cleanup Jan Kratochvil
` (4 preceding siblings ...)
2015-05-08 20:22 ` [PATCH 6/6] Make regcache_cpy_no_passthrough static Jan Kratochvil
@ 2015-05-13 12:55 ` Pedro Alves
2015-05-13 13:56 ` [commit] " Jan Kratochvil
5 siblings, 1 reply; 19+ messages in thread
From: Pedro Alves @ 2015-05-13 12:55 UTC (permalink / raw)
To: Jan Kratochvil, gdb-patches
On 05/08/2015 09:21 PM, Jan Kratochvil wrote:
> Hi,
>
> both dummy_frame_dtor_ftype and call_function_by_hand_dummy_dtor_ftype
> represent the same type, there was some mistake/duplication during check-in.
>
OK.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 19+ messages in thread