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 557DE3858C1F for ; Tue, 29 Nov 2022 15:19:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 557DE3858C1F Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=foss.st.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=foss.st.com Received: from pps.filterd (m0241204.ppops.net [127.0.0.1]) by mx07-00178001.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 2ATEK6hZ001333; Tue, 29 Nov 2022 16:19:41 +0100 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=foss.st.com; h=message-id : date : mime-version : subject : to : cc : references : from : in-reply-to : content-type : content-transfer-encoding; s=selector1; bh=N9CKEzRS67RWWbtmtFkkZfOn5EVDkgPjJHXS1jxafmM=; b=efjqodImfcS/u1WGjGPviwa1VxWSNvzPy3XM2Gwn0B+xLOukFeIgPOy9WaCnTykmbagJ zAiDfw529GOlgZFb05fOp7inHfcqCAiO/Vf09Ml3jvYRcXw6Hd9ZXJO8g3hmhRE3w7wD NxaHPmM5IT7PzcgR+7rovy6hxLN3tOfb428Otg++ZmCgBW5mYwOkQILv94ZgoIEspT3A gqHnVz4DBIocjWn8xPw9/xeitoE1WC1sgnpPC2TtJPnAGCcR5aTA24HkkWSchsRfDRSj DI3n6YduRiKy8kf+QOzX0nDxi4wZ0eUkNdEHmFs/sCowXlu2j7/D1OF3lO87zu51w0Xd hw== 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 3m5krggbrv-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 29 Nov 2022 16:19:41 +0100 Received: from euls16034.sgp.st.com (euls16034.sgp.st.com [10.75.44.20]) by beta.dmz-eu.st.com (STMicroelectronics) with ESMTP id D2CC410002A; Tue, 29 Nov 2022 16:19:36 +0100 (CET) Received: from Webmail-eu.st.com (shfdag1node3.st.com [10.75.129.71]) by euls16034.sgp.st.com (STMicroelectronics) with ESMTP id CC58B22A6D8; Tue, 29 Nov 2022 16:19:36 +0100 (CET) Received: from [10.210.55.83] (10.210.55.83) by SHFDAG1NODE3.st.com (10.75.129.71) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.6; Tue, 29 Nov 2022 16:19:36 +0100 Message-ID: <6bf733c9-ed5f-76e6-d1b8-35bf4eff9720@foss.st.com> Date: Tue, 29 Nov 2022 16:19:35 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.5.0 Subject: Re: [PATCH v2 4/4] gdb/arm: Use new dwarf2 function cache Content-Language: en-US To: Luis Machado , CC: , Yvan Roux References: <20221118155252.113476-1-torbjorn.svensson@foss.st.com> <20221118155252.113476-5-torbjorn.svensson@foss.st.com> From: Torbjorn SVENSSON In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.210.55.83] X-ClientProxiedBy: EQNCAS1NODE4.st.com (10.75.129.82) To SHFDAG1NODE3.st.com (10.75.129.71) X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.219,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1 definitions=2022-11-29_10,2022-11-29_01,2022-06-22_01 X-Spam-Status: No, score=-10.5 required=5.0 tests=BAYES_00,BODY_8BITS,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,GIT_PATCH_0,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,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: Hi, I've had a long discussion with Luis on IRC regarding the points mentioned here, but I'll reply to the list now in order to get more eyes on the topic. On 2022-11-21 22:04, Luis Machado wrote: > Hi, > > On 11/18/22 15:52, Torbjörn SVENSSON wrote: >> This patch resolves the performance issue reported in pr/29738 by >> caching the values for the stack pointers for the inner frame.  By >> doing so, the impact can be reduced to checking the state and >> returning the appropriate value. >> >> Signed-off-by: Torbjörn SVENSSON >> Signed-off-by: Yvan Roux >> --- >>   gdb/arm-tdep.c | 96 +++++++++++++++++++++++++++++++++----------------- >>   1 file changed, 64 insertions(+), 32 deletions(-) >> >> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c >> index c011b2aa973..59cd0964d96 100644 >> --- a/gdb/arm-tdep.c >> +++ b/gdb/arm-tdep.c >> @@ -3953,6 +3953,18 @@ struct frame_base arm_normal_base = { >>     arm_normal_frame_base >>   }; >> +struct arm_dwarf2_prev_register_cache >> +{ >> +  /* Cached value of the coresponding stack pointer for the inner >> frame.  */ > > coresponding -> corresponding > >> +  CORE_ADDR sp; >> +  CORE_ADDR msp; >> +  CORE_ADDR msp_s; >> +  CORE_ADDR msp_ns; >> +  CORE_ADDR psp; >> +  CORE_ADDR psp_s; >> +  CORE_ADDR psp_ns; >> +}; >> + > > Given SP is the cfa, do we need to cache it here? As I said off-list, it's not the value of sp, msp etc, it's the value of the inner frame, so what is actually "cached" here is basically the state of the frame, not the values. The cache could be simplified in a few ways, but before doing the polishing, I would like to know if it's an acceptable way to implement the fix for the performance issue. > >>   static struct value * >>   arm_dwarf2_prev_register (frame_info_ptr this_frame, void **this_cache, >>                 int regnum) >> @@ -3961,6 +3973,48 @@ arm_dwarf2_prev_register (frame_info_ptr >> this_frame, void **this_cache, >>     arm_gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); >>     CORE_ADDR lr; >>     ULONGEST cpsr; >> +  struct arm_dwarf2_prev_register_cache *cache >> +    = (struct arm_dwarf2_prev_register_cache *) >> dwarf2_frame_get_fn_data ( >> +      this_frame, this_cache, arm_dwarf2_prev_register); >> + >> +  if (!cache) >> +    { >> +      const unsigned int size = sizeof (struct >> arm_dwarf2_prev_register_cache); >> +      cache = (struct arm_dwarf2_prev_register_cache *) >> +    dwarf2_frame_allocate_fn_data (this_frame, this_cache, >> +                       arm_dwarf2_prev_register, size); >> + >> +      if (tdep->have_sec_ext) >> +    { >> +      cache->sp >> +        = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM); > > We fetch ARM_SP_REGNUM in both legs of the conditional. How about moving > it outside of the conditional blocks? Sure. Before doing more here, I would like to get the agreement that this is a good approach to the problem. >> + >> +      cache->msp_s >> +        = get_frame_register_unsigned (this_frame, >> +                       tdep->m_profile_msp_s_regnum); >> +      cache->msp_ns >> +        = get_frame_register_unsigned (this_frame, >> +                       tdep->m_profile_msp_ns_regnum); >> +      cache->psp_s >> +        = get_frame_register_unsigned (this_frame, >> +                       tdep->m_profile_psp_s_regnum); >> +      cache->psp_ns >> +        = get_frame_register_unsigned (this_frame, >> +                       tdep->m_profile_psp_ns_regnum); >> +    } >> +      else if (tdep->is_m) >> +    { >> +      cache->sp >> +        = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM); >> + >> +      cache->msp >> +        = get_frame_register_unsigned (this_frame, >> +                       tdep->m_profile_msp_regnum); >> +      cache->psp >> +        = get_frame_register_unsigned (this_frame, >> +                       tdep->m_profile_psp_regnum); >> +    } >> +    } >>     if (regnum == ARM_PC_REGNUM) >>       { >> @@ -4000,33 +4054,18 @@ arm_dwarf2_prev_register (frame_info_ptr >> this_frame, void **this_cache, >>         if (tdep->have_sec_ext) >>       { >> -      CORE_ADDR sp >> -        = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM); >> -      CORE_ADDR msp_s >> -        = get_frame_register_unsigned (this_frame, >> -                       tdep->m_profile_msp_s_regnum); >> -      CORE_ADDR msp_ns >> -        = get_frame_register_unsigned (this_frame, >> -                       tdep->m_profile_msp_ns_regnum); >> -      CORE_ADDR psp_s >> -        = get_frame_register_unsigned (this_frame, >> -                       tdep->m_profile_psp_s_regnum); >> -      CORE_ADDR psp_ns >> -        = get_frame_register_unsigned (this_frame, >> -                       tdep->m_profile_psp_ns_regnum); >> - >>         bool is_msp = (regnum == tdep->m_profile_msp_regnum) >> -        && (msp_s == sp || msp_ns == sp); >> +        && (cache->msp_s == cache->sp || cache->msp_ns == cache->sp); >>         bool is_msp_s = (regnum == tdep->m_profile_msp_s_regnum) >> -        && (msp_s == sp); >> +        && (cache->msp_s == cache->sp); >>         bool is_msp_ns = (regnum == tdep->m_profile_msp_ns_regnum) >> -        && (msp_ns == sp); >> +        && (cache->msp_ns == cache->sp); >>         bool is_psp = (regnum == tdep->m_profile_psp_regnum) >> -        && (psp_s == sp || psp_ns == sp); >> +        && (cache->psp_s == cache->sp || cache->psp_ns == cache->sp); >>         bool is_psp_s = (regnum == tdep->m_profile_psp_s_regnum) >> -        && (psp_s == sp); >> +        && (cache->psp_s == cache->sp); >>         bool is_psp_ns = (regnum == tdep->m_profile_psp_ns_regnum) >> -        && (psp_ns == sp); >> +        && (cache->psp_ns == cache->sp); >>         override_with_sp_value = is_msp || is_msp_s || is_msp_ns >>           || is_psp || is_psp_s || is_psp_ns; >> @@ -4034,17 +4073,10 @@ arm_dwarf2_prev_register (frame_info_ptr >> this_frame, void **this_cache, >>       } >>         else if (tdep->is_m) >>       { >> -      CORE_ADDR sp >> -        = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM); >> -      CORE_ADDR msp >> -        = get_frame_register_unsigned (this_frame, >> -                       tdep->m_profile_msp_regnum); >> -      CORE_ADDR psp >> -        = get_frame_register_unsigned (this_frame, >> -                       tdep->m_profile_psp_regnum); >> - >> -      bool is_msp = (regnum == tdep->m_profile_msp_regnum) && (sp == >> msp); >> -      bool is_psp = (regnum == tdep->m_profile_psp_regnum) && (sp == >> psp); >> +      bool is_msp = (regnum == tdep->m_profile_msp_regnum) >> +        && (cache->sp == cache->msp); >> +      bool is_psp = (regnum == tdep->m_profile_psp_regnum) >> +        && (cache->sp == cache->psp); >>         override_with_sp_value = is_msp || is_psp; >>       } > > As we've discussed off-list, I think we can reduce the number of > get_frame_register_unsigned calls we do for each call to > arm_dwarf2_prev_register by using some conditionals. Likely, but let's focus on the dwarf2 part of the patch first and do the polishing after, okay? Kind regards, Torbjörn