From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id Ji2+Icd0hmcY0w4AWB0awg (envelope-from ) for ; Tue, 14 Jan 2025 09:29:27 -0500 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=Z3qZOiYu; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 7CFEB1E100; Tue, 14 Jan 2025 09:29:27 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 4.0.0 (2022-12-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-6.4 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_MED 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 BC2441E05C for ; Tue, 14 Jan 2025 09:29:26 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 40F0C3856953 for ; Tue, 14 Jan 2025 14:29:26 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 40F0C3856953 Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=Z3qZOiYu 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 E2FD53856942 for ; Tue, 14 Jan 2025 14:28:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org E2FD53856942 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 E2FD53856942 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=1736864930; cv=none; b=ZVirHdwOpNyXR5MBBW5kzfRacTfFREz8HbVlrTXk4HG+sjz1B0qVQLLAun46YsXTAxtT+GnPW2X9fTe1yNR2WXgQhJ9qU4W6LufGzRDOYoihnHO/wi29bLGRhNVyGbMFRajJxg/584XAvaJCT7214VhE9hnQNcdkgi09YG+X5vM= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1736864930; c=relaxed/simple; bh=C0QEGuzn7f1hOWsmeZmZgpRCiI/Pc8S0UAI2gSrvbsY=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=ZJBVctOx5j1pSNXLh67c85k3KzCE5P+uIHHN2O8SxLSZ9w5hgcV1a92akqml0gLpRaBxR/YzXSR0mg+Vlr3fOK6j1kvC3BRdCD0zY5aXcy6ZAdTAh4F3FXOAi2E7tVug7mCJMdFedqQS4Bs2NV0KjhV+B6/QoGAZwb8RVXR2mds= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org E2FD53856942 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1736864929; 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: in-reply-to:in-reply-to:references:references; bh=/inpVtX/oXwSHqFi2v+qt99uN2AekdzqrmFyGo/KKIw=; b=Z3qZOiYuXk7R19qefVq3tZxGWbZZvwgb+LLzhu3blFCcaFtHE9oX1bLm7pSrhhT1IEertx ajEAiCD9xFe2Mm7kyPaYniNVEcIBhbgYxgXWZ9qlo/8U1W0FuLx1dWXMlc8j2sef4pBjWP QJrAlxVQwgNJTWmB8yY08gayLnCA3ck= Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-311-o6xVekShMSibRqHJSmL3qg-1; Tue, 14 Jan 2025 09:28:47 -0500 X-MC-Unique: o6xVekShMSibRqHJSmL3qg-1 X-Mimecast-MFC-AGG-ID: o6xVekShMSibRqHJSmL3qg Received: by mail-wr1-f70.google.com with SMTP id ffacd0b85a97d-3860bc1d4f1so3535243f8f.2 for ; Tue, 14 Jan 2025 06:28:46 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736864926; x=1737469726; 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=/inpVtX/oXwSHqFi2v+qt99uN2AekdzqrmFyGo/KKIw=; b=AXah3ELpeIBdFxICB17US71kNl8v5DahgO78i6HKYfHxkBgNjNSbta4JCl7pWIlS+g F0iG40U28D5Bpl2gtgib9FNRSdkajGgpWwhMB3STDsLuLd97LbiChU0jnKJppbLU+iWC gis/4zSB1gobU/HFLZ+4F5wrlaRrHbmAFnPtqs3I7BZKZuHFsIxWMOCgAPge6ejw5StQ PviqhluSmA65HHDToWdPMV2/EJwcZafkatcQiF2Lwsb3QieXbQ8mR5YCMnks9IN30cxq ektmzsbuFqmgZY46FWzzG1bXGSllSoeD+LYm5e2gIa5c7tdAFYeRRv0iykefby4hwmSr Ud+g== X-Forwarded-Encrypted: i=1; AJvYcCWbSisvq+TtSaBjA1kYLqmcAJte2GBw1sMKZUxlyXXE+F1xnqY5Dk6EUVUvt4hK4fHTt4Mg/AiDj1/4ew==@sourceware.org X-Gm-Message-State: AOJu0Yyv4oZobLKdZYLmrf5rR5Z6tkS/EY3w+y3dkSBcFnkPRilWIfLs 4pJc4xAg+ybwPOQB33MkkT+nK11X49aqKraQsQ2kn0WzrOwrhs0UL0u5HfeRIj8zR4VnC5nFL7C Xz4tyeYgk50no3XdfkHruo+O/tGwvRfum5BH8zR5tUR6H0opnSb92iCZt6MI= X-Gm-Gg: ASbGncuABTDmDxUu5xGVtVw4PQpZUdFUKpY2SOXDByXnlUo+3nfundZj4sozJQ8dTHm gUB+sojtHGM+kOhcYEUT/JTlZt3XPF26v8EZp8fCrhbFbt6g0MvSwdV+nHoCpNIws8yIlCLDHLz HW/Wb0RFBUR/N5idd6c04SQxaFFlD//JG5VQrCGOO+3tPVWbmrG9IuvW65jL++t2wgPxXalu6Vj PNJmHjmOYJW6xT2Dl568LtcQ4maHVxAZr9veIXZbQpdznapqJCaKD3P/qGoZYhTqOFdMJKcSgdB In4XCw== X-Received: by 2002:a05:6000:461e:b0:38a:41a3:ac4 with SMTP id ffacd0b85a97d-38a87331092mr23476006f8f.45.1736864925808; Tue, 14 Jan 2025 06:28:45 -0800 (PST) X-Google-Smtp-Source: AGHT+IEAv1Mx9CyPaT63WBNkF1A8llcj+6FfqEDwLtNCrSMhXYP6QhV6/0OzPZii9ML2yc6hiiJ9mw== X-Received: by 2002:a05:6000:461e:b0:38a:41a3:ac4 with SMTP id ffacd0b85a97d-38a87331092mr23475987f8f.45.1736864925342; Tue, 14 Jan 2025 06:28:45 -0800 (PST) Received: from localhost (44.226.159.143.dyn.plus.net. [143.159.226.44]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-38a8e38c697sm15294920f8f.52.2025.01.14.06.28.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 14 Jan 2025 06:28:45 -0800 (PST) From: Andrew Burgess To: Guinevere Larsen , gdb-patches@sourceware.org Cc: Guinevere Larsen , Thiago Jung Bauermann Subject: Re: [PATCH v8 1/5] gdb: make gdbarch store a vector of frame unwinders In-Reply-To: <20241210195115.3046370-2-guinevere@redhat.com> References: <20241210195115.3046370-1-guinevere@redhat.com> <20241210195115.3046370-2-guinevere@redhat.com> Date: Tue, 14 Jan 2025 14:28:44 +0000 Message-ID: <87ikqhqxk3.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: Dug_8eBX65wZVx0tiPBK3iSdeY8DWtsnkmHSH1CT5Es_1736864926 X-Mimecast-Originator: redhat.com 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 Guinevere Larsen writes: > Before this commit, all frame unwinders would be stored in the obstack > of a gdbarch and accessed by using the registry system. This made for > unwieldy code, and unnecessarily complex logic in the frame_unwinder > implementation, along with making frame_unwind structs be unable to have > non-trivial destructors. > > Seeing as a future patch of this series wants to refactor the > frame_unwind struct to use inheritance, and we'd like to not restrict > the future derived classes on what destructors are allowed. In > preparation for that change, this commit changes the registry in gdbarch > to instead store an std::vector, which doesn't require using an obstack > and doesn't rely on a linked list. > > There should be no user-visible changes. > > Reviewed-by: Thiago Jung Bauermann > --- > gdb/frame-unwind.c | 107 +++++++++++++++------------------------------ > 1 file changed, 36 insertions(+), 71 deletions(-) > > diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c > index 352779fcdcc..e61f6244913 100644 > --- a/gdb/frame-unwind.c > +++ b/gdb/frame-unwind.c > @@ -31,61 +31,42 @@ > #include "cli/cli-cmds.h" > #include "inferior.h" > > -struct frame_unwind_table_entry > +/* Default sniffers, that must always be the first in the unwinder list, > + no matter the architecture. */ > +static constexpr auto standard_unwinders = I'm not a huge fan of `auto` when the type is well known. My thinking is write once, read many times. So I'd rather have the type information available when I read the code. For me: static constexpr std::initializer_list standard_unwinders = tells me what's going on... > { > - const struct frame_unwind *unwinder; > - struct frame_unwind_table_entry *next; > + &dummy_frame_unwind, > + /* The DWARF tailcall sniffer must come before the inline sniffer. > + Otherwise, we can end up in a situation where a DWARF frame finds > + tailcall information, but then the inline sniffer claims a frame > + before the tailcall sniffer, resulting in confusion. This is > + safe to do always because the tailcall sniffer can only ever be > + activated if the newer frame was created using the DWARF > + unwinder, and it also found tailcall information. */ > + &dwarf2_tailcall_frame_unwind, > + &inline_frame_unwind, > }; > > -struct frame_unwind_table > -{ > - struct frame_unwind_table_entry *list = nullptr; > - /* The head of the OSABI part of the search list. */ > - struct frame_unwind_table_entry **osabi_head = nullptr; > -}; > +/* If an unwinder should be prepended to the list, this is the > + index in which it should be inserted. */ > +static constexpr int prepend_unwinder_index = standard_unwinders.size (); > > -static const registry::key > +static const registry::key> > frame_unwind_data; > > -/* A helper function to add an unwinder to a list. LINK says where to > - install the new unwinder. The new link is returned. */ > - > -static struct frame_unwind_table_entry ** > -add_unwinder (struct obstack *obstack, const struct frame_unwind *unwinder, > - struct frame_unwind_table_entry **link) > -{ > - *link = OBSTACK_ZALLOC (obstack, struct frame_unwind_table_entry); > - (*link)->unwinder = unwinder; > - return &(*link)->next; > -} > - > -static struct frame_unwind_table * > +/* Retrieve the list of frame unwinders available in GDBARCH. > + If this list is empty, it is initialized before being returned. */ > +static std::vector * > get_frame_unwind_table (struct gdbarch *gdbarch) Given you're changing this code anyway, how about returning a reference rather than a pointer, i.e.: static std::vector & get_frame_unwind_table (struct gdbarch *gdbarch) { ... } The users of get_frame_unwind_table will need to be updated to match. > { > - struct frame_unwind_table *table = frame_unwind_data.get (gdbarch); > + std::vector *table = frame_unwind_data.get (gdbarch); > if (table != nullptr) > return table; > > - table = new frame_unwind_table; > - > - /* Start the table out with a few default sniffers. OSABI code > - can't override this. */ > - struct frame_unwind_table_entry **link = &table->list; > + table = new std::vector; > + table->insert (table->begin (), standard_unwinders.begin (), > + standard_unwinders.end ()); > > - struct obstack *obstack = gdbarch_obstack (gdbarch); > - link = add_unwinder (obstack, &dummy_frame_unwind, link); > - /* The DWARF tailcall sniffer must come before the inline sniffer. > - Otherwise, we can end up in a situation where a DWARF frame finds > - tailcall information, but then the inline sniffer claims a frame > - before the tailcall sniffer, resulting in confusion. This is > - safe to do always because the tailcall sniffer can only ever be > - activated if the newer frame was created using the DWARF > - unwinder, and it also found tailcall information. */ > - link = add_unwinder (obstack, &dwarf2_tailcall_frame_unwind, link); > - link = add_unwinder (obstack, &inline_frame_unwind, link); > - > - /* The insertion point for OSABI sniffers. */ > - table->osabi_head = link; > frame_unwind_data.set (gdbarch, table); > > return table; > @@ -95,27 +76,16 @@ void > frame_unwind_prepend_unwinder (struct gdbarch *gdbarch, > const struct frame_unwind *unwinder) > { > - struct frame_unwind_table *table = get_frame_unwind_table (gdbarch); > - struct frame_unwind_table_entry *entry; > - > - /* Insert the new entry at the start of the list. */ > - entry = GDBARCH_OBSTACK_ZALLOC (gdbarch, struct frame_unwind_table_entry); > - entry->unwinder = unwinder; > - entry->next = (*table->osabi_head); > - (*table->osabi_head) = entry; > + std::vector *table = get_frame_unwind_table (gdbarch); > + > + table->insert (table->begin () + prepend_unwinder_index, unwinder); > } > > void > frame_unwind_append_unwinder (struct gdbarch *gdbarch, > const struct frame_unwind *unwinder) > { > - struct frame_unwind_table *table = get_frame_unwind_table (gdbarch); > - struct frame_unwind_table_entry **ip; > - > - /* Find the end of the list and insert the new entry there. */ > - for (ip = table->osabi_head; (*ip) != NULL; ip = &(*ip)->next); > - (*ip) = GDBARCH_OBSTACK_ZALLOC (gdbarch, struct frame_unwind_table_entry); > - (*ip)->unwinder = unwinder; > + get_frame_unwind_table (gdbarch)->push_back (unwinder); > } > > /* Call SNIFFER from UNWINDER. If it succeeded set UNWINDER for > @@ -188,9 +158,6 @@ frame_unwind_find_by_frame (const frame_info_ptr &this_frame, void **this_cache) > FRAME_SCOPED_DEBUG_ENTER_EXIT; > frame_debug_printf ("this_frame=%d", frame_relative_level (this_frame)); > > - struct gdbarch *gdbarch = get_frame_arch (this_frame); > - struct frame_unwind_table *table = get_frame_unwind_table (gdbarch); > - struct frame_unwind_table_entry *entry; > const struct frame_unwind *unwinder_from_target; > > unwinder_from_target = target_get_unwinder (); > @@ -205,8 +172,10 @@ frame_unwind_find_by_frame (const frame_info_ptr &this_frame, void **this_cache) > unwinder_from_target)) > return; > > - for (entry = table->list; entry != NULL; entry = entry->next) > - if (frame_unwind_try_unwinder (this_frame, this_cache, entry->unwinder)) > + struct gdbarch *gdbarch = get_frame_arch (this_frame); > + std::vector *table = get_frame_unwind_table (gdbarch); > + for (auto unwinder : *table) I think you can use: for (const auto &unwinder : *table) here. The 'const' is just good style as you're not going to modify it. The '&' is not really necessary as the type is actually a pointer. Better still might be: for (const frame_unwind *unwinder : *table) But I don't mind the 'auto' here too much as the type is on the line immediately above. But I do think, if you're sticking with 'auto' here then throwing the '&' in is a good idea. > + if (frame_unwind_try_unwinder (this_frame, this_cache, unwinder)) > return; > > internal_error (_("frame_unwind_find_by_frame failed")); > @@ -347,7 +316,7 @@ static void > maintenance_info_frame_unwinders (const char *args, int from_tty) > { > gdbarch *gdbarch = current_inferior ()->arch (); > - struct frame_unwind_table *table = get_frame_unwind_table (gdbarch); > + std::vector *table = get_frame_unwind_table (gdbarch); > > ui_out *uiout = current_uiout; > ui_out_emit_table table_emitter (uiout, 2, -1, "FrameUnwinders"); > @@ -355,15 +324,11 @@ maintenance_info_frame_unwinders (const char *args, int from_tty) > uiout->table_header (25, ui_left, "type", "Type"); > uiout->table_body (); > > - for (struct frame_unwind_table_entry *entry = table->list; entry != NULL; > - entry = entry->next) > + for (auto unwinder : *table) See the comments above. If you're happy making this changes I suggest then: Approved-By: Andrew Burgess Thanks, Andrew > { > - const char *name = entry->unwinder->name; > - const char *type = frame_type_str (entry->unwinder->type); > - > ui_out_emit_list tuple_emitter (uiout, nullptr); > - uiout->field_string ("name", name); > - uiout->field_string ("type", type); > + uiout->field_string ("name", unwinder->name); > + uiout->field_string ("type", frame_type_str (unwinder->type)); > uiout->text ("\n"); > } > } > -- > 2.47.0