From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx07-00178001.pphosted.com (mx07-00178001.pphosted.com [185.132.182.106]) by sourceware.org (Postfix) with ESMTPS id 820053955C9D for ; Thu, 2 Jun 2022 09:20:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 820053955C9D Received: from pps.filterd (m0241204.ppops.net [127.0.0.1]) by mx07-00178001.pphosted.com (8.17.1.5/8.17.1.5) with ESMTP id 2527vniW004986; Thu, 2 Jun 2022 11:20:23 +0200 Received: from beta.dmz-eu.st.com (beta.dmz-eu.st.com [164.129.1.35]) by mx07-00178001.pphosted.com (PPS) with ESMTPS id 3gbc50xk2p-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 02 Jun 2022 11:20:23 +0200 Received: from euls16034.sgp.st.com (euls16034.sgp.st.com [10.75.44.20]) by beta.dmz-eu.st.com (STMicroelectronics) with ESMTP id 5B943100034; Thu, 2 Jun 2022 11:20:22 +0200 (CEST) Received: from Webmail-eu.st.com (shfdag1node2.st.com [10.75.129.70]) by euls16034.sgp.st.com (STMicroelectronics) with ESMTP id 368162171F9; Thu, 2 Jun 2022 11:20:22 +0200 (CEST) Received: from gnbcxd0114.gnb.st.com (10.75.127.49) by SHFDAG1NODE2.st.com (10.75.129.70) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.2308.20; Thu, 2 Jun 2022 11:20:21 +0200 Date: Thu, 2 Jun 2022 11:20:20 +0200 From: Yvan Roux To: CC: Luis Machado , Torbjorn SVENSSON , Christophe Lyon Subject: Re: [PATCH 1/3] gdb/arm: Set the correct address to the FPU register on Message-ID: <20220602092020.GA20273@gnbcxd0114.gnb.st.com> References: <20220516135454.GA27993@gnbcxd0114.gnb.st.com> <20220516135803.GB27993@gnbcxd0114.gnb.st.com> <8596b573-ad8e-df22-592d-b342c89a315e@arm.com> <98ee6a2d-1af4-404f-bf47-9e730e84642a@arm.com> <10c5644f-ad8a-2d9f-5b9e-c383abfaa27a@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) X-Originating-IP: [10.75.127.49] X-ClientProxiedBy: SFHDAG2NODE3.st.com (10.75.127.6) To SHFDAG1NODE2.st.com (10.75.129.70) X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.874,Hydra:6.0.517,FMLib:17.11.64.514 definitions=2022-06-02_01,2022-06-01_01,2022-02-23_01 X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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, 02 Jun 2022 09:20:31 -0000 Thnaks for the reviews, this patch is obsoleted by the coming one. On Thu, May 19, 2022 at 03:24:46PM +0200, Torbjorn SVENSSON wrote: > Hello, > > > -----Original Message----- > > From: Luis Machado > > Sent: den 19 maj 2022 10:17 > > To: Torbjorn SVENSSON ; Christophe Lyon > > ; Yvan ROUX - foss ; > > gdb-patches@sourceware.org > > Subject: Re: [PATCH 1/3] gdb/arm: Set the correct address to the FPU register > > on > > > > On 5/18/22 20:24, Torbjorn SVENSSON wrote: > > > Hello, > > > > > >> -----Original Message----- > > >> From: Luis Machado > > >> Sent: den 17 maj 2022 17:45 > > >> To: Torbjorn SVENSSON ; Christophe Lyon > > >> ; Yvan ROUX - foss > > ; > > >> gdb-patches@sourceware.org > > >> Subject: Re: [PATCH 1/3] gdb/arm: Set the correct address to the FPU > > >> register on > > >> > > >> On 5/17/22 10:49, Torbjorn SVENSSON via Gdb-patches wrote: > > >>> Hello, > > >>> > > >>> > > >>> ST Restricted > > >>> > > >>>> -----Original Message----- > > >>>> From: Christophe Lyon > > >>>> Sent: den 16 maj 2022 16:47 > > >>>> To: Yvan ROUX - foss ; gdb- > > >>>> patches@sourceware.org > > >>>> Cc: Torbjorn SVENSSON > > >>>> Subject: Re: [PATCH 1/3] gdb/arm: Set the correct address to the FPU > > >>>> register on > > >>>> > > >>>> Hi! > > >>>> > > >>>> > > >>>> On 5/16/22 15:58, Yvan Roux via Gdb-patches wrote: > > >>>>> Registers offsets weren't computed from SP address. > > >>>>> > > >>>>> Signed-off-by: Torbj�rn SVENSSON > > >>>>> Signed-off-by: Yvan Roux > > >>>>> --- > > >>>>> gdb/arm-tdep.c | 2 +- > > >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > > >>>>> > > >>>>> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c > > >>>>> index 49664093f00..c37254c2ce1 100644 > > >>>>> --- a/gdb/arm-tdep.c > > >>>>> +++ b/gdb/arm-tdep.c > > >>>>> @@ -3475,7 +3475,7 @@ arm_m_exception_cache (struct frame_info > > >>>> *this_frame) > > >>>>> if (tdep->have_sec_ext && !default_callee_register_stacking) > > >>>>> { > > >>>>> /* Handle floating-point callee saved registers. */ > > >>>>> - fpu_regs_stack_offset = 0x90; > > >>>>> + fpu_regs_stack_offset = unwound_sp + 0x90; > > >>>> > > >>>> Sorry, it looks like this was my mistake when I committed > > ef273377587d. > > >>>> > > >>>> I haven't checked the manual, and I may have forgotten, but shouldn't > > >>>> this be > > >>>> fpu_regs_stack_offset = unwound_sp + sp_r0_offset + 0x90? > > >>> > > >>> As I see it, it can be either > > >>> fpu_regs_stack_offset = unwound_sp + 0x90; > > >>> or > > >>> fpu_regs_stack_offset = unwound_sp + sp_r0_offset + 0x68; > > >>> The offset values (0x90 and 0x68) can be seen in the "Stack Frame" > > chapter > > >> at https://developer.arm.com/documentation/100701/0200/Exception- > > >> handlers > > >> > > >> It would be nice to #define these constants and put some comments > > >> pointing at this documentation. > > > > > > While it would be "nice" to have defines, the defines would just have a > > single use and I think it might cause more clobber than it helps. > > > > The problem here is that 0x90 doesn't translate to anything meaningful. A > > constant would provide an anchor that we can > > document properly. > > I agree that this would be a good thing if it were only the 0x90 constant, but the code has ever 4th value between 0x00 and 0xD0, so it's a lot of constants that will only be used in one location of the code. > Would you like this changeset to add constants for every one of these values? > > > If you're looking at this for the first time, it is hard to understand where it is > > coming from. But it is clearly incorrect now, so > > we have a chance to fix this and document it properly. > > > > > Maybe a compromise would be to extend the function comment to point to > > the documentation and mention that the offset values are taken from it? > > > > What would be the practical difference? GDB is getting more and more C++ > > these days, and some of the code tends to be more verbose. We want the > > code to be easy to > > maintain and read. > > If you think it will be easier to maintain, then we can obviously add the constants. > Please let me know what you think. > > > >>> Based on the rest of the code, maybe "unwound_sp + sp_r0_offset + > > >> 0x68" is the preferred expression here as it removes the "integer callee > > >> saved" part from the equation. > > >>> > > >>>> How did you test this? Does your patch actually work? > > >>> > > >>> I've not tested the code on target (I don't know how to reliably trigger > > the > > >> code), I've just looked at how the rest of the register values are handled in > > >> the same block of code in GDB and they are all using the unwound_sp > > >> variable to identify the appropriate address. > > >> > > >> Was this spotted on visual inspection only? Would it be possible to craft > > some > > >> code that runs into an exception and then forces GDB to go through the > > >> frame restoring the registers? > > > > > > It was spotted using visual inspection yes. > > > I've been trying to create something that would reliably trigger this, but I'm > > not certain of what kind of scenario that triggers the code. > > > > Ok. So this is not related to the bug you reported, it is a separate fix to which > > we don't have a reproducer. Could we have some sort of validation before > > pursuing it then? > > The bug report is about the remaining (known) problems with the unwinding of the stack frames on Cortex-M33, and this particular issue was spotted when looking at the code to try to identify what was the reason the unwind was not correct. > The fix in this patch is really a fix for an issue introduced when the previous patches for Cortex-M33 unwinding were accepted. > > Kind regards, > Torbjörn > -- Y.