Patchwork [RFA] Fix leaks by clearing registers and frame caches.

login
register
mail settings
Submitter Philippe Waroquiers
Date May 1, 2019, 5:37 p.m.
Message ID <20190501173738.18155-1-philippe.waroquiers@skynet.be>
Download mbox | patch
Permalink /patch/32475/
State New
Headers show

Comments

Philippe Waroquiers - May 1, 2019, 5:37 p.m.
Valgrind reports leaks such as the below in the tests:
gdb.threads/corethreads.exp
gdb.threads/gcore-thread.exp
gdb.ada/task_switch_in_core.exp
gdb.trace/tfile.exp
gdb.base/siginfo-thread.exp

==12701== 1,123 (72 direct, 1,051 indirect) bytes in 1 blocks are definitely lost in loss record 2,928 of 3,247
==12701==    at 0x4C2C4CC: operator new(unsigned long) (vg_replace_malloc.c:344)
==12701==    by 0x5CF771: get_thread_arch_aspace_regcache(ptid_t, gdbarch*, address_space*) (regcache.c:330)
==12701==    by 0x5CF92A: get_thread_regcache (regcache.c:366)
==12701==    by 0x5CF92A: get_current_regcache() (regcache.c:372)
==12701==    by 0x4C7964: get_current_frame() (frame.c:1587)
==12701==    by 0x4C7A3C: get_selected_frame(char const*) (frame.c:1651)
==12701==    by 0x669EAD: print_thread_info_1(ui_out*, char const*, int, int, int) (thread.c:1151)
==12701==    by 0x66A9A1: info_threads_command(char const*, int) (thread.c:1217)
==12701==    by 0x40A878: cmd_func(cmd_list_element*, char const*, int) (cli-decode.c:1892)
...

Fix these leaks by clearing registers and frame caches.
This leak and fix is similar to the leak fixed by 799efbe8e01
---
 gdb/ChangeLog  | 5 +++++
 gdb/corelow.c  | 2 ++
 gdb/inferior.c | 4 ++++
 3 files changed, 11 insertions(+)
Kevin Buettner - May 3, 2019, 1:32 a.m.
Philippe Waroquiers <philippe.waroquiers@skynet.be> wrote:

> diff --git a/gdb/corelow.c b/gdb/corelow.c
> index 3ce612d31b..9fcc2d2372 100644
> --- a/gdb/corelow.c
> +++ b/gdb/corelow.c
> @@ -535,6 +535,8 @@ core_target::detach (inferior *inf, int from_tty)
>       'this'.  */
>    unpush_target (this);
>  
> +  /* Clear the register cache and the frame cache.  */
> +  registers_changed ();

I'm wondering if it might be better to call

  registers_changed_ptid (ptid_t (inf->pid));

instead?  (I haven't checked to see if this compiles, but hopefully you get
the idea.)

That way, we only touch the register cache for the inferior being
detached.

>    reinit_frame_cache ();
>    maybe_say_no_core_file_now (from_tty);
>  }
> diff --git a/gdb/inferior.c b/gdb/inferior.c
> index e7b49376e9..abfef700c3 100644
> --- a/gdb/inferior.c
> +++ b/gdb/inferior.c
> @@ -208,6 +208,10 @@ exit_inferior_1 (struct inferior *inftoex, int silent)
>    inf->pending_detach = 0;
>    /* Reset it.  */
>    inf->control = inferior_control_state (NO_STOP_QUIETLY);
> +
> +  /* Clear the register cache and the frame cache.  */
> +  registers_changed ();

Likewise here.

> +  reinit_frame_cache ();
>  }

It seems okay to me otherwise...

Kevin
Philippe Waroquiers - May 4, 2019, 5:51 a.m.
On Thu, 2019-05-02 at 18:32 -0700, Kevin Buettner wrote:
> Philippe Waroquiers <philippe.waroquiers@skynet.be> wrote:
> 
> > diff --git a/gdb/corelow.c b/gdb/corelow.c
> > index 3ce612d31b..9fcc2d2372 100644
> > --- a/gdb/corelow.c
> > +++ b/gdb/corelow.c
> > @@ -535,6 +535,8 @@ core_target::detach (inferior *inf, int from_tty)
> >       'this'.  */
> >    unpush_target (this);
> >  
> > +  /* Clear the register cache and the frame cache.  */
> > +  registers_changed ();
> 
> I'm wondering if it might be better to call
> 
>   registers_changed_ptid (ptid_t (inf->pid));
> 
> instead?  (I haven't checked to see if this compiles, but hopefully you get
> the idea.)
> 
> That way, we only touch the register cache for the inferior being
> detached.
Yes, I contemplated doing that.
But the detach code is somewhat tricky, as it is not clear up to
what point the 'inf' and/or inf->pid stays valid + the detach
method is target dependent and can do whatever.
(see e.g. the comment about the 'this' that becomes dangling).

So, it looked safer to me to just clear the register and framecache
when an inferior exits and/or detach, as the efficiency for such
operation is not critical (and there are already many places that
are doing that when attaching/detaching/forking/...).


> 
> >    reinit_frame_cache ();
> >    maybe_say_no_core_file_now (from_tty);
> >  }
> > diff --git a/gdb/inferior.c b/gdb/inferior.c
> > index e7b49376e9..abfef700c3 100644
> > --- a/gdb/inferior.c
> > +++ b/gdb/inferior.c
> > @@ -208,6 +208,10 @@ exit_inferior_1 (struct inferior *inftoex, int silent)
> >    inf->pending_detach = 0;
> >    /* Reset it.  */
> >    inf->control = inferior_control_state (NO_STOP_QUIETLY);
> > +
> > +  /* Clear the register cache and the frame cache.  */
> > +  registers_changed ();
> 
> Likewise here.
> 
> > +  reinit_frame_cache ();
> >  }
> 
> It seems okay to me otherwise...
Thanks for the review, I haved pushed the fix with the 'global clear'
(for the reasons explained above) but if there is a need to
have a more 'precise clear', just tell me, and I can rework this
in a follow up.

Philippe

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index eba0426463..ce18121a13 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -6229,3 +6229,8 @@  version-control: never
 coding: utf-8
 End:
 
+2019-04-01  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
+
+	* corelow.c (core_target::detach): Ensure frame cache and
+	register caches are cleared.
+	inferior.c (exit_inferior_1): Likewise.
diff --git a/gdb/corelow.c b/gdb/corelow.c
index 3ce612d31b..9fcc2d2372 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -535,6 +535,8 @@  core_target::detach (inferior *inf, int from_tty)
      'this'.  */
   unpush_target (this);
 
+  /* Clear the register cache and the frame cache.  */
+  registers_changed ();
   reinit_frame_cache ();
   maybe_say_no_core_file_now (from_tty);
 }
diff --git a/gdb/inferior.c b/gdb/inferior.c
index e7b49376e9..abfef700c3 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -208,6 +208,10 @@  exit_inferior_1 (struct inferior *inftoex, int silent)
   inf->pending_detach = 0;
   /* Reset it.  */
   inf->control = inferior_control_state (NO_STOP_QUIETLY);
+
+  /* Clear the register cache and the frame cache.  */
+  registers_changed ();
+  reinit_frame_cache ();
 }
 
 void