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

[Andrew Burgess <aburgess@redhat.com>]

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.

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