From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 9hqiLJrKhmf9Kw8AWB0awg (envelope-from ) for ; Tue, 14 Jan 2025 15:35:38 -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=cktX/srt; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id A80461E100; Tue, 14 Jan 2025 15:35:38 -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 8EED51E05C for ; Tue, 14 Jan 2025 15:35:37 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 0377938560BA for ; Tue, 14 Jan 2025 20:35:37 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 0377938560BA 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=cktX/srt 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 710693858414 for ; Tue, 14 Jan 2025 20:35:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 710693858414 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 710693858414 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=1736886902; cv=none; b=kLTW3oITQ49r8YhAB+mmnXjuOvQmYLoF0TmKOWDllfZgtgB4N+/SqH4Pb7XSWUTZH3hW9tFk6mdmwt2wmnYodwKO0CIWU4LpB2tzzwP71xbzzNhzuY3svCqONtrSHggZynTFP8z/7dE/tF5QbN6BACiHzYfN5T2ZzuAySA4H2Vg= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1736886902; c=relaxed/simple; bh=UobqNiOKH5gOZgCYRpTLSmW6ph9Ue1XX5Eq5HY3WsTk=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=N7hVAGrEB2P0hN9Fp33iA0rloMvedlLHaC1q/jbzCzyLv2RKO8zykgdpGZG8ecRr4wanRFwImYxBCTzVqP7kNRdu+FDGF2YSmLQKU00P1o+2/I91TldvkeRU/GhOIk+YXEAHocwSjWQWRh3T4oQ2jb9jsaYHw6QyjAFQvaMaWwM= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 710693858414 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1736886902; 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=tqioAawYgPcrg3YZ5JwGdRJiI7ULbBjndfuyd2DprZE=; b=cktX/srt5snhr8gLkk2sXk1zG3L1r1FHru+RPr9NeHMoQu5pNlV6U98SQ1q4dZLbRc4x54 yNxCO7cNz7svKm8u1BLZ1OtQ2cfD2yziO2x1HauTT7ZsN8vlglfm7d6srNUG1lNGkBEcW1 t7W230SStI8yVH9iml6LYIWZ4soF42w= Received: from mail-oa1-f71.google.com (mail-oa1-f71.google.com [209.85.160.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-208-yKKsLoGlMYO3Uz3VjYUAqA-1; Tue, 14 Jan 2025 15:35:01 -0500 X-MC-Unique: yKKsLoGlMYO3Uz3VjYUAqA-1 X-Mimecast-MFC-AGG-ID: yKKsLoGlMYO3Uz3VjYUAqA Received: by mail-oa1-f71.google.com with SMTP id 586e51a60fabf-2a983c18244so4352940fac.1 for ; Tue, 14 Jan 2025 12:35:01 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736886900; x=1737491700; 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=tqioAawYgPcrg3YZ5JwGdRJiI7ULbBjndfuyd2DprZE=; b=ta0vRMk6kpnVzdC6F9gZ7mrTAHv4pc/vsNLJNULJ5PhPotAdGLtquZBUduFJISElk8 iCKY4keONLJ1GZ9ZTkdnpO6pX1y2+oYh9c/FwOjCXdwFKaAL8TVSts/qGA9DdCXFqTZs s1AAPU8KlOh32oIa5svyTWJEYoPN8CkOkZZkCqnU1p2qpDxB3Oo7vfgNzelQ/iiGpkfs 6Fz2CZAHhXqnjm2jENR0tpbU732xrV24Gg4tf6PRRdl30P3LsMp30sXv/DSYg5d0+8El VMqvCCMDQNHPIO4oXh3Aio0PcBZDecyBPlR+WWMcB56r7NuyOUvZkwaCj5MG1bk8RrjY 63aQ== X-Forwarded-Encrypted: i=1; AJvYcCVbxdcTJSj6RlEV3vi5I/z5Q0slDNV4BKO0mxfdUL20uj3zto1eDC/O6J+iN04ASmvq1rkxrl3oRZhGqQ==@sourceware.org X-Gm-Message-State: AOJu0Ywk7QTfBDyQ9js0PBZNHpfChHYkbIKn2rn/2ImvAoneCgHLBIOP DQ8lplRzvvZ6MbTCZWAdPqEhsvpvFBLk4GQl9ffxMJw0XcuNgjpHXi4eMUiaz16YDPtJBN7Ipwe asgkiVgZ64182F4HIjhPmCQ4j+Kb9PQqg30Dr+NoALTulDiJ4crfN8n2rMRs= X-Gm-Gg: ASbGncv7gOrn+amLffqz4bK4W/u1cpuAjvpycCI37n3pllJc1/sOz8LgT7gL2HHbYr/ C0xbD4hhojaacQpUOTavNLDbxIb9MAQVK9hHcXyE/E+h2WhfQLoSQQ1C6xfWEsP9cSmtG2fzK96 PVknmL6+DfOfqW6sic6SXGdXUw8l1HsOTSlXCE1sWUkyj9p0fTLV6i3f1rDrnce0Cb4KUAYHtKz cAE+YJSJfXl8b6C6PvBND2pdilwvajwzsfecKGi4bqtZVaHV0OtQrooEqs22zy6ZZlo X-Received: by 2002:a05:6870:ae06:b0:29f:df27:8b80 with SMTP id 586e51a60fabf-2aa06761bf3mr13819180fac.24.1736886899440; Tue, 14 Jan 2025 12:34:59 -0800 (PST) X-Google-Smtp-Source: AGHT+IEFdzac7RxIIV6Myya9ZEWhDJ8cR4GqvMi2MOQAOOSsKYfUWHVFkdxzN2M9Oq58j2emMDViGw== X-Received: by 2002:a05:6870:ae06:b0:29f:df27:8b80 with SMTP id 586e51a60fabf-2aa06761bf3mr13819169fac.24.1736886898943; Tue, 14 Jan 2025 12:34:58 -0800 (PST) Received: from ?IPV6:2804:14d:8084:9a69::1000? ([2804:14d:8084:9a69::1000]) by smtp.gmail.com with ESMTPSA id 586e51a60fabf-2ad805476f3sm5460879fac.15.2025.01.14.12.34.56 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 14 Jan 2025 12:34:58 -0800 (PST) Message-ID: <10f92b64-4865-499b-8003-036a28228a39@redhat.com> Date: Tue, 14 Jan 2025 17:34:55 -0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v8 1/5] gdb: make gdbarch store a vector of frame unwinders To: Andrew Burgess , gdb-patches@sourceware.org Cc: Thiago Jung Bauermann References: <20241210195115.3046370-1-guinevere@redhat.com> <20241210195115.3046370-2-guinevere@redhat.com> <87ikqhqxk3.fsf@redhat.com> From: Guinevere Larsen In-Reply-To: <87ikqhqxk3.fsf@redhat.com> X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: tI0mDQr8Y1HACJnlirWKkgzQIF_exrzmlOVvrvZLj1I_1736886900 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 1/14/25 11:28 AM, Andrew Burgess wrote: > 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. I decided to go with the "auto" version, since table is always declared close to the usage, and that doesn't use auto. > >> + 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 for the review, I went along with the changes and added your approval! -- Cheers, Guinevere Larsen She/Her/Hers > > 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