diff mbox

[2/6] Call dummy_frame_dtor_ftype also from remove_dummy_frame

Message ID 20150508202128.15830.40142.stgit@host1.jankratochvil.net
State New
Headers show

Commit Message

Jan Kratochvil May 8, 2015, 8:21 p.m. UTC
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(-)

Comments

Pedro Alves May 13, 2015, 12:57 p.m. UTC | #1
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
Jan Kratochvil May 13, 2015, 1:52 p.m. UTC | #2
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
Jan Kratochvil May 13, 2015, 6:53 p.m. UTC | #3
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
diff mbox

Patch

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