From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id ANGFJikjhmNHQR4AWB0awg (envelope-from ) for ; Tue, 29 Nov 2022 10:20:09 -0500 Received: by simark.ca (Postfix, from userid 112) id 9B53E1E11E; Tue, 29 Nov 2022 10:20:09 -0500 (EST) Authentication-Results: simark.ca; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.a=rsa-sha256 header.s=default header.b=cFNIwjPc; dkim-atps=neutral X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-8.0 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,NICE_REPLY_A, RCVD_IN_DNSWL_HI,RDNS_DYNAMIC,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 Received: from sourceware.org (ip-8-43-85-97.sourceware.org [8.43.85.97]) (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 ESMTPS id 1602F1E0D3 for ; Tue, 29 Nov 2022 10:20:09 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 562FC3858C36 for ; Tue, 29 Nov 2022 15:20:08 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 562FC3858C36 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1669735208; bh=N9CKEzRS67RWWbtmtFkkZfOn5EVDkgPjJHXS1jxafmM=; h=Date:Subject:To:CC:References:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=cFNIwjPcp9F1lkf6PxTB2127/xajsPX8LIpZ15FHpbqedIy+sl5EGlc/Qf6RXxI69 hh99TThM2VeChtAQRp5U/eIIVWBKq83dOF0/bhgoWCren4RyZxBsXht6a6OLYQacW7 qnfPUTeURsa3cAL4J/83OPueeKapzhZPi6l9sn64= 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 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 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> 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-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: , From: Torbjorn SVENSSON via Gdb-patches Reply-To: Torbjorn SVENSSON Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" 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