From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id LjXAJJL7AWcS7QIAWB0awg (envelope-from ) for ; Sat, 05 Oct 2024 22:53:06 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1728183186; bh=zjeV2S+7VZogzF1W7DpCam+jYHk9mTW1AIDuHO5tNOw=; h=Date:Subject:To:Cc:References:From:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=A0SHAqR9B0Ey6NpIlzSqGRc4BQwFzlOe4FVxr7JfqtH5gBV5qIi293P6yLnBGHeKL +6+jv62OXY9kx9H37siLt+8L6fHBdRoIGfbAQ8zrNY9FXrypHEg1XVgkeXW5kRAZqV YjBlSYxEf/IKsJq7kIik6LyM0TK5O62M6WS/BJKU= Received: by simark.ca (Postfix, from userid 112) id 81EFA1E355; Sat, 5 Oct 2024 22:53:06 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.0 (2022-12-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-6.8 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, 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,URIBL_BLOCKED,URIBL_DBL_BLOCKED_OPENDNS autolearn=unavailable autolearn_force=no version=4.0.0 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=dAdTVQYI; dkim=pass (1024-bit key) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=QKjpMWOH; dkim-atps=neutral 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 3CF8D1E353 for ; Sat, 5 Oct 2024 22:53:05 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 9EE6A385ED4C for ; Sun, 6 Oct 2024 02:53:04 +0000 (GMT) Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 4BD943858D26 for ; Sun, 6 Oct 2024 02:51:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 4BD943858D26 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 4BD943858D26 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=158.69.221.121 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1728183088; cv=none; b=rDNSGhk54l/Vu4bYUu5Ku8uSRMhq+VACMaAnx+hMm8SiGWykwqI98rm00WS/aW1J1mQSbu/0gljbXcAwWoOoOeFDgW0O9wU/x97osRCpku8wz0pstEKv4rL0Q9ZKpPmkk76hL5ATf+n4SD5cIaFaAsE9E/kY7OU7+yg2ooCX2+s= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1728183088; c=relaxed/simple; bh=zjeV2S+7VZogzF1W7DpCam+jYHk9mTW1AIDuHO5tNOw=; h=DKIM-Signature:DKIM-Signature:Message-ID:Date:MIME-Version: Subject:To:From; b=ZeCNda+kIIzWopX2q2gGkmTqAzryDRtvVG8vWVRzamrVGBajPZ0kwilloQF+A9g/PTNgFSa4cb42+a56ua/1m6CWU6L0s6l5Hoqn2PSWf2uOTAkXogPi9y7hpFJ3IC9MAf3I/U9E6hUZflpH+6oIrAx5pAHxcZITw6nKoJdrcko= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1728183083; bh=zjeV2S+7VZogzF1W7DpCam+jYHk9mTW1AIDuHO5tNOw=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=dAdTVQYIFzh11HETJJreWcHITpNwk5PRchaOLCarGA+g61AW66A9bsBQ65ACw/Ofb zc1ThgzIk+BkvQFru1j9fJ07itUy3H9NW04QFzHnQH9NTolriE5oxwQzdzut2sq7bI 6yZiaLtiWbrtqqgV2Lnfvzn2NeglRlO/ZRCltLgM= Received: by simark.ca (Postfix, from userid 112) id 2A61E1E356; Sat, 5 Oct 2024 22:51:23 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1728183081; bh=zjeV2S+7VZogzF1W7DpCam+jYHk9mTW1AIDuHO5tNOw=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=QKjpMWOHOrenn5YldgPvl0qUagazApR+Aj9o8I2LwMVXqrklvT8uIeMjKK17YX7tS g5W8IBrXz3bZSzRpQrwOESE7DaCOD0tCTjS4Bx39+oWhbpozVku9xA4Staw6z+PFIB Bxp45fHC+9Yu2DF3FZ20m+GXfx85NI+GmaKe1nBw= Received: from [10.0.0.11] (modemcable238.237-201-24.mc.videotron.ca [24.201.237.238]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 388991E353; Sat, 5 Oct 2024 22:51:21 -0400 (EDT) Message-ID: Date: Sat, 5 Oct 2024 22:51:20 -0400 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v5 4/5] gdb: introduce ability to disable frame unwinders To: Guinevere Larsen , gdb-patches@sourceware.org Cc: Guinevere Larsen References: <20241001184235.3710608-1-guinevere@redhat.com> <20241001184235.3710608-5-guinevere@redhat.com> Content-Language: en-US From: Simon Marchi In-Reply-To: <20241001184235.3710608-5-guinevere@redhat.com> Content-Type: text/plain; charset=UTF-8 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 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. > 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. OTOH, I don't think accepting user inputs FRAME_UNWIND_ is really necessary. > + 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. > + 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. > @@ -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. > + 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. > @@ -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? Simon