From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id qGhHL6XIBmeAswcAWB0awg (envelope-from ) for ; Wed, 09 Oct 2024 14:17:09 -0400 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=UMPoM+jE; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id BDD111E05C; Wed, 9 Oct 2024 14:17:09 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.0 (2022-12-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-7.8 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIMWL_WL_HIGH,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 2D4CF1E05C for ; Wed, 9 Oct 2024 14:17:08 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id C2F7538650DD for ; Wed, 9 Oct 2024 18:17:07 +0000 (GMT) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id 315FA3858417 for ; Wed, 9 Oct 2024 18:16:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 315FA3858417 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 315FA3858417 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1728497807; cv=none; b=AmXqoIWbHQCuyPIO2aygWGiYHwMPN8KNjFAYlCFuzG0q2LMNc+j1zUAYCLyjHIJvUaQneqW+U4E4HP3ASMp/jRep3xTCKPAD4P6ZYSrhpGrs/7NMXASIY6jhM4Dxv0fF/0gjg0jRBuR+4R2tsgeXHDBJj56yyDcO+YgkrL3F+KE= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1728497807; c=relaxed/simple; bh=GxCBSNGQOp39PvFsH9SPX8z3+i9K06VcNmU8tbe68Bs=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=v5Km/gvXiILy9at8k9w2NU0kHWjMA+T8y9v5wamoeFv4jK1uRMyl+5J+2qG2ivzngtXcXYxipzjCzdlYNx5MQ0zzWZpxWAWnkeM2ZVlJ04ocdPd2oEpx0sxpLghDN4kqMmgQ0lwPwgUH9frqJpou/hwtCDSfMryXUzanvdr9xyM= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1728497804; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=loqpVq++kIDkyAKNcwjMK86omabu6TomU1X1F4qhPe0=; b=UMPoM+jE7LVGEdaK84zeJyHY63kYDtNoIu5Q8igEcVZty/mQzs3WGeNK63cF3RO7j7bFGl N5mQbToveLWGiRxRsgSjrLTo9UnSdaHALS/vlx4aMAjAGfA8ss9bcX2lf9sDRvINzTN+Y+ 4FaCZewGdmPUyjJR37WQPm9BdQDmbOI= Received: from mail-qk1-f199.google.com (mail-qk1-f199.google.com [209.85.222.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-564-icPAiB9EOYuDQL0Z5rtZ5A-1; Wed, 09 Oct 2024 14:16:43 -0400 X-MC-Unique: icPAiB9EOYuDQL0Z5rtZ5A-1 Received: by mail-qk1-f199.google.com with SMTP id af79cd13be357-7b111381632so2906585a.3 for ; Wed, 09 Oct 2024 11:16:43 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1728497802; x=1729102602; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=loqpVq++kIDkyAKNcwjMK86omabu6TomU1X1F4qhPe0=; b=CkZx4BDqDDqzH1orGdEHTJWnFrH4Sxsi0bo+dQ1RxoXHsEJkNq05ldwkBB04VBGcIL Q5ui+3u2xqsnjnY1xt3Z3N1tMiY5ofpisTb0jqHL8Il/mp9nwxb0YJSqYex3lSIxjYiH DbX42vbtFqfqolZDyu5lOR/jLhOA4CZw/o8knzxOMaI28ueo8V0mbKrkGMlcK7f817DN o3ogjWZSpMgemN3muSFXFqLc96uKdYZandZboBZZaX4hznAiH8RcVHvINe2M8dfVa6ok mj31i8JoZxTeybgRfbP5cU3b5eCYTYBPamoEz+a+qbclI8oIFiEmvw7qbuPaH7Afyaab MtAg== X-Gm-Message-State: AOJu0YzvyLDV0AATSmIdzR1bX150mjYaclgiTxDGbmtsblReqJWvK7A1 qo35MAJmGkM1PxoPQKOleQnE2JofYWQPpO4OE0/B93+d95WqKn3s6M6oOvYfBy6OysoDYwLFyOG kAkvMLcNcfGnG60y2Ff1+ykIFFtlMVnBFOE3fJLqJOmcRMduOYotHLn2n/4Qmmg611xs= X-Received: by 2002:a05:620a:1926:b0:7b1:123a:2185 with SMTP id af79cd13be357-7b1123a21c1mr90823485a.54.1728497802470; Wed, 09 Oct 2024 11:16:42 -0700 (PDT) X-Google-Smtp-Source: AGHT+IE/kXO1YhlrFwFUD545IcL/lhN41Vkv7oIf+yyXOV3mN9+udcfzITjxHPXncJQt7k5XWJiprQ== X-Received: by 2002:a05:620a:1926:b0:7b1:123a:2185 with SMTP id af79cd13be357-7b1123a21c1mr90819985a.54.1728497802012; Wed, 09 Oct 2024 11:16:42 -0700 (PDT) Received: from ?IPV6:2804:14d:8084:92c5::1000? ([2804:14d:8084:92c5::1000]) by smtp.gmail.com with ESMTPSA id af79cd13be357-7ae757627fdsm474003385a.100.2024.10.09.11.16.40 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 09 Oct 2024 11:16:41 -0700 (PDT) Message-ID: Date: Wed, 9 Oct 2024 15:16:39 -0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v5 3/5] gdb: Migrate frame unwinders to use C++ classes To: Thiago Jung Bauermann Cc: gdb-patches@sourceware.org References: <20241001184235.3710608-1-guinevere@redhat.com> <20241001184235.3710608-4-guinevere@redhat.com> <87wmiq9gr5.fsf@linaro.org> From: Guinevere Larsen In-Reply-To: <87wmiq9gr5.fsf@linaro.org> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed 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 On 10/2/24 9:23 PM, Thiago Jung Bauermann wrote: > 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. Hi Thiago, I was apparently over-complicating the changes. You're right, they are simple enough to add to this patch. I fixed everything you mentioned here. -- Cheers, Guinevere Larsen She/Her/Hers > > 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 >> + { } >> +};