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 2E08C3858417 for ; Mon, 7 Nov 2022 16:02:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 2E08C3858417 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 2A7G2CUU028390 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 7 Nov 2022 11:02:17 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 2A7G2CUU028390 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=polymtl.ca; s=default; t=1667836937; bh=IeGyOgrXj/biJ+O7cm0IP8dWpzYr1UqFhcus5M/hJJA=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=Yu6aYP2cXDbq69t30KtzDMEUbmILa6IJNLo8X0fojVhSeCOBPqo2lLnMcIy7mSr6O 6wDUleHcr4iucJ12spiYev006LxSKlSkVzgmSRb4x9po/TS38H9EkPtZ28Zfpc3a9o XtSUPP7ALesZ7eAzbhsvLpX1oy6vlNxbjVtCmGCM= Received: from simark.localdomain (unknown [217.28.27.60]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 732501E12A; Mon, 7 Nov 2022 10:53:12 -0500 (EST) From: Simon Marchi To: gdb-patches@sourceware.org Cc: Simon Marchi Subject: [PATCH 7/7] gdb: add special handling for frame level 0 in frame_info_ptr Date: Mon, 7 Nov 2022 10:53:10 -0500 Message-Id: <20221107155310.2590069-7-simon.marchi@polymtl.ca> X-Mailer: git-send-email 2.38.1 In-Reply-To: <20221107155310.2590069-1-simon.marchi@polymtl.ca> References: <20221107155310.2590069-1-simon.marchi@polymtl.ca> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Mon, 7 Nov 2022 16:02:12 +0000 X-Spam-Status: No, score=-3189.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,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: 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 --- 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); + + 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); + + if (m_ptr != nullptr) + { + /* The frame_info wasn't invalidated, no need to reinflate. */ + return; + } + + if (m_cached_level == 0) + m_ptr = get_current_frame ().get (); + else + { + gdb_assert (frame_id_p (m_cached_id)); + m_ptr = frame_find_by_id (m_cached_id).get (); + } - if (m_ptr == nullptr) - m_ptr = frame_find_by_id (m_cached_id).get (); gdb_assert (m_ptr != nullptr); } diff --git a/gdb/frame-info.h b/gdb/frame-info.h index 1d2d4bdc7e68..3369b114326f 100644 --- a/gdb/frame-info.h +++ b/gdb/frame-info.h @@ -56,16 +56,21 @@ class frame_info_ptr : public intrusive_list_node } frame_info_ptr (const frame_info_ptr &other) - : m_ptr (other.m_ptr), m_cached_id (other.m_cached_id) + : m_ptr (other.m_ptr), + m_cached_id (other.m_cached_id), + m_cached_level (other.m_cached_level) { frame_list.push_back (*this); } frame_info_ptr (frame_info_ptr &&other) - : m_ptr (other.m_ptr), m_cached_id (other.m_cached_id) + : m_ptr (other.m_ptr), + m_cached_id (other.m_cached_id), + m_cached_level (other.m_cached_level) { other.m_ptr = nullptr; other.m_cached_id = null_frame_id; + other.m_cached_level = invalid_level; frame_list.push_back (*this); } @@ -78,6 +83,7 @@ class frame_info_ptr : public intrusive_list_node { m_ptr = other.m_ptr; m_cached_id = other.m_cached_id; + m_cached_level = other.m_cached_level; return *this; } @@ -85,6 +91,7 @@ class frame_info_ptr : public intrusive_list_node { m_ptr = nullptr; m_cached_id = null_frame_id; + m_cached_level = invalid_level; return *this; } @@ -92,8 +99,10 @@ class frame_info_ptr : public intrusive_list_node { m_ptr = other.m_ptr; m_cached_id = other.m_cached_id; + m_cached_level = other.m_cached_level; other.m_ptr = nullptr; other.m_cached_id = null_frame_id; + other.m_cached_level = invalid_level; return *this; } @@ -136,6 +145,10 @@ class frame_info_ptr : public intrusive_list_node void reinflate (); private: + /* We sometimes need to construct frame_info_ptr objects around the + sentinel_frame, which has level -1. Therefore, make the invalid frame + level value -2. */ + static constexpr int invalid_level = -2; /* The underlying pointer. */ frame_info *m_ptr = nullptr; @@ -143,6 +156,9 @@ class frame_info_ptr : public intrusive_list_node /* The frame_id of the underlying pointer. */ frame_id m_cached_id = null_frame_id; + /* The frame level of the underlying pointer. */ + int m_cached_level = invalid_level; + /* All frame_info_ptr objects are kept on an intrusive list. This keeps their construction and destruction costs reasonably small. */ -- 2.38.1