From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx07-00178001.pphosted.com (mx08-00178001.pphosted.com [91.207.212.93]) by sourceware.org (Postfix) with ESMTPS id 82D083858421 for ; Fri, 20 Jan 2023 14:12:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 82D083858421 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 (m0046661.ppops.net [127.0.0.1]) by mx07-00178001.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 30KAcsrU032445; Fri, 20 Jan 2023 15:12:51 +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=dnrLksJDReihdRj/G8bwIU05PnekDXrRgkZDo8RU6UE=; b=pb/OBcMTOiLD53Sd3YsGFLKIqqjx5cgFmmqTqwcuiLnwFqE1AlfeCcNX32la31ADAPC7 d9n6/uObG8UdM38uaTaBloVRKgXWo5KF3Hu0IxpKnCS/vra000a/uic4N4Vs3zrO46S5 PChiDZl/OupEIUwDTbCMbjzR5M1Pgdy4sOR1kXNcMyBzZHUB0KunXmSgNkeCTF5qmrFH zD8+mq+sb2vDSBJ5jePlB9KPvJWCTMIxhEQ2W1Gwvg8et6UhrLzRl/1z6+4kMbBJiae9 TzkRNrbn7UIs1p7pSYxgEOsjtZLZAy2y+lB2gCBlBklwzl9m4x67YcnY6ieaNQPC8hmn 8Q== 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 3n3m5qq12d-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 20 Jan 2023 15:12:51 +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 0AE1110002A; Fri, 20 Jan 2023 15:12:50 +0100 (CET) Received: from Webmail-eu.st.com (shfdag1node3.st.com [10.75.129.71]) by euls16034.sgp.st.com (STMicroelectronics) with ESMTP id F2D8D2207AB; Fri, 20 Jan 2023 15:12:49 +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.13; Fri, 20 Jan 2023 15:12:49 +0100 Message-ID: <1ca2f916-21be-1627-7cc8-50da1a773cb8@foss.st.com> Date: Fri, 20 Jan 2023 15:12:48 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 Subject: Re: [PATCH v3 1/2] gdb: dwarf2 generic implementation for caching function data To: Simon Marchi , CC: , , Yvan Roux References: <20230119102948.3069226-1-torbjorn.svensson@foss.st.com> Content-Language: en-US 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: SHFCAS1NODE2.st.com (10.75.129.73) To SHFDAG1NODE3.st.com (10.75.129.71) X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.219,Aquarius:18.0.930,Hydra:6.0.562,FMLib:17.11.122.1 definitions=2023-01-20_08,2023-01-20_01,2022-06-22_01 X-Spam-Status: No, score=-5.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,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: On 2023-01-19 17:53, Simon Marchi wrote: > > > On 1/19/23 05:29, Torbjörn SVENSSON via Gdb-patches wrote: >> v2 -> v3: >> >> Addressed comments from Tom in >> https://sourceware.org/pipermail/gdb-patches/2023-January/195882.html >> >> >> --- >> >> When there is no dwarf2 data for a register, a function can be called >> to provide the value of this register. In some situations, it might >> not be trivial to determine the value to return and it would cause a >> performance bottleneck to do the computation each time. >> >> This patch allows the called function to have a "cache" object that it >> can use to store some metadata between calls to reduce the performance >> impact of the complex logic. >> >> The cache object is unique for each function and frame, so if there are >> more than one function pointer stored in the dwarf2_frame_cache->reg >> array, then the appropriate pointer will be supplied (the type is not >> known by the dwarf2 implementation). >> >> dwarf2_frame_get_fn_data can be used to retrieve the function unique >> cache object. >> dwarf2_frame_allocate_fn_data can be used to allocate and retrieve the >> function unique cache object. >> >> Signed-off-by: Torbjörn SVENSSON >> Signed-off-by: Yvan Roux > > Hi, > > My main question is: this approach requires each custom prev_register > function to set up and manage its own cache. Did you consider caching > values at a higher level? > > Perhaps: > > 1. In dwarf2_frame_prev_register, just for DWARF2_FRAME_REG_FN > 2. In dwarf2_frame_prev_register, but for all kinds of registers > 3. In frame_unwind_register_value, which would cache all register values > for all frames regardless of the unwinder > > I don't know if other ways of unwinding would benefit that much from > caching, but I guess it wouldn't hurt. For instance, evaluating DWARF > expressions is probably generally not super expensive, but it might > still benefit from getting cached. And it would be nice to write the > caching code just once and have it work transparently for everything. I suppose a generic cache might be useful in the long run, but it would have an impact on other code paths and I'm not comfortable of doing this major change now. Doing this change would imply that almost everything related to unwinding is impacted in one way or another and I suppose we would need to test that. With the solution I proposed, only Arm Cortex would be impacted and the scope for testing would be rather small. I'm also aiming to resolve this for GDB13, so doing this major change is a bit late in the sprint, right? If you have a better idea on how to cache everything, like in your 3 ideas above, please don't hesitate do share the implementation. :) > >> @@ -262,4 +264,35 @@ extern int dwarf2_fetch_cfa_info (struct gdbarch *gdbarch, CORE_ADDR pc, >> const gdb_byte **cfa_start_out, >> const gdb_byte **cfa_end_out); >> >> + >> +/* Allocate a new instance of the function unique data. >> + >> + The main purpose of this custom function data object is to allow caching the >> + value of expensive lookups in the prev_register implementation. > > I would replace "in the prev_register implementation" with "in > prev_register implementations". There isn't only one prev_register > implementation AFAIK. Will do that for v4 or when merging if no v4 is needed. > > Simon Kind regards, Torbjörn