From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id sIGiF+TgBmfCyQcAWB0awg (envelope-from ) for ; Wed, 09 Oct 2024 16:00:36 -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=Zm605Bpd; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 5A5321E356; Wed, 9 Oct 2024 16:00:36 -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 autolearn=unavailable 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 45BA31E05C for ; Wed, 9 Oct 2024 16:00:35 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id DF48E385E459 for ; Wed, 9 Oct 2024 20:00:34 +0000 (GMT) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTP id A0AD6385841E for ; Wed, 9 Oct 2024 20:00:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org A0AD6385841E 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 A0AD6385841E Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1728504014; cv=none; b=vpAWcMiVGIecUD8xyI5vlXcEjmbeFVWpVtYsNHDzPmfuSQ8KQngm0EzA5FKm/4lSoc48H8rC494gMXKQBe+mEbOiCEbbGrjfT7kFjBclOEvZsd0jH0hF3LYq98UbhtDfXJlajAMeCHUcnb5paoWY3vMvYeSRxZaoIEB9UuW7gFU= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1728504014; c=relaxed/simple; bh=MnbOLhkH9U0ElJE1Q7b7hmUg57USnlw0JHV+gY2HTaU=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=RwkWMdCdFBmZRDrnT0vHA3t691aajNqyJXLkPzpxOdvaofaD34XdF7nnwpXTqUgEk/69kEWXIOXBFSgeORVHluW847+4ES04H+OSnuxJOBrm8ecUHxYRXcKVP1Ped6BByLFZndJDsrScRVGGOy44qLhf2thkeIxvPiciS5ZDoi4= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1728504011; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=z1VLYZLit/jiMfZJAJ07Hjnl5woKLc5PfpI3gIaiuwI=; b=Zm605BpdlPg09q68i57wbHjXvIssxkANpw11jAIiRWjnxiBuHWVCgW2KlEup0dKJdLGuTd FdzPgacx3RqnX2UMUl60//zEh6OQW81f8kXSTTeg30+ta8rl7WJezV4gS1P+p6Yho2aSrc Rzf9Rx7/lAyMvDqkTAEJAocz2LC3B/0= 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-465-d1JQ5kHYPI-upRTIC73D-g-1; Wed, 09 Oct 2024 16:00:10 -0400 X-MC-Unique: d1JQ5kHYPI-upRTIC73D-g-1 Received: by mail-qk1-f199.google.com with SMTP id af79cd13be357-7acb19747a8so27923385a.0 for ; Wed, 09 Oct 2024 13:00:10 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1728504009; x=1729108809; h=content-transfer-encoding:in-reply-to:from:content-language :references:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=z1VLYZLit/jiMfZJAJ07Hjnl5woKLc5PfpI3gIaiuwI=; b=fNLjC1x6ohfKoEoy7M2bsL5AVISPO+HuBl18sRMy/5AKAEq4z7qzlgT/gZw5x26uGI Eucsoz0BmojKxJmCo+rw4zyiacv/5gTymket40WcFb9duMk07SaW6sRKilmkq2cNC5Qt ZDGNOcXSFr5bdSoRWubxg5FF+j3Ks8PmKzkzK3K0brss4H3ZV82k/cdJkrkA+y268Lf1 /Gj8K5pU36dtw3FioE8mw6pr5gOgGKzmcw6AcoiUiQ1YkZj+my+kNBAJM+QLvZR/1HVf TUqNOCVzp54Eynk07t6aITTPJxGONXOKMHpNyp9HtY0L4Gbpkr9iJI/IrHJUMsc3MY30 EwWg== X-Forwarded-Encrypted: i=1; AJvYcCWIXSTFIqY+gTO6wH/mGbZnMz9ZpDqi1ES2jr8wOX7rFwljo4wXmxkC+fpd21iP1+HYd1RRq3Z+7deDaQ==@sourceware.org X-Gm-Message-State: AOJu0YzXyCoNKHo+S3ES1jUOR6zmlz1paggFjAErJ2Wj4DRdCOl5eCw0 onKQUla8SFTf2ULowNusf4GB8l3aatRZb74FbfRqAwXoM6PeVjOLZezSHmFXwaLoiFwfXU5HfDK sx74z99Yrb9wjHos5pKKKLMruNgtaXwAgsd7LF0iH8XXJ82pBiih7tgB69PamXgBFFAs= X-Received: by 2002:a05:6214:3d9e:b0:6cb:48c4:a557 with SMTP id 6a1803df08f44-6cbc955a98amr78794406d6.26.1728504009322; Wed, 09 Oct 2024 13:00:09 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHOT8LNEUweLemvUwLa3GwETGVJW/TuzgubR7vpEpXx5J5/ljtzmNcPmeauF+tp26nescF5jQ== X-Received: by 2002:a05:6214:3d9e:b0:6cb:48c4:a557 with SMTP id 6a1803df08f44-6cbc955a98amr78793556d6.26.1728504008819; Wed, 09 Oct 2024 13:00:08 -0700 (PDT) Received: from ?IPV6:2804:14d:8084:92c5::1000? ([2804:14d:8084:92c5::1000]) by smtp.gmail.com with ESMTPSA id 6a1803df08f44-6cbc9c8983bsm10020026d6.106.2024.10.09.13.00.07 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 09 Oct 2024 13:00:08 -0700 (PDT) Message-ID: Date: Wed, 9 Oct 2024 17:00:06 -0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v5 3/5] gdb: Migrate frame unwinders to use C++ classes To: Simon Marchi , gdb-patches@sourceware.org References: <20241001184235.3710608-1-guinevere@redhat.com> <20241001184235.3710608-4-guinevere@redhat.com> From: Guinevere Larsen In-Reply-To: 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/3/24 5:06 PM, Simon Marchi wrote: >> @@ -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. */ Fixed. > >> 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. Fixed. > >> +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... I do prefer this style, so I went with it for all places where it would be under 80 columns. > >> + >> + 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. Fixed. > >> + { >> + 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" I went with the second. > >> + 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). Fixed. I was reticent of doing it because I thought I'd need to change other sniffers, it didn't occur to me that I could pass "this" from inside the frame_unwind_legacy. > >> + 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? Yeah, this sounds like a reasonable further improvement. I just worry about changing many in a single commit, so I'll leave it for a future series. > >> + >> + /* 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. I'll defer to your expertise on this. I have no actual idea how the cache stuff works in here, so if you say that this is a reasonable implementation and my regression tests didn't show any issues, I'm happy to go along with it. > >> + } >> + >> + /* 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. Ok, fixed. > > >> + } >> +}; >> + >> +/* 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. Huh... I thought I had seen a few parameters and variables that seemed to use _p for pointer. Fixed to use m_ instead. > >> 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. Fixed. > >> +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] Thiago agrees that no other frame unwinder uses frame_data, so I think they should probably be fixed up. However, again, I worry about too many actual changes apart from migrating to classes, so I think this is better suited for a follow up series. > >> + >> + int sniffer(const frame_unwind *self, const frame_info_ptr &this_frame, > Missing space. fixed. > >> + 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. Ok, removed the fixme, and the method as a whole, so the base dealloc_cache implementation is used. > >> @@ -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 > I'll work on your second email tomorrow, and hopefully be able to send a v6 with all fixes by the end of the week. -- Cheers, Guinevere Larsen She/Her/Hers