From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id x6/5Iwd8BWejWAYAWB0awg (envelope-from ) for ; Tue, 08 Oct 2024 14:37:59 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1728412679; bh=Ekv6090agzdrrJ8dwP3PuENWsMzRl7F6fz7VwFx8Hdg=; h=Date:Subject:To:Cc:References:From:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=mVnu8C1cEkH15e/MYfdPAMNeu7/U4Xxq+bAcybNedSoFZGwi3CkVH2it4IOXYWovh FIOuyC/CEAIoqMldaVotCFSaAn1Yp1qpIRZGuBZssM0KWCVVpnGm5qKaRXAc5XJHpW Fz81ZW19xyTmaMG5NukmeEiRAV+GkZ8YMBMb81Yo= Received: by simark.ca (Postfix, from userid 112) id 7F8D31E355; Tue, 8 Oct 2024 14:37:59 -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=ojadf8PE; dkim=pass (1024-bit key) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=sYAM35mA; 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 C27331E05C for ; Tue, 8 Oct 2024 14:37:58 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 53A01385DDDD for ; Tue, 8 Oct 2024 18:37:58 +0000 (GMT) Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id A481E385DDCA for ; Tue, 8 Oct 2024 18:37:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org A481E385DDCA 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 A481E385DDCA 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=1728412658; cv=none; b=aXxlvyHhju2Sh+7qX3ucUjnCUNYHgiCdr4X+DHVWnPfqJngDlvhOH7uN+9PxA20nNbSTPxrUjeNTQ/EykkLt9yfIu8fUyUX2Oki7LuEhhsP6zAGHhgD0MFy1jXR6T1GimKnDNZdNEDZuRLE4+vFvj4Pl3IpSJgv/amf6lMGr+UI= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1728412658; c=relaxed/simple; bh=Ekv6090agzdrrJ8dwP3PuENWsMzRl7F6fz7VwFx8Hdg=; h=DKIM-Signature:DKIM-Signature:Message-ID:Date:MIME-Version: Subject:To:From; b=H4Ig0T3qLBcX4/qDxCv7sp3hQ5kf2K5IZNhuAOvh1rpU+vJCnv5rxA1H4UXoqrNyloLOxculGVndsx6/hX64Vr/FbashB2DS4Gg8z6sy3Haa4K56RTBuiB6u6lwye/Y/uc8Tq3g4iPVPnJGR4LJezU5/z9i0/5+yTwTzawVP4k0= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1728412655; bh=Ekv6090agzdrrJ8dwP3PuENWsMzRl7F6fz7VwFx8Hdg=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=ojadf8PE2CcjvQzFUShbuTqKDUUi3b3Hj4z+lY0aWo8eFftUo/ZDb2i38jTSWXCUi XzyhEnIvpzLjV2iJ6ItoOvy2lciJHGqXfu/Peg2sqq02pJ2opv94s79NZDGpU8ViIR /hxFdyVaMwttBsNYgvCL/MJGBeMFbc5e5LPRiiwY= Received: by simark.ca (Postfix, from userid 112) id 0E0F61E357; Tue, 8 Oct 2024 14:37:35 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1728412652; bh=Ekv6090agzdrrJ8dwP3PuENWsMzRl7F6fz7VwFx8Hdg=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=sYAM35mAZ4tH3Kyzw2JtUfCeVr7xyJyAkau3xXoPi6RlT/wB4qJimwFW6PMKYQF2K dVQGZd7ZpBd24M2MTw4K4/8JeqSPB0St4CTDlx9R8zdyP3auKC3NfidLO+b3jMwTlA dcZZIaw5G6aOOPsaaTKOJ5/CWfTKrL06npyd/k18= 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 C13B01E05C; Tue, 8 Oct 2024 14:37:32 -0400 (EDT) Message-ID: Date: Tue, 8 Oct 2024 14:37:32 -0400 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v5 2/5] gdb: add "unwinder class" to frame unwinders To: Guinevere Larsen , gdb-patches@sourceware.org Cc: Eli Zaretskii References: <20241001184235.3710608-1-guinevere@redhat.com> <20241001184235.3710608-3-guinevere@redhat.com> Content-Language: en-US From: Simon Marchi In-Reply-To: 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-08 14:22, Guinevere Larsen wrote: > On 10/3/24 3:46 PM, Simon Marchi wrote: >> On 10/1/24 2:42 PM, Guinevere Larsen wrote: >>> From: Guinevere Larsen >>> >>> A future patch will add a way to disable certain unwinders based on >>> different characteristics. This patch aims to make it more convenient >>> to disable related unwinders in bulk, such as architecture specific >>> ones, by indentifying all unwinders by which part of the code adds it. >> indentifying -> identifying >> >>> diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c >>> index b69ae8596a2..afc1258c6a9 100644 >>> --- a/gdb/frame-unwind.c >>> +++ b/gdb/frame-unwind.c >>> @@ -30,6 +30,17 @@ >>> #include "dwarf2/frame-tailcall.h" >>> #include "cli/cli-cmds.h" >>> #include "inferior.h" >>> +#include >>> + >>> +/* Conversion list between the enum for frame_unwind_class and >>> + string. */ >>> +static std::map unwind_class_conversion = >>> +{ >>> + {FRAME_UNWIND_GDB, "GDB"}, >>> + {FRAME_UNWIND_ARCH, "ARCH"}, >>> + {FRAME_UNWIND_EXTENSION, "EXTENSION"}, >>> + {FRAME_UNWIND_DEBUGINFO, "DEBUGINFO"}, >>> +}; >> It's not reaaally a big deal, but using an std::map for this is kind of >> heavyweight. It could probably be a simple array, still indexed by the >> enum value. Personally, I used functions with switches to handle cases >> this (see target_waitkind_str), because compilers can warn you if you >> forget to handle an enumerator (which can easily happen when a new one >> is added). > > Since I have 2 functions, one to convert from enum to string, and another from string to enum, I'll go with a simple array that makes it easy to do both. > > This is my proposed change: > > diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c > index ef05b658f94..10d9f2e9f88 100644 > --- a/gdb/frame-unwind.c > +++ b/gdb/frame-unwind.c > @@ -34,12 +34,12 @@ > > /* Conversion list between the enum for frame_unwind_class and > string. */ > -static std::map unwind_class_conversion = > +static const char * unwind_class_conversion[] = I think we don't want a space after the `*`. > { > - {FRAME_UNWIND_GDB, "GDB"}, > - {FRAME_UNWIND_ARCH, "ARCH"}, > - {FRAME_UNWIND_EXTENSION, "EXTENSION"}, > - {FRAME_UNWIND_DEBUGINFO, "DEBUGINFO"}, > + "GDB", > + "ARCH", > + "EXTENSION", > + "DEBUGINFO", > }; > > /* Default sniffers, that must always be the first in the unwinder list, > @@ -88,9 +88,8 @@ get_frame_unwind_table (struct gdbarch *gdbarch) > static const char * > frame_unwinder_class_str (frame_unwind_class uclass) > { > - auto location = unwind_class_conversion.find (uclass); > - gdb_assert (location != unwind_class_conversion.end ()); > - return location->second; > + gdb_assert (uclass < UNWIND_CLASS_NUMBER); > + return unwind_class_conversion[uclass]; > } > > /* Case insensitive search for a frame_unwind_class based on the given > @@ -102,10 +101,10 @@ str_to_frame_unwind_class (const char *class_str) > /* Skip the prefix if present. */ > if (strncasecmp (class_str, prefix, strlen(prefix)) == 0) > class_str += strlen (prefix); > - for (const auto &it : unwind_class_conversion) > + for (int i = 0; i < UNWIND_CLASS_NUMBER; i++) > { > - if (strcasecmp (it.second, class_str) == 0) > - return it.first; > + if (strcasecmp (unwind_class_conversion[i], class_str) == 0) > + return (frame_unwind_class) i; I'd suggest using static_cast instead of a C-style cast, since we use C++. Otherwise this diff LGTM. > } > error (_("Unknown frame unwind class: %s"), class_str); > } > diff --git a/gdb/frame-unwind.h b/gdb/frame-unwind.h > index f5ea8d9f690..555a5d98d5b 100644 > --- a/gdb/frame-unwind.h > +++ b/gdb/frame-unwind.h > @@ -167,6 +167,8 @@ enum frame_unwind_class > FRAME_UNWIND_DEBUGINFO, > /* The unwinder was created and handles target dependent things. */ > FRAME_UNWIND_ARCH, > + /* Meta enum value, to ensure we're always sent a valid unwinder class. */ > + UNWIND_CLASS_NUMBER, > }; I thought I made the suggestion to use "enum class" for new stuff, but now I can't find it. >> Approved-By: Simon Marchi > Can I add the tag with that change? (just making sure you're ok with my solution (: ) Yes, I'm fine with you addressing these comments without a further review (although I guess it would make sense to wait until the whole series is approved before pushing, in case this needs to change): Approved-By: Simon Marchi Simon