From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id pemyI+ZPimf/RxIAWB0awg (envelope-from ) for ; Fri, 17 Jan 2025 07:41:10 -0500 Authentication-Results: simark.ca; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=PBR2VY8N; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 7C6B81E100; Fri, 17 Jan 2025 07:41:10 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 4.0.0 (2022-12-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-5.1 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIM_INVALID,DKIM_SIGNED,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 4003D1E08E for ; Fri, 17 Jan 2025 07:41:08 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id BE9963848439 for ; Fri, 17 Jan 2025 12:41:07 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org BE9963848439 Authentication-Results: sourceware.org; dkim=fail reason="signature verification failed" (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=PBR2VY8N 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 0A79C3848361 for ; Fri, 17 Jan 2025 12:40:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 0A79C3848361 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 0A79C3848361 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=1737117623; cv=none; b=mYL2jYlNOSEJG+bXHBq3Gizd5GKKzW5png/hUAiIMNR939MogzQQPvD8zkFm0nbRnXy5ayGom2mgAPLdushm/FpNLcrO7aZRW8gnfZfJG0OXgRQIC1QwFajXtqWrGx+otNY2vMkDPQnuWRZ6UG9BVRx2EVAcG/tB1AG0jMH6av8= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1737117623; c=relaxed/simple; bh=2J3OzAL/k0hfBbACPF2hm4A2FVMD9wqKg5poc2SZsW0=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=DpSQLviWFLj3uMYLHjHF1y5FINyQCvQhDnb6hqA8SMa/q7jbu63yr/6P5RIiVYgMV3uTM4Sw4HqlgvduQoMJVYXPFaYv6NsIKGcCi/W7j3Qf/w/b9NjuyUye4M+Oc3hPaETmSAmnmNlcbS7EJPOzeTR4P1FY4IPKHGDR2MlKeMs= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 0A79C3848361 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1737117622; 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=mi6BwfUKQOX8fezCb9prorrGqTrBtN0NPdBC1UQpAN8=; b=PBR2VY8NoDS2uS26OGOKTWN4Z8fhk657RU5Jw08MwBDgCrR0KWImVTWRq6sr6O7XCfVeg0 jhTRtjeryfuv9CsxTK7uE9qRd80apVza73YZ1weM/2zkMaqFfdyQQ6c/QkwU9bnUSbSRgq Rw1O1wo8PjJkC1EIQG6nOEMZTbZLSVc= Received: from mail-pj1-f72.google.com (mail-pj1-f72.google.com [209.85.216.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-615-feNGhUuZMjy646IyisxVYQ-1; Fri, 17 Jan 2025 07:40:21 -0500 X-MC-Unique: feNGhUuZMjy646IyisxVYQ-1 X-Mimecast-MFC-AGG-ID: feNGhUuZMjy646IyisxVYQ Received: by mail-pj1-f72.google.com with SMTP id 98e67ed59e1d1-2ef728e36d5so4085180a91.3 for ; Fri, 17 Jan 2025 04:40:21 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1737117620; x=1737722420; 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=0rxjNXYQGLNoDcP9xN1n2sZCA/OjzH/CKm3mpDYgiR4=; b=PwgnKkjsNEghYHf5jiX81zv8ybD6udY5GmXzL1hsZzyh0CitsziAMTRCmt3wi44gOC LhlGG3FSF8uhHWoznRmfxYVcL6ti9zt63w0tiVQWzExrY12BGYDhqY6g66CmWpp6WjUi UPQ3+HRGnnsydtHzaVisqnjpzlqGTfGSuI+iKm/dyBhu7ojGjy7FZXloKEOQ3wn9++x9 zoDse+ld6ggzyoTCaqibNTOJAvOdvI6wnqHNoM5Ry0Z+kRfMbL4nDAfLE9cO3GTzQAJV K7FlKiIBruUcAjKAscJIHCaYfuzj9DHHDGB1DNVTLZEClecr2rAjnKnMSUShJHHPI93d 9tWw== X-Forwarded-Encrypted: i=1; AJvYcCX43v1jBJILiKsp8vL/ga+wlt4gtYAxZA4o8qiHGYBu4zR8QEQCTJE5RSy8pBQ7UIunL1xsbO7R6zMBpw==@sourceware.org X-Gm-Message-State: AOJu0YzIavtT8VLr31+vBipWDMNBEXXuJUjlHRHwPW9DBOvOhMoA2kFy ePVA5WIRTqxE51ONtj0LTy0rmw+RoV6B0yE2pGE/ABzQ6K0is0bUf4qBc0DUToFYox58bJFfCtp cjvlSWJMZ2b30MPkwj/PU/ff3yHLdAdAt+hWTD0NPKBEbPSBjpkx6Eubkwgw= X-Gm-Gg: ASbGncsRmNwTDn17+/q5D1CgHHO4KkZ3WD0kxRE53C2nI8vs+i1oUbm4EmcL0KxqILg ZzERw2XQLEflYvOibU4I0bmveglfGDOtw2yVVGT+G33h8qNhQBg4RY764/GEr3c7sZDcJTkOcgj ESDEDHIKVh3zfVbbtkRcQbG5B1pAXYao28lhzlslhc9K5PQ00QUwwVyNZ17bm8rsz2sySCwuW4+ Qe+gKzx26cwIY1PhHceK817nyvso7G7oX8GJfeOFGCBafKA4QLZ6v2+owgZf+iEwut9 X-Received: by 2002:a17:90b:3503:b0:2ea:61de:38f7 with SMTP id 98e67ed59e1d1-2f782d59adbmr3422182a91.29.1737117619680; Fri, 17 Jan 2025 04:40:19 -0800 (PST) X-Google-Smtp-Source: AGHT+IEHS2SCPWabJjHV8w67OLyqoqjJpYlRK/XG3acbfOAScMYuQYMCbg1nxzSx8T1iIPU99QLCJg== X-Received: by 2002:a17:90b:3503:b0:2ea:61de:38f7 with SMTP id 98e67ed59e1d1-2f782d59adbmr3422115a91.29.1737117618788; Fri, 17 Jan 2025 04:40:18 -0800 (PST) Received: from ?IPV6:2804:14d:8084:9a69::1002? ([2804:14d:8084:9a69::1002]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-21c2d3eff31sm14624125ad.200.2025.01.17.04.40.15 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 17 Jan 2025 04:40:17 -0800 (PST) Message-ID: <46aa12d0-d127-4fc9-ba6f-0b5b269eff22@redhat.com> Date: Fri, 17 Jan 2025 09:40:13 -0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v8 4/5] gdb: introduce ability to disable frame unwinders To: Andrew Burgess , gdb-patches@sourceware.org Cc: Eli Zaretskii , Thiago Jung Bauermann References: <20241210195115.3046370-1-guinevere@redhat.com> <20241210195115.3046370-5-guinevere@redhat.com> <87a5brotdk.fsf@redhat.com> From: Guinevere Larsen In-Reply-To: <87a5brotdk.fsf@redhat.com> X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: 5U1EeIJ-eKkNS5hHiZ0MZVXokWQhACIEeiHW7FEF5Vw_1737117620 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/16/25 9:06 AM, Andrew Burgess wrote: > Guinevere Larsen writes: > >> Sometimes, in the GDB testsuite, we want to test the ability of specific >> unwinders to handle some piece of code. Usually this is done by trying >> to outsmart GDB, or by coercing the compiler to remove information that >> GDB would rely on. Both approaches have problems as GDB gets smarter >> with time, and that compilers might differ in version and behavior, or >> simply introduce new useful information. This was requested back in 2003 >> in PR backtrace/8434. >> >> To improve our ability to thoroughly test GDB, this patch introduces a >> new maintenance command that allows a user to disable some unwinders, >> based on either the name of the unwinder or on its class. With this >> change, it will now be possible for GDB to not find any frame unwinders >> for a given frame, which would previously cause GDB to assert. GDB will >> now check if any frame unwinder has been disabled, and if some has, it >> will just error out instead of asserting. >> >> Unwinders can be disabled or re-enabled in 3 different ways: >> * Disabling/enabling all at once (using '-all'). >> * By specifying an unwinder class to be disabled (option '-class'). >> * By specifying the name of an unwinder (option '-name'). >> >> If you give no options to the command, GDB assumes the input is an >> unwinder class. '-class' would make no difference if used, is just here >> for completeness. >> >> This command is meant to be used once the inferior is already at the >> desired location for the test. An example session would be: >> >> (gdb) start >> Temporary breakpoint 1, main () at omp.c:17 >> 17 func(); >> (gdb) maint frame-unwinder disable ARCH >> (gdb) bt >> \#0 main () at omp.c:17 >> (gdb) maint frame-unwinder enable ARCH >> (gdb) cont >> Continuing. >> >> This commit is a more generic version of commit 3c3bb0580be0, >> and so, based on the final paragraph of the commit message: >> gdb: Add switch to disable DWARF stack unwinders >> <...> >> If in the future we find ourselves adding more switches to disable >> different unwinders, then we should probably move to a more generic >> solution, and remove this patch. >> this patch also reverts 3c3bb0580be0 >> >> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=8434 >> Reviewed-By: Eli Zaretskii >> Reviewed-by: Thiago Jung Bauermann >> --- >> gdb/NEWS | 17 ++ >> gdb/doc/gdb.texinfo | 49 ++-- >> gdb/dwarf2/frame-tailcall.c | 3 - >> gdb/dwarf2/frame.c | 30 --- >> gdb/dwarf2/frame.h | 6 - >> gdb/frame-unwind.c | 237 +++++++++++++++++- >> gdb/frame-unwind.h | 9 + >> .../gdb.base/frame-info-consistent.exp | 8 +- >> gdb/testsuite/gdb.base/frame-unwind-disable.c | 22 ++ >> .../gdb.base/frame-unwind-disable.exp | 137 ++++++++++ >> gdb/testsuite/gdb.base/maint.exp | 4 - >> 11 files changed, 437 insertions(+), 85 deletions(-) >> create mode 100644 gdb/testsuite/gdb.base/frame-unwind-disable.c >> create mode 100644 gdb/testsuite/gdb.base/frame-unwind-disable.exp >> >> diff --git a/gdb/NEWS b/gdb/NEWS >> index 245b355669a..ef3317c11cb 100644 >> --- a/gdb/NEWS >> +++ b/gdb/NEWS >> @@ -125,6 +125,13 @@ maintenance info blocks [ADDRESS] >> are listed starting at the inner global block out to the most inner >> block. >> >> +maintenance frame-unwinder disable [-all | -name NAME | [-class] CLASS] >> +maintenance frame-unwinder enable [-all | -name NAME | [-class] CLASS] >> + Enable or disable frame unwinders. This is only meant to be used when >> + testing unwinders themselves, and you want to ensure that a fallback >> + algorithm won't obscure a regression. GDB is not expected to behave >> + well if you try to execute the inferior with unwinders disabled. >> + >> info missing-objfile-handlers >> List all the registered missing-objfile handlers. >> >> @@ -167,6 +174,16 @@ maintenance print remote-registers >> mainenance info frame-unwinders >> Add a CLASS column to the output. This class is a somewhat arbitrary >> grouping of unwinders, based on which area of GDB adds the unwinder. >> + Also add an ENABLED column, that will show if the unwinder is enabled >> + or not. >> + >> +maintenance set dwarf unwinders (on|off) >> + This command has been removed because the same functionality can be >> + achieved with maint frame-unwinder (enable|disable) DEBUGINFO. >> + >> +maintenance show dwarf unwinders >> + This command has been removed since the functionality can be achieved >> + by checking the last column of maint info frame-unwinders. >> >> show configuration >> Now includes the version of GNU Readline library that GDB is using. >> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo >> index 115c1f46b7f..10e6654a03b 100644 >> --- a/gdb/doc/gdb.texinfo >> +++ b/gdb/doc/gdb.texinfo >> @@ -42210,31 +42210,6 @@ On hosts without threading, or where worker threads have been disabled >> at runtime, this setting has no effect, as DWARF reading is always >> done on the main thread, and is therefore always synchronous. >> >> -@kindex maint set dwarf unwinders >> -@kindex maint show dwarf unwinders >> -@item maint set dwarf unwinders >> -@itemx maint show dwarf unwinders >> -Control use of the DWARF frame unwinders. >> - >> -@cindex DWARF frame unwinders >> -Many targets that support DWARF debugging use @value{GDBN}'s DWARF >> -frame unwinders to build the backtrace. Many of these targets will >> -also have a second mechanism for building the backtrace for use in >> -cases where DWARF information is not available, this second mechanism >> -is often an analysis of a function's prologue. >> - >> -In order to extend testing coverage of the second level stack >> -unwinding mechanisms it is helpful to be able to disable the DWARF >> -stack unwinders, this can be done with this switch. >> - >> -In normal use of @value{GDBN} disabling the DWARF unwinders is not >> -advisable, there are cases that are better handled through DWARF than >> -prologue analysis, and the debug experience is likely to be better >> -with the DWARF frame unwinders enabled. >> - >> -If DWARF frame unwinders are not supported for a particular target >> -architecture, then enabling this flag does not cause them to be used. >> - >> @kindex maint info frame-unwinders >> @item maint info frame-unwinders >> List the frame unwinders currently in effect, starting with the highest >> @@ -42252,6 +42227,30 @@ Unwinders installed by debug information readers. >> Unwinders installed by the architecture specific code. >> @end table >> >> +@kindex maint frame-unwinder disable >> +@kindex maint frame-unwinder enable >> +@item maint frame-unwinder disable [@code{-all} | @code{-name} @var{name} | [@code{-class}] @var{class}] >> +@item maint frame-unwinder enable [@code{-all} | @code{-name} @var{name} | [@code{-class}] @var{class}] >> +Disable or enable frame unwinders. This is meant only as a testing tool, and >> +@value{GDBN} is not guaranteed to work properly if it is unable to find >> +valid frame unwinders. >> + >> +The meaning of each of the invocations are as follows: >> + >> +@table @samp >> +@item @code{-all} >> +Disable or enable all unwinders. >> +@item @code{-name} @var{name} >> +@var{name} is the exact name of the unwinder to be disabled or enabled. >> +@item [@code{-class}] @var{class} >> +@var{class} is the class of frame unwinders to be disabled or enabled. >> +The class may include the prefix @code{FRAME_UNWINDER_}, but it is not >> +required. >> +@end table >> + >> +@var{name} and @var{class} are always case insensitive. If no option >> +starting wth @code{-} is given, @value{GDBN} assumes @code{-class}. >> + >> @kindex maint set worker-threads >> @kindex maint show worker-threads >> @item maint set worker-threads >> diff --git a/gdb/dwarf2/frame-tailcall.c b/gdb/dwarf2/frame-tailcall.c >> index 54813d00b03..2d7ab740f62 100644 >> --- a/gdb/dwarf2/frame-tailcall.c >> +++ b/gdb/dwarf2/frame-tailcall.c >> @@ -321,9 +321,6 @@ tailcall_frame_sniffer (const struct frame_unwind *self, >> int next_levels; >> struct tailcall_cache *cache; >> >> - if (!dwarf2_frame_unwinders_enabled_p) >> - return 0; >> - >> /* Inner tail call element does not make sense for a sentinel frame. */ >> next_frame = get_next_frame (this_frame); >> if (next_frame == NULL) >> diff --git a/gdb/dwarf2/frame.c b/gdb/dwarf2/frame.c >> index 85e1d59bc09..e0e8eb5dbec 100644 >> --- a/gdb/dwarf2/frame.c >> +++ b/gdb/dwarf2/frame.c >> @@ -183,9 +183,6 @@ static ULONGEST read_encoded_value (struct comp_unit *unit, gdb_byte encoding, >> unrelocated_addr func_base); >> >> >> -/* See dwarf2/frame.h. */ >> -bool dwarf2_frame_unwinders_enabled_p = true; >> - >> /* Store the length the expression for the CFA in the `cfa_reg' field, >> which is unused in that case. */ >> #define cfa_exp_len cfa_reg >> @@ -1306,9 +1303,6 @@ static int >> dwarf2_frame_sniffer (const struct frame_unwind *self, >> const frame_info_ptr &this_frame, void **this_cache) >> { >> - if (!dwarf2_frame_unwinders_enabled_p) >> - return 0; >> - >> /* Grab an address that is guaranteed to reside somewhere within the >> function. get_frame_pc(), with a no-return next function, can >> end up returning something past the end of this function's body. >> @@ -2253,34 +2247,10 @@ dwarf2_build_frame_info (struct objfile *objfile) >> set_comp_unit (objfile, unit.release ()); >> } >> >> -/* Handle 'maintenance show dwarf unwinders'. */ >> - >> -static void >> -show_dwarf_unwinders_enabled_p (struct ui_file *file, int from_tty, >> - struct cmd_list_element *c, >> - const char *value) >> -{ >> - gdb_printf (file, >> - _("The DWARF stack unwinders are currently %s.\n"), >> - value); >> -} >> - >> void _initialize_dwarf2_frame (); >> void >> _initialize_dwarf2_frame () >> { >> - add_setshow_boolean_cmd ("unwinders", class_obscure, >> - &dwarf2_frame_unwinders_enabled_p , _("\ >> -Set whether the DWARF stack frame unwinders are used."), _("\ >> -Show whether the DWARF stack frame unwinders are used."), _("\ >> -When enabled the DWARF stack frame unwinders can be used for architectures\n\ >> -that support the DWARF unwinders. Enabling the DWARF unwinders for an\n\ >> -architecture that doesn't support them will have no effect."), >> - NULL, >> - show_dwarf_unwinders_enabled_p, >> - &set_dwarf_cmdlist, >> - &show_dwarf_cmdlist); >> - >> #if GDB_SELF_TEST >> selftests::register_test_foreach_arch ("execute_cfa_program", >> selftests::execute_cfa_program_test); >> diff --git a/gdb/dwarf2/frame.h b/gdb/dwarf2/frame.h >> index 2167310fbdf..fe2d669a983 100644 >> --- a/gdb/dwarf2/frame.h >> +++ b/gdb/dwarf2/frame.h >> @@ -198,12 +198,6 @@ struct dwarf2_frame_state >> bool armcc_cfa_offsets_reversed = false; >> }; >> >> -/* When this is true the DWARF frame unwinders can be used if they are >> - registered with the gdbarch. Not all architectures can or do use the >> - DWARF unwinders. Setting this to true on a target that does not >> - otherwise support the DWARF unwinders has no effect. */ >> -extern bool dwarf2_frame_unwinders_enabled_p; >> - >> /* Set the architecture-specific register state initialization >> function for GDBARCH to INIT_REG. */ >> >> diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c >> index 3ab6af1c4da..968bb846402 100644 >> --- a/gdb/frame-unwind.c >> +++ b/gdb/frame-unwind.c >> @@ -29,6 +29,7 @@ >> #include "gdbarch.h" >> #include "dwarf2/frame-tailcall.h" >> #include "cli/cli-cmds.h" >> +#include "cli/cli-option.h" >> #include "inferior.h" >> >> /* Conversion list between the enum for frame_unwind_class and >> @@ -89,6 +90,20 @@ frame_unwinder_class_str (frame_unwind_class uclass) >> return unwind_class_conversion[uclass]; >> } >> >> +/* Case insensitive search for a frame_unwind_class based on the given >> + string. */ >> +static enum frame_unwind_class >> +str_to_frame_unwind_class (const char *class_str) >> +{ >> + for (int i = 0; i < UNWIND_CLASS_NUMBER; i++) >> + { >> + if (strcasecmp (unwind_class_conversion[i], class_str) == 0) >> + return (frame_unwind_class) i; >> + } >> + >> + error (_("Unknown frame unwind class: %s"), class_str); >> +} >> + >> void >> frame_unwind_prepend_unwinder (struct gdbarch *gdbarch, >> const struct frame_unwind *unwinder) >> @@ -175,27 +190,46 @@ 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)); >> >> - const struct frame_unwind *unwinder_from_target; >> + /* If we see a disabled unwinder, we assume some test is being run on >> + GDB, and we don't want to internal_error at the end of this function. */ >> + bool seen_disabled_unwinder = false; >> + /* Lambda to factor out the logic of checking if an unwinder is enabled, >> + testing it and otherwise recording if we saw a disable unwinder. */ >> + auto test_unwinder = [&] (const struct frame_unwind *unwinder) >> + { >> + if (unwinder == nullptr) >> + return false; >> + >> + if (!unwinder->enabled ()) >> + { >> + seen_disabled_unwinder = true; >> + return false; >> + } >> + >> + return frame_unwind_try_unwinder (this_frame, >> + this_cache, >> + unwinder); >> + }; >> >> - unwinder_from_target = target_get_unwinder (); >> - if (unwinder_from_target != NULL >> - && frame_unwind_try_unwinder (this_frame, this_cache, >> - unwinder_from_target)) >> + if (test_unwinder (target_get_unwinder ())) >> return; >> >> - unwinder_from_target = target_get_tailcall_unwinder (); >> - if (unwinder_from_target != NULL >> - && frame_unwind_try_unwinder (this_frame, this_cache, >> - unwinder_from_target)) >> + if (test_unwinder (target_get_tailcall_unwinder ())) >> return; >> >> struct gdbarch *gdbarch = get_frame_arch (this_frame); >> std::vector *table = get_frame_unwind_table (gdbarch); >> for (auto unwinder : *table) >> - if (frame_unwind_try_unwinder (this_frame, this_cache, unwinder)) >> - return; >> + { >> + if (test_unwinder (unwinder)) >> + return; >> + } >> >> - internal_error (_("frame_unwind_find_by_frame failed")); >> + if (seen_disabled_unwinder) >> + error (_("Required frame unwinder may have been disabled" >> + ", see 'maint info frame-unwinders'")); >> + else >> + internal_error (_("frame_unwind_find_by_frame failed")); >> } >> >> /* A default frame sniffer which always accepts the frame. Used by >> @@ -394,10 +428,11 @@ maintenance_info_frame_unwinders (const char *args, int from_tty) >> std::vector *table = get_frame_unwind_table (gdbarch); >> >> ui_out *uiout = current_uiout; >> - ui_out_emit_table table_emitter (uiout, 3, -1, "FrameUnwinders"); >> + ui_out_emit_table table_emitter (uiout, 4, -1, "FrameUnwinders"); >> uiout->table_header (27, ui_left, "name", "Name"); >> uiout->table_header (25, ui_left, "type", "Type"); >> uiout->table_header (9, ui_left, "class", "Class"); >> + uiout->table_header (8, ui_left, "enabled", "Enabled"); >> uiout->table_body (); >> >> for (auto unwinder : *table) >> @@ -407,10 +442,145 @@ maintenance_info_frame_unwinders (const char *args, int from_tty) >> uiout->field_string ("type", frame_type_str (unwinder->type ())); >> uiout->field_string ("class", frame_unwinder_class_str ( >> unwinder->unwinder_class ())); >> + uiout->field_string ("enabled", unwinder->enabled () ? "Y" : "N"); >> uiout->text ("\n"); >> } >> } >> >> +/* Options for disabling frame unwinders. */ >> +struct maint_frame_unwind_options >> +{ >> + std::string unwinder_name; >> + const char *unwinder_class = nullptr; >> + bool all = false; >> +}; >> + >> +static const gdb::option::option_def maint_frame_unwind_opt_defs[] = { >> + >> + gdb::option::flag_option_def { >> + "all", >> + [] (maint_frame_unwind_options *opt) { return &opt->all; }, >> + nullptr, >> + N_("Change the state of all unwinders") >> + }, >> + gdb::option::string_option_def { >> + "name", >> + [] (maint_frame_unwind_options *opt) { return &opt->unwinder_name; }, >> + nullptr, >> + N_("The name of the unwinder to have its state changed") >> + }, >> + gdb::option::enum_option_def { >> + "class", >> + unwind_class_conversion, >> + [] (maint_frame_unwind_options *opt) { return &opt->unwinder_class; }, >> + nullptr, >> + N_("The class of unwinders to have their states changed") >> + } >> +}; >> + >> +static inline gdb::option::option_def_group >> +make_frame_unwind_enable_disable_options (maint_frame_unwind_options *opts) >> +{ >> + return {{maint_frame_unwind_opt_defs}, opts}; >> +} >> + >> +/* Helper function to both enable and disable frame unwinders. >> + If ENABLE is true, this call will be enabling unwinders, >> + otherwise the unwinders will be disabled. */ >> +static void >> +enable_disable_frame_unwinders (const char *args, int from_tty, bool enable) >> +{ >> + if (args == nullptr) >> + { >> + if (enable) >> + error (_("Specify which frame unwinder(s) should be enabled")); >> + else >> + error (_("Specify which frame unwinder(s) should be disabled")); >> + } >> + >> + struct gdbarch* gdbarch = current_inferior ()->arch (); >> + std::vector *unwinder_list >> + = get_frame_unwind_table (gdbarch); >> + >> + maint_frame_unwind_options opts; >> + gdb::option::process_options >> + (&args, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_ERROR, >> + make_frame_unwind_enable_disable_options (&opts)); >> + >> + if ((opts.all && !opts.unwinder_name.empty ()) >> + || (opts.all && opts.unwinder_class != nullptr) >> + || (!opts.unwinder_name.empty () && opts.unwinder_class != nullptr)) >> + error (_("Options are mutually exclusive")); >> + >> + /* First see if the user wants to change all unwinders. */ >> + if (opts.all) >> + { >> + for (const frame_unwind *u : *unwinder_list) >> + u->set_enabled (enable); >> + >> + reinit_frame_cache (); >> + return; >> + } >> + >> + /* If user entered a specific unwinder name, handle it here. If the >> + unwinder is already at the expected state, error out. */ >> + if (!opts.unwinder_name.empty ()) >> + { >> + bool did_something = false; >> + for (const frame_unwind *unwinder : *unwinder_list) >> + { >> + if (strcasecmp (unwinder->name (), >> + opts.unwinder_name.c_str ()) == 0) >> + { >> + if (unwinder->enabled () == enable) >> + { >> + if (unwinder->enabled ()) >> + error (_("unwinder %s is already enabled"), >> + unwinder->name ()); >> + else >> + error (_("unwinder %s is already disabled"), >> + unwinder->name ()); >> + } >> + unwinder->set_enabled (enable); >> + >> + did_something = true; >> + break; >> + } >> + } >> + if (!did_something) >> + error (_("couldn't find unwinder named %s"), >> + opts.unwinder_name.c_str ()); >> + } >> + else >> + { >> + if (opts.unwinder_class == nullptr) >> + opts.unwinder_class = args; >> + enum frame_unwind_class dclass = str_to_frame_unwind_class >> + (opts.unwinder_class); >> + for (auto unwinder: *unwinder_list) > I think you should stick with the form you use earlier: > > (const frame_unwind *unwinder : *unwinder_list) > > But if you really want to stick with auto, then I think: > > (const auto &unwinder : *unwinder_list) > > would be OK. > > >> + { >> + if (unwinder->unwinder_class () == dclass) >> + unwinder->set_enabled (enable); >> + } >> + } >> + >> + reinit_frame_cache (); >> +} >> + >> +/* Implement "maint frame-unwinder disable" command. */ >> +static void >> +maintenance_disable_frame_unwinders (const char *args, int from_tty) >> +{ >> + enable_disable_frame_unwinders (args, from_tty, false); >> +} >> + >> +/* Implement "maint frame-unwinder enable" command. */ >> +static void >> +maintenance_enable_frame_unwinders (const char *args, int from_tty) >> +{ >> + enable_disable_frame_unwinders (args, from_tty, true); >> +} >> + >> void _initialize_frame_unwind (); >> void >> _initialize_frame_unwind () >> @@ -423,4 +593,45 @@ _initialize_frame_unwind () >> List the frame unwinders currently in effect.\n\ >> Unwinders are listed starting with the highest priority."), >> &maintenanceinfolist); >> + >> + /* Add "maint frame-unwinder disable/enable". */ >> + static struct cmd_list_element *maint_frame_unwinder; >> + >> + add_basic_prefix_cmd ("frame-unwinder", class_maintenance, >> + _("Commands handling frame unwinders."), >> + &maint_frame_unwinder, 0, &maintenancelist); >> + >> + add_cmd ("disable", class_maintenance, maintenance_disable_frame_unwinders, >> + _("\ >> +Disable one or more frame unwinder(s).\n\ >> +Usage: maint frame-unwinder disable [-all | -name NAME | [-class] CLASS]\n\ >> +\n\ >> +These are the meanings of the options:\n\ >> +\t-all - All available unwinders will be disabled\n\ >> +\t-name - NAME is the exact name of the frame unwinder to be disabled\n\ >> +\t-class - CLASS is the class of unwinders to be disabled. Valid classes are:\n\ >> +\t\tGDB - Unwinders added by GDB core;\n\ >> +\t\tEXTENSION - Unwinders added by extension languages;\n\ >> +\t\tDEBUGINFO - Unwinders that handle debug information;\n\ >> +\t\tARCH - Unwinders that use architecture-specific information;\n\ >> +\n\ >> +UNWINDER and NAME are case insensitive."), >> + &maint_frame_unwinder); >> + >> + add_cmd ("enable", class_maintenance, maintenance_enable_frame_unwinders, >> + _("\ >> +Enable one or more frame unwinder(s).\n\ >> +Usage: maint frame-unwinder enable [-all | -name NAME | [-class] CLASS]\n\ >> +\n\ >> +These are the meanings of the options:\n\ >> +\t-all - All available unwinders will be enabled\n\ >> +\t-name - NAME is the exact name of the frame unwinder to be enabled\n\ >> +\t-class - CLASS is the class of unwinders to be enabled. Valid classes are:\n\ >> +\t\tGDB - Unwinders added by GDB core;\n\ >> +\t\tEXTENSION - Unwinders added by extension languages;\n\ >> +\t\tDEBUGINFO - Unwinders that handle debug information;\n\ >> +\t\tARCH - Unwinders that use architecture-specific information;\n\ >> +\n\ >> +UNWINDER and NAME are case insensitive."), >> + &maint_frame_unwinder); >> } > At the bottom of this email you'll find a small patch which applies on > top of this and adds some basic command completion. It doesn't complete > the NAMEs, I'm not sure if there's a good way to do that (yet). It also > doesn't switch the help text to use the %OPTIONS% mechanism, I tried > that, but I think there may be some bugs with the formatting (I'll look > into it), so I've left that off. > > You don't have to, we can add completion later, but I do think it would > be good to merge the patch below with this one. But it is up to you. > Take a look and see what you think. > >> diff --git a/gdb/frame-unwind.h b/gdb/frame-unwind.h >> index 13f18618d24..adf8b47982c 100644 >> --- a/gdb/frame-unwind.h >> +++ b/gdb/frame-unwind.h >> @@ -192,6 +192,12 @@ class frame_unwind >> const frame_data *unwind_data () const >> { return m_unwind_data; } >> >> + bool enabled () const >> + { return m_enabled; } >> + >> + void set_enabled (bool state) const >> + { m_enabled = state; } >> + >> /* Default stop_reason implementation. It reports NO_REASON, unless the >> frame is the outermost. */ >> >> @@ -245,6 +251,9 @@ class frame_unwind >> frame_unwind_class m_unwinder_class; >> >> const frame_data *m_unwind_data; >> + >> + /* Whether this unwinder can be used when sniffing. */ >> + mutable bool m_enabled = true; >> }; >> >> /* This is a legacy version of the frame unwinder. The original struct >> diff --git a/gdb/testsuite/gdb.base/frame-info-consistent.exp b/gdb/testsuite/gdb.base/frame-info-consistent.exp >> index fe0cfad95bc..4f483111a91 100644 >> --- a/gdb/testsuite/gdb.base/frame-info-consistent.exp >> +++ b/gdb/testsuite/gdb.base/frame-info-consistent.exp >> @@ -95,11 +95,11 @@ proc compare_frames {frames} { >> } >> } >> >> -proc test {dwarf_unwinder} { >> +proc test {enable} { >> >> clean_restart $::binfile >> >> - gdb_test_no_output "maint set dwarf unwinder $dwarf_unwinder" >> + gdb_test_no_output "maint frame-unwinder $enable DEBUGINFO" >> >> if {![runto_main]} { >> return 0 >> @@ -134,6 +134,6 @@ proc test {dwarf_unwinder} { >> } >> } >> >> -foreach_with_prefix dwarf {"off" "on"} { >> - test $dwarf >> +foreach_with_prefix action {"disable" "enable"} { >> + test $action >> } >> diff --git a/gdb/testsuite/gdb.base/frame-unwind-disable.c b/gdb/testsuite/gdb.base/frame-unwind-disable.c >> new file mode 100644 >> index 00000000000..bbcfb01316e >> --- /dev/null >> +++ b/gdb/testsuite/gdb.base/frame-unwind-disable.c >> @@ -0,0 +1,22 @@ >> +/* This testcase is part of GDB, the GNU debugger. >> + >> + Copyright 2024 Free Software Foundation, Inc. > Remember to update the copyright date when rebasing. > >> + >> + This program is free software; you can redistribute it and/or modify >> + it under the terms of the GNU General Public License as published by >> + the Free Software Foundation; either version 3 of the License, or >> + (at your option) any later version. >> + >> + This program is distributed in the hope that it will be useful, >> + but WITHOUT ANY WARRANTY; without even the implied warranty of >> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + GNU General Public License for more details. >> + >> + You should have received a copy of the GNU General Public License >> + along with this program. If not, see . */ >> + >> +int >> +main () >> +{ >> + return 0; >> +} >> diff --git a/gdb/testsuite/gdb.base/frame-unwind-disable.exp b/gdb/testsuite/gdb.base/frame-unwind-disable.exp >> new file mode 100644 >> index 00000000000..f4cad9a47fd >> --- /dev/null >> +++ b/gdb/testsuite/gdb.base/frame-unwind-disable.exp >> @@ -0,0 +1,137 @@ >> +# Copyright 2024 Free Software Foundation, Inc. > And again with the date. > >> + >> +# This program is free software; you can redistribute it and/or modify >> +# it under the terms of the GNU General Public License as published by >> +# the Free Software Foundation; either version 3 of the License, or >> +# (at your option) any later version. >> +# >> +# This program is distributed in the hope that it will be useful, >> +# but WITHOUT ANY WARRANTY; without even the implied warranty of >> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> +# GNU General Public License for more details. >> +# >> +# You should have received a copy of the GNU General Public License >> +# along with this program. If not, see . >> + >> +# Test multiple situations in which we may use the maintenance command to >> +# disable and enable frame unwinders, and check that they really are >> +# disabled when they say the are. >> + >> +standard_testfile .c > You can drop '.c' here, it's the default. > >> + >> +# Proc to check if the unwinder of the given name is in the desired state. >> +# STATE can be either Y or N. >> +proc check_unwinder_state { unwinder_name state {testname ""} } { >> + set should_pass false >> + set command "maint info frame-unwinders" >> + if {${testname} == ""} { >> + set testname "checking state ${state} for ${unwinder_name}" >> + } >> + gdb_test_multiple "${command}" "${testname}" -lbl { >> + -re "${unwinder_name}\\s+\\w+\\s+\\w+\\s+${state}\\s+(?=\r\n)" { >> + set should_pass true >> + exp_continue >> + } >> + -re "${command}" { >> + exp_continue >> + } >> + -re "\\w+\\s+\\w+\\s+\\w+\\s+\\w+\\s+(?=\r\n)" { >> + exp_continue >> + } >> + -re -wrap "" { >> + gdb_assert ${should_pass} "${gdb_test_name}" > I don't think the quotes around "${gdb_test_name}" are needed here? In > fact, there seem to be a few places where you place quotes around > expansion of a single variable (e.g. "${command}" on the > gdb_test_multiple line), I don't think any of these are needed. > > With these changes (and with or without the completion patch): I applied all the changes, and I like the idea of adding completion, but I wonder: should we add a test for completion? If so, I think this is better saved for a later patch, so that we can work on a good test and further improve the completion, because it would be nice to also complete for "-class", "-name" and "-all", but I'm not sure how that'd work yet. If a test isn't all that important we can add it now and do more work later on. I also have an inlined styling question > > Approved-By: Andrew Burgess > > Thanks, > Andrew > >> + } >> + } >> +} >> + >> +# Check if all unwinders of class UNWINDER_CLASS are in the state STATE. >> +# STATE can be either Y or N. >> +# UNWINDER_CLASS can be one of: GDB, ARCH, EXTENSION, DEBUGINFO. It can >> +# also be \\w+ if checking all unwinders. >> +proc check_unwinder_class { unwinder_class state {testname ""} } { >> + set command "maint info frame-unwinders" >> + set should_pass true >> + if {$testname == ""} { >> + set testname "checking if ${unwinder_class} state is ${state}" >> + } >> + gdb_test_multiple "${command}" "${testname}" -lbl { >> + -re "^\[^\r\n\]+\\s+\\w+\\s+${unwinder_class}\\s+\(\[YN\]\)\\s+(?=\r\n)" { >> + # The unwinder name may have multiple words, so we need to use the >> + # more generic [^\r\n] pattern to match the unwinders. >> + set cur_state $expect_out(1,string) >> + if {$cur_state == $state} { >> + set should_pass false >> + } >> + exp_continue >> + } >> + -re "${command}" { >> + exp_continue >> + } >> + -re -wrap "" { >> + gdb_assert ${should_pass} "${gdb_test_name}" >> + } >> + } >> +} >> + >> +if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile}]} { >> + return -1 >> +} >> + >> +if {![runto_main]} { >> + untested "couldn't run to main" >> + return >> +} >> + >> +# Test disabling all unwinders. >> +check_unwinder_class "\\w+" "Y" \ >> + "all unwinders enabled before any changes" >> +gdb_test_no_output "maint frame-unwinder disable -all" >> +check_unwinder_class "\\w+" "N" \ >> + "all unwinders were properly disabled" >> + >> +# Test if GDB can still make a backtrace once all unwinders are disabled. >> +# It should be impossible. >> +gdb_test "backtrace" \ >> + ".*Required frame unwinder may have been disabled, see 'maint info frame-unwinders'.*" \ >> + "no suitable unwinders should be found" >> + >> +# Reenable all unwinders. >> +gdb_test_no_output "maint frame-unwinder enable -all" >> +check_unwinder_class "\\w+" "Y" \ >> + "all unwinders should be re-enabled" >> + >> +# Check that we are able to get backtraces once again. >> +gdb_test "backtrace" ".0\\s+main .. at.*" \ >> + "we can get usable backtraces again" >> + >> +# Check if we can disable an unwinder based on the name. >> +check_unwinder_state "dummy" "Y" >> +gdb_test_no_output "maint frame-unwinder disable -name dummy" >> +check_unwinder_state "dummy" "N" >> +# And verify what happens if you try to disable it again. >> +gdb_test "maint frame-unwinder disable -name dummy" \ >> + "unwinder dummy is already disabled" \ >> + "disable already disabled unwinder" >> +check_unwinder_state "dummy" "N" "dummy should continue disabled" >> + >> +foreach class {GDB ARCH DEBUGINFO EXTENSION} { >> + # Disable all unwinders of type CLASS, and check that the command worked. >> + gdb_test_no_output "maint frame-unwinder disable ${class}" >> + check_unwinder_class "${class}" "N" >> +} >> + >> +# Now check if we are able to enable a single unwinder, and what happens if we >> +# enable it twice. >> +gdb_test_no_output "maint frame-unwinder enable -name dummy" >> +check_unwinder_state "dummy" "Y" "successfully enabled dummy unwinder" >> +gdb_test "maint frame-unwinder enable -name dummy" \ >> + "unwinder dummy is already enabled" \ >> + "enable already enabled unwinder" >> +check_unwinder_state "dummy" "Y" "dummy should continue enabled" >> + >> +foreach class {GDB ARCH DEBUGINFO EXTENSION} { >> + # Enable all unwinders of type CLASS, and check that the command worked, >> + # using "-class" option to ensure it works. Should make no difference. >> + gdb_test_no_output "maint frame-unwinder enable -class ${class}" >> + check_unwinder_class "${class}" "Y" >> +} >> diff --git a/gdb/testsuite/gdb.base/maint.exp b/gdb/testsuite/gdb.base/maint.exp >> index 2c58ffa36c5..ae7ad0787e6 100644 >> --- a/gdb/testsuite/gdb.base/maint.exp >> +++ b/gdb/testsuite/gdb.base/maint.exp >> @@ -454,10 +454,6 @@ gdb_test_no_output "maint info line-table xxx.c" \ >> >> set timeout $oldtimeout >> >> -# Just check that the DWARF unwinders control flag is visible. >> -gdb_test "maint show dwarf unwinders" \ >> - "The DWARF stack unwinders are currently (on|off)\\." >> - >> #============test help on maint commands >> >> test_prefix_command_help {"maint info" "maintenance info"} { >> -- >> 2.47.0 > --- > > diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c > index 1417651108f..f3019b7ec18 100644 > --- a/gdb/frame-unwind.c > +++ b/gdb/frame-unwind.c > @@ -40,6 +40,7 @@ static const char * unwind_class_conversion[] = > "EXTENSION", > "DEBUGINFO", > "ARCH", > + nullptr > }; > > /* Default sniffers, that must always be the first in the unwinder list, > @@ -579,6 +580,29 @@ enable_disable_frame_unwinders (const char *args, int from_tty, bool enable) > reinit_frame_cache (); > } > > +/* Completer for the "maint frame-unwinder enable|disable" commands. */ > + > +static void > +enable_disable_frame_unwinders_completer (struct cmd_list_element *ignore, > + completion_tracker &tracker, > + const char *text, > + const char */*word*/) Would it be alright to write "const char * /*word*/" ? the */* is confusing my editor and it looks like malformed comments, and if I add the space, the red highlights go away. -- Cheers, Guinevere Larsen She/Her/Hers > +{ > + maint_frame_unwind_options opts; > + const auto group = make_frame_unwind_enable_disable_options (&opts); > + > + const char *start = text; > + if (gdb::option::complete_options > + (tracker, &text, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_OPERAND, group)) > + return; > + > + /* Only complete a class name as a stand-alone operand when no options > + are given. */ > + if (start == text) > + complete_on_enum (tracker, unwind_class_conversion, text, text); > + return; > +} > + > /* Implement "maint frame-unwinder disable" command. */ > static void > maintenance_disable_frame_unwinders (const char *args, int from_tty) > @@ -613,8 +637,9 @@ Unwinders are listed starting with the highest priority."), > _("Commands handling frame unwinders."), > &maint_frame_unwinder, 0, &maintenancelist); > > - add_cmd ("disable", class_maintenance, maintenance_disable_frame_unwinders, > - _("\ > + cmd_list_element *c > + = add_cmd ("disable", class_maintenance, maintenance_disable_frame_unwinders, > + _("\ > Disable one or more frame unwinder(s).\n\ > Usage: maint frame-unwinder disable [-all | -name NAME | [-class] CLASS]\n\ > \n\ > @@ -628,10 +653,13 @@ These are the meanings of the options:\n\ > \t\tARCH - Unwinders that use architecture-specific information;\n\ > \n\ > UNWINDER and NAME are case insensitive."), > - &maint_frame_unwinder); > + &maint_frame_unwinder); > + set_cmd_completer_handle_brkchars (c, > + enable_disable_frame_unwinders_completer); > > - add_cmd ("enable", class_maintenance, maintenance_enable_frame_unwinders, > - _("\ > + c = > + add_cmd ("enable", class_maintenance, maintenance_enable_frame_unwinders, > + _("\ > Enable one or more frame unwinder(s).\n\ > Usage: maint frame-unwinder enable [-all | -name NAME | [-class] CLASS]\n\ > \n\ > @@ -645,5 +673,7 @@ These are the meanings of the options:\n\ > \t\tARCH - Unwinders that use architecture-specific information;\n\ > \n\ > UNWINDER and NAME are case insensitive."), > - &maint_frame_unwinder); > + &maint_frame_unwinder); > + set_cmd_completer_handle_brkchars (c, > + enable_disable_frame_unwinders_completer); > } >