From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x334.google.com (mail-wm1-x334.google.com [IPv6:2a00:1450:4864:20::334]) by sourceware.org (Postfix) with ESMTPS id 79F863858C50 for ; Sat, 14 Jan 2023 06:54:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 79F863858C50 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=adacore.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=adacore.com Received: by mail-wm1-x334.google.com with SMTP id m3so16542804wmq.0 for ; Fri, 13 Jan 2023 22:54:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=adacore.com; s=google; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=hsv8k8nOMMvhqQucxQC+A9X73ITLKx0MmelC1xeIeZY=; b=YCyseYk3jZBHf21FM4kaFUdp3FAlFQDhNmtvzLCGMg5QBOqw8hRyPnwY3xXCpLmfhM 22NqmBJl+hnQmHs0dM5f8AsogHb/GwiHP6MyIBEYXRNUDZa/JDiIay4WjXvGKETMuxRT QboESQksK6RFc3dEqxevzs0MHPemt0U4oyjF8d8l1hFuyjP8lvlziLwWPOL8/DgoJroE +Rcd0BMQWGp8UVuEEzwBR+gtAhvilr3peqh9l3lPb+wMz9tqcQ5KgO30213JOFd+eb5+ 3nV2n+W3BssXoTfh2Ez0wwb7ZXl2naj+GvvnVHBqNXrWBTxgOPp4BC1129X+1bx8hA9g IvFQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=hsv8k8nOMMvhqQucxQC+A9X73ITLKx0MmelC1xeIeZY=; b=TmndKOGnBwyT7EPXRBcdp8oAgRvWYVFJ6fla3Dtau/M+f4bXSvzO2OhHJ8u4v8WM7N PE90zw+9GBso4F9JT0gVvQCiZagJ0IuvVjSMbWTBbZlBTxxf+5sw83v8b7W4QHw6idV0 dhH8tV4z1yHxOdLk3DtdX5OIf53Y7Z1UVXC5uYXEGqbW9wwEB6Pt8MkM4UCkB5euepyw J3SQApXIISTHy6C43SPRqzJ9G4EsC84UFgXb2gWNu6/6Zhsjcty1EagUr92f69uuG8fU 7kFmjbwu4R13eSuJ1Te/JkLkR13C30gPDiAb7bGUib2NE5cTkzxU0NEyO6g01c71xBbi p1JA== X-Gm-Message-State: AFqh2kobGL4ORvT8z3kbwiG4E60yjBLPe/3C8TuTAe78160pfkgaFNy8 7UZPOi5dPseGt6hSiiR2/N/5 X-Google-Smtp-Source: AMrXdXux7pLjSdbngCoHJT72Ep1b3vLcCsJWiAR1mBA3gnDLf9aWiU/HpS5KPomL54wZerrrPbbuSQ== X-Received: by 2002:a1c:7919:0:b0:3d3:58d1:2588 with SMTP id l25-20020a1c7919000000b003d358d12588mr1733837wme.41.1673679275291; Fri, 13 Jan 2023 22:54:35 -0800 (PST) Received: from takamaka.gnat.com (lfbn-reu-1-488-54.w92-130.abo.wanadoo.fr. [92.130.77.54]) by smtp.gmail.com with ESMTPSA id n36-20020a05600c502400b003da0b75de94sm7347743wmr.8.2023.01.13.22.54.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 13 Jan 2023 22:54:34 -0800 (PST) Received: by takamaka.gnat.com (Postfix, from userid 1000) id 72BFA8219E; Sat, 14 Jan 2023 10:54:32 +0400 (+04) Date: Sat, 14 Jan 2023 10:54:32 +0400 From: Joel Brobecker To: Torbjorn SVENSSON via Gdb-patches Cc: Tom Tromey , luis.machado@arm.com, vanekt@volny.cz, Yvan Roux , Joel Brobecker Subject: Re: [PATCH v2 3/4] gdb: dwarf2 generic implementation for caching function data Message-ID: References: <20221118155252.113476-1-torbjorn.svensson@foss.st.com> <20221118155252.113476-4-torbjorn.svensson@foss.st.com> <87ili5kcu6.fsf@tromey.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,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 Wed, Dec 28, 2022 at 05:16:03PM +0100, Torbjorn SVENSSON via Gdb-patches wrote: > > 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. See for instance is_fixed_point_type's declaration in gdbtypes.h: /* Return True if TYPE is a TYPE_CODE_FIXED_POINT or if TYPE is a range type whose base type is a TYPE_CODE_FIXED_POINT. */ extern bool is_fixed_point_type (struct type *type); ... where we provide a description of what the function does, including information about all the parameters. And then the corresponding implementation in gdbtypes.c: /* See gdbtypes.h. */ bool is_fixed_point_type (struct type *type) { The goal here is to provide the function's documentation at the API level, rather than at the implementation level, for all situations where it is declared like so. For situations where the function is declared static, we put the function's documentation next to the implementation, as there isn't always a declaration. The "See xxx.h" comment is there to confirm where the function's documentation can be found. -- Joel