From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gmmr-2.centrum.cz (gmmr-2.centrum.cz [46.255.227.203]) by sourceware.org (Postfix) with ESMTPS id C65403858D1E for ; Tue, 29 Nov 2022 16:24:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C65403858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=volny.cz Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=volny.cz Received: from gmmr-1.centrum.cz (envoy-stl.cent [10.32.56.18]) by gmmr-2.centrum.cz (Postfix) with ESMTP id 1C314226613F for ; Tue, 29 Nov 2022 17:24:33 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=volny.cz; s=mail; t=1669739073; bh=+eP/B5ygw34hhURgxRu9u5oIaXcXMLajfbcpyb5TRyU=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=k6DsBWfrHZ6Gh4wobcNCzPk4syCiNoLFUhLken5w1lnAVI2jf8xntHf1EDIvhhNjY Y6OSD63M2fY1WGJMgLxj1r7Gu14cviSBLVcjjeL/JmStM9AvubbMR4f7wCVA5K8nGx 46Vs7XguWll0pXfOxyH5hAHawmc9rgzaUWGD1xIQ= Received: from gmmr-1.centrum.cz (localhost [127.0.0.1]) by gmmr-1.centrum.cz (Postfix) with ESMTP id 189B4E2 for ; Tue, 29 Nov 2022 17:24:33 +0100 (CET) Received: from vm2.excello.cz (vm2.excello.cz [212.24.139.173]) by gmmr-1.centrum.cz (Postfix) with QMQP id 17E55E0 for ; Tue, 29 Nov 2022 17:24:33 +0100 (CET) Received: from vm2.excello.cz by vm2.excello.cz (VF-Scanner: Clear:RC:0(2a00:da80:1:502::8):SC:0(-5.8/5.0):CC:0:; processed in 0.9 s); 29 Nov 2022 16:24:33 +0000 X-VF-Scanner-ID: 20221129162432.210175.24305.vm2.excello.cz.0 X-Spam-Status: No, score=-12.5 required=5.0 tests=BAYES_00,BODY_8BITS,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 Received: from gmmr-4.centrum.cz (2a00:da80:1:502::8) by out1.virusfree.cz with ESMTPS (TLSv1.3, TLS_AES_256_GCM_SHA384); 29 Nov 2022 17:24:32 +0100 Received: from gm-smtp10.centrum.cz (envoy-stl.cent [10.32.56.18]) by gmmr-4.centrum.cz (Postfix) with ESMTP id 2D00E203BAAF for ; Tue, 29 Nov 2022 17:24:32 +0100 (CET) Received: from ktus.lan (217-115-245-101.cust.avonet.cz [217.115.245.101]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by gm-smtp10.centrum.cz (Postfix) with ESMTPSA id 238A41683C2 for ; Tue, 29 Nov 2022 17:24:32 +0100 (CET) Received: by ktus.lan (Postfix, from userid 209) id E5B8C3148B5; Tue, 29 Nov 2022 17:24:31 +0100 (CET) X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-Spam-Level: X-VF-Scanner-Moved-X-Spam-Status: No, score=-3.7 required=5.0 tests=ALL_TRUSTED,BAYES_00,FROM_CZ, NICE_REPLY_A autolearn=disabled version=3.4.0 Received: from [192.168.33.9] (217-115-245-101.cust.avonet.cz [217.115.245.101]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: vanekt) by ktus.lan (Postfix) with ESMTPSA id 918413148B1; Tue, 29 Nov 2022 17:24:25 +0100 (CET) Message-ID: Date: Tue, 29 Nov 2022 17:24:27 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.13.1 Subject: Re: [PATCH v2 3/4] gdb: dwarf2 generic implementation for caching function data Content-Language: en-US To: Torbjorn SVENSSON , Luis Machado , gdb-patches@sourceware.org Cc: Yvan Roux References: <20221118155252.113476-1-torbjorn.svensson@foss.st.com> <20221118155252.113476-4-torbjorn.svensson@foss.st.com> <4461f93e-b29d-80bc-3249-04cbcb94be99@foss.st.com> From: Tomas Vanek In-Reply-To: <4461f93e-b29d-80bc-3249-04cbcb94be99@foss.st.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit List-Id: Hi Torbjorn, On 29/11/2022 16:19, Torbjorn SVENSSON wrote: > 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:16, Luis Machado wrote: >> Hi, >> >> On 11/18/22 15:52, Torbjörn SVENSSON wrote: >>> 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 unqiue cache object. >> >> unqiue -> unique >> >>> >>> Signed-off-by: Torbjörn SVENSSON >>> Signed-off-by: Yvan Roux >>> --- >>>   gdb/dwarf2/frame.c | 48 >>> ++++++++++++++++++++++++++++++++++++++++++++++ >>>   gdb/dwarf2/frame.h | 20 +++++++++++++++++-- >>>   2 files changed, 66 insertions(+), 2 deletions(-) >>> >>> diff --git a/gdb/dwarf2/frame.c b/gdb/dwarf2/frame.c >>> index 3f884abe1d5..bff3b706e7e 100644 >>> --- a/gdb/dwarf2/frame.c >>> +++ b/gdb/dwarf2/frame.c >>> @@ -831,6 +831,14 @@ dwarf2_fetch_cfa_info (struct gdbarch *gdbarch, >>> CORE_ADDR pc, >>>   } >>>   >>> +struct dwarf2_frame_fn_data >>> +{ >>> +  struct value *(*fn) (frame_info_ptr this_frame, void **this_cache, >>> +               int regnum); >>> +  void *data; >>> +  struct dwarf2_frame_fn_data* next; >>> +}; >>> + >> >> I'm wondering if we really need to have a function pointer here. >> Isn't the cache supposed to be frame-wide and not >> function-specific? >> >> If we don't need it, the cache just becomes an opaque data pointer. >> >>>   struct dwarf2_frame_cache >>>   { >>>     /* DWARF Call Frame Address.  */ >>> @@ -862,6 +870,8 @@ struct dwarf2_frame_cache >>>        dwarf2_tailcall_frame_unwind unwinder so this field does not >>> apply for >>>        them.  */ >>>     void *tailcall_cache; >>> + >>> +  struct dwarf2_frame_fn_data *fn_data; >>>   }; >>>   static struct dwarf2_frame_cache * >>> @@ -1221,6 +1231,44 @@ dwarf2_frame_prev_register (frame_info_ptr >>> this_frame, void **this_cache, >>>       } >>>   } >>> +void *dwarf2_frame_get_fn_data (frame_info_ptr this_frame, void >>> **this_cache, >>> +                fn_prev_register fn) >>> +{ >>> +  struct dwarf2_frame_fn_data *fn_data = nullptr; >>> +  struct dwarf2_frame_cache *cache >>> +    = dwarf2_frame_cache (this_frame, this_cache); >>> + >>> +  /* Find the object for the function.  */ >>> +  for (fn_data = cache->fn_data; fn_data; fn_data = fn_data->next) >>> +    if (fn_data->fn == fn) >>> +      return fn_data->data; >>> + >>> +  return nullptr; >>> +} >>> + >>> +void *dwarf2_frame_allocate_fn_data (frame_info_ptr this_frame, >>> +                     void **this_cache, >>> +                     fn_prev_register fn, unsigned long size) >>> +{ >>> +  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; >>> +} >> >> And if we only have a data pointer, we can return a reference to it >> through the argument, and then DWARF can cache it. >> >> We could even have a destructor/cleanup that can get called once the >> frames are destroyed. > > I don't think we can do that without introducing a lot more changes to > the common code. My changes are designed in a way that would only have > an impact on arm (as they are the only users for the functionality > right now) and not for every target out there that GDB supports. If > going for a simpler solution, it would mean that every target needs to > be re-tested in order to get the confirmation that the implementation > would not break some other target. > > >> >>> + >>>   /* Proxy for tailcall_frame_dealloc_cache for bottom frame of a >>> virtual tail >>>      call frames chain.  */ >>> diff --git a/gdb/dwarf2/frame.h b/gdb/dwarf2/frame.h >>> index 06c8a10c178..444afd9f8eb 100644 >>> --- a/gdb/dwarf2/frame.h >>> +++ b/gdb/dwarf2/frame.h >>> @@ -66,6 +66,9 @@ enum dwarf2_frame_reg_rule >>>   /* Register state.  */ >>> +typedef struct value *(*fn_prev_register) (frame_info_ptr this_frame, >>> +                       void **this_cache, int regnum); >>> + >>>   struct dwarf2_frame_state_reg >>>   { >>>     /* Each register save state can be described in terms of a CFA >>> slot, >>> @@ -78,8 +81,7 @@ struct dwarf2_frame_state_reg >>>         const gdb_byte *start; >>>         ULONGEST len; >>>       } exp; >>> -    struct value *(*fn) (frame_info_ptr this_frame, void **this_cache, >>> -             int regnum); >>> +    fn_prev_register fn; >>>     } loc; >>>     enum dwarf2_frame_reg_rule how; >>>   }; >>> @@ -262,4 +264,18 @@ 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.  */ >>> + >>> +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); >>> + >>>   #endif /* dwarf2-frame.h */ >> >> As we've discussed before, I think the cache idea is nice if we have >> to deal with targets with multiple CFA's (in our case, we have either >> 4 SP's or 2 SP's, plus aliases). >> >> DWARF doesn't seem to support this at the moment, and the function >> HOW for DWARF is not smart enough to remember a previously-fetched >> value. So it seems we have room >> for some improvement, unless there is enough reason elsewhere about >> why we shouldn't have a cache. > > > This patch does not provide a cache or anything, it just provides a > way for the callback function to save some additional data between > calls for the same frame. > The code above is generic in the way that it has one data object per > function and frame. The reason for this implementation is that it's > rather easy to ensure that the data object is okay for the function > that uses it without any inter-dependencies with some other function > that might be called for some other register on the same frame. You > could even consider having a shared function to be a callback > function. In the case of a shared function, that would mean that the > object would be large and public and then it would simply make more > sense to make the dwarf2 object public and extend it instead. > My approach ensures that each callback function has its own data and > the data structure is "private" to the function. It's possible to > share the struct for the data object between 2 functions, but it's not > possible to share the instance of the struct between 2 functions. Sorry, the per-function-pointers looks like an overkill to me. Maybe I'm just an old school programmer and don't like associative arrays... - frame unwinders use a generic pointer and ensuring the proper type cast is fully in responsibility of the implementation. - we need just to replicate the similar functionality for architecture dependent handling of dwarf2 frames - functions assigned to a dwarf2 frame by how = DWARF2_FRAME_REG_FN are never isolated functions from different parts of code: a gdbarch can set only one initializer by dwarf2_frame_set_init_reg() and it sets all functions - if we ever have more than one function assigned in one dwarf2 frame, seems me likely that all functions would prefer a single cache over isolated ones > > >> It would be nice to have some opinions from others, so we can >> potentially shape this in a way that makes it useful for the general >> case. > > Yes. Please give me some more feedback to work on! > > Kind regards, > Torbjörn regards     Tomas