Re: [RFA] Fix regcache leak, and avoid possible regcache access after detach.

[Kevin Buettner <kevinb at redhat dot com>]

On Sat, 16 Feb 2019 21:58:10 +0100
Philippe Waroquiers <philippe.waroquiers@skynet.be> wrote:

> Valgrind reports leaks like the below in various tests,
> e.g. gdb.threads/attach-slow-waitpid.exp, gdb.ada/task_switch_in_core.exp, ...
> 
> Fix the leak by clearing the regcache when detaching from an inferior.
> Note that these leaks are 'created' when GDB exits,
> when the regcache::current_regcache is destroyed : the elements
> of the forward_list are pointers, and the 'pointed to' memory is not
> deleted by the forward_list destructor.
> 
> Nevertheless, fixing this leak is good as it makes a bunch of
> tests 'leak clean'.
> 
> Also, it seems strange to keep a register cache for a process from
> which GDB detached : it is not clear if this cache is still valid
> after detach.  And effectively, when clearing only the regcache,
> (and not the frame cache), then the frame cache was still 'pointing'
> at this regcache and was used when switching to the child process
> in the test gdb.threads/watchpoint-fork.exp, which seems strange.
> 
> So, we solve the leak and avoid possible accesses to the regcache
> and frame cache of the detached inferior, by clearing both the
> regcache and the frame cache.
> 
> Tested on debian/amd64, natively and under Valgrind.

Before committing, could you also test against native-gdbserver?
I.e...

make check RUNTESTFLAGS="--target_board=native-gdbserver"

(I'd like the detach code in remote.c to be exercised as well.  I've
run into some subtle bugs in this area of remote.c in the past.)

My remaining comments consist of a few nits...

> diff --git a/gdb/target.c b/gdb/target.c
> index 116510e8cb..0f10628590 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -2010,6 +2010,8 @@ target_preopen (int from_tty)
>  void
>  target_detach (inferior *inf, int from_tty)
>  {
> +  ptid_t pid_ptid = ptid_t (inf->pid);
> +

Please move this declaration / initialization to the line just before
where it's used.  Since it's only used once, you could also just pass
ptid_t (inf->pid) directly to registers_changed_ptid() and get rid
of the declaration entirely.

>    /* As long as some to_detach implementations rely on the current_inferior
>       (either directly, or indirectly, like through target_gdbarch or by
>       reading memory), INF needs to be the current inferior.  When that
> @@ -2029,6 +2031,15 @@ target_detach (inferior *inf, int from_tty)
>    prepare_for_detach ();
>  
>    current_top_target ()->detach (inf, from_tty);
> +
> +  /* After we have detached, clear the register cache for this inferior.  */
> +  registers_changed_ptid (pid_ptid);
> +
> +  /* We have to ensure we have no frame cached left.  Normally,

s/cached/cache/

> +     registers_changed_ptid (pid_ptid) calls reinit_frame_cache when
> +     inferior_ptid matches pid_ptid, but in our case, it does not
> +     call it, as inferior_pid has been reset.  */

s/inferior_pid/inferior_ptid/

Thanks for that comment though; I was wondering why reinit_frame_cache
needed to be called.

> +  reinit_frame_cache ();
>  }

Okay with those changes, assuming that all goes well with the
native-gdbserver testing.