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

Andrew Burgess aburgess@redhat.com
Wed Mar 23 19:11:28 GMT 2022


Andrew Burgess <aburgess@redhat.com> writes:

> 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.

Also, this patch should definitely be back ported to the GDB 12 branch.

Thanks,
Andrew

>
> 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://sourceware.org/pipermail/gdb-patches/2022-March/186867.html
>     
>     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



More information about the Gdb-patches mailing list