Re: [pushed] gdb/mi: consistently notify user when GDB/MI client uses -thread-select

[Jan Vrany <jan.vrany@labware.com>]

On Wed, 2022-03-23 at 19:09 +0000, Andrew Burgess wrote:
> Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> > On Wed, 2022-03-23 at 11:16 -0400, Simon Marchi wrote:
> > > > I gave it a go but I'm sorry: everything seem to work as expected 
> > > > here. I do not get "[Switching to thread 1 (process 1891548)]" message
> > > > on CLI when -thread-select already selected thread. 
> > > 
> > > Ok, strange.
> > > 
> > > > I must be doing something differently. What commit is failing for you?
> > > > What's your ./configure incantation?
> > > 
> > > My configure arguments are:
> > > 
> > > 'CC=ccache gcc-11' 'CXX=ccache g++-11' '--disable-binutils' '--disable-gold' '--disable-ld' '--disable-gprof' '--disable-gas' '--with-guile' 'CFLAGS=-g3 -O0
> > > -fdiagnostics-color=always -fmax-errors=1 -fsanitize=address' 'CXXFLAGS=-std=c++11 -g3 -O0 -fdiagnostics-color=always -fmax-errors=1 -fsanitize=address -
> > > D_GLIBCXX_DEBUG=1 -D_GLIBCXX_DEBUG_PEDANTIC=1 -D_GLIBCXX_SANITIZE_VECTOR=1' 'LDFLAGS=-fuse-ld=lld -fsanitize=address' '--prefix=/usr' '--enable-ubsan' '--
> > > with-python=python3' '--with-debuginfod' '--enable-silent-rules' '--enable-werror'
> > > 
> > 
> > Thanks! Now it is failing for me too! 
> > I just recompiled GDB using the above command. I'll have a 
> > look tomorrow. 
> 
> These failures are totally my fault.  Sorry!
> 
> The patch below should fix the problem.  Let me know what you think.

I wrote patch pretty much the same as yours when I noticed your email!

"deprecated_SAFE_get_selected_frame()" turned out not to be as safe
as we both thought it is :-)

Thanks a lot!
 
Jan


> 
> Thanks,
> Andrew
> 
> ---
> 
> commit fcac0d17e64a18e0600372c2bed51f9e2d9e9d6a
> Author: Andrew Burgess <aburgess@redhat.com>
> Date:   Wed Mar 23 19:00:35 2022 +0000
> 
>     gdb/mi: fix use after free of frame_info causing spurious notifications
>     
>     In commit:
>     
>       commit a2757c4ed693cef4ecc4dcdcb2518353eb6b3c3f
>       Date:   Wed Mar 16 15:08:22 2022 +0000
>     
>           gdb/mi: consistently notify user when GDB/MI client uses -thread-select
>     
>     Changes were made to GDB to address some inconsistencies in when
>     notifications are sent from a MI terminal to a CLI terminal (when
>     multiple terminals are in use, see new-ui command).
>     
>     Unfortunately, in order to track when the currently selected frame has
>     changed, that commit grabs a frame_info pointer before and after an MI
>     command has executed, and compares the pointers to see if the frame
>     has changed.
>     
>     This is not safe.
>     
>     If the frame cache is deleted for any reason then the frame_info
>     pointer captured before the command started, is no longer valid, and
>     any comparisons based on that pointer are undefined.
>     
>     This was leading to random test failures for some folk, see:
>     
>       https://urldefense.proofpoint.com/v2/url?u=https-3A__sourceware.org_pipermail_gdb-2Dpatches_2022-2DMarch_186867.html&d=DwIFaQ&c=sPZ6DeHLiehUHQWKIrsNwWp3t7snrE-az24ztT0w7Jc&r=WpFFGgYa98Yp-c29WHTCwU1wAGFBvszA6a4RzgpMSqc&m=fuC61Jpzxz_BRT-kfZ5C3exaG0I5TSRJt2JTG-bPdV8&s=yf5HLpnQ2WSg3e1_HMVDT5XNWSyoRYDpTsxso1TMQOE&e= 
>     
>     This commit changes GDB so we no longer hold frame_info pointers, but
>     instead store the frame_id and frame_level, this is safe even when the
>     frame cache is flushed.
> 
> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> index abd033b22ae..4f32bb58939 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -1974,27 +1974,40 @@ struct user_selected_context
>  {
>    /* Constructor.  */
>    user_selected_context ()
> -    : m_previous_ptid (inferior_ptid),
> -      m_previous_frame (deprecated_safe_get_selected_frame ())
> -  { /* Nothing.  */ }
> +    : m_previous_ptid (inferior_ptid)
> +  {
> +    save_selected_frame (&m_previous_frame_id, &m_previous_frame_level);
> +  }
>  
>    /* Return true if the user selected context has changed since this object
>       was created.  */
>    bool has_changed () const
>    {
> +    /* Grab details of the currently selected frame, for comparison.  */
> +    frame_id current_frame_id;
> +    int current_frame_level;
> +    save_selected_frame (&current_frame_id, &current_frame_level);
> +
> +    /* If we end up trying to compare two invalid frame-id's then these
> +       will always report themselves as not equal.  However, if we are at
> +       the top-most level of the stack then we don't want to consider this
> +       as a frame change.  */
>      return ((m_previous_ptid != null_ptid
>  	     && inferior_ptid != null_ptid
>  	     && m_previous_ptid != inferior_ptid)
> -	    || m_previous_frame != deprecated_safe_get_selected_frame ());
> +	    || current_frame_level != m_previous_frame_level
> +	    || (current_frame_level != -1
> +		&& !frame_id_eq (current_frame_id, m_previous_frame_id)));
>    }
>  private:
>    /* The previously selected thread.  This might be null_ptid if there was
>       no previously selected thread.  */
>    ptid_t m_previous_ptid;
>  
> -  /* The previously selected frame.  This might be nullptr if there was no
> -     previously selected frame.  */
> -  frame_info *m_previous_frame;
> +  /* The previously selected frame.  The frame_id might be null_frame_id
> +     if no frame is currently selected.  */
> +  frame_id m_previous_frame_id;
> +  int m_previous_frame_level;
>  };
>  
>  static void
>