Re: [PATCHv2 4/5] gdb/gdbarch: remove the 'invalid=None' state from gdbarch_components.py

[Simon Marchi <simark@simark.ca>]

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