From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id ogdDEGN7/2aqsAAAWB0awg (envelope-from ) for ; Fri, 04 Oct 2024 01:21:39 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1728019299; bh=xrG9aGvosbFpopN6x82dhghPGJR6R83fJ3wDlbee88Y=; h=Date:Subject:From:To:Cc:References:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=mPmKRj3M9WLUivMuDZ5dDHmm3Xd9M5XM6pCi7PTLFbOkuLVP/aRoX53jKFzqwdPd2 vpGK12gKFTzHiSiaLAv6NtXU9aAJnLn+chXlxRdLysDRorxHoTgHXeMPX6k+IawHYH mCsX94nTk9110XcpbpNCK8nDOTfpCm+x1ThGQxOU= Received: by simark.ca (Postfix, from userid 112) id 2EB681E355; Fri, 4 Oct 2024 01:21:39 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.0 (2022-12-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-6.8 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI, RCVD_IN_DNSWL_BLOCKED,RCVD_IN_VALIDITY_CERTIFIED,RCVD_IN_VALIDITY_RPBL, RCVD_IN_VALIDITY_SAFE,URIBL_BLOCKED,URIBL_DBL_BLOCKED_OPENDNS autolearn=unavailable autolearn_force=no version=4.0.0 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=bqnGPNt8; dkim=pass (1024-bit key) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=jDpVIau3; dkim-atps=neutral Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 969051E353 for ; Fri, 4 Oct 2024 01:21:38 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 324C738460B1 for ; Fri, 4 Oct 2024 05:21:38 +0000 (GMT) Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 52675385DDDA for ; Fri, 4 Oct 2024 05:21:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 52675385DDDA Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 52675385DDDA Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=158.69.221.121 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1728019279; cv=none; b=QYH6R5JbtaKNM1AXukHj73Lum/of+QM8HxPbmSHN6fgLEVEW5FslZ+4ZvUmT7qtWWq8tqPpluvbIqN7yW/f3UYOLbGdqjZqQK0a5YFZuGSkLkOATX/ZVKD2VLJj+TOpVUfnZZPpvqnevtzbsGC1FXaDuD0gZwHdwNiPvMwupkWU= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1728019279; c=relaxed/simple; bh=xrG9aGvosbFpopN6x82dhghPGJR6R83fJ3wDlbee88Y=; h=DKIM-Signature:DKIM-Signature:Message-ID:Date:MIME-Version: Subject:From:To; b=C5ih7m8rDOpbk9/vTY8lWJjcXZBJilDuVaciZShLpZlb8K25RKzcuds1KEgOZp1+7mtaJ1wjMhTEbejaqnNsWazpfLUR249gXtXX6S/Zg1fyuy+8KwPFdGYu/UcUOzCNwU5BG0b4s/paElAeTVLMPq8VhIKD6Q7v/GreajpswuI= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1728019276; bh=xrG9aGvosbFpopN6x82dhghPGJR6R83fJ3wDlbee88Y=; h=Date:Subject:From:To:Cc:References:In-Reply-To:From; b=bqnGPNt8S07bXMxYjGoV8bObvk+ntwVCKcXQc2PZsY6jNasedtds/VvzPoc57ZTW3 RHKeiXjJQxgRnzBzN9p9cUZRYL3XE1Y9sX9WN0LJ5S6FCuPhSp4VHVfpkREPpOJCfr TDqsoA3wLaxYiL07Mm6QyhDBt/1JkGzEpGHuVlsE= Received: by simark.ca (Postfix, from userid 112) id BB9B51E356; Fri, 4 Oct 2024 01:21:16 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1728019275; bh=xrG9aGvosbFpopN6x82dhghPGJR6R83fJ3wDlbee88Y=; h=Date:Subject:From:To:Cc:References:In-Reply-To:From; b=jDpVIau3PhG+UYovytpN1BJPE50BrRylPBEXQvA14pAjRO5UBIWJzDLzziGlElSQD 735wTbduRasvC8MoU0xZywwW1e6csoOSe0b3/y6dc9uL8iqA06UYnUJPfONfvOt+Pe Yo8DgChHC265YlGu2St/meNfXVUi6uxYe4A+88/o= Received: from [10.0.0.11] (modemcable238.237-201-24.mc.videotron.ca [24.201.237.238]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id F33681E353; Fri, 4 Oct 2024 01:21:14 -0400 (EDT) Message-ID: Date: Fri, 4 Oct 2024 01:21:14 -0400 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v5 3/5] gdb: Migrate frame unwinders to use C++ classes From: Simon Marchi To: Guinevere Larsen , gdb-patches@sourceware.org Cc: Guinevere Larsen References: <20241001184235.3710608-1-guinevere@redhat.com> <20241001184235.3710608-4-guinevere@redhat.com> Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org Some more things I thought about... >> + /* Default frame sniffer. Will always return that the unwinder >> + is able to unwind the frame. */ >> + virtual int sniffer (const frame_unwind *self, One nit: I think this method should be called just "sniff". When a method is named after a thing, my brain adds an implicit "get_" in front of it. So to me, a method named "sniffer" would return some sniffer object. Here, I think it would make more sense to use an action verb, because we want the unwinder to "sniff". Oh and I guess another naming nit, I think frame_unwind should be named frame_unwinder. A "thing" that unwinds stuff would be called an "unwinder", not an "unwind". I don't know how you feel about changing that. And something else I just remembered, I am not really a fan of the very abbreviated parameter names like this: frame_unwind_trampoline (enum frame_type t, const struct frame_data *d, frame_prev_arch_ftype *pa) : frame_unwind ("trampoline", t, FRAME_UNWIND_GDB, d), prev_arch_p (pa) { } When using an editor that provides tooltips when writing a function call, the abbreviated names in the tooltip are not very helpful. I would suggest using proper names. >> +public: >> + frame_unwind_trampoline (enum frame_type t, const struct frame_data *d, >> + frame_prev_arch_ftype *pa) >> + : frame_unwind ("trampoline", t, FRAME_UNWIND_GDB, d), prev_arch_p (pa) >> + { } > > This "frame_data" thing appears to be some kind of opaque / private data > that frame_unwind implementations can use? For those that you converted > to proper classes (trampoline and python), could that data be stored in > the frame_unwind_* object directly? It would remove some unnecessary > indirections and make things simpler, I think. > > Actually, I gave it a try, and trampoline and python are the only > implementations actually using the frame_data pointer. So if they stop > using it, we can just get rid of that. That requires touching all the > frame_unwind_legacy constructor call sites, but I was able to do that > relatively easily with a global regex find & replace in VScode. [last > minute edit: I just realized that I was building without > --enable-targets=all, so take this with a grain of salt] One more thought about all this: in an ideal world, the tramp_frame uses should probably become sub-classes of frame_unwind_trampoline. Only those sites that want to override the prev_arch method would need to do it (AFAIK, the only one is aarch64_linux_rt_sigframe). The `init` and `validate` callbacks in `tramp_frame` would become virtual methods of `frame_unwind_trampoline`, and the other data fields of `tramp_frame` would become fields of `frame_unwind_trampoline`. I started to prototype it on top of this patch, and it would be quite a lot of work. So let's keep that on the wishlist for now. Simon