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 3EF593858D33 for ; Wed, 28 Dec 2022 16:16:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3EF593858D33 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 2BS9bRoX008835; Wed, 28 Dec 2022 17:16:47 +0100 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=foss.st.com; h=message-id : date : mime-version : from : subject : to : cc : references : in-reply-to : content-type : content-transfer-encoding; s=selector1; bh=vOpCkln5qikuRhVxVQ39/H/5eSuCa+W55N6VtEucwX8=; b=Q8TQq2wEdmsu/jAXCXC9jwY69hsw56ZnH04HufTKfaEO6C0LPpNK+L53U//saoCrzLyS frtzQ4EktCeGqorJpHooj7t2HAR9b9Mk2Up0Zx3mNPD9bGw2MJkWWnG4cSvLCypR9zar O8IMM0bhJM8ldHpQO5Ns3wBSzlN+EQizAniSPbLldCy6+M07nfwryMo8KKSIFr1qml1s XWZzBnIs+wwuGgz4Hl6i9sWUQzEW4/NLsgHytII2sYDZmIQxdTXRGvhq8ggkLNJCL/an doJK2YRNwGldjEXgp329d6d7/pRA55Z7JYF7FeGT1kwoXPgoLRgXu6uHfVAIqDhaez7y Vw== 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 3mns6pjkmx-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 28 Dec 2022 17:16:47 +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 24408100038; Wed, 28 Dec 2022 17:16:46 +0100 (CET) Received: from Webmail-eu.st.com (shfdag1node3.st.com [10.75.129.71]) by euls16034.sgp.st.com (STMicroelectronics) with ESMTP id D4B1B2278A1; Wed, 28 Dec 2022 17:16:05 +0100 (CET) Received: from [10.252.10.162] (10.252.10.162) 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; Wed, 28 Dec 2022 17:16:04 +0100 Message-ID: Date: Wed, 28 Dec 2022 17:16:03 +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 From: Torbjorn SVENSSON Subject: Re: [PATCH v2 3/4] gdb: dwarf2 generic implementation for caching function data To: Tom Tromey , =?UTF-8?Q?Torbj=c3=b6rn_SVENSSON_via_Gdb-patches?= CC: , , Yvan Roux References: <20221118155252.113476-1-torbjorn.svensson@foss.st.com> <20221118155252.113476-4-torbjorn.svensson@foss.st.com> <87ili5kcu6.fsf@tromey.com> Content-Language: en-US In-Reply-To: <87ili5kcu6.fsf@tromey.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.252.10.162] 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.205,Aquarius:18.0.923,Hydra:6.0.545,FMLib:17.11.122.1 definitions=2022-12-28_13,2022-12-28_02,2022-06-22_01 X-Spam-Status: No, score=-6.4 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 2022-12-20 22:02, Tom Tromey wrote: >>>>>> Torbjörn SVENSSON via Gdb-patches writes: > >> 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. > > Thanks for the patch. > >> 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). > > Does this ever happen? If not perhaps a simpler approach would be > better. Right now; I don't know, but as the fn member in the dwarf2_frame_state_reg struct contains one function pointer per register, the architecture allows more than one function pointer per frame. If we went with a simpler solution, to only have one data block per frame, regardless of what function that is "owning" the data, then it could lead to nasty surprises if there is some unwinder that expects to be able to use more than data type... If we move the function pointer from the register scope to the frame scope, then I agree that only one data object would be needed. If we stick to having the function pointer per register, I could accept to have one data block, but somewhere, an assert should added so that the wrongful assumption mentioned above would be caught early rather than leading to strange bugs. This would mean that the type needs to be stored in the dwarf2_frame_cache struct somehow, but the type is currently internal to another compile unit. This is basically the reason why I went with the decoupled solution that is provided in this patch. > >> +struct dwarf2_frame_fn_data >> +{ > > New type should have a comment. Okay, I'll add comments, but I would like to know if this is the way to go or if there should be some alternative implementation before spending more time on this. > >> + struct value *(*fn) (frame_info_ptr this_frame, void **this_cache, >> + int regnum); > > Shouldn't this use the fn_prev_register typedef? Indeed. > >> + void *data; >> + struct dwarf2_frame_fn_data* next; > > Wrong placement of the "*", but really a lot of the code isn't following > the GNU / gdb style. Don't know why the contrib/check_GNU_style.py in the GCC source tree did not flag this. Anyway, will be fixed in v3. >> +void *dwarf2_frame_get_fn_data (frame_info_ptr this_frame, void **this_cache, >> + fn_prev_register fn) > > Normally new functions get a comment referring to the header where they > are declared. Can you point me to an example and I will do something similar for these new functions if we decide to go this way. >> + >> +/* Allocate a new instance of the function unique data. */ >> + >> +extern void *dwarf2_frame_allocate_fn_data (frame_info_ptr this_frame, >> + void **this_cache, >> + fn_prev_register fn, >> + unsigned long size); >> + >> +/* Retrieve the function unique data for this frame. */ >> + >> +extern void *dwarf2_frame_get_fn_data (frame_info_ptr this_frame, > > I think these comments could perhaps be expanded a bit. What more detail would you like to include? > > Tom Kind regards, Torbjörn