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

Andrew Burgess via Gdb-patches
2022-03-23T19:10:09Z

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