From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id C56773858C2C for ; Thu, 24 Mar 2022 14:47:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C56773858C2C Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 22OElUsW014221 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 24 Mar 2022 10:47:35 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 22OElUsW014221 Received: from [172.16.0.95] (192-222-180-24.qc.cable.ebox.net [192.222.180.24]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 35F321F0BB; Thu, 24 Mar 2022 10:47:30 -0400 (EDT) Message-ID: Date: Thu, 24 Mar 2022 10:47:29 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.0 Subject: Re: [pushed] gdb/mi: consistently notify user when GDB/MI client uses -thread-select Content-Language: tl To: Andrew Burgess , Jan Vrany , Simon Marchi , gdb-patches@sourceware.org References: <20220316150914.1254897-1-jan.vrany@labware.com> <78570a7f-f6c0-5e1b-4178-c9ce401feab7@simark.ca> <897bd508133687e4d30994deecdd87786d71334d.camel@labware.com> <0bcb4286c01054c30c3539c76bd6426ff3b333eb.camel@labware.com> <14d27757ce39bcc7a19f74508e689acfa7fa33c2.camel@labware.com> <82992b26-8010-d7cd-8c27-5e07350d43ab@polymtl.ca> <87fsn8scy1.fsf@redhat.com> From: Simon Marchi In-Reply-To: <87fsn8scy1.fsf@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Thu, 24 Mar 2022 14:47:30 +0000 X-Spam-Status: No, score=-3039.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 24 Mar 2022 14:47:40 -0000 On 2022-03-23 15:09, Andrew Burgess wrote: > Jan Vrany via Gdb-patches 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 > 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 (¤t_frame_id, ¤t_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))); That's a subjective request, but could you rewrite this as separate statements, like this? I find this easier to parse and debug than one big statement. /* Did the selected thread change? */ if (m_previous_ptid != null_ptid && inferior_ptid != null_ptid && m_previous_ptid != inferior_ptid) return true; /* Did the selected frame level change? */ if (current_frame_level != m_previous_frame_level) return true; /* Did the selected frame id change? */ if (current_frame_level != -1 && !frame_id_eq (current_frame_id, m_previous_frame_id)) return true; return false; > } > 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. */ Maybe keep using the past tense? "if no frame was selected" (it implicitly says: "at the time of construction", although that could be made explicit). I would also use "is" instead of "might": if there was no selected frame at the time of construction, then this is null_frame_id, there's no doubt about it. Simon > + frame_id m_previous_frame_id; > + int m_previous_frame_level; > }; > > static void >