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 50C6A385841D for ; Thu, 19 Jan 2023 10:31:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 50C6A385841D 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 30J9ELUc009811; Thu, 19 Jan 2023 11:31:22 +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=ggssMNINOYFQJBPC/9j+Umy9L6xlPGt5ZaWzdl7tdoM=; b=1i2nb0oyUnBxQsWx8wVkVTmRikkKNtK1uXQnEhy1ZJ0SYHHsWp+eRZ09w3EOcW2tINKa gKbgtbe5wSoIWspF6rkKvdlvxs+WRelR/ZG5p7ns+gdIXbig2JbFAUsOPB3uZr996H7I CwbW8cu/LIrsTN+XZ9icowpp64815J5IEAXIXBBf13xRPPvL+FEx9ihIQWlY2DQlGDg5 jkvaQeA+5rbeoR//XNbABpFcAk8drjWV1sl8fLUiueCJRCzDRscXOoPYoXS8Frk3JZQR yS2ePhNfcseg6k5LuPRJE4T996wn2k7cxFUhYpfUBmI7tVBa8id+nDcszFjLxVr3SUbM Cg== 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 3n3mm6wub2-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 19 Jan 2023 11:31:21 +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 A83FB10003B; Thu, 19 Jan 2023 11:31:14 +0100 (CET) Received: from Webmail-eu.st.com (shfdag1node3.st.com [10.75.129.71]) by euls16034.sgp.st.com (STMicroelectronics) with ESMTP id A1A522122F4; Thu, 19 Jan 2023 11:31:14 +0100 (CET) Received: from [10.252.28.76] (10.252.28.76) 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; Thu, 19 Jan 2023 11:31:11 +0100 Message-ID: <27594754-50fe-10a5-ee41-3d20dae411cb@foss.st.com> Date: Thu, 19 Jan 2023 11:31:10 +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 v2 3/4] gdb: dwarf2 generic implementation for caching function data Content-Language: en-US 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> <878rhz1xzr.fsf@tromey.com> From: Torbjorn SVENSSON In-Reply-To: <878rhz1xzr.fsf@tromey.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.252.28.76] 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-19_07,2023-01-19_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-18 19:47, 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. > >> 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. > >> +struct dwarf2_frame_fn_data >> +{ > > This struct should have some kind of introductory comment, as should the > fields. Fixed in v3. > >> + struct value *(*fn) (frame_info_ptr this_frame, void **this_cache, >> + int regnum); > > Seems like this should use the fn_prev_register typedef. (But see below.) > >> + void *data; >> + struct dwarf2_frame_fn_data* next; > > Wrong "*" placement. Fixed in v3. > >> +void *dwarf2_frame_get_fn_data (frame_info_ptr this_frame, void **this_cache, >> + fn_prev_register fn) > > Wrong formatting. > >> +void *dwarf2_frame_allocate_fn_data (frame_info_ptr this_frame, >> + void **this_cache, >> + fn_prev_register fn, unsigned long size) > > Wrong formatting. I think the format is correct, but it looks strange in the mail as the '+' sign is "eaten" by the first tab. Looking in my editor for the source code, it looks correct. >> +{ >> + struct dwarf2_frame_fn_data *fn_data = nullptr; >> + struct dwarf2_frame_cache *cache >> + = dwarf2_frame_cache (this_frame, this_cache); >> + >> + /* First try to find an existing object. */ >> + void *data = dwarf2_frame_get_fn_data (this_frame, this_cache, fn); >> + if (data) >> + return data; >> + >> + /* No object found, lets create a new instance. */ >> + fn_data = FRAME_OBSTACK_ZALLOC (struct dwarf2_frame_fn_data); >> + fn_data->fn = fn; >> + fn_data->data = frame_obstack_zalloc (size); >> + fn_data->next = cache->fn_data; >> + cache->fn_data = fn_data; >> + >> + return fn_data->data; > > This API seems a bit weird to me. > > It seems like the 'fn' parameter is never really used. It's maybe just > a sort of cookie. But if so, I think it would be better to just use a > 'const void *' or 'const char *' or something like that. (A string is > nice because then it can also be seen in the debugger and give a clue > where it came from.) > > Ok, I dug up the follow-up patch and indeed it is just a cookie. I > think naming it as such and changing the type would make this more > clear. Yes, it's the cookie/key to identify the right object for the prev_register function. In v3, I've renamed the variable (and the member) to be called "cookie" instead if that's better. The "fn" name came from the struct where it was originally defined. The cookie/key could be a string, but then it should be auto-generated when calling the functions rather than letting the user type it. The reason is that if the user types it, it's less clear what the consequences of reusing it will be. Also, if we go for a string, it would consume more memory than having a function pointer (not that memory should be an issue, but anyway...). If there is a function pointer, doesn't GDB lookup the symbol for the function pointer if there are debug symbols in the application? > > Also in the follow-up I see that it calls dwarf2_frame_fn_data first. > So if you're going to go that route, then it seems that > dwarf2_frame_allocate_fn_data does not need to find an existing > object -- it can just assert there isn't one. What happens if there are 2 prev_register functions that both wants some custom data? With the approach I have (looping over the list and returning the matching data for the cookie/key) would allow us to reuse the existing object rather than create a new one. I could change the block that checks if there was a match and returns it to an assert, but what would the benefit be? Instead of reusing the existing object, we would crash GDB. Is this better? > >> + >> +/* 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, >> + void **this_cache, >> + fn_prev_register fn); > > IMO both of these could use a longer comment. From this it's impossible > to tell what the point of them is. Improved in v3. > > thanks, > Tom Kind regards, Torbjörn + Yvan