From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id CHmDEEPk/WYlQT8AWB0awg (envelope-from ) for ; Wed, 02 Oct 2024 20:24:35 -0400 Authentication-Results: simark.ca; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.a=rsa-sha256 header.s=google header.b=aI7zCWYD; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 2F6B51E353; Wed, 2 Oct 2024 20:24:35 -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=ham autolearn_force=no version=4.0.0 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 09F371E08F for ; Wed, 2 Oct 2024 20:24:34 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id AB8FF385840E for ; Thu, 3 Oct 2024 00:24:32 +0000 (GMT) Received: from mail-pf1-x42c.google.com (mail-pf1-x42c.google.com [IPv6:2607:f8b0:4864:20::42c]) by sourceware.org (Postfix) with ESMTPS id 789813858408 for ; Thu, 3 Oct 2024 00:23:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 789813858408 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 789813858408 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::42c ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1727915029; cv=none; b=AX5IkvFOWYyyO5Xy/M5gtVBNimbiJIDrbBZV7oLHrO9ct1vBCWhVyRTf+PAJZ5vmnixHYVBmh5tHkOtMtGsv42W2Huf0EKn4Dco5FtaUPodbCFZsyIXrWgxPgIw4jT36ZDcQUeZqehMm+583a5/1fjx1+RX39xODmsYmTgFeSKM= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1727915029; c=relaxed/simple; bh=P4aJiFZHUzgR8lZ3loebffijRw2sDwAbu6KJ6HAR5HU=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=InRJW4/61sMZzWXHFNr+aohdlkRQElFNpbQ7YiYQyOP1Ftw/RTzp3T9pmh/YB4uknqXv6gtNBH1LnPXtuncdBFGPerz4dkUG467Hk7ai2xCkdCnN0T3BqwhvLv1ZjVJJrlDEmJJmFDq7676l1NCi3gXBxAoQSSKxraKSlzql6ng= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-pf1-x42c.google.com with SMTP id d2e1a72fcca58-71dbdb7afe7so349113b3a.0 for ; Wed, 02 Oct 2024 17:23:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1727914994; x=1728519794; darn=sourceware.org; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:from:to:cc:subject:date:message-id:reply-to; bh=R03C01AZbhr1jeBAoSoAaThO9Q7+Z8vHFolxYmAVx+4=; b=aI7zCWYDLrYvCmF8PJkx/ByoJaf4S5N7HJbevGDbHcoa8Z4GZQMAm9ODXxqdegS4WZ nIquVq+IxGVaRGB6Biks7bZdzYY2au7oRsJTQcgiesahYZCCijqEKJJzH0xAAPIMlbxW NszuAPca+WWdltViD2DBpK1BXTkv3O1sbEK4wd2ZYae7ByqAyy90tEeenu0/ReFXf1lG HxkzyqQKQC84XypRgUJownM8O5BRzegPsXuKFF+IxAMtKnvLCH7J0QZ8LSrZxLvRewD+ xNV0lMplsltA24ITmC8kLKzSQD21OMCWyyK4mwfkIMzlVLNQhs70UnJ2KZPU2zaL4vYP E6ZQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1727914994; x=1728519794; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=R03C01AZbhr1jeBAoSoAaThO9Q7+Z8vHFolxYmAVx+4=; b=jlX3p4b3HxI6NomZNH9E05l6pwogp4SuLBz3b3mG6KeB45PNVlkPsOYMMlPQnny8yD Ia/rDkR/R0HZHqhpoOHfztmv71mqvdPnleMSLu+mfN08cxTsLOETxxX12musj3F8CA7k SMqvYZDjVHpQylrRh4ZUrMBqTmfAJrYiGgp9RtoZHcuS9hOIvdUMhWA7tYC0i2dxJDvB arL2mUnfz2imkBU13Mhjd6cQo2Zla7xwPYMSGE6CDNAIurRjgvqrcnp9orOilK36OITt e7Ywg0ZQZ1KqLgzRNcAacW3WPf6wEDZsV8WW4VYtdVKlYixlMpUvxazhQP75t1Dp+l+P FwQQ== X-Gm-Message-State: AOJu0YyEpn3oYGLpIktFI9tv5uYfK59fZ1U8JmOrcjp++0lp1mUwoMzK Y8VAuNs4OiZNgqtYvlOejCCxTeuaF+dvN1IpfMXUx3PLtFGH5ozdUJ1HNyE4xW0= X-Google-Smtp-Source: AGHT+IH3w248gJBG3pQPu8I4XMR6lMz0V4Dt315Q+E4lb6NvfOdvLCDsFefYLWIITdRUWPmWmHHOnQ== X-Received: by 2002:a05:6a00:1a8a:b0:717:88b6:6b1e with SMTP id d2e1a72fcca58-71dc5d53677mr6909101b3a.18.1727914994256; Wed, 02 Oct 2024 17:23:14 -0700 (PDT) Received: from localhost ([2804:14d:7e39:8470:c692:4473:f1b3:494c]) by smtp.gmail.com with ESMTPSA id 41be03b00d2f7-7e9d8523d8dsm7176a12.5.2024.10.02.17.23.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 02 Oct 2024 17:23:13 -0700 (PDT) From: Thiago Jung Bauermann To: Guinevere Larsen Cc: gdb-patches@sourceware.org, Guinevere Larsen Subject: Re: [PATCH v5 3/5] gdb: Migrate frame unwinders to use C++ classes In-Reply-To: <20241001184235.3710608-4-guinevere@redhat.com> (Guinevere Larsen's message of "Tue, 1 Oct 2024 15:42:33 -0300") References: <20241001184235.3710608-1-guinevere@redhat.com> <20241001184235.3710608-4-guinevere@redhat.com> Date: Wed, 02 Oct 2024 21:23:10 -0300 Message-ID: <87wmiq9gr5.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain 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 This looks great, thank you for considering and incorporating my suggestions. I agree that a number of them would make the diff harder to review and can be considered future improvements. But there are two changes which IMHO don't change the diff very much and are important when converting to C++. More below. Also a couple of formatting nits. Guinevere Larsen writes: > diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c > index afc1258c6a9..7be01ec2203 100644 > --- a/gdb/frame-unwind.c > +++ b/gdb/frame-unwind.c > @@ -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 > +{ > + return stop_reason_p (this_frame, this_prologue_cache); > +} > + > +/* This method just passes the parameters to the callback pointer. */ > +void > +frame_unwind_legacy::this_id (const frame_info_ptr &this_frame, > + void **this_prologue_cache, > + struct frame_id *id) const > +{ > + return this_id_p (this_frame, this_prologue_cache, id); > +} > + > +/* This method just passes the parameters to the callback pointer. */ > +struct value * > +frame_unwind_legacy::prev_register (const frame_info_ptr &this_frame, > + void **this_prologue_cache, > + int regnum) const > +{ > + return prev_register_p (this_frame, this_prologue_cache, regnum); > +} > + > +/* This method just passes the parameters to the callback pointer. */ > +int > +frame_unwind_legacy::sniffer (const struct frame_unwind *self, > + const frame_info_ptr &this_frame, > + void **this_prologue_cache) const > +{ > + return sniffer_p (self, this_frame, this_prologue_cache); > +} > + > +/* This method just passes the parameters to the callback pointer. */ > +void > +frame_unwind_legacy::dealloc_cache (frame_info *self, void *this_cache) const > +{ > + if (dealloc_cache_p != nullptr) > + dealloc_cache_p (self, this_cache); > +} > + > +/* This method just passes the parameters to the callback pointer. */ > +struct gdbarch * > +frame_unwind_legacy::prev_arch (const frame_info_ptr &this_frame, > + void **this_prologue_cache) const > +{ > + if (prev_arch_p == nullptr) > + error (_("No prev_arch callback installed")); > + return prev_arch_p (this_frame, this_prologue_cache); > +} As I mentioned before, changing this to: struct gdbarch * frame_unwind_legacy::prev_arch (const frame_info_ptr &this_frame, void **this_prologue_cache) const { if (prev_arch_p == nullptr) return frame_unwind::prev_arch (this_frame, this_prologue_cache); return prev_arch_p (this_frame, this_prologue_cache); } and frame_unwind::prev_arch () to: virtual struct gdbarch *prev_arch (const frame_info_ptr &this_frame, void **this_prologue_cache) const { return get_frame_arch (this_frame); } Allows calling this method without the try/catch in frame_unwind_arch (). When class inheritance provides an adequate solution to method dispatch, I don't see a reason to throw and catch an exception to accomplish the same task. Also, the diff is simple: https://git.linaro.org/people/thiago.bauermann/binutils-gdb.git/commit/?h=review/modernize-frame-unwinders&id=88db6b3f96c3afe7b3f45525b7f786d14a89bbc5 And (I could be wrong of course but) I don't see a risk for regressions, so I don't see a need to leave this change for a future patch. > 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; > +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; > + } > + > + enum frame_type type () const > + { > + return m_type; > + } > + > + enum frame_unwind_class unwinder_class () const > + { > + 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. */ > + 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, > + const frame_info_ptr &this_frame, > + void **this_prologue_cache) const > + { > + return 1; > + } The other thing I believe should be addressed in a C++ conversion is to avoid having a method argument which duplicates the implicit 'this' argument, as is the case of the 'self' argument in the sniffer () method. Fortunately the diff is also small: https://git.linaro.org/people/thiago.bauermann/binutils-gdb.git/commit/?h=review/modernize-frame-unwinders&id=9555b3ae4feea0f4ff15028257716ff2c3c42732 So IMHO it can be done in this patch. Also, a nit: the lines starting with "const ..." and "void ..." above are unaligned. > 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; > +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) > + { } > + > + int sniffer(const frame_unwind *self, const frame_info_ptr &this_frame, > + void **this_prologue_cache) const override; Space before '('. > + > + 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); > + } With the prev_arch () changes I'm suggesting, this error () can be changed to internal_error (). > + > + /* 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 > + { } > +}; -- Thiago