From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 2ID8CRIcyWMhNhoAWB0awg (envelope-from ) for ; Thu, 19 Jan 2023 05:31:46 -0500 Received: by simark.ca (Postfix, from userid 112) id 25CB21E128; Thu, 19 Jan 2023 05:31:46 -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=VxxsXGd2; 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 9CAF71E0D3 for ; Thu, 19 Jan 2023 05:31:45 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 471773858C53 for ; Thu, 19 Jan 2023 10:31:45 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 471773858C53 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1674124305; bh=ggssMNINOYFQJBPC/9j+Umy9L6xlPGt5ZaWzdl7tdoM=; 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=VxxsXGd2abrHx5G100zS6Pyc+7oOAoamLt9OHQrgLYyP+Zd9F9vReLPGU/lXTsaGw zSmjd5yDwFQ06rXUAwirioj3I9HXp4caWs+0V/FEJH82qqRM0DU0pSmOs62WBJcAWj etIDlyki3r5hWhlw6eeufQXv1ZOlLF9GhRVdq1PM= 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 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 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> 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-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" 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