From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id sL5BIgiGBme1bwcAWB0awg (envelope-from ) for ; Wed, 09 Oct 2024 09:32:56 -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=VUVONPGa; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 815B21E356; Wed, 9 Oct 2024 09:32:56 -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 2FE2B1E354 for ; Wed, 9 Oct 2024 09:32:55 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 896C338650E7 for ; Wed, 9 Oct 2024 13:32:54 +0000 (GMT) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id 5FBD6385DDD7 for ; Wed, 9 Oct 2024 13:32:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5FBD6385DDD7 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 5FBD6385DDD7 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=1728480753; cv=none; b=R4DAltkfbl9YkLkzVVoOYzwapV0Vn/FC9XkezppYjlCfHu2hNFpWZ+zxVjJcLuzCwyeknpY/9e+QiCQ8j5hfKQ9uoLrHYoMlsV2T+RVBAR9Y2x8B8YQLiMhHvIeOLWGwYKWQt4AkfQOHjwQq5tGzwIYTnRLZ+XpzhwcjqqqwtcI= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1728480753; c=relaxed/simple; bh=naEoKhejJyRhuwOh8FLKICIoDCLt0fyhwFQSDRKNwg8=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=mZIRrWF9o8zKUIKqEXrZTtTHlSfuFaQrbl5A8gQJ6ASFckFdBARo2vEYpeN1Ok2nEvPOlZCtKWb0Oce71qXTxJcuYIz8wTM929lABypIgLCpsMde/xDLvTNx9U6/T33oeA0P3c7z3uwDNUXHEw3f49CF1JF/2jMgRz0rjiLISdM= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1728480750; 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=Ymom8A5H/xP2wKKSbUz3vTbZO0ZZ4/c9/zBSrIgoh0M=; b=VUVONPGapJNxFFhHuBAucJhUcfJ6+r6hOut8VRs7JeTQQoBGTPt4dIjnzmQhsoNtmMaSnj T/dWAgeANb+6BY/wi4Dp+BuuG3KtbxC8IQAne+GtBhIOVVXkma1ochypobi4EYeTwBDeBp xl+dim7+m2bw9YZX8biB46flmK9KIyg= Received: from mail-oo1-f69.google.com (mail-oo1-f69.google.com [209.85.161.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-646-4TQIemH7NiSm2X1xphh5Eg-1; Wed, 09 Oct 2024 09:32:29 -0400 X-MC-Unique: 4TQIemH7NiSm2X1xphh5Eg-1 Received: by mail-oo1-f69.google.com with SMTP id 006d021491bc7-5e98cd475a8so233819eaf.3 for ; Wed, 09 Oct 2024 06:32:29 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1728480749; x=1729085549; 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=Ymom8A5H/xP2wKKSbUz3vTbZO0ZZ4/c9/zBSrIgoh0M=; b=RsFl1S2RBkimS76yVpCf9CXCQ+kZgxVy2ktMWt6sMKqdNoBcmZoyl4CNZVUdlWonKh AuOX7pQL+8UkeWR7YVGZGGLU6DN9DjlDMT/ndib/Q31xH73e4T2CNnUaymEn2JEKa4IJ WZCwCd7eCFSWYFtmlqDmxdBfdZXYDoYfojBiprpnAiPZkix90NfmR5/z+kTXWvMq0jb9 viIjZPbT48hN9peBIaogAoqYtZ8FE2uFh4PNrtSrfdPF5B1+lL7a4ltEISd2Be3lTega BkKAFmh4AmYKcrd0RrTNzCzUDlrnUdPcc/wRuQmGnld0jCwzM+0ZLIAlYV8I/KishZmT Dv0Q== X-Forwarded-Encrypted: i=1; AJvYcCUVICMBF8TyPm9F3hsB44JDilBDsyB/dpdK6edxvekRzRAQgKJL9cdS07j6rk8ZsmR5aKJlE+v+Jf9fhQ==@sourceware.org X-Gm-Message-State: AOJu0YxS2QndbiGqZOTXunHXH5jtLlLGyjJYVDfO3ncB+MRWBf1XTst7 q1hPYjl80GKwG1uPWSbHRCI0SUzeSJoy3YdHhlIthdovWliuIt3pCEjZwsHjobbp1rOb/vNKztm Y/fF21Npws2Xrj6p1LiXiDpndEO8rAQt/yM5quPhZcbp3kZvk8fT6iTyw6IU= X-Received: by 2002:a05:6358:60c9:b0:1b5:c4f3:faec with SMTP id e5c5f4694b2df-1c308085cc8mr18590355d.4.1728480748872; Wed, 09 Oct 2024 06:32:28 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGuT6xTV2Ucvv5w6pecrZln26CNzrXp616gA7/+V4OTA3wiF75qcBu5AijPmYaicyp4yPhnjg== X-Received: by 2002:a05:6358:60c9:b0:1b5:c4f3:faec with SMTP id e5c5f4694b2df-1c308085cc8mr18588255d.4.1728480748429; Wed, 09 Oct 2024 06:32:28 -0700 (PDT) Received: from ?IPV6:2804:14d:8084:92c5::1000? ([2804:14d:8084:92c5::1000]) by smtp.gmail.com with ESMTPSA id af79cd13be357-7af9a257640sm167245285a.85.2024.10.09.06.32.27 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 09 Oct 2024 06:32:28 -0700 (PDT) Message-ID: <9753fe9a-e940-4994-9e2a-acbe491a6cb3@redhat.com> Date: Wed, 9 Oct 2024 10:32:25 -0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v5 4/5] gdb: introduce ability to disable frame unwinders To: Simon Marchi , gdb-patches@sourceware.org Cc: Guinevere Larsen References: <20241001184235.3710608-1-guinevere@redhat.com> <20241001184235.3710608-5-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/5/24 11:51 PM, Simon Marchi wrote: > > On 2024-10-01 14:42, Guinevere Larsen wrote: >> From: Guinevere Larsen >> >> 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. > Heh, it's funny, to me the natural way would have been to make -name the > default case. Heh, I guess thinking about the next test where I want to disable all debuginfo unwinders and it just felt like the obvious choice. >> diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c >> index 7be01ec2203..f66f233c781 100644 >> --- a/gdb/frame-unwind.c >> +++ b/gdb/frame-unwind.c >> @@ -94,6 +94,23 @@ frame_unwinder_class_str (frame_unwind_class uclass) >> return location->second; >> } >> >> +/* 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) >> +{ >> + const char *prefix = "FRAME_UNWIND_"; >> + /* Skip the prefix if present. */ >> + if (strncasecmp (class_str, prefix, strlen(prefix)) == 0) > Missing space. Fixed > OTOH, I don't think accepting user inputs FRAME_UNWIND_ is really > necessary. That's a fair question, but I don't think this is too costly (for performance, or maintenance) to keep it, in case someone reads the code instead of documentation (not that I do that or anything o.o'). If you disagree, I am fine with removing it honestly, especially with documenting the classes that you suggested later. > >> + class_str += strlen (prefix); >> + for (const auto &it : unwind_class_conversion) >> + { >> + if (strcasecmp (it.second, class_str) == 0) >> + return it.first; >> + } > I would suggest spacing things out a bit, addking blank lines after the > declaration of `prefix`, after the if and after the for. I have a hard > time following things when they're all packed. done > > >> + error (_("Unknown frame unwind class: %s"), class_str); >> +} >> + >> void >> frame_unwind_prepend_unwinder (struct gdbarch *gdbarch, >> const struct frame_unwind *unwinder) >> @@ -182,25 +199,47 @@ frame_unwind_find_by_frame (const frame_info_ptr &this_frame, void **this_cache) >> >> 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 assert at the end of this function. */ >> + bool seen_disabled_unwinder = false; >> + >> unwinder_from_target = target_get_unwinder (); >> if (unwinder_from_target != NULL >> + && unwinder_from_target->enabled () >> && frame_unwind_try_unwinder (this_frame, this_cache, >> unwinder_from_target)) >> return; >> + else if (unwinder_from_target != nullptr >> + && !unwinder_from_target->enabled ()) >> + seen_disabled_unwinder = true; > There would be less repetition like this: > > if (unwinder_from_target != nullptr) > { > if (unwinder_from_target->enabled ()) > { > if (frame_unwind_try_unwinder (this_frame, this_cache, > unwinder_from_target)) > return; > } > else > seen_disabled_unwinder = true; > } > > You could also factor out this logic to a lambda and use it at the > multiple spots in the function where this is done. Good idea. I factored into a lambda that receives the unwinder as a parameter, so I can use it every time frame_unwind_try_unwinder. > >> @@ -405,15 +445,97 @@ maintenance_info_frame_unwinders (const char *args, int from_tty) >> const char *name = unwinder->name (); >> const char *type = frame_type_str (unwinder->type ()); >> const char *uclass = frame_unwinder_class_str (unwinder->unwinder_class ()); >> + const char *enabled = unwinder->enabled () ? "Y" : "N"; >> >> ui_out_emit_list tuple_emitter (uiout, nullptr); >> uiout->field_string ("name", name); >> uiout->field_string ("type", type); >> uiout->field_string ("class", uclass); >> + uiout->field_string ("enabled", enabled); >> uiout->text ("\n"); >> } >> } >> >> +/* 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) >> +{ >> + reinit_frame_cache (); >> + if (args == nullptr) >> + { >> + if (enable) >> + error (_("specify which frame unwinder(s) should be enabled")); >> + else >> + error (_("specify which frame unwinder(s) should be disabled")); >> + } > Should the error messages be capitalized? > > Not a big deal, but I would put the reinit_frame_cache a bit later, when > the unwinder list actually gets modified. If we error out before, we > technically don't need to reinit the frame cache. Fixed both. > >> + struct gdbarch* gdbarch = current_inferior ()->arch (); >> + std::vector unwinder_list >> + = gdbarch_unwinder_list (gdbarch); >> + >> + /* First see if the user wants to change all unwinders. */ >> + if (check_for_argument (&args, "-all")) >> + { >> + for (const frame_unwind *u : unwinder_list) >> + u->set_enabled (enable); >> + return; >> + } > Did you consider using the gdb::option framework for this (found in > cli/cli-option.h)? It's been a while since I touched it, I don't recall > if it would be a good fit for this, but it would be worth checking. I hadn't looked into using this framework when I first implemented it. However, looking at it now, it looks like overkill for what I need. The gdb::option path need the option array (with all the stuff that goes along with it), to end up with a very similar setup in the function itself. Now, if you think I'm overselling the complexity or that we are likely to add more options to this command, I can add it for the next version. > >> @@ -425,4 +547,37 @@ _initialize_frame_unwind () >> _("List the frame unwinders currently in effect, " >> "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.\n\ > Should the help maybe list the valid classes? Makes sense, fixed! > > Simon > -- Cheers, Guinevere Larsen She/Her/Hers