From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 32BC03858D39 for ; Mon, 6 Mar 2023 20:13:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 32BC03858D39 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca Received: from [10.0.0.11] (unknown [217.28.27.60]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id B6E3D1E0D3; Mon, 6 Mar 2023 15:13:51 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1678133631; bh=PuBzMPT0uUbl0JFTqx6F27/dofnx6WayAcggNFwy4oY=; h=Date:Subject:To:References:From:In-Reply-To:From; b=rxbKDadLSCqscqERXbvTPm9RizpgpEYNAdRiRm9EE/L+ijJuHWTesRt5rfG5gAbfD w8lC3nR0VMNW/4CronXCh42cpi9ih8S0zweI+y68CpqT7wnA1XcEL9s6py4wulbinY CY9kCqxrPOc7tHL7bMF7ZaY/ytLSd/9ecJPPUMOo= Message-ID: <4a98e17c-7175-6127-74b2-ae41f87920d6@simark.ca> Date: Mon, 6 Mar 2023 15:13:51 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 Subject: Re: [PATCHv2 4/5] gdb/gdbarch: remove the 'invalid=None' state from gdbarch_components.py Content-Language: en-US To: Andrew Burgess , gdb-patches@sourceware.org References: <7452e65c89feea14efa71eb0272e00c2e68f6afd.1678116328.git.aburgess@redhat.com> From: Simon Marchi In-Reply-To: <7452e65c89feea14efa71eb0272e00c2e68f6afd.1678116328.git.aburgess@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-4.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 3/6/23 10:31, Andrew Burgess via Gdb-patches wrote: > This commit ensures that the 'invalid' property of all components is > either True, False, or a string. > > Additionally, this commit allows a component to have both a predicate > and for the 'invalid' property to be a string. > > Removing the option for 'invalid' to be None allows us to simplify the > algorithms in gdbarch.py a little. > > Allowing a component to have both a predicate and an 'invalid' string > means that we can validate the value that a tdep sets into a field, > but also allow a predicate to ensure that the field has changed from > the default. > > This functionality isn't going to be used in this series, but I have > tested it locally and believe that it would work, and this might make > it easier for others to add new components in the future. > > In gdbarch_types.py, I've updated the type annotations to show that > the 'invalid' field should not be None, and I've changed the default > for this field from None to False. > > Additionally, in gdbarch_types.py I've removed an assert from > Component.get_predicate. This assert ensured that we didn't have the > predicate field set to True and the invalid field set to a string. > However, no component currently uses this configuration, and after > this commit, that combination is now supported, so the assert can be > removed. > > As a consequence of the gdbarch_types.py changes we see some > additional comments generated in gdbarch.c about verification being > skipped due to the invalid field being False. > > In gdbarch_components.py I've had to add 'invalid=True' for two > components: gcore_bfd_target and max_insn_length, without this the > validation in the gdbarch getter would disappear. So, in my version of this, I gave `invalid` the default value True, whereas you gave it the default False. Upon further reflexion, I think False is a good default. It allows to set a predefault value and, if the arch does not override it, that value is the effective value. Seems like a sane and simple default behavior. This means you can remove all the `invalid=False,` from gdbarch_components.py now (one find and replace). I think you can tack it as a separate patch on top to avoid noise, since you have other changes in gdbarch_components.py already in this patch. > @@ -1490,6 +1500,7 @@ const struct floatformat ** > gdbarch_bfloat16_format (struct gdbarch *gdbarch) > { > gdb_assert (gdbarch != NULL); > + /* Skip verify of bfloat16_format, invalid_p == 0 */ I don't think the addition of these "Skip verify" comments in these functions (that invoke the gdbarch functions and methods) make sense. Simon