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 A6C2A3858D3C for ; Thu, 10 Nov 2022 16:30:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A6C2A3858D3C Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=polymtl.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=polymtl.ca 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 2AAGUNF9004218 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 10 Nov 2022 11:30:28 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 2AAGUNF9004218 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=polymtl.ca; s=default; t=1668097829; bh=SSxGsFG1PBGjYGY++OXkgQv9MIIB7cJncNEJCwEsavo=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=qE5OgKDuFp407RjrWTaOVvbxurMPAYhPirDfO/R1eME7WTOo9PxdL22DqGSKWKgXV qsneZZSzQFLp8UoEU9gM+y6rewtnGp1Iy0KwmBiwUk/eFjfIyawCL7VU8ilQgGLpwT LIL1Zwso195SF/BfGr8TiiMX4yrKPPbU+mRbTd74= Received: from [10.0.0.11] (unknown [217.28.27.60]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 73F211E0CB; Thu, 10 Nov 2022 11:30:23 -0500 (EST) Message-ID: <8d57bfca-47de-56c1-e7f2-d4c5980becce@polymtl.ca> Date: Thu, 10 Nov 2022 11:30:22 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.1 Subject: Re: [PATCH 7/7] gdb: add special handling for frame level 0 in frame_info_ptr Content-Language: en-US To: Bruno Larsen , gdb-patches@sourceware.org Cc: Simon Marchi References: <20221107155310.2590069-1-simon.marchi@polymtl.ca> <20221107155310.2590069-7-simon.marchi@polymtl.ca> <24a4edb5-4081-7b34-a5ca-291d51decea4@redhat.com> <4ea9edd4-b268-5e8c-b697-b5dd09e2e25a@polymtl.ca> <306755c6-eeb3-63a1-4d9a-a4678d13b8a4@redhat.com> From: Simon Marchi In-Reply-To: <306755c6-eeb3-63a1-4d9a-a4678d13b8a4@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Thu, 10 Nov 2022 16:30:23 +0000 X-Spam-Status: No, score=-3036.9 required=5.0 tests=BAYES_00,BODY_8BITS,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 autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 11/10/22 11:28, Bruno Larsen wrote: > On 08/11/2022 17:19, Simon Marchi wrote: >> On 11/8/22 05:40, Bruno Larsen wrote: >>> On 07/11/2022 16:53, Simon Marchi via Gdb-patches wrote: >>>> From: Simon Marchi >>>> >>>> I noticed this problem while preparing the initial submission for the >>>> ROCm GDB port.  One particularity of this patch set is that it does not >>>> support unwinding frames, that requires support of some DWARF extensions >>>> that will come later.  It was still possible to run to a breakpoint and >>>> print frame #0, though. >>>> >>>> When rebasing on top of the frame_info_ptr work, GDB started tripping on >>>> a prepare_reinflate call, making it not possible anymore to event print >>>> the frame when stopping on a breakpoint.  One thing to know about frame >>>> 0 is that its id is lazily computed when something requests it through >>>> get_frame_id.  See: >>>> >>>>     https://gitlab.com/gnutools/binutils-gdb/-/blob/23912acd402f5af9caf91b257e5209ec4c58a09c/gdb/frame.c#L2070-2080 >>>> >>>> So, up to that prepare_reinflate call, frame 0's id was not computed, >>>> and prepare_reinflate, calling get_frame_id, forces it to be computed. >>>> Computing the frame id generally requires unwinding the previous frame, >>>> which with my ROCm GDB patch fails.  An exception is thrown and the >>>> printing of the frame is simply abandonned. >>>> >>>> Regardless of this ROCm GDB problem (which is admittedly temporary, it >>>> will be possible to unwind with subsequent patches), we want to avoid >>>> prepare_reinflate to force the computing of the frame id, for the same >>>> reasons we lazily compute it in the first place. >>>> >>>> In addition, frame 0's id is subject to change across a frame cache >>>> reset.  This is why save_selected_frame and restore_selected_frame have >>>> special handling for frame 0: >>>> >>>>     https://gitlab.com/gnutools/binutils-gdb/-/blob/23912acd402f5af9caf91b257e5209ec4c58a09c/gdb/frame.c#L1841-1863 >>>> >>>> For this last reason, we also need to handle frame 0 specially in >>>> prepare_reinflate / reinflate.  Because the frame id of frame 0 can >>>> change across a frame cache reset, we must not rely on the frame id from >>>> that frame to reinflate it.  We should instead just re-fetch the current >>>> frame at that point. >>>> >>>> This patch adds a frame_info_ptr::m_cached_level field, set in >>>> frame_info_ptr::prepare_reinflate, so we can tell if a frame is frame 0. >>>> There are cases where a frame_info_ptr object wraps a sentinel frame, >>>> for which frame_relative_level returns -1, so I have chosen the value -2 >>>> to represent "invalid frame level", for when the frame_info_ptr object >>>> is empty. >>>> >>>> In frame_info_ptr::prepare_reinflate, only cache the frame id if the >>>> frame level is not 0.  It's fine to cache the frame id for the sentinel >>>> frame, it will be properly handled by frame_find_by_id later. >>>> >>>> In frame_info_ptr::reinflate, if the frame level is 0, call >>>> get_current_frame to get the target's current frame.  Otherwise, use >>>> frame_find_by_id just as before. >>>> >>>> This patch should not have user-visible changes with upstream GDB.  But >>>> it will avoid forcing the computation of frame 0's when calling >>>> prepare_reinflate.  And, well, it fixes the upcoming ROCm GDB patch >>>> series. >>>> >>>> Change-Id: I176ed7ee9317ddbb190acee8366e087e08e4d266 >>> This all makes sense. I have a small style preference below, but even if you dislike it, the code is still fine. >>> >>> Reviewed-By: Bruno Larsen >>> >>>> --- >>>>    gdb/frame-info.c | 24 ++++++++++++++++++++---- >>>>    gdb/frame-info.h | 20 ++++++++++++++++++-- >>>>    2 files changed, 38 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/gdb/frame-info.c b/gdb/frame-info.c >>>> index 584222dc490f..e3ee9f8174e1 100644 >>>> --- a/gdb/frame-info.c >>>> +++ b/gdb/frame-info.c >>>> @@ -31,7 +31,11 @@ intrusive_list frame_info_ptr::frame_list; >>>>    void >>>>    frame_info_ptr::prepare_reinflate () >>>>    { >>>> -  m_cached_id = get_frame_id (*this); >>>> +  m_cached_level = frame_relative_level (*this); >>>> +  gdb_assert (m_cached_level >= -1); >>> Since you have declared invalid_level = -2 for this class, I feel like it would be more better to have the assert be >>> >>> gdb_assert (m_cached_level > invalid_level); >> This form assumes that invalid_level is -2, defeating the purpose to >> have the abstraction in the first place.  If we changed invalid_level to >> be INT_MAX, for intsance, the assertion wouldn't be right anymore. >> >>> This way there is no need to wonder why -1 is a valid level, and makes it easier to grep for the comment in the file, should someone want to know. >> In my vision of things, the sentinel frame having level -1 is well >> known, because it's the frame just below the current frame, which is >> known to have level 0.  So while it looks like a magic random value, >> it's not really.  The numerical value has a meaning.  We wouldn't want >> to change the sentinel frame's level value to be any other arbitrary >> numerical value. > Ok, I see your point. It does make sense when you put it like that. >> >> Here, I can just drop the assert.  It's basically just checking that >> frame_relative_level didn't return something that doesn't make sense. >> But there's no reason for frame_relative_level to return something that >> doesn't make sense in the first place.  Other callers of >> frame_relative_level don't do this assert, they just trust that >> frame_relative_level returns something that makes sense, nothing really >> different here. >> >>>> + >>>> +  if (m_cached_level != 0) >>>> +    m_cached_id = get_frame_id (*this); >>>>    } >>>>      /* See frame-info-ptr.h.  */ >>>> @@ -39,9 +43,21 @@ frame_info_ptr::prepare_reinflate () >>>>    void >>>>    frame_info_ptr::reinflate () >>>>    { >>>> -  gdb_assert (frame_id_p (m_cached_id)); >>>> +  gdb_assert (m_cached_level >= -1); >>> Likewise >> Here, I could add a comment like: >> >>    /* Ensure we have a valid frame level, indicating prepare_reinflate >>       was called.  */ > > Yeah, this comment fixes any possible confusion. You convinced me :-) > > Reviewed-By: Bruno Larsen Thanks to you and Tom for your review on all patches, I will push shortly. Simon