From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id /YRKLlT5/maiRAAAWB0awg (envelope-from ) for ; Thu, 03 Oct 2024 16:06:44 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1727986004; bh=S7mA5IZ/tFAL8fY9VP/0M65eJt9w/YIdWJabOpDF1yo=; h=Date:Subject:To:Cc:References:From:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=IgwcXsU+niQ1h4KBlpKDZH/DMYPr+696sSTdWb1MErE3TQsHPeVVCKoizQs/NAoWg lp8WfYYEWaDmGcVZ/dNM1k8VRhUdQz6a6rpeQycuVCkNxv37H/6fzhzSmp81IxnutX YTcv/KdX0XlHqmf5qIZPQtDsNhy+lIGtYsgnwGQ4= Received: by simark.ca (Postfix, from userid 112) id 995671E355; Thu, 3 Oct 2024 16:06:44 -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=OfF1ZXLa; dkim=pass (1024-bit key) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=fxb7aRP9; 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 2F3A41E05C for ; Thu, 3 Oct 2024 16:06:43 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id C3D363849AE9 for ; Thu, 3 Oct 2024 20:06:42 +0000 (GMT) Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 7CE333858C53 for ; Thu, 3 Oct 2024 20:06:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 7CE333858C53 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 7CE333858C53 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=1727985981; cv=none; b=oYtfo20daw8MXm56CjalDwRsmKanXRh9oQg4P68WVrzVow3Torlq8++t7D4yGT7jVOxzw0SoPdLZzQwE2T/LN+WaoyaML+NIAZszNBBD6dV2VtOgS9K4CzmVR8z4Vc1TfYWw9VtaYkeQYFrkUYU+howMbSfpfLCsPJlo7/JE444= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1727985981; c=relaxed/simple; bh=S7mA5IZ/tFAL8fY9VP/0M65eJt9w/YIdWJabOpDF1yo=; h=DKIM-Signature:DKIM-Signature:Message-ID:Date:MIME-Version: Subject:To:From; b=lZdX/JBpdOAKIJY1r0dLTP0Fia5HFtlXTrIhez4mJSPAfBICuBxW7ALLhlfsmZolqmFje4ofPzD4dR7yxLetrn/0/lWRfhl6g2mgaN6hxZSOZ84IQw6aD31Qwo076HAWJSSOlCZu5AtzEaShk8Kk71a8VYnyZFTzm5z4JB1h7d0= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1727985979; bh=S7mA5IZ/tFAL8fY9VP/0M65eJt9w/YIdWJabOpDF1yo=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=OfF1ZXLacnzs3l8CNp25y8ncZhFsnWthtyZ7LWLw3s7i1Hn/yPcg3fi8h5ZxqDCKT jXvNmBQaTjVa1kwva0CKjlpgPXWTcUHqx18keEy6GJqpTzCS/Rj7s/NgXbTWbza5wz zn2rWBmMQPF0fBWKCX2ntxpTOXj880w+m7H40mhw= Received: by simark.ca (Postfix, from userid 112) id 048111E356; Thu, 3 Oct 2024 16:06:19 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1727985976; bh=S7mA5IZ/tFAL8fY9VP/0M65eJt9w/YIdWJabOpDF1yo=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=fxb7aRP90VY67BGE/AflEKe4A3u3YoL+JCh2yGtac14PpSJBcnHE14p9e09e4Q/bP sE1UMaP5lHSo7zj+gNn9es+BHXKeRmAwAwrOE6u5xPIrHn8/4NEN/Q8nGDrxX/aNUZ vfW7NtIWDxqUPoC/qIDvubOi+x5nEYRI+CPOdocs= Received: from [172.16.0.192] (96-127-217-162.qc.cable.ebox.net [96.127.217.162]) (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 1F1621E05C; Thu, 3 Oct 2024 16:06:16 -0400 (EDT) Message-ID: Date: Thu, 3 Oct 2024 16:06:15 -0400 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v5 3/5] gdb: Migrate frame unwinders to use C++ classes 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: fr From: Simon Marchi In-Reply-To: <20241001184235.3710608-4-guinevere@redhat.com> 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 > @@ -332,6 +332,59 @@ frame_unwind_got_address (const frame_info_ptr &frame, int regnum, > return reg_val; > } > > +/* This method just passes the parameters to the callback pointer. */ > +enum unwind_stop_reason > +frame_unwind_legacy::stop_reason (const frame_info_ptr &this_frame, > + void **this_prologue_cache) const Since these methods are already documented in the .h, the comment should just say: /* See frame-unwind.h. */ > diff --git a/gdb/frame-unwind.h b/gdb/frame-unwind.h > index deab4f7dbfb..b078945a200 100644 > --- a/gdb/frame-unwind.h > +++ b/gdb/frame-unwind.h > @@ -169,25 +169,148 @@ enum frame_unwind_class > FRAME_UNWIND_ARCH, > }; > > -struct frame_unwind > +class frame_unwind > { > - const char *name; > + const char *m_name; > /* The frame's type. Should this instead be a collection of > predicates that test the frame for various attributes? */ > - enum frame_type type; > + enum frame_type m_type; > /* What kind of unwinder is this. It generally follows from where > the unwinder was added or where it looks for information to do the > unwinding. */ > - enum frame_unwind_class unwinder_class; > + enum frame_unwind_class m_unwinder_class; > + const struct frame_data *m_unwind_data; To be consistent, please move the private fields at the end, like we do elsewhere. > +public: > + frame_unwind (const char *name, frame_type type, frame_unwind_class uclass, > + const struct frame_data *data) > + : m_name (name), m_type (type), m_unwinder_class (uclass), > + m_unwind_data (data) > + { } > + > + const char *name () const > + { > + return m_name; > + } If you want to save some lines, we use this style elsewhere for trivial getters: const char *name () const { return m_name; } I don't really mind if you prefer not, we currently have both styles in the code base... > + > + enum frame_type type () const > + { > + return m_type; > + } > + > + enum frame_unwind_class unwinder_class () const To be more C++-y, I would suggest dropping enum/struct/class keywords where possible, for instance the return value above. > + { > + return m_unwinder_class; > + } > + > + const struct frame_data *unwind_data () const > + { > + return m_unwind_data; > + } > + > + /* Default stop_reason function. It reports NO_REASON, unless the > + frame is the outermost. */ function -> method, I would say. Or "Default stop_reason implementation" > + virtual enum unwind_stop_reason stop_reason (const frame_info_ptr &this_frame, > + void **this_prologue_cache) const > + { > + return default_frame_unwind_stop_reason (this_frame, this_prologue_cache); > + } > + > + /* Default frame sniffer. Will always return that the unwinder > + is able to unwind the frame. */ > + virtual int sniffer (const frame_unwind *self, I agree with Thiago's comment about not passing "self" (unless there's something I don't see here). > + const frame_info_ptr &this_frame, > + void **this_prologue_cache) const > + { > + return 1; > + } > + > + /* Calculate the ID of the given frame using this unwinder. */ > + virtual void this_id (const frame_info_ptr &this_frame, > + void **this_prologue_cache, > + struct frame_id *id) const = 0; This is pre-existing code so it makes sense to have it like that for the moment, but I don't see why this has to return the value by parameter, surely the method could return a frame_id directly? > + > + /* Get the value of a register in a previous frame. */ > + virtual struct value *prev_register (const frame_info_ptr &this_frame, > + void **this_prologue_cache, > + int regnum) const = 0; > + > + /* The following methods are here mostly for interface functionality. They > + all throw an error when called, as a safe way to check if an unwinder has > + implemented the desired functionality. */ > + > + /* Properly deallocate the cache. */ > + virtual void dealloc_cache (frame_info *self, void *this_cache) const > + { > + internal_error (_("No method dealloc_cache implemented for unwinder %s"), > + m_name); I think a reasonable default implementation here would be to do nothing. After all, that's what the current code (function frame_info_del) does: if the callback is nullptr, nothing is done. An implementation that doesn't use any frame cache (is there any?) will just be able to omit this method. > + } > + > + /* Get the previous architecture. */ > + virtual struct gdbarch *prev_arch (const frame_info_ptr &this_frame, > + void **this_prologue_cache) const > + { > + error (_("No method prev_arch implemented for unwinder %s"), m_name); I think Thiago's suggestion makes sense, a default implementation of returning this_frame's arch would be reasonable. It is by far the most common and intuitive case that the arch for a given frame will be the same as the arch for the frame it got unwound from. Only the few implementations that want to do something different (e.g. sentinel) can implement it. > + } > +}; > + > +/* This is a legacy version of the frame unwinder. The original struct > + used function pointers for callbacks, this updated version has a > + method that just passes the parameters along to the callback > + pointer. > + Do not use this class for new unwinders. Instead, see other classes > + that inherit from frame_unwind, such as the python unwinder. */ > +class frame_unwind_legacy : public frame_unwind > +{ > +private: > /* Should an attribute indicating the frame's address-in-block go > here? */ > - frame_unwind_stop_reason_ftype *stop_reason; > - frame_this_id_ftype *this_id; > - frame_prev_register_ftype *prev_register; > - const struct frame_data *unwind_data; > - frame_sniffer_ftype *sniffer; > - frame_dealloc_cache_ftype *dealloc_cache; > - frame_prev_arch_ftype *prev_arch; > + frame_unwind_stop_reason_ftype *stop_reason_p; > + frame_this_id_ftype *this_id_p; > + frame_prev_register_ftype *prev_register_p; > + frame_sniffer_ftype *sniffer_p; > + frame_dealloc_cache_ftype *dealloc_cache_p; > + frame_prev_arch_ftype *prev_arch_p; Move the private fields at the end, and prefix with `m_`. Also, in GNU code the `_p` prefix usually means "predicate", so usually for boolean values or functions returning booleans. I guess here with `_p` you meant "pointer". Just doing with "m_stop_reason" & co would be fine I think, it's not worse than what was there previously. > diff --git a/gdb/tramp-frame.c b/gdb/tramp-frame.c > index 6368f67a2e7..b1f3be66bc1 100644 > --- a/gdb/tramp-frame.c > +++ b/gdb/tramp-frame.c > @@ -57,10 +57,45 @@ tramp_frame_cache (const frame_info_ptr &this_frame, > return tramp_cache->trad_cache; > } > > -static void > -tramp_frame_this_id (const frame_info_ptr &this_frame, > - void **this_cache, > - struct frame_id *this_id) > +class frame_unwind_trampoline : public frame_unwind > +{ > +private: > + frame_prev_arch_ftype *prev_arch_p; Please move this at the end of the class. And I suggest naming it "m_prev_arch", for the same reason as explained above. > +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] > + > + int sniffer(const frame_unwind *self, const frame_info_ptr &this_frame, Missing space. > + void **this_prologue_cache) const override; > + > + void this_id (const frame_info_ptr &this_frame, void **this_prologue_cache, > + struct frame_id *id) const override; > + > + struct value *prev_register (const frame_info_ptr &this_frame, > + void **this_prologue_cache, > + int regnum) const override; > + > + struct gdbarch *prev_arch (const frame_info_ptr &this_frame, > + void **this_prologue_cache) const override > + { > + if (prev_arch_p == nullptr) > + error (_("No prev_arch callback installed")); > + return prev_arch_p (this_frame, this_prologue_cache); > + } > + > + /* FIXME: This should have a proper algorithm to deallocate the cache, > + otherwise memory is leaked. This method is empty here just so the > + migration to c++ classes doesn't add regressions. */ > + void dealloc_cache (frame_info *self, void *this_cache) const override > + { } >From what I can see, this unwinder uses the "trad frame" cache, which allocates everything on the frame_cache_obstack obstack, which gets cleared in reinit_frame_cache(). So I think this unwinder could just omit the dealloc_cache method, as it would not need it. > @@ -161,16 +196,11 @@ tramp_frame_prepend_unwinder (struct gdbarch *gdbarch, > gdb_assert (tramp_frame->insn_size <= sizeof (tramp_frame->insn[0].bytes)); > > data = GDBARCH_OBSTACK_ZALLOC (gdbarch, struct frame_data); > - unwinder = GDBARCH_OBSTACK_ZALLOC (gdbarch, struct frame_unwind); > - > data->tramp_frame = tramp_frame; > - unwinder->type = tramp_frame->frame_type; > - unwinder->unwind_data = data; > - unwinder->unwinder_class = FRAME_UNWIND_GDB; > - unwinder->sniffer = tramp_frame_sniffer; > - unwinder->stop_reason = default_frame_unwind_stop_reason; > - unwinder->this_id = tramp_frame_this_id; > - unwinder->prev_register = tramp_frame_prev_register; > - unwinder->prev_arch = tramp_frame->prev_arch; > + > + unwinder = obstack_new (gdbarch_obstack (gdbarch), > + tramp_frame->frame_type, > + data, > + tramp_frame->prev_arch); Here, given that `data` points to the `frame_data` object, which contains a pointer to the `tramp_frame`, I think it's redundant to pass that in addition to `frame_type` and `prev_arch`. I think you could replace all that by a single argument of type `tramp_frame *` (and store that pointer directly in the `frame_unwind_trampoline` object, instead of passing through that `frame_data` thing). Simon