From patchwork Fri Mar 10 18:43:13 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 66247 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 53E2C385B532 for ; Fri, 10 Mar 2023 18:44:24 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 53E2C385B532 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1678473864; bh=KNRg2BFonZ7vTDejg9ZHl1oMx9xrBTKS3l2rqIObapM=; h=To:Cc:Subject:Date:In-Reply-To:References:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=fW8Zy/puHD5RRnSFZm9cnyvwr/Lx/K4x9Y5VKK6/18hyBDCi+1Of6ZIMmmj9mEnW5 exs6Z7XY7SOqd2ozhdms84cFwpkn6M3a1O3zdNGuwyCqtsiNRiJssZxKJZwT99EGE9 toMttHkuWct/sCtVVkpLGIw5NMP53F18t3yKENFs= X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 5425E3858D3C for ; Fri, 10 Mar 2023 18:43:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5425E3858D3C Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-588-JqTbEAhOOcWXysUl9hVROw-1; Fri, 10 Mar 2023 13:43:29 -0500 X-MC-Unique: JqTbEAhOOcWXysUl9hVROw-1 Received: by mail-wm1-f69.google.com with SMTP id s18-20020a7bc392000000b003deaf780ab6so2326289wmj.4 for ; Fri, 10 Mar 2023 10:43:28 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678473807; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=KNRg2BFonZ7vTDejg9ZHl1oMx9xrBTKS3l2rqIObapM=; b=CnjSGb2ki3mnmSiHY95TAOxGs6Btfx4WBJl5whOOV9zyVWHu43AInaIMkTjGdqpARL tf3BtpS4uVS5H7Lzva6vVKns/2jbA9CcLeohEG3yJSdwC8GRjpuI2kfnp9tpvTC8fuH4 AU3UI1edyyAOXO5ZFBP6Ag0a2HmiFLkDiQHnechPQoJqCzCAZfuR0arzPYsp7NKZL/yj 7aetTHBoOPQQcMf2EGtxc9FVszGQs1iYICBUAoGFyM7WgwsCbeLdwRhlQZzkDl3VgUdH 3Zp4pwcq4BjJhLoRa/CTudJ2jpPPxAeo91wfq/UeKSJBmvHoRAT1ociwGlxExumbLwxc kBGw== X-Gm-Message-State: AO0yUKUOLVcsFaDKHCYpoaiAMUjdmewt/SjaV8wSdFZ0S+z2/OPWxOyE IuADh5nLfHcCJH/NfWLK53HNmmqi/JN9pGcizYn4UYwaju5eTwu+Jf060mjTGJuGXtps7ci0KU+ uK/ufuXA7mohrQImoVk74/svrys5bFPGKQyS2H1Vlka5oEyHT0gcpqV7jKy2m2tB4HR/kgsfIYw eelje2Xw== X-Received: by 2002:a05:600c:1c96:b0:3eb:399d:ab24 with SMTP id k22-20020a05600c1c9600b003eb399dab24mr3595817wms.28.1678473807492; Fri, 10 Mar 2023 10:43:27 -0800 (PST) X-Google-Smtp-Source: AK7set+IfTQLurObj9QMuOo751iwowVSKUWgMEIUvHgXoS5sxtgTiMuC3AwRJHB3VaUo9g3xuGDL0w== X-Received: by 2002:a05:600c:1c96:b0:3eb:399d:ab24 with SMTP id k22-20020a05600c1c9600b003eb399dab24mr3595798wms.28.1678473807101; Fri, 10 Mar 2023 10:43:27 -0800 (PST) Received: from localhost (95.72.115.87.dyn.plus.net. [87.115.72.95]) by smtp.gmail.com with ESMTPSA id q4-20020a1ce904000000b003dd1bd0b915sm619878wmc.22.2023.03.10.10.43.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 10 Mar 2023 10:43:26 -0800 (PST) To: gdb-patches@sourceware.org Cc: Andrew Burgess Subject: [PATCHv3 4/9] gdb/gdbarch: remove the 'invalid=None' state from gdbarch_components.py Date: Fri, 10 Mar 2023 18:43:13 +0000 Message-Id: X-Mailer: git-send-email 2.25.4 In-Reply-To: References: MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, 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 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Andrew Burgess via Gdb-patches From: Andrew Burgess Reply-To: Andrew Burgess Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" 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. The change to using False as the default is temporary. Later in this series I'm going to change the default to True, but we need more fixes before that can be done. 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. This comment is inline with plenty of other getters that also have a similar comment. Plenty of the getters do have validation, so I think it is reasonable to have a comment noting that the validation has been skipped for a specific reason, rather than due to some bug. 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. And in gdbarch.py I've reworked the logic for generating the verify_gdbarch function, and for generating the getter functions. The logic for generating the getter functions is still not ideal, I ended up having to add this additional logic block: elif c.postdefault is not None and c.predefault is not None: print(" /* Check variable changed from pre-default. */", file=f) print(f" gdb_assert (gdbarch->{c.name} != {c.predefault});", file=f) which was needed to ensure we continued to generate the same code as before, without this the fact that invalid is now False when it would previously have been None, meant that we dropped the gdb_assert in favour of a comment like: print(f" /* Skip verify of {c.name}, invalid_p == 0 */", file=f) which is clearly not a good change. We could potentially look at improving this in a later commit, but I don't plan to do that in this series. --- gdb/gdbarch.c | 16 ++++++++++++++++ gdb/gdbarch.py | 36 +++++++++++++++--------------------- gdb/gdbarch_components.py | 2 ++ gdb/gdbarch_types.py | 7 +++---- 4 files changed, 36 insertions(+), 25 deletions(-) diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c index 7e4e34d5aca..efd111eeabc 100644 --- a/gdb/gdbarch.c +++ b/gdb/gdbarch.c @@ -298,29 +298,38 @@ verify_gdbarch (struct gdbarch *gdbarch) /* Skip verify of bfloat16_bit, invalid_p == 0 */ if (gdbarch->bfloat16_format == 0) gdbarch->bfloat16_format = floatformats_bfloat16; + /* Skip verify of bfloat16_format, invalid_p == 0 */ /* Skip verify of half_bit, invalid_p == 0 */ if (gdbarch->half_format == 0) gdbarch->half_format = floatformats_ieee_half; + /* Skip verify of half_format, invalid_p == 0 */ /* Skip verify of float_bit, invalid_p == 0 */ if (gdbarch->float_format == 0) gdbarch->float_format = floatformats_ieee_single; + /* Skip verify of float_format, invalid_p == 0 */ /* Skip verify of double_bit, invalid_p == 0 */ if (gdbarch->double_format == 0) gdbarch->double_format = floatformats_ieee_double; + /* Skip verify of double_format, invalid_p == 0 */ /* Skip verify of long_double_bit, invalid_p == 0 */ if (gdbarch->long_double_format == 0) gdbarch->long_double_format = floatformats_ieee_double; + /* Skip verify of long_double_format, invalid_p == 0 */ /* Skip verify of wchar_bit, invalid_p == 0 */ if (gdbarch->wchar_signed == -1) gdbarch->wchar_signed = 1; + /* Skip verify of wchar_signed, invalid_p == 0 */ /* Skip verify of floatformat_for_type, invalid_p == 0 */ /* Skip verify of ptr_bit, invalid_p == 0 */ if (gdbarch->addr_bit == 0) gdbarch->addr_bit = gdbarch_ptr_bit (gdbarch); + /* Skip verify of addr_bit, invalid_p == 0 */ if (gdbarch->dwarf2_addr_size == 0) gdbarch->dwarf2_addr_size = gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT; + /* Skip verify of dwarf2_addr_size, invalid_p == 0 */ if (gdbarch->char_signed == -1) gdbarch->char_signed = 1; + /* Skip verify of char_signed, invalid_p == 0 */ /* Skip verify of read_pc, has predicate. */ /* Skip verify of write_pc, has predicate. */ /* Skip verify of virtual_frame_pointer, invalid_p == 0 */ @@ -412,6 +421,7 @@ verify_gdbarch (struct gdbarch *gdbarch) /* Skip verify of skip_trampoline_code, invalid_p == 0 */ if (gdbarch->so_ops == 0) gdbarch->so_ops = &solib_target_so_ops; + /* Skip verify of so_ops, invalid_p == 0 */ /* Skip verify of skip_solib_resolver, invalid_p == 0 */ /* Skip verify of in_solib_return_trampoline, invalid_p == 0 */ /* Skip verify of in_indirect_branch_thunk, invalid_p == 0 */ @@ -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 */ if (gdbarch_debug >= 2) gdb_printf (gdb_stdlog, "gdbarch_bfloat16_format called\n"); return gdbarch->bfloat16_format; @@ -1523,6 +1534,7 @@ const struct floatformat ** gdbarch_half_format (struct gdbarch *gdbarch) { gdb_assert (gdbarch != NULL); + /* Skip verify of half_format, invalid_p == 0 */ if (gdbarch_debug >= 2) gdb_printf (gdb_stdlog, "gdbarch_half_format called\n"); return gdbarch->half_format; @@ -1556,6 +1568,7 @@ const struct floatformat ** gdbarch_float_format (struct gdbarch *gdbarch) { gdb_assert (gdbarch != NULL); + /* Skip verify of float_format, invalid_p == 0 */ if (gdbarch_debug >= 2) gdb_printf (gdb_stdlog, "gdbarch_float_format called\n"); return gdbarch->float_format; @@ -1589,6 +1602,7 @@ const struct floatformat ** gdbarch_double_format (struct gdbarch *gdbarch) { gdb_assert (gdbarch != NULL); + /* Skip verify of double_format, invalid_p == 0 */ if (gdbarch_debug >= 2) gdb_printf (gdb_stdlog, "gdbarch_double_format called\n"); return gdbarch->double_format; @@ -1622,6 +1636,7 @@ const struct floatformat ** gdbarch_long_double_format (struct gdbarch *gdbarch) { gdb_assert (gdbarch != NULL); + /* Skip verify of long_double_format, invalid_p == 0 */ if (gdbarch_debug >= 2) gdb_printf (gdb_stdlog, "gdbarch_long_double_format called\n"); return gdbarch->long_double_format; @@ -3304,6 +3319,7 @@ const struct target_so_ops * gdbarch_so_ops (struct gdbarch *gdbarch) { gdb_assert (gdbarch != NULL); + /* Skip verify of so_ops, invalid_p == 0 */ if (gdbarch_debug >= 2) gdb_printf (gdb_stdlog, "gdbarch_so_ops called\n"); return gdbarch->so_ops; diff --git a/gdb/gdbarch.py b/gdb/gdbarch.py index 5437c827f34..7ff2cabe2e8 100755 --- a/gdb/gdbarch.py +++ b/gdb/gdbarch.py @@ -212,17 +212,12 @@ with open("gdbarch.c", "w") as f: print(f" gdbarch->{c.name} = {c.postdefault};", file=f) # Now validate the value. - if c.invalid is False: - print(f" /* Skip verify of {c.name}, invalid_p == 0 */", file=f) - elif c.predicate: - print(f" /* Skip verify of {c.name}, has predicate. */", file=f) - elif c.invalid is None: - # No validation has been requested for this component. - pass - elif isinstance(c.invalid, str): + if isinstance(c.invalid, str): print(f" if ({c.invalid})", file=f) print(f""" log.puts ("\\n\\t{c.name}");""", file=f) - elif c.invalid is True: + elif c.predicate: + print(f" /* Skip verify of {c.name}, has predicate. */", file=f) + elif c.invalid: if c.postdefault is not None: # This component has its 'invalid' field set to True, but # also has a postdefault. This makes no sense, the @@ -236,14 +231,7 @@ with open("gdbarch.c", "w") as f: print(f" if (gdbarch->{c.name} == {init_value})", file=f) print(f""" log.puts ("\\n\\t{c.name}");""", file=f) else: - # We should not allow ourselves to simply do nothing here - # because no other case applies. If we end up here then - # either the input data needs adjusting so one of the - # above cases matches, or we need additional cases adding - # here. - raise Exception( - f"unhandled case when generating gdbarch validation: {c.name}" - ) + print(f" /* Skip verify of {c.name}, invalid_p == 0 */", file=f) print(" if (!log.empty ())", file=f) print( """ internal_error (_("verify_gdbarch: the following are invalid ...%s"),""", @@ -360,14 +348,20 @@ with open("gdbarch.c", "w") as f: print(f"gdbarch_{c.name} (struct gdbarch *gdbarch)", file=f) print("{", file=f) print(" gdb_assert (gdbarch != NULL);", file=f) - if c.invalid is False: - print(f" /* Skip verify of {c.name}, invalid_p == 0 */", file=f) - elif isinstance(c.invalid, str): + if isinstance(c.invalid, str): print(" /* Check variable is valid. */", file=f) print(f" gdb_assert (!({c.invalid}));", file=f) - elif c.predefault: + elif c.postdefault is not None and c.predefault is not None: print(" /* Check variable changed from pre-default. */", file=f) print(f" gdb_assert (gdbarch->{c.name} != {c.predefault});", file=f) + elif c.invalid: + if c.predefault: + print(" /* Check variable changed from pre-default. */", file=f) + print( + f" gdb_assert (gdbarch->{c.name} != {c.predefault});", file=f + ) + else: + print(f" /* Skip verify of {c.name}, invalid_p == 0 */", file=f) print(" if (gdbarch_debug >= 2)", file=f) print( f""" gdb_printf (gdb_stdlog, "gdbarch_{c.name} called\\n");""", diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py index 1f217123216..8576ccfaf2b 100644 --- a/gdb/gdbarch_components.py +++ b/gdb/gdbarch_components.py @@ -1675,6 +1675,7 @@ BFD target to use when generating a core file. predicate=True, predefault="0", printer="pstring (gdbarch->gcore_bfd_target)", + invalid=True, ) Value( @@ -1719,6 +1720,7 @@ The maximum length of an instruction on this architecture in bytes. name="max_insn_length", predicate=True, predefault="0", + invalid=True, ) Method( diff --git a/gdb/gdbarch_types.py b/gdb/gdbarch_types.py index 988da80dd7c..d40851d127f 100755 --- a/gdb/gdbarch_types.py +++ b/gdb/gdbarch_types.py @@ -46,7 +46,7 @@ class Component: predicate: bool = False, predefault: Optional[str] = None, postdefault: Optional[str] = None, - invalid: Optional[Union[bool, str]] = None, + invalid: Union[bool, str] = False, params: Optional[List[Tuple[str, str]]] = None, param_checks: Optional[List[str]] = None, result_checks: Optional[List[str]] = None, @@ -74,7 +74,6 @@ class Component: def get_predicate(self): "Return the expression used for validity checking." - assert self.predicate and not isinstance(self.invalid, str) if self.predefault: predicate = f"gdbarch->{self.name} != {self.predefault}" else: @@ -98,7 +97,7 @@ class Value(Component): predicate: bool = False, predefault: Optional[str] = None, postdefault: Optional[str] = None, - invalid: Optional[Union[bool, str]] = None, + invalid: Union[bool, str] = False, printer: Optional[str] = None, ): super().__init__( @@ -126,7 +125,7 @@ class Function(Component): predicate: bool = False, predefault: Optional[str] = None, postdefault: Optional[str] = None, - invalid: Optional[Union[bool, str]] = None, + invalid: Union[bool, str] = False, printer: Optional[str] = None, param_checks: Optional[List[str]] = None, result_checks: Optional[List[str]] = None,