[PATCH 0/2] Add new gdbarch::displaced_step_max_buffer_length field

[PATCH 0/2] Add new gdbarch::displaced_step_max_buffer_length field

[Andrew Burgess]

The first patch in this series adjusts the gdbarch.py algorithm
slightly to allow for slightly more flexibility when validating
gdbarch fields.

I then make use of this in the second patch to add a new gdbarch
field.

---

Andrew Burgess (2):
  gdb: updates to gdbarch.py algorithm
  gdb: add gdbarch::displaced_step_max_buffer_length

 gdb/aarch64-linux-tdep.c  |  4 ++-
 gdb/arm-tdep.c            |  4 ++-
 gdb/displaced-stepping.c  |  6 ++--
 gdb/gdbarch-gen.h         | 17 +++++++--
 gdb/gdbarch.c             | 26 ++++++++++++++
 gdb/gdbarch.py            | 31 +++++++++++------
 gdb/gdbarch_components.py | 72 ++++++++++++++++++++-------------------
 gdb/linux-tdep.c          |  2 +-
 gdb/rs6000-tdep.c         |  6 ++--
 9 files changed, 111 insertions(+), 57 deletions(-)


base-commit: 2968b79fca38cf18e8eef360c36de7a6e3846d3c
-- 
2.25.4

[PATCH 1/2] gdb: updates to gdbarch.py algorithm

[Andrew Burgess]

Restructure how gdbarch.py generates the verify_gdbarch function.
Previously the postdefault handling was bundled together with the
validation.  This means that a field can't have both a postdefault,
and set its invalid attribute to a string.

This doesn't seem reasonable to me, I see no reason why a field can't
have both a postdefault (used when the tdep doesn't set the field),
and an invalid expression, which can be used to validate the value
that a tdep might set.

In this commit I restructure the verify_gdbarch generation code to
allow the above, there is no change in the actual generated code in
this commit, that will come in later commit.

I did end up having to remove the "invalid" attribute (where the
attribute was set to True) from a number of fields in this commit.
This invalid attribute was never having an effect as these components
all have a postdefault.  Consider; the "postdefault" is applied if the
field still has its initial value, while an "invalid" attribute set to
True means error if the field still has its default value.  But the
field never will have its default value, it will always have its
postdefault value.
---
 gdb/gdbarch.py            | 31 ++++++++++++++++---------
 gdb/gdbarch_components.py | 49 ++++++++++++++-------------------------
 2 files changed, 37 insertions(+), 43 deletions(-)

diff --git a/gdb/gdbarch.py b/gdb/gdbarch.py
index 93b1e8bf84e..7fea41c9572 100755
--- a/gdb/gdbarch.py
+++ b/gdb/gdbarch.py
@@ -203,35 +203,44 @@ with open("gdbarch.c", "w") as f:
         file=f,
     )
     for c in filter(not_info, components):
-        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 isinstance(c.invalid, str) and c.postdefault is not None:
-            print(f"  if ({c.invalid})", file=f)
-            print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
-        elif c.predefault is not None and c.postdefault is not None:
+        # An opportunity to write in the 'postdefault' value.
+        if c.postdefault is not None and c.predefault is not None:
             print(f"  if (gdbarch->{c.name} == {c.predefault})", file=f)
             print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
         elif c.postdefault is not None:
             print(f"  if (gdbarch->{c.name} == 0)", file=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):
             print(f"  if ({c.invalid})", file=f)
             print(f"""    log.puts ("\\n\\t{c.name}");""", file=f)
-        elif c.predefault is not None:
+        elif c.invalid is True and c.predefault is not None:
             print(f"  if (gdbarch->{c.name} == {c.predefault})", file=f)
             print(f"""    log.puts ("\\n\\t{c.name}");""", file=f)
-        elif c.invalid is True:
+        elif c.invalid is True and c.postdefault is None:
             print(f"  if (gdbarch->{c.name} == 0)", file=f)
             print(f"""    log.puts ("\\n\\t{c.name}");""", file=f)
+        elif c.invalid is True:
+            # This component has its 'invalid' field set to True, but
+            # also has a postdefault.  This makes no sense, the
+            # postdefault will have been applied above, so this field
+            # will not have a zero value.
+            raise Exception(f"component {c.name} has postdefault and invalid set to True")
         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("unhandled case when generating gdbarch validation")
+            raise Exception(f"unhandled case when generating gdbarch validation: {c.name}")
     print("  if (!log.empty ())", file=f)
     print(
         """    internal_error (_("verify_gdbarch: the following are invalid ...%s"),""",
diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
index caa65c334ec..1d420a513f9 100644
--- a/gdb/gdbarch_components.py
+++ b/gdb/gdbarch_components.py
@@ -63,34 +63,28 @@
 # * "predefault", "postdefault", and "invalid" - These are used for
 # the initialization and verification steps:
 #
-# A gdbarch is zero-initialized.  Then, if a field has a pre-default,
-# the field is set to that value.  After initialization is complete
-# (that is, after the tdep code has a chance to change the settings),
-# the post-initialization step is done.
+# A gdbarch is zero-initialized.  Then, if a field has a "predefault",
+# the field is set to that value.  This becomes the fields initial
+# value.
 #
-# There is a generic algorithm to generate a "validation function" for
-# all fields.  If the field has an "invalid" attribute with a string
-# value, then this string is the expression (note that a string-valued
-# "invalid" and "predicate" are mutually exclusive; and the case where
-# invalid is True means to ignore this field and instead use the
-# default checking that is about to be described).  Otherwise, if
-# there is a "predefault", then the field is valid if it differs from
-# the predefault.  Otherwise, the check is done against 0 (really NULL
-# for function pointers, but same idea).
-#
-# In post-initialization / validation, there are several cases.
+# After initialization is complete (that is, after the tdep code has a
+# chance to change the settings), the post-initialization step is
+# done.
 #
-# * If "invalid" is False, or if the field specifies "predicate",
-# validation is skipped.  Otherwise, a validation step is emitted.
+# If the field still has its initial value (see above), and the field
+# has a "postdefault", then the post field is set to this value.
 #
-# * Otherwise, the validity is checked using the usual validation
-# function (see above).  If the field is considered valid, nothing is
-# done.
+# After the possible "postdefault" assignment, validation is
+# performed for fields that don't have a "predicate".
 #
-# * Otherwise, the field's value is invalid.  If there is a
-# "postdefault", then the field is assigned that value.
+# If the field has an "invalid" attribute with a string value, then
+# this string is the expression that should evaluate to true when the
+# field is invalid.
 #
-# * Otherwise, the gdbarch will fail validation and gdb will crash.
+# Otherwise, if "invalid" is True, then the generic validation
+# function is used: the field is considered invalid it it still
+# contains its default value.  This validation is what is used within
+# the _p predicate function if the field has "predicate" set to True.
 #
 # Function and Method share:
 #
@@ -206,7 +200,6 @@ Value(
     type="const struct floatformat **",
     name="bfloat16_format",
     postdefault="floatformats_bfloat16",
-    invalid=True,
     printer="pformat (gdbarch, gdbarch->bfloat16_format)",
 )
 
@@ -221,7 +214,6 @@ Value(
     type="const struct floatformat **",
     name="half_format",
     postdefault="floatformats_ieee_half",
-    invalid=True,
     printer="pformat (gdbarch, gdbarch->half_format)",
 )
 
@@ -236,7 +228,6 @@ Value(
     type="const struct floatformat **",
     name="float_format",
     postdefault="floatformats_ieee_single",
-    invalid=True,
     printer="pformat (gdbarch, gdbarch->float_format)",
 )
 
@@ -251,7 +242,6 @@ Value(
     type="const struct floatformat **",
     name="double_format",
     postdefault="floatformats_ieee_double",
-    invalid=True,
     printer="pformat (gdbarch, gdbarch->double_format)",
 )
 
@@ -266,7 +256,6 @@ Value(
     type="const struct floatformat **",
     name="long_double_format",
     postdefault="floatformats_ieee_double",
-    invalid=True,
     printer="pformat (gdbarch, gdbarch->long_double_format)",
 )
 
@@ -289,7 +278,6 @@ One if `wchar_t' is signed, zero if unsigned.
     name="wchar_signed",
     predefault="-1",
     postdefault="1",
-    invalid=True,
 )
 
 Method(
@@ -332,7 +320,6 @@ addr_bit is the size of a target address as represented in gdb
     name="addr_bit",
     predefault="0",
     postdefault="gdbarch_ptr_bit (gdbarch)",
-    invalid=True,
 )
 
 Value(
@@ -355,7 +342,6 @@ and if Dwarf versions < 4 need to be supported.
     name="dwarf2_addr_size",
     predefault="0",
     postdefault="gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT",
-    invalid=True,
 )
 
 Value(
@@ -366,7 +352,6 @@ One if `char' acts like `signed char', zero if `unsigned char'.
     name="char_signed",
     predefault="-1",
     postdefault="1",
-    invalid=True,
 )
 
 Function(
-- 
2.25.4

Re: [PATCH 1/2] gdb: updates to gdbarch.py algorithm

[Simon Marchi]

On 2/28/23 11:51, Andrew Burgess via Gdb-patches wrote:
> Restructure how gdbarch.py generates the verify_gdbarch function.
> Previously the postdefault handling was bundled together with the
> validation.  This means that a field can't have both a postdefault,
> and set its invalid attribute to a string.
> 
> This doesn't seem reasonable to me, I see no reason why a field can't
> have both a postdefault (used when the tdep doesn't set the field),
> and an invalid expression, which can be used to validate the value
> that a tdep might set.
> 
> In this commit I restructure the verify_gdbarch generation code to
> allow the above, there is no change in the actual generated code in
> this commit, that will come in later commit.
> 
> I did end up having to remove the "invalid" attribute (where the
> attribute was set to True) from a number of fields in this commit.
> This invalid attribute was never having an effect as these components
> all have a postdefault.  Consider; the "postdefault" is applied if the
> field still has its initial value, while an "invalid" attribute set to
> True means error if the field still has its default value.  But the
> field never will have its default value, it will always have its
> postdefault value.
> ---
>  gdb/gdbarch.py            | 31 ++++++++++++++++---------
>  gdb/gdbarch_components.py | 49 ++++++++++++++-------------------------
>  2 files changed, 37 insertions(+), 43 deletions(-)
> 
> diff --git a/gdb/gdbarch.py b/gdb/gdbarch.py
> index 93b1e8bf84e..7fea41c9572 100755
> --- a/gdb/gdbarch.py
> +++ b/gdb/gdbarch.py
> @@ -203,35 +203,44 @@ with open("gdbarch.c", "w") as f:
>          file=f,
>      )
>      for c in filter(not_info, components):
> -        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 isinstance(c.invalid, str) and c.postdefault is not None:
> -            print(f"  if ({c.invalid})", file=f)
> -            print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
> -        elif c.predefault is not None and c.postdefault is not None:
> +        # An opportunity to write in the 'postdefault' value.
> +        if c.postdefault is not None and c.predefault is not None:
>              print(f"  if (gdbarch->{c.name} == {c.predefault})", file=f)
>              print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
>          elif c.postdefault is not None:
>              print(f"  if (gdbarch->{c.name} == 0)", file=f)
>              print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)

I would find this postdefault snippet easier to read like this, with a
single "if c.postdefault is not None", and then another condition inside
to decide what we should compare against:

        if c.postdefault is not None:
            if c.predefault is not None:
                print(f"  if (gdbarch->{c.name} == {c.predefault})", file=f)
                print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
            else:
                print(f"  if (gdbarch->{c.name} == 0)", file=f)
                print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)

or even

        if c.postdefault is not None:
            predefault = c.predefault or "0"
            print(f"  if (gdbarch->{c.name} == {predefault})", file=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:

I think it's confusing for the "invalid" parameter to be able to be
None, that it's one to many state versus what we need to be able to
represent.  I think we can get by with string, True and False, where
True means "auto", where the validity check is generated if it makes
sense to.  Having one less state would help simplify things.  I hacked
this locally and it seems to work.  I can post this as a cleanup before
or on top of your patch, as you prefer.

Another cleanup that would help me understand what is going on would be
to change this long list of if/elif to something that looks more like a
decision tree.  On top of your patch, and on top of my suggestion to get
rid of the invalid=None state, this is what I made looks like:

        predefault = c.predefault or "0"

        # Now validate the value.
        if type(c.invalid) is str:
            print(f"  if ({c.invalid})", file=f)
            print(f"""    log.puts ("\\n\\t{c.name}");""", file=f)
        elif c.invalid:
            if c.predicate:
                print(f"  /* Skip verify of {c.name}, has predicate.  */", file=f)
            elif c.postdefault:
                # We currently don't print anything, but we could print:
                # print(f"  /* Skip verify of {c.name}, has predicate.  */", file=f)
                pass
            else:
                print(f"  if (gdbarch->{c.name} == {predefault})", file=f)
                print(f"""    log.puts ("\\n\\t{c.name}");""", file=f)
        else:
            print(f"  /* Skip verify of {c.name}, invalid_p == 0 */", file=f)

That structure is clearer to me.  We see clearly the portions handling
the three states of "invalid" (str, True and False).  Inside invalid ==
True (which really means "auto"), we see that we skip generating the
check when either predicate or postdefault is set, the two situations
where generating the check doesn't make sense.

Another nice thing about this is that there isn't the "unhandled case
when generating gdbarch validation" case at the end.  Each branch of the
decision tree has an outcome.

Again, if you agree with this cleanup, we could do it before or after
your patch, as you wish.

> diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
> index caa65c334ec..1d420a513f9 100644
> --- a/gdb/gdbarch_components.py
> +++ b/gdb/gdbarch_components.py
> @@ -63,34 +63,28 @@
>  # * "predefault", "postdefault", and "invalid" - These are used for
>  # the initialization and verification steps:
>  #
> -# A gdbarch is zero-initialized.  Then, if a field has a pre-default,
> -# the field is set to that value.  After initialization is complete
> -# (that is, after the tdep code has a chance to change the settings),
> -# the post-initialization step is done.
> +# A gdbarch is zero-initialized.  Then, if a field has a "predefault",
> +# the field is set to that value.  This becomes the fields initial

Are you missing an apostrophe after fields?

> +# value.
>  #
> -# There is a generic algorithm to generate a "validation function" for
> -# all fields.  If the field has an "invalid" attribute with a string
> -# value, then this string is the expression (note that a string-valued
> -# "invalid" and "predicate" are mutually exclusive; and the case where
> -# invalid is True means to ignore this field and instead use the
> -# default checking that is about to be described).  Otherwise, if
> -# there is a "predefault", then the field is valid if it differs from
> -# the predefault.  Otherwise, the check is done against 0 (really NULL
> -# for function pointers, but same idea).
> -#
> -# In post-initialization / validation, there are several cases.
> +# After initialization is complete (that is, after the tdep code has a
> +# chance to change the settings), the post-initialization step is
> +# done.
>  #
> -# * If "invalid" is False, or if the field specifies "predicate",
> -# validation is skipped.  Otherwise, a validation step is emitted.
> +# If the field still has its initial value (see above), and the field
> +# has a "postdefault", then the post field is set to this value.

Do you really mean to say "the post field", and not just "the field"?

>  #
> -# * Otherwise, the validity is checked using the usual validation
> -# function (see above).  If the field is considered valid, nothing is
> -# done.
> +# After the possible "postdefault" assignment, validation is
> +# performed for fields that don't have a "predicate".
>  #
> -# * Otherwise, the field's value is invalid.  If there is a
> -# "postdefault", then the field is assigned that value.
> +# If the field has an "invalid" attribute with a string value, then
> +# this string is the expression that should evaluate to true when the
> +# field is invalid.
>  #
> -# * Otherwise, the gdbarch will fail validation and gdb will crash.
> +# Otherwise, if "invalid" is True, then the generic validation
> +# function is used: the field is considered invalid it it still

double "it"

Simon

Re: [PATCH 1/2] gdb: updates to gdbarch.py algorithm

[Andrew Burgess]

Simon Marchi <simark@simark.ca> writes:

> On 2/28/23 11:51, Andrew Burgess via Gdb-patches wrote:
>> Restructure how gdbarch.py generates the verify_gdbarch function.
>> Previously the postdefault handling was bundled together with the
>> validation.  This means that a field can't have both a postdefault,
>> and set its invalid attribute to a string.
>> 
>> This doesn't seem reasonable to me, I see no reason why a field can't
>> have both a postdefault (used when the tdep doesn't set the field),
>> and an invalid expression, which can be used to validate the value
>> that a tdep might set.
>> 
>> In this commit I restructure the verify_gdbarch generation code to
>> allow the above, there is no change in the actual generated code in
>> this commit, that will come in later commit.
>> 
>> I did end up having to remove the "invalid" attribute (where the
>> attribute was set to True) from a number of fields in this commit.
>> This invalid attribute was never having an effect as these components
>> all have a postdefault.  Consider; the "postdefault" is applied if the
>> field still has its initial value, while an "invalid" attribute set to
>> True means error if the field still has its default value.  But the
>> field never will have its default value, it will always have its
>> postdefault value.
>> ---
>>  gdb/gdbarch.py            | 31 ++++++++++++++++---------
>>  gdb/gdbarch_components.py | 49 ++++++++++++++-------------------------
>>  2 files changed, 37 insertions(+), 43 deletions(-)
>> 
>> diff --git a/gdb/gdbarch.py b/gdb/gdbarch.py
>> index 93b1e8bf84e..7fea41c9572 100755
>> --- a/gdb/gdbarch.py
>> +++ b/gdb/gdbarch.py
>> @@ -203,35 +203,44 @@ with open("gdbarch.c", "w") as f:
>>          file=f,
>>      )
>>      for c in filter(not_info, components):
>> -        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 isinstance(c.invalid, str) and c.postdefault is not None:
>> -            print(f"  if ({c.invalid})", file=f)
>> -            print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
>> -        elif c.predefault is not None and c.postdefault is not None:
>> +        # An opportunity to write in the 'postdefault' value.
>> +        if c.postdefault is not None and c.predefault is not None:
>>              print(f"  if (gdbarch->{c.name} == {c.predefault})", file=f)
>>              print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
>>          elif c.postdefault is not None:
>>              print(f"  if (gdbarch->{c.name} == 0)", file=f)
>>              print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
>
> I would find this postdefault snippet easier to read like this, with a
> single "if c.postdefault is not None", and then another condition inside
> to decide what we should compare against:
>
>         if c.postdefault is not None:
>             if c.predefault is not None:
>                 print(f"  if (gdbarch->{c.name} == {c.predefault})", file=f)
>                 print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
>             else:
>                 print(f"  if (gdbarch->{c.name} == 0)", file=f)
>                 print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
>
> or even
>
>         if c.postdefault is not None:
>             predefault = c.predefault or "0"
>             print(f"  if (gdbarch->{c.name} == {predefault})", file=f)
>             print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)

I went with the second approach, I like removing the duplicate print
calls.

>
>> +
>> +        # 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:
>
> I think it's confusing for the "invalid" parameter to be able to be
> None, that it's one to many state versus what we need to be able to
> represent.  I think we can get by with string, True and False, where
> True means "auto", where the validity check is generated if it makes
> sense to.  Having one less state would help simplify things.  I hacked
> this locally and it seems to work.  I can post this as a cleanup before
> or on top of your patch, as you prefer.
>
> Another cleanup that would help me understand what is going on would be
> to change this long list of if/elif to something that looks more like a
> decision tree.  On top of your patch, and on top of my suggestion to get
> rid of the invalid=None state, this is what I made looks like:
>
>         predefault = c.predefault or "0"
>
>         # Now validate the value.
>         if type(c.invalid) is str:
>             print(f"  if ({c.invalid})", file=f)
>             print(f"""    log.puts ("\\n\\t{c.name}");""", file=f)
>         elif c.invalid:
>             if c.predicate:
>                 print(f"  /* Skip verify of {c.name}, has predicate.  */", file=f)
>             elif c.postdefault:
>                 # We currently don't print anything, but we could print:
>                 # print(f"  /* Skip verify of {c.name}, has predicate.  */", file=f)
>                 pass
>             else:
>                 print(f"  if (gdbarch->{c.name} == {predefault})", file=f)
>                 print(f"""    log.puts ("\\n\\t{c.name}");""", file=f)
>         else:
>             print(f"  /* Skip verify of {c.name}, invalid_p == 0 */", file=f)
>
> That structure is clearer to me.  We see clearly the portions handling
> the three states of "invalid" (str, True and False).  Inside invalid ==
> True (which really means "auto"), we see that we skip generating the
> check when either predicate or postdefault is set, the two situations
> where generating the check doesn't make sense.
>
> Another nice thing about this is that there isn't the "unhandled case
> when generating gdbarch validation" case at the end.  Each branch of the
> decision tree has an outcome.
>
> Again, if you agree with this cleanup, we could do it before or after
> your patch, as you wish.

Yeah, I think what you have above would be a great improvement.  I like
that we can (with what you propose) now have an "invalid" string and
also have a predicate, that feels like an improvement.

I've included an update of my patch below.   I haven't included the
"invalid" refactor that you describe above as it sounds like you already
have a patch for this work, and it would end up as a separate patch
anyway.  I'm fine with it going in after my work of before, whatever
works best.

Let me know what you think.

Thanks,
Andrew

---

commit 25850b05dce62e0c83cd6040670b72c77e6ed7ea
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Thu Feb 23 18:18:45 2023 +0000

    gdb: updates to gdbarch.py algorithm
    
    Restructure how gdbarch.py generates the verify_gdbarch function.
    Previously the postdefault handling was bundled together with the
    validation.  This means that a field can't have both a postdefault,
    and set its invalid attribute to a string.
    
    This doesn't seem reasonable to me, I see no reason why a field can't
    have both a postdefault (used when the tdep doesn't set the field),
    and an invalid expression, which can be used to validate the value
    that a tdep might set.
    
    In this commit I restructure the verify_gdbarch generation code to
    allow the above, there is no change in the actual generated code in
    this commit, that will come in later commit.
    
    I did end up having to remove the "invalid" attribute (where the
    attribute was set to True) from a number of fields in this commit.
    This invalid attribute was never having an effect as these components
    all have a postdefault.  Consider; the "postdefault" is applied if the
    field still has its initial value, while an "invalid" attribute set to
    True means error if the field still has its default value.  But the
    field never will have its default value, it will always have its
    postdefault value.

diff --git a/gdb/gdbarch.py b/gdb/gdbarch.py
index 93b1e8bf84e..2447b249594 100755
--- a/gdb/gdbarch.py
+++ b/gdb/gdbarch.py
@@ -203,35 +203,44 @@ with open("gdbarch.c", "w") as f:
         file=f,
     )
     for c in filter(not_info, components):
+        # An opportunity to write in the 'postdefault' value.  We
+        # change field's value to the postdefault if its current value
+        # is not different to the initial value of the field.
+        if c.postdefault is not None:
+            init_value = c.predefault or "0"
+            print(f"  if (gdbarch->{c.name} == {init_value})", file=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 isinstance(c.invalid, str) and c.postdefault is not None:
-            print(f"  if ({c.invalid})", file=f)
-            print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
-        elif c.predefault is not None and c.postdefault is not None:
-            print(f"  if (gdbarch->{c.name} == {c.predefault})", file=f)
-            print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
-        elif c.postdefault is not None:
-            print(f"  if (gdbarch->{c.name} == 0)", file=f)
-            print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
+        elif c.invalid is None:
+            # No validation has been requested for this component.
+            pass
         elif isinstance(c.invalid, str):
             print(f"  if ({c.invalid})", file=f)
             print(f"""    log.puts ("\\n\\t{c.name}");""", file=f)
-        elif c.predefault is not None:
+        elif c.invalid is True and c.predefault is not None:
             print(f"  if (gdbarch->{c.name} == {c.predefault})", file=f)
             print(f"""    log.puts ("\\n\\t{c.name}");""", file=f)
-        elif c.invalid is True:
+        elif c.invalid is True and c.postdefault is None:
             print(f"  if (gdbarch->{c.name} == 0)", file=f)
             print(f"""    log.puts ("\\n\\t{c.name}");""", file=f)
+        elif c.invalid is True:
+            # This component has its 'invalid' field set to True, but
+            # also has a postdefault.  This makes no sense, the
+            # postdefault will have been applied above, so this field
+            # will not have a zero value.
+            raise Exception(f"component {c.name} has postdefault and invalid set to True")
         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("unhandled case when generating gdbarch validation")
+            raise Exception(f"unhandled case when generating gdbarch validation: {c.name}")
     print("  if (!log.empty ())", file=f)
     print(
         """    internal_error (_("verify_gdbarch: the following are invalid ...%s"),""",
diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
index caa65c334ec..5446b8fcaf8 100644
--- a/gdb/gdbarch_components.py
+++ b/gdb/gdbarch_components.py
@@ -63,34 +63,28 @@
 # * "predefault", "postdefault", and "invalid" - These are used for
 # the initialization and verification steps:
 #
-# A gdbarch is zero-initialized.  Then, if a field has a pre-default,
-# the field is set to that value.  After initialization is complete
-# (that is, after the tdep code has a chance to change the settings),
-# the post-initialization step is done.
+# A gdbarch is zero-initialized.  Then, if a field has a "predefault",
+# the field is set to that value.  This becomes the field's initial
+# value.
 #
-# There is a generic algorithm to generate a "validation function" for
-# all fields.  If the field has an "invalid" attribute with a string
-# value, then this string is the expression (note that a string-valued
-# "invalid" and "predicate" are mutually exclusive; and the case where
-# invalid is True means to ignore this field and instead use the
-# default checking that is about to be described).  Otherwise, if
-# there is a "predefault", then the field is valid if it differs from
-# the predefault.  Otherwise, the check is done against 0 (really NULL
-# for function pointers, but same idea).
-#
-# In post-initialization / validation, there are several cases.
+# After initialization is complete (that is, after the tdep code has a
+# chance to change the settings), the post-initialization step is
+# done.
 #
-# * If "invalid" is False, or if the field specifies "predicate",
-# validation is skipped.  Otherwise, a validation step is emitted.
+# If the field still has its initial value (see above), and the field
+# has a "postdefault", then the field is set to this value.
 #
-# * Otherwise, the validity is checked using the usual validation
-# function (see above).  If the field is considered valid, nothing is
-# done.
+# After the possible "postdefault" assignment, validation is
+# performed for fields that don't have a "predicate".
 #
-# * Otherwise, the field's value is invalid.  If there is a
-# "postdefault", then the field is assigned that value.
+# If the field has an "invalid" attribute with a string value, then
+# this string is the expression that should evaluate to true when the
+# field is invalid.
 #
-# * Otherwise, the gdbarch will fail validation and gdb will crash.
+# Otherwise, if "invalid" is True, then the generic validation
+# function is used: the field is considered invalid it still contains
+# its default value.  This validation is what is used within the _p
+# predicate function if the field has "predicate" set to True.
 #
 # Function and Method share:
 #
@@ -206,7 +200,6 @@ Value(
     type="const struct floatformat **",
     name="bfloat16_format",
     postdefault="floatformats_bfloat16",
-    invalid=True,
     printer="pformat (gdbarch, gdbarch->bfloat16_format)",
 )
 
@@ -221,7 +214,6 @@ Value(
     type="const struct floatformat **",
     name="half_format",
     postdefault="floatformats_ieee_half",
-    invalid=True,
     printer="pformat (gdbarch, gdbarch->half_format)",
 )
 
@@ -236,7 +228,6 @@ Value(
     type="const struct floatformat **",
     name="float_format",
     postdefault="floatformats_ieee_single",
-    invalid=True,
     printer="pformat (gdbarch, gdbarch->float_format)",
 )
 
@@ -251,7 +242,6 @@ Value(
     type="const struct floatformat **",
     name="double_format",
     postdefault="floatformats_ieee_double",
-    invalid=True,
     printer="pformat (gdbarch, gdbarch->double_format)",
 )
 
@@ -266,7 +256,6 @@ Value(
     type="const struct floatformat **",
     name="long_double_format",
     postdefault="floatformats_ieee_double",
-    invalid=True,
     printer="pformat (gdbarch, gdbarch->long_double_format)",
 )
 
@@ -289,7 +278,6 @@ One if `wchar_t' is signed, zero if unsigned.
     name="wchar_signed",
     predefault="-1",
     postdefault="1",
-    invalid=True,
 )
 
 Method(
@@ -332,7 +320,6 @@ addr_bit is the size of a target address as represented in gdb
     name="addr_bit",
     predefault="0",
     postdefault="gdbarch_ptr_bit (gdbarch)",
-    invalid=True,
 )
 
 Value(
@@ -355,7 +342,6 @@ and if Dwarf versions < 4 need to be supported.
     name="dwarf2_addr_size",
     predefault="0",
     postdefault="gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT",
-    invalid=True,
 )
 
 Value(
@@ -366,7 +352,6 @@ One if `char' acts like `signed char', zero if `unsigned char'.
     name="char_signed",
     predefault="-1",
     postdefault="1",
-    invalid=True,
 )
 
 Function(

Re: [PATCH 1/2] gdb: updates to gdbarch.py algorithm

[Simon Marchi]

On 3/2/23 05:13, Andrew Burgess wrote:
> Simon Marchi <simark@simark.ca> writes:
> 
>> On 2/28/23 11:51, Andrew Burgess via Gdb-patches wrote:
>>> Restructure how gdbarch.py generates the verify_gdbarch function.
>>> Previously the postdefault handling was bundled together with the
>>> validation.  This means that a field can't have both a postdefault,
>>> and set its invalid attribute to a string.
>>>
>>> This doesn't seem reasonable to me, I see no reason why a field can't
>>> have both a postdefault (used when the tdep doesn't set the field),
>>> and an invalid expression, which can be used to validate the value
>>> that a tdep might set.
>>>
>>> In this commit I restructure the verify_gdbarch generation code to
>>> allow the above, there is no change in the actual generated code in
>>> this commit, that will come in later commit.
>>>
>>> I did end up having to remove the "invalid" attribute (where the
>>> attribute was set to True) from a number of fields in this commit.
>>> This invalid attribute was never having an effect as these components
>>> all have a postdefault.  Consider; the "postdefault" is applied if the
>>> field still has its initial value, while an "invalid" attribute set to
>>> True means error if the field still has its default value.  But the
>>> field never will have its default value, it will always have its
>>> postdefault value.
>>> ---
>>>  gdb/gdbarch.py            | 31 ++++++++++++++++---------
>>>  gdb/gdbarch_components.py | 49 ++++++++++++++-------------------------
>>>  2 files changed, 37 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/gdb/gdbarch.py b/gdb/gdbarch.py
>>> index 93b1e8bf84e..7fea41c9572 100755
>>> --- a/gdb/gdbarch.py
>>> +++ b/gdb/gdbarch.py
>>> @@ -203,35 +203,44 @@ with open("gdbarch.c", "w") as f:
>>>          file=f,
>>>      )
>>>      for c in filter(not_info, components):
>>> -        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 isinstance(c.invalid, str) and c.postdefault is not None:
>>> -            print(f"  if ({c.invalid})", file=f)
>>> -            print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
>>> -        elif c.predefault is not None and c.postdefault is not None:
>>> +        # An opportunity to write in the 'postdefault' value.
>>> +        if c.postdefault is not None and c.predefault is not None:
>>>              print(f"  if (gdbarch->{c.name} == {c.predefault})", file=f)
>>>              print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
>>>          elif c.postdefault is not None:
>>>              print(f"  if (gdbarch->{c.name} == 0)", file=f)
>>>              print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
>>
>> I would find this postdefault snippet easier to read like this, with a
>> single "if c.postdefault is not None", and then another condition inside
>> to decide what we should compare against:
>>
>>         if c.postdefault is not None:
>>             if c.predefault is not None:
>>                 print(f"  if (gdbarch->{c.name} == {c.predefault})", file=f)
>>                 print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
>>             else:
>>                 print(f"  if (gdbarch->{c.name} == 0)", file=f)
>>                 print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
>>
>> or even
>>
>>         if c.postdefault is not None:
>>             predefault = c.predefault or "0"
>>             print(f"  if (gdbarch->{c.name} == {predefault})", file=f)
>>             print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
> 
> I went with the second approach, I like removing the duplicate print
> calls.
> 
>>
>>> +
>>> +        # 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:
>>
>> I think it's confusing for the "invalid" parameter to be able to be
>> None, that it's one to many state versus what we need to be able to
>> represent.  I think we can get by with string, True and False, where
>> True means "auto", where the validity check is generated if it makes
>> sense to.  Having one less state would help simplify things.  I hacked
>> this locally and it seems to work.  I can post this as a cleanup before
>> or on top of your patch, as you prefer.
>>
>> Another cleanup that would help me understand what is going on would be
>> to change this long list of if/elif to something that looks more like a
>> decision tree.  On top of your patch, and on top of my suggestion to get
>> rid of the invalid=None state, this is what I made looks like:
>>
>>         predefault = c.predefault or "0"
>>
>>         # Now validate the value.
>>         if type(c.invalid) is str:
>>             print(f"  if ({c.invalid})", file=f)
>>             print(f"""    log.puts ("\\n\\t{c.name}");""", file=f)
>>         elif c.invalid:
>>             if c.predicate:
>>                 print(f"  /* Skip verify of {c.name}, has predicate.  */", file=f)
>>             elif c.postdefault:
>>                 # We currently don't print anything, but we could print:
>>                 # print(f"  /* Skip verify of {c.name}, has predicate.  */", file=f)
>>                 pass
>>             else:
>>                 print(f"  if (gdbarch->{c.name} == {predefault})", file=f)
>>                 print(f"""    log.puts ("\\n\\t{c.name}");""", file=f)
>>         else:
>>             print(f"  /* Skip verify of {c.name}, invalid_p == 0 */", file=f)
>>
>> That structure is clearer to me.  We see clearly the portions handling
>> the three states of "invalid" (str, True and False).  Inside invalid ==
>> True (which really means "auto"), we see that we skip generating the
>> check when either predicate or postdefault is set, the two situations
>> where generating the check doesn't make sense.
>>
>> Another nice thing about this is that there isn't the "unhandled case
>> when generating gdbarch validation" case at the end.  Each branch of the
>> decision tree has an outcome.
>>
>> Again, if you agree with this cleanup, we could do it before or after
>> your patch, as you wish.
> 
> Yeah, I think what you have above would be a great improvement.  I like
> that we can (with what you propose) now have an "invalid" string and
> also have a predicate, that feels like an improvement.

I'm not sure if it was intended, but I don't mind.  It would have been
useful for my return_value patch (which we will not use in the end).  In
there, I wanted an "invalid" expression to validate that return_value
was not set at the same time as return_value_as_value.  But it was also
possible to set neither, so I wanted a predicate as well (which would
just check != 0).

> I've included an update of my patch below.   I haven't included the
> "invalid" refactor that you describe above as it sounds like you already
> have a patch for this work, and it would end up as a separate patch
> anyway.  I'm fine with it going in after my work of before, whatever
> works best.
> 
> Let me know what you think.

Thanks, I think this looks good, and we can make further improvements
from there.

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

Re: [PATCH 1/2] gdb: updates to gdbarch.py algorithm

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> This doesn't seem reasonable to me, I see no reason why a field can't
Andrew> have both a postdefault (used when the tdep doesn't set the field),
Andrew> and an invalid expression, which can be used to validate the value
Andrew> that a tdep might set.

Andrew> In this commit I restructure the verify_gdbarch generation code to
Andrew> allow the above, there is no change in the actual generated code in
Andrew> this commit, that will come in later commit.

FWIW I'm generally in favor of anything that simplifies how one writes
new gdbarch hooks.  The current approach has always been pretty
confusing.

Andrew> I did end up having to remove the "invalid" attribute (where the
Andrew> attribute was set to True) from a number of fields in this commit.

Most likely these were all that way in the old gdbarch.sh and nobody
ever noticed.

Tom

[PATCH 2/2] gdb: add gdbarch::displaced_step_max_buffer_length

[Andrew Burgess]

The gdbarch::max_insn_length field is used mostly to support displaced
stepping; it controls the size of the buffers allocated for the
displaced-step instruction, and is also used when first copying the
instruction, and later, when fixing up the instruction, in order to
read in and parse the instruction being stepped.

However, it has started to be used in other places in GDB, for
example, it's used in the Python disassembler API, and it is used on
amd64 as part of branch-tracing instruction classification.

The problem is that the value assigned to max_insn_length is not
always the maximum instruction length, but sometimes is a multiple of
that length, as required to support displaced stepping, see rs600,
ARM, and AArch64 for examples of this.

It seems to me that we are overloading the meaning of the
max_insn_length field, and I think that could potentially lead to
confusion.

I propose that we add a new gdbarch field,
gdbarch::displaced_step_max_buffer_length, this new field will do
exactly what it says on the tin; represent the required displaced step
buffer size.  The max_insn_length field can then do exactly what it
claims to do; represent the maximum length of a single instruction.

As some architectures (e.g. i386, and amd64) only require their
displaced step buffers to be a single instruction in size, I propose
that the default for displaced_step_max_buffer_length will be the
value of max_insn_length.  Architectures than need more buffer space
can then override this default as needed.

I've updated all architectures to setup the new field if appropriate,
and I've audited all calls to gdbarch_max_insn_length and switched to
gdbarch_displaced_step_max_buffer_length where appropriate.

There should be no user visible changes after this commit.
---
 gdb/aarch64-linux-tdep.c  |  4 +++-
 gdb/arm-tdep.c            |  4 +++-
 gdb/displaced-stepping.c  |  6 +++---
 gdb/gdbarch-gen.h         | 17 ++++++++++++++---
 gdb/gdbarch.c             | 26 ++++++++++++++++++++++++++
 gdb/gdbarch_components.py | 23 ++++++++++++++++++++---
 gdb/linux-tdep.c          |  2 +-
 gdb/rs6000-tdep.c         |  6 ++++--
 8 files changed, 74 insertions(+), 14 deletions(-)

diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index 0000b498f89..3eaf4e1131f 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -2240,7 +2240,9 @@ aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   set_gdbarch_get_syscall_number (gdbarch, aarch64_linux_get_syscall_number);
 
   /* Displaced stepping.  */
-  set_gdbarch_max_insn_length (gdbarch, 4 * AARCH64_DISPLACED_MODIFIED_INSNS);
+  set_gdbarch_max_insn_length (gdbarch, 4);
+  set_gdbarch_displaced_step_max_buffer_length
+    (gdbarch, 4 * AARCH64_DISPLACED_MODIFIED_INSNS);
   set_gdbarch_displaced_step_copy_insn (gdbarch,
 					aarch64_displaced_step_copy_insn);
   set_gdbarch_displaced_step_fixup (gdbarch, aarch64_displaced_step_fixup);
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index b64c21ce68f..d1941992c77 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -10662,7 +10662,9 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   /* Note: for displaced stepping, this includes the breakpoint, and one word
      of additional scratch space.  This setting isn't used for anything beside
      displaced stepping at present.  */
-  set_gdbarch_max_insn_length (gdbarch, 4 * ARM_DISPLACED_MODIFIED_INSNS);
+  set_gdbarch_displaced_step_max_buffer_length
+    (gdbarch, 4 * ARM_DISPLACED_MODIFIED_INSNS);
+  set_gdbarch_max_insn_length (gdbarch, 4);
 
   /* This should be low enough for everything.  */
   tdep->lowest_pc = 0x20;
diff --git a/gdb/displaced-stepping.c b/gdb/displaced-stepping.c
index 06b32a80f6a..72b6366390f 100644
--- a/gdb/displaced-stepping.c
+++ b/gdb/displaced-stepping.c
@@ -55,7 +55,7 @@ displaced_step_buffers::prepare (thread_info *thread, CORE_ADDR &displaced_pc)
   regcache *regcache = get_thread_regcache (thread);
   const address_space *aspace = regcache->aspace ();
   gdbarch *arch = regcache->arch ();
-  ULONGEST len = gdbarch_max_insn_length (arch);
+  ULONGEST len = gdbarch_displaced_step_max_buffer_length (arch);
 
   /* Search for an unused buffer.  */
   displaced_step_buffer *buffer = nullptr;
@@ -243,7 +243,7 @@ displaced_step_buffers::finish (gdbarch *arch, thread_info *thread,
      below.  */
   thread->inf->displaced_step_state.unavailable = false;
 
-  ULONGEST len = gdbarch_max_insn_length (arch);
+  ULONGEST len = gdbarch_displaced_step_max_buffer_length (arch);
 
   /* Restore memory of the buffer.  */
   write_memory_ptid (thread->ptid, buffer->addr,
@@ -302,7 +302,7 @@ displaced_step_buffers::restore_in_ptid (ptid_t ptid)
 
       regcache *regcache = get_thread_regcache (buffer.current_thread);
       gdbarch *arch = regcache->arch ();
-      ULONGEST len = gdbarch_max_insn_length (arch);
+      ULONGEST len = gdbarch_displaced_step_max_buffer_length (arch);
 
       write_memory_ptid (ptid, buffer.addr, buffer.saved_copy.data (), len);
 
diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
index ddb97f60315..1cfbbcf1730 100644
--- a/gdb/gdbarch-gen.h
+++ b/gdb/gdbarch-gen.h
@@ -1018,7 +1018,10 @@ typedef void (gdbarch_skip_permanent_breakpoint_ftype) (struct regcache *regcach
 extern void gdbarch_skip_permanent_breakpoint (struct gdbarch *gdbarch, struct regcache *regcache);
 extern void set_gdbarch_skip_permanent_breakpoint (struct gdbarch *gdbarch, gdbarch_skip_permanent_breakpoint_ftype *skip_permanent_breakpoint);
 
-/* The maximum length of an instruction on this architecture in bytes. */
+/* The maximum length of an instruction on this architecture in octets.
+   This must be set for architectures that support displaced-stepping.
+   Setting this for other architectures improves error detection within
+   the Python disassembler API. */
 
 extern bool gdbarch_max_insn_length_p (struct gdbarch *gdbarch);
 
@@ -1039,8 +1042,8 @@ extern void set_gdbarch_max_insn_length (struct gdbarch *gdbarch, ULONGEST max_i
    see the comments in infrun.c.
 
    The TO area is only guaranteed to have space for
-   gdbarch_max_insn_length (arch) bytes, so this function must not
-   write more bytes than that to that area.
+   gdbarch_displaced_step_max_buffer_length (arch) octets, so this
+   function must not write more octets than that to this area.
 
    If you do not provide this function, GDB assumes that the
    architecture does not support displaced stepping.
@@ -1122,6 +1125,14 @@ typedef void (gdbarch_displaced_step_restore_all_in_ptid_ftype) (inferior *paren
 extern void gdbarch_displaced_step_restore_all_in_ptid (struct gdbarch *gdbarch, inferior *parent_inf, ptid_t child_ptid);
 extern void set_gdbarch_displaced_step_restore_all_in_ptid (struct gdbarch *gdbarch, gdbarch_displaced_step_restore_all_in_ptid_ftype *displaced_step_restore_all_in_ptid);
 
+/* The maximum length in octets required for a displaced-step instruction
+   buffer.  By default this will be the same as gdbarch::max_insn_length,
+   but should be overridden for architectures that might expand a
+   displaced-step instruction to multiple replacement instructions. */
+
+extern ULONGEST gdbarch_displaced_step_max_buffer_length (struct gdbarch *gdbarch);
+extern void set_gdbarch_displaced_step_max_buffer_length (struct gdbarch *gdbarch, ULONGEST displaced_step_max_buffer_length);
+
 /* Relocate an instruction to execute at a different address.  OLDLOC
    is the address in the inferior memory where the instruction to
    relocate is currently at.  On input, TO points to the destination
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 7e4e34d5aca..49e2b63d9ff 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -192,6 +192,7 @@ struct gdbarch
   gdbarch_displaced_step_finish_ftype *displaced_step_finish = NULL;
   gdbarch_displaced_step_copy_insn_closure_by_addr_ftype *displaced_step_copy_insn_closure_by_addr = nullptr;
   gdbarch_displaced_step_restore_all_in_ptid_ftype *displaced_step_restore_all_in_ptid = nullptr;
+  ULONGEST displaced_step_max_buffer_length = 0;
   gdbarch_relocate_instruction_ftype *relocate_instruction = NULL;
   gdbarch_overlay_update_ftype *overlay_update = nullptr;
   gdbarch_core_read_description_ftype *core_read_description = nullptr;
@@ -453,6 +454,10 @@ verify_gdbarch (struct gdbarch *gdbarch)
     log.puts ("\n\tdisplaced_step_finish");
   /* Skip verify of displaced_step_copy_insn_closure_by_addr, has predicate.  */
   /* Skip verify of displaced_step_restore_all_in_ptid, invalid_p == 0 */
+  if (gdbarch->displaced_step_max_buffer_length == 0)
+    gdbarch->displaced_step_max_buffer_length = gdbarch->max_insn_length;
+  if (gdbarch->displaced_step_max_buffer_length < gdbarch->max_insn_length)
+    log.puts ("\n\tdisplaced_step_max_buffer_length");
   /* Skip verify of relocate_instruction, has predicate.  */
   /* Skip verify of overlay_update, has predicate.  */
   /* Skip verify of core_read_description, has predicate.  */
@@ -1111,6 +1116,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
   gdb_printf (file,
 	      "gdbarch_dump: displaced_step_restore_all_in_ptid = <%s>\n",
 	      host_address_to_string (gdbarch->displaced_step_restore_all_in_ptid));
+  gdb_printf (file,
+	      "gdbarch_dump: displaced_step_max_buffer_length = %s\n",
+	      plongest (gdbarch->displaced_step_max_buffer_length));
   gdb_printf (file,
 	      "gdbarch_dump: gdbarch_relocate_instruction_p() = %d\n",
 	      gdbarch_relocate_instruction_p (gdbarch));
@@ -4153,6 +4161,24 @@ set_gdbarch_displaced_step_restore_all_in_ptid (struct gdbarch *gdbarch,
   gdbarch->displaced_step_restore_all_in_ptid = displaced_step_restore_all_in_ptid;
 }
 
+ULONGEST
+gdbarch_displaced_step_max_buffer_length (struct gdbarch *gdbarch)
+{
+  gdb_assert (gdbarch != NULL);
+  /* Check variable is valid.  */
+  gdb_assert (!(gdbarch->displaced_step_max_buffer_length < gdbarch->max_insn_length));
+  if (gdbarch_debug >= 2)
+    gdb_printf (gdb_stdlog, "gdbarch_displaced_step_max_buffer_length called\n");
+  return gdbarch->displaced_step_max_buffer_length;
+}
+
+void
+set_gdbarch_displaced_step_max_buffer_length (struct gdbarch *gdbarch,
+					      ULONGEST displaced_step_max_buffer_length)
+{
+  gdbarch->displaced_step_max_buffer_length = displaced_step_max_buffer_length;
+}
+
 bool
 gdbarch_relocate_instruction_p (struct gdbarch *gdbarch)
 {
diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
index 1d420a513f9..a00398bb03d 100644
--- a/gdb/gdbarch_components.py
+++ b/gdb/gdbarch_components.py
@@ -1752,7 +1752,10 @@ Advance PC to next instruction in order to skip a permanent breakpoint.
 
 Value(
     comment="""
-The maximum length of an instruction on this architecture in bytes.
+The maximum length of an instruction on this architecture in octets.
+This must be set for architectures that support displaced-stepping.
+Setting this for other architectures improves error detection within
+the Python disassembler API.
 """,
     type="ULONGEST",
     name="max_insn_length",
@@ -1777,8 +1780,8 @@ For a general explanation of displaced stepping and how GDB uses it,
 see the comments in infrun.c.
 
 The TO area is only guaranteed to have space for
-gdbarch_max_insn_length (arch) bytes, so this function must not
-write more bytes than that to that area.
+gdbarch_displaced_step_max_buffer_length (arch) octets, so this
+function must not write more octets than that to this area.
 
 If you do not provide this function, GDB assumes that the
 architecture does not support displaced stepping.
@@ -1890,6 +1893,20 @@ contents of all displaced step buffers in the child's address space.
     invalid=False,
 )
 
+Value(
+    comment="""
+The maximum length in octets required for a displaced-step instruction
+buffer.  By default this will be the same as gdbarch::max_insn_length,
+but should be overridden for architectures that might expand a
+displaced-step instruction to multiple replacement instructions.
+""",
+    type="ULONGEST",
+    name="displaced_step_max_buffer_length",
+    predefault="0",
+    postdefault="gdbarch->max_insn_length",
+    invalid="gdbarch->displaced_step_max_buffer_length < gdbarch->max_insn_length",
+)
+
 Method(
     comment="""
 Relocate an instruction to execute at a different address.  OLDLOC
diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index e6ce13a1c67..ce48515d8ca 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -2603,7 +2603,7 @@ linux_displaced_step_prepare (gdbarch *arch, thread_info *thread,
 	 at DISP_STEP_BUF_ADDR.  They are all of size BUF_LEN.  */
       CORE_ADDR disp_step_buf_addr
 	= linux_displaced_step_location (thread->inf->gdbarch);
-      int buf_len = gdbarch_max_insn_length (arch);
+      int buf_len = gdbarch_displaced_step_max_buffer_length (arch);
 
       linux_gdbarch_data *gdbarch_data = get_linux_gdbarch_data (arch);
       gdb_assert (gdbarch_data->num_disp_step_buffers > 0);
diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 104515de030..9fc4574bb41 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -889,7 +889,8 @@ ppc_displaced_step_copy_insn (struct gdbarch *gdbarch,
 			      CORE_ADDR from, CORE_ADDR to,
 			      struct regcache *regs)
 {
-  size_t len = gdbarch_max_insn_length (gdbarch);
+  size_t len = gdbarch_displaced_step_max_buffer_length (gdbarch);
+  gdb_assert (len > PPC_INSN_SIZE);
   std::unique_ptr<ppc_displaced_step_copy_insn_closure> closure
     (new ppc_displaced_step_copy_insn_closure (len));
   gdb_byte *buf = closure->buf.data ();
@@ -8363,8 +8364,9 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_displaced_step_finish (gdbarch, ppc_displaced_step_finish);
   set_gdbarch_displaced_step_restore_all_in_ptid
     (gdbarch, ppc_displaced_step_restore_all_in_ptid);
+  set_gdbarch_displaced_step_max_buffer_length (gdbarch, 2 * PPC_INSN_SIZE);
 
-  set_gdbarch_max_insn_length (gdbarch, 2 * PPC_INSN_SIZE);
+  set_gdbarch_max_insn_length (gdbarch, PPC_INSN_SIZE);
 
   /* Hook in ABI-specific overrides, if they have been registered.  */
   info.target_desc = tdesc;
-- 
2.25.4

Re: [PATCH 2/2] gdb: add gdbarch::displaced_step_max_buffer_length

[Simon Marchi]

On 2/28/23 11:51, Andrew Burgess via Gdb-patches wrote:
> The gdbarch::max_insn_length field is used mostly to support displaced
> stepping; it controls the size of the buffers allocated for the
> displaced-step instruction, and is also used when first copying the
> instruction, and later, when fixing up the instruction, in order to
> read in and parse the instruction being stepped.
> 
> However, it has started to be used in other places in GDB, for
> example, it's used in the Python disassembler API, and it is used on
> amd64 as part of branch-tracing instruction classification.
> 
> The problem is that the value assigned to max_insn_length is not
> always the maximum instruction length, but sometimes is a multiple of
> that length, as required to support displaced stepping, see rs600,
> ARM, and AArch64 for examples of this.
> 
> It seems to me that we are overloading the meaning of the
> max_insn_length field, and I think that could potentially lead to
> confusion.
> 
> I propose that we add a new gdbarch field,
> gdbarch::displaced_step_max_buffer_length, this new field will do
> exactly what it says on the tin; represent the required displaced step
> buffer size.  The max_insn_length field can then do exactly what it
> claims to do; represent the maximum length of a single instruction.
> 
> As some architectures (e.g. i386, and amd64) only require their
> displaced step buffers to be a single instruction in size, I propose
> that the default for displaced_step_max_buffer_length will be the
> value of max_insn_length.  Architectures than need more buffer space
> can then override this default as needed.
> 
> I've updated all architectures to setup the new field if appropriate,
> and I've audited all calls to gdbarch_max_insn_length and switched to
> gdbarch_displaced_step_max_buffer_length where appropriate.

Naming nit: the displaced step buffer length isn't variable, so I don't
think it makes sense to have "max" in the name.  I think it should be
named "displaced_step_buffer_length", since it's _the_ displaced step
buffer length.


> diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
> index 1d420a513f9..a00398bb03d 100644
> --- a/gdb/gdbarch_components.py
> +++ b/gdb/gdbarch_components.py
> @@ -1752,7 +1752,10 @@ Advance PC to next instruction in order to skip a permanent breakpoint.
>  
>  Value(
>      comment="""
> -The maximum length of an instruction on this architecture in bytes.
> +The maximum length of an instruction on this architecture in octets.
> +This must be set for architectures that support displaced-stepping.
> +Setting this for other architectures improves error detection within
> +the Python disassembler API.

I'm not sure I understand why you would mention that this needs to be
set for architectures supporting displaced-stepping, since the point of
your change is to use displaced_step_max_buffer_length for those use
cases.

These were only minor comments, the patch LGTM in general:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

[PATCHv2 0/5] Add new gdbarch::displaced_step_buffer_length field

[Andrew Burgess]

Changes in v2:

  - The final patch (#5), actually adding displaced_step_buffer_length
    has been updated inline with Simon's feedback.  The changes are
    pretty minor so I've added the Approved-By tag.

  - Patches #1 and #2 are new in this version.  These two patches are
    removing fields from gdbarch_components.py that have no effect on
    the generated source.  Removing these fields makes it easier to
    refactor the gdbarch.py code in later commits.

  - Patch #3 is from the v1 series and hasn't changed significantly.
    I dropped the comment change on max_insn_length.  Simon was
    correct in his feedback, the changes I made to this comment made
    little sense.  The rest of the patch is basically unchagned.

  - Patch #4 is new in this version.  This makes the changes Simon
    wanted to see about the 'invalid' field no longer being able to
    take the value 'None', and also we can now have both a predicate
    and a string 'invalid' field.

    I did play with splitting these two changes (invalid can't be None
    and predicate+string invalid), but this made little sense, and was
    really forced.  Allowing the second seems to fall naturally out of
    refactoring the 'invalid' field to stop it taking the value 'None'.

Thoughts?

Thanks,
Andrew

---

Andrew Burgess (5):
  gdb/gdbarch: remove unused 'invalid=True' from gdbarch_components.py
  gdb/gdbarch: remove yet more 'invalid=True' from gdbarch_components.py
  gdb/gdbarch: split postdefault setup from invalid check in gdbarch.py
  gdb/gdbarch: remove the 'invalid=None' state from
    gdbarch_components.py
  gdb: add gdbarch::displaced_step_buffer_length

 gdb/aarch64-linux-tdep.c  |   4 +-
 gdb/arm-tdep.c            |   4 +-
 gdb/displaced-stepping.c  |   6 +-
 gdb/gdbarch-gen.h         |  12 +++-
 gdb/gdbarch.c             |  42 ++++++++++++
 gdb/gdbarch.py            |  60 ++++++++---------
 gdb/gdbarch_components.py | 131 ++++++++++----------------------------
 gdb/gdbarch_types.py      |   7 +-
 gdb/linux-tdep.c          |   2 +-
 gdb/rs6000-tdep.c         |   6 +-
 10 files changed, 134 insertions(+), 140 deletions(-)


base-commit: 6a208145d24c47912c8beb4f1f4b9abeb8d51134
-- 
2.25.4

[PATCHv2 1/5] gdb/gdbarch: remove unused 'invalid=True' from gdbarch_components.py

[Andrew Burgess]

Due to the algorithm used to generate verify_gdbarch in gdbarch.py, if
a component has a predicate, then a validation check will never be
generated.

There are a bunch of components that are declared with both a
predicate AND have 'invalid=True' set.  The 'invalid=True' has no
effect.

In this commit I clean things up by removing all these additional
'invalid=True' lines.  There's no change in any of the generated files
after this commit.
---
 gdb/gdbarch_components.py | 64 ---------------------------------------
 1 file changed, 64 deletions(-)

diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
index caa65c334ec..656c6a9f905 100644
--- a/gdb/gdbarch_components.py
+++ b/gdb/gdbarch_components.py
@@ -374,7 +374,6 @@ Function(
     name="read_pc",
     params=[("readable_regcache *", "regcache")],
     predicate=True,
-    invalid=True,
 )
 
 Function(
@@ -382,7 +381,6 @@ Function(
     name="write_pc",
     params=[("struct regcache *", "regcache"), ("CORE_ADDR", "val")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -411,7 +409,6 @@ Method(
         ("gdb_byte *", "buf"),
     ],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -425,7 +422,6 @@ never be called.
     name="pseudo_register_read_value",
     params=[("readable_regcache *", "regcache"), ("int", "cookednum")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -437,7 +433,6 @@ Method(
         ("const gdb_byte *", "buf"),
     ],
     predicate=True,
-    invalid=True,
 )
 
 Value(
@@ -469,7 +464,6 @@ Return -1 if something goes wrong, 0 otherwise.
     name="ax_pseudo_register_collect",
     params=[("struct agent_expr *", "ax"), ("int", "reg")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -482,7 +476,6 @@ Return -1 if something goes wrong, 0 otherwise.
     name="ax_pseudo_register_push_stack",
     params=[("struct agent_expr *", "ax"), ("int", "reg")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -495,7 +488,6 @@ UIOUT is the output stream where the handler will place information.
     name="report_signal_info",
     params=[("struct ui_out *", "uiout"), ("enum gdb_signal", "siggnal")],
     predicate=True,
-    invalid=True,
 )
 
 Value(
@@ -649,7 +641,6 @@ Method(
         ("CORE_ADDR", "struct_addr"),
     ],
     predicate=True,
-    invalid=True,
 )
 
 Value(
@@ -673,7 +664,6 @@ Method(
         ("struct regcache *", "regcache"),
     ],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -721,7 +711,6 @@ Method(
         ("const char *", "args"),
     ],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -763,7 +752,6 @@ FRAME corresponds to the longjmp frame.
     name="get_longjmp_target",
     params=[("frame_info_ptr", "frame"), ("CORE_ADDR *", "pc")],
     predicate=True,
-    invalid=True,
 )
 
 Value(
@@ -845,7 +833,6 @@ Method(
     name="integer_to_address",
     params=[("struct type *", "type"), ("const gdb_byte *", "buf")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -952,7 +939,6 @@ Method(
     name="skip_main_prologue",
     params=[("CORE_ADDR", "ip")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -973,7 +959,6 @@ is not used.
     name="skip_entrypoint",
     params=[("CORE_ADDR", "ip")],
     predicate=True,
-    invalid=True,
 )
 
 Function(
@@ -1034,7 +1019,6 @@ Method(
     name="adjust_breakpoint_address",
     params=[("CORE_ADDR", "bpaddr")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1094,7 +1078,6 @@ Fetch the target specific address used to represent a load module.
     name="fetch_tls_load_module_address",
     params=[("struct objfile *", "objfile")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1109,7 +1092,6 @@ be zero for statically linked multithreaded inferiors.
     name="get_thread_local_address",
     params=[("ptid_t", "ptid"), ("CORE_ADDR", "lm_addr"), ("CORE_ADDR", "offset")],
     predicate=True,
-    invalid=True,
 )
 
 Value(
@@ -1143,7 +1125,6 @@ frame-base.  Enable frame-base before frame-unwind.
     name="frame_num_args",
     params=[("frame_info_ptr", "frame")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1151,7 +1132,6 @@ Method(
     name="frame_align",
     params=[("CORE_ADDR", "address")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1314,7 +1294,6 @@ past a conditional branch to self.
     name="software_single_step",
     params=[("struct regcache *", "regcache")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1326,7 +1305,6 @@ further single-step is needed before the instruction finishes.
     name="single_step_through_delay",
     params=[("frame_info_ptr", "frame")],
     predicate=True,
-    invalid=True,
 )
 
 Function(
@@ -1425,7 +1403,6 @@ the main symbol table and DWARF-2 records.
     name="elf_make_msymbol_special",
     params=[("asymbol *", "sym"), ("struct minimal_symbol *", "msym")],
     predicate=True,
-    invalid=True,
 )
 
 Function(
@@ -1513,7 +1490,6 @@ Function(
     name="address_class_type_flags",
     params=[("int", "byte_size"), ("int", "dwarf2_addr_class")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1521,7 +1497,6 @@ Method(
     name="address_class_type_flags_to_name",
     params=[("type_instance_flags", "type_flags")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1546,7 +1521,6 @@ type_flags was set, false otherwise.
     name="address_class_name_to_type_flags",
     params=[("const char *", "name"), ("type_instance_flags *", "type_flags_ptr")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1572,7 +1546,6 @@ Fetch the pointer to the ith function argument.
         ("struct type *", "type"),
     ],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1592,7 +1565,6 @@ sections.
         ("const struct regcache *", "regcache"),
     ],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1603,7 +1575,6 @@ Create core file notes
     name="make_corefile_notes",
     params=[("bfd *", "obfd"), ("int *", "note_size")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1614,7 +1585,6 @@ Find core file memory regions
     name="find_memory_regions",
     params=[("find_memory_region_ftype", "func"), ("void *", "data")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1625,7 +1595,6 @@ Given a bfd OBFD, segment ADDRESS and SIZE, create a memory tag section to be du
     name="create_memtag_section",
     params=[("bfd *", "obfd"), ("CORE_ADDR", "address"), ("size_t", "size")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1636,7 +1605,6 @@ Given a memory tag section OSEC, fill OSEC's contents with the appropriate tag d
     name="fill_memtag_section",
     params=[("asection *", "osec")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1654,7 +1622,6 @@ If no tags were found, return an empty vector.
         ("size_t", "length"),
     ],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1668,7 +1635,6 @@ failed, otherwise, return the red length of READBUF.
     name="core_xfer_shared_libraries",
     params=[("gdb_byte *", "readbuf"), ("ULONGEST", "offset"), ("ULONGEST", "len")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1681,7 +1647,6 @@ Return the number of bytes read (zero indicates failure).
     name="core_xfer_shared_libraries_aix",
     params=[("gdb_byte *", "readbuf"), ("ULONGEST", "offset"), ("ULONGEST", "len")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1692,7 +1657,6 @@ How the core target converts a PTID from a core file to a string.
     name="core_pid_to_str",
     params=[("ptid_t", "ptid")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1703,7 +1667,6 @@ How the core target extracts the name of a thread from a core file.
     name="core_thread_name",
     params=[("struct thread_info *", "thr")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1716,7 +1679,6 @@ of bytes read (zero indicates EOF, a negative value indicates failure).
     name="core_xfer_siginfo",
     params=[("gdb_byte *", "readbuf"), ("ULONGEST", "offset"), ("ULONGEST", "len")],
     predicate=True,
-    invalid=True,
 )
 
 Value(
@@ -1727,7 +1689,6 @@ BFD target to use when generating a core file.
     name="gcore_bfd_target",
     predicate=True,
     predefault="0",
-    invalid=True,
     printer="pstring (gdbarch->gcore_bfd_target)",
 )
 
@@ -1773,7 +1734,6 @@ The maximum length of an instruction on this architecture in bytes.
     name="max_insn_length",
     predicate=True,
     predefault="0",
-    invalid=True,
 )
 
 Method(
@@ -1806,7 +1766,6 @@ that case.
     name="displaced_step_copy_insn",
     params=[("CORE_ADDR", "from"), ("CORE_ADDR", "to"), ("struct regcache *", "regs")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1856,7 +1815,6 @@ see the comments in infrun.c.
     ],
     predicate=True,
     predefault="NULL",
-    invalid=True,
 )
 
 Method(
@@ -1869,7 +1827,6 @@ Throw an exception if any unexpected error happens.
     name="displaced_step_prepare",
     params=[("thread_info *", "thread"), ("CORE_ADDR &", "displaced_pc")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1891,7 +1848,6 @@ Return the closure associated to the displaced step buffer that is at ADDR.
     name="displaced_step_copy_insn_closure_by_addr",
     params=[("inferior *", "inf"), ("CORE_ADDR", "addr")],
     predicate=True,
-    invalid=True,
 )
 
 Function(
@@ -1924,7 +1880,6 @@ offset adjusted; etc.
     params=[("CORE_ADDR *", "to"), ("CORE_ADDR", "from")],
     predicate=True,
     predefault="NULL",
-    invalid=True,
 )
 
 Function(
@@ -1935,7 +1890,6 @@ Refresh overlay mapped state for section OSECT.
     name="overlay_update",
     params=[("struct obj_section *", "osect")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1943,7 +1897,6 @@ Method(
     name="core_read_description",
     params=[("struct target_ops *", "target"), ("bfd *", "abfd")],
     predicate=True,
-    invalid=True,
 )
 
 Value(
@@ -1967,7 +1920,6 @@ Return -1 if something goes wrong, 0 otherwise.
     name="process_record",
     params=[("struct regcache *", "regcache"), ("CORE_ADDR", "addr")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1979,7 +1931,6 @@ Return -1 if something goes wrong, 0 otherwise.
     name="process_record_signal",
     params=[("struct regcache *", "regcache"), ("enum gdb_signal", "signal")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1996,7 +1947,6 @@ headers.  This is mainly used when cross-debugging core files ---
     name="gdb_signal_from_target",
     params=[("int", "signo")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -2013,7 +1963,6 @@ signal number is invalid.
     name="gdb_signal_to_target",
     params=[("enum gdb_signal", "signal")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -2026,7 +1975,6 @@ Return a type suitable to inspect extra signal information.
     name="get_siginfo_type",
     params=[],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -2037,7 +1985,6 @@ Record architecture-specific information from the symbol table.
     name="record_special_symbol",
     params=[("struct objfile *", "objfile"), ("asymbol *", "sym")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -2049,7 +1996,6 @@ Get architecture-specific system calls information from registers.
     name="get_syscall_number",
     params=[("thread_info *", "thread")],
     predicate=True,
-    invalid=True,
 )
 
 Value(
@@ -2219,7 +2165,6 @@ something like `(%', do not match just the `('.
     name="stap_is_single_operand",
     params=[("const char *", "s")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -2250,7 +2195,6 @@ parser), and should advance the buffer pointer (p->arg).
     name="stap_parse_special_token",
     params=[("struct stap_parse_info *", "p")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -2287,7 +2231,6 @@ The rationale for this can be found at PR breakpoints/24541.
         ("int", "regnum"),
     ],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -2300,7 +2243,6 @@ NARG must be >= 0.
     name="dtrace_parse_probe_argument",
     params=[("int", "narg")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -2312,7 +2254,6 @@ corresponding to a disabled DTrace is-enabled probe.
     name="dtrace_probe_is_enabled",
     params=[("CORE_ADDR", "addr")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -2323,7 +2264,6 @@ Enable a DTrace is-enabled probe at ADDR.
     name="dtrace_enable_probe",
     params=[("CORE_ADDR", "addr")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -2334,7 +2274,6 @@ Disable a DTrace is-enabled probe at ADDR.
     name="dtrace_disable_probe",
     params=[("CORE_ADDR", "addr")],
     predicate=True,
-    invalid=True,
 )
 
 Value(
@@ -2476,7 +2415,6 @@ Implement the "info proc" command.
     name="info_proc",
     params=[("const char *", "args"), ("enum info_proc_what", "what")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -2489,7 +2427,6 @@ one for live targets.
     name="core_info_proc",
     params=[("const char *", "args"), ("enum info_proc_what", "what")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -2585,7 +2522,6 @@ Return 1 if an entry was read into *TYPEP and *VALP.
         ("CORE_ADDR *", "valp"),
     ],
     predicate=True,
-    invalid=True,
 )
 
 Method(
-- 
2.25.4

[PATCHv2 2/5] gdb/gdbarch: remove yet more 'invalid=True' from gdbarch_components.py

[Andrew Burgess]

Following on from the previous commit, this commit removes yet more
'invalid=True' lines from gdbarch_components.py where the invalid
setting has no effect.

Due to the algorithm used in gdbarch.py for generated verify_gdbarch,
if a component has a postdefault value then no invalid check will ever
be generated for the component, as such setting 'invalid=True' on the
component is pointless.  This commit removes the setting of invalid.

There is no change in the generated code after this commit.
---
 gdb/gdbarch_components.py | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
index 656c6a9f905..d3dfcfc806f 100644
--- a/gdb/gdbarch_components.py
+++ b/gdb/gdbarch_components.py
@@ -206,7 +206,6 @@ Value(
     type="const struct floatformat **",
     name="bfloat16_format",
     postdefault="floatformats_bfloat16",
-    invalid=True,
     printer="pformat (gdbarch, gdbarch->bfloat16_format)",
 )
 
@@ -221,7 +220,6 @@ Value(
     type="const struct floatformat **",
     name="half_format",
     postdefault="floatformats_ieee_half",
-    invalid=True,
     printer="pformat (gdbarch, gdbarch->half_format)",
 )
 
@@ -236,7 +234,6 @@ Value(
     type="const struct floatformat **",
     name="float_format",
     postdefault="floatformats_ieee_single",
-    invalid=True,
     printer="pformat (gdbarch, gdbarch->float_format)",
 )
 
@@ -251,7 +248,6 @@ Value(
     type="const struct floatformat **",
     name="double_format",
     postdefault="floatformats_ieee_double",
-    invalid=True,
     printer="pformat (gdbarch, gdbarch->double_format)",
 )
 
@@ -266,7 +262,6 @@ Value(
     type="const struct floatformat **",
     name="long_double_format",
     postdefault="floatformats_ieee_double",
-    invalid=True,
     printer="pformat (gdbarch, gdbarch->long_double_format)",
 )
 
@@ -289,7 +284,6 @@ One if `wchar_t' is signed, zero if unsigned.
     name="wchar_signed",
     predefault="-1",
     postdefault="1",
-    invalid=True,
 )
 
 Method(
@@ -332,7 +326,6 @@ addr_bit is the size of a target address as represented in gdb
     name="addr_bit",
     predefault="0",
     postdefault="gdbarch_ptr_bit (gdbarch)",
-    invalid=True,
 )
 
 Value(
@@ -355,7 +348,6 @@ and if Dwarf versions < 4 need to be supported.
     name="dwarf2_addr_size",
     predefault="0",
     postdefault="gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT",
-    invalid=True,
 )
 
 Value(
@@ -366,7 +358,6 @@ One if `char' acts like `signed char', zero if `unsigned char'.
     name="char_signed",
     predefault="-1",
     postdefault="1",
-    invalid=True,
 )
 
 Function(
-- 
2.25.4

[PATCHv2 3/5] gdb/gdbarch: split postdefault setup from invalid check in gdbarch.py

[Andrew Burgess]

Restructure how gdbarch.py generates the verify_gdbarch function.
Previously the postdefault handling was bundled together with the
validation.  This means that a field can't have both a postdefault,
and set its invalid attribute to a string.

This doesn't seem reasonable to me, I see no reason why a field can't
have both a postdefault (used when the tdep doesn't set the field),
and an invalid expression, which can be used to validate the value
that a tdep might set.

In this commit I restructure the verify_gdbarch generation code to
allow the above, there is no change in the actual generated code in
this commit, that will come in later commit.

I did end up having to remove the "invalid" attribute (where the
attribute was set to True) from a number of fields in this commit.
This invalid attribute was never having an effect as these components
all have a postdefault.  Consider; the "postdefault" is applied if the
field still has its initial value, while an "invalid" attribute set to
True means error if the field still has its default value.  But the
field never will have its default value, it will always have its
postdefault value.
---
 gdb/gdbarch.py            | 38 ++++++++++++++++++++++---------------
 gdb/gdbarch_components.py | 40 +++++++++++++++++----------------------
 2 files changed, 40 insertions(+), 38 deletions(-)

diff --git a/gdb/gdbarch.py b/gdb/gdbarch.py
index 93b1e8bf84e..806aee26831 100755
--- a/gdb/gdbarch.py
+++ b/gdb/gdbarch.py
@@ -203,35 +203,43 @@ with open("gdbarch.c", "w") as f:
         file=f,
     )
     for c in filter(not_info, components):
+        # An opportunity to write in the 'postdefault' value.  We
+        # change field's value to the postdefault if its current value
+        # is not different to the initial value of the field.
+        if c.postdefault is not None:
+            init_value = c.predefault or "0"
+            print(f"  if (gdbarch->{c.name} == {init_value})", file=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 isinstance(c.invalid, str) and c.postdefault is not None:
-            print(f"  if ({c.invalid})", file=f)
-            print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
-        elif c.predefault is not None and c.postdefault is not None:
-            print(f"  if (gdbarch->{c.name} == {c.predefault})", file=f)
-            print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
-        elif c.postdefault is not None:
-            print(f"  if (gdbarch->{c.name} == 0)", file=f)
-            print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
+        elif c.invalid is None:
+            # No validation has been requested for this component.
+            pass
         elif isinstance(c.invalid, str):
             print(f"  if ({c.invalid})", file=f)
             print(f"""    log.puts ("\\n\\t{c.name}");""", file=f)
-        elif c.predefault is not None:
-            print(f"  if (gdbarch->{c.name} == {c.predefault})", file=f)
-            print(f"""    log.puts ("\\n\\t{c.name}");""", file=f)
         elif c.invalid is True:
-            print(f"  if (gdbarch->{c.name} == 0)", file=f)
-            print(f"""    log.puts ("\\n\\t{c.name}");""", file=f)
+            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
+                # postdefault will have been applied above, so this field
+                # will not have a zero value.
+                raise Exception(f"component {c.name} has postdefault and invalid set to True")
+            else:
+                init_value = c.predefault or "0"
+                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("unhandled case when generating gdbarch validation")
+            raise Exception(f"unhandled case when generating gdbarch validation: {c.name}")
     print("  if (!log.empty ())", file=f)
     print(
         """    internal_error (_("verify_gdbarch: the following are invalid ...%s"),""",
diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
index d3dfcfc806f..1f217123216 100644
--- a/gdb/gdbarch_components.py
+++ b/gdb/gdbarch_components.py
@@ -63,34 +63,28 @@
 # * "predefault", "postdefault", and "invalid" - These are used for
 # the initialization and verification steps:
 #
-# A gdbarch is zero-initialized.  Then, if a field has a pre-default,
-# the field is set to that value.  After initialization is complete
-# (that is, after the tdep code has a chance to change the settings),
-# the post-initialization step is done.
+# A gdbarch is zero-initialized.  Then, if a field has a "predefault",
+# the field is set to that value.  This becomes the field's initial
+# value.
 #
-# There is a generic algorithm to generate a "validation function" for
-# all fields.  If the field has an "invalid" attribute with a string
-# value, then this string is the expression (note that a string-valued
-# "invalid" and "predicate" are mutually exclusive; and the case where
-# invalid is True means to ignore this field and instead use the
-# default checking that is about to be described).  Otherwise, if
-# there is a "predefault", then the field is valid if it differs from
-# the predefault.  Otherwise, the check is done against 0 (really NULL
-# for function pointers, but same idea).
-#
-# In post-initialization / validation, there are several cases.
+# After initialization is complete (that is, after the tdep code has a
+# chance to change the settings), the post-initialization step is
+# done.
 #
-# * If "invalid" is False, or if the field specifies "predicate",
-# validation is skipped.  Otherwise, a validation step is emitted.
+# If the field still has its initial value (see above), and the field
+# has a "postdefault", then the field is set to this value.
 #
-# * Otherwise, the validity is checked using the usual validation
-# function (see above).  If the field is considered valid, nothing is
-# done.
+# After the possible "postdefault" assignment, validation is
+# performed for fields that don't have a "predicate".
 #
-# * Otherwise, the field's value is invalid.  If there is a
-# "postdefault", then the field is assigned that value.
+# If the field has an "invalid" attribute with a string value, then
+# this string is the expression that should evaluate to true when the
+# field is invalid.
 #
-# * Otherwise, the gdbarch will fail validation and gdb will crash.
+# Otherwise, if "invalid" is True, then the generic validation
+# function is used: the field is considered invalid it still contains
+# its default value.  This validation is what is used within the _p
+# predicate function if the field has "predicate" set to True.
 #
 # Function and Method share:
 #
-- 
2.25.4

Re: [PATCHv2 3/5] gdb/gdbarch: split postdefault setup from invalid check in gdbarch.py

[Simon Marchi]

On 3/6/23 10:31, Andrew Burgess via Gdb-patches wrote:
> Restructure how gdbarch.py generates the verify_gdbarch function.
> Previously the postdefault handling was bundled together with the
> validation.  This means that a field can't have both a postdefault,
> and set its invalid attribute to a string.
> 
> This doesn't seem reasonable to me, I see no reason why a field can't
> have both a postdefault (used when the tdep doesn't set the field),
> and an invalid expression, which can be used to validate the value
> that a tdep might set.
> 
> In this commit I restructure the verify_gdbarch generation code to
> allow the above, there is no change in the actual generated code in
> this commit, that will come in later commit.
> 
> I did end up having to remove the "invalid" attribute (where the
> attribute was set to True) from a number of fields in this commit.
> This invalid attribute was never having an effect as these components
> all have a postdefault.  Consider; the "postdefault" is applied if the
> field still has its initial value, while an "invalid" attribute set to
> True means error if the field still has its default value.  But the
> field never will have its default value, it will always have its
> postdefault value.

The last paragraph can probably be removed, since that work was done in
separate patches.

Simon

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

[Andrew Burgess]

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.

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            | 32 +++++++++++++-------------------
 gdb/gdbarch_components.py |  2 ++
 gdb/gdbarch_types.py      |  7 +++----
 4 files changed, 34 insertions(+), 23 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 806aee26831..f296c160bbc 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
@@ -234,12 +229,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"),""",
@@ -356,14 +346,18 @@ 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..1eef2fb584e 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,
-- 
2.25.4

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

[Simon Marchi]

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

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

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> --- a/gdb/gdbarch_components.py
Andrew> +++ b/gdb/gdbarch_components.py
Andrew> @@ -1675,6 +1675,7 @@ BFD target to use when generating a core file.
Andrew>      predicate=True,
Andrew>      predefault="0",
Andrew>      printer="pstring (gdbarch->gcore_bfd_target)",
Andrew> +    invalid=True

Super nit, but it's better to have a trailing comma here, because it
means that future additions can be done without touching this line.

Tom

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

[Simon Marchi]

On 3/7/23 10:17, Tom Tromey wrote:
>>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Andrew> --- a/gdb/gdbarch_components.py
> Andrew> +++ b/gdb/gdbarch_components.py
> Andrew> @@ -1675,6 +1675,7 @@ BFD target to use when generating a core file.
> Andrew>      predicate=True,
> Andrew>      predefault="0",
> Andrew>      printer="pstring (gdbarch->gcore_bfd_target)",
> Andrew> +    invalid=True
> 
> Super nit, but it's better to have a trailing comma here, because it
> means that future additions can be done without touching this line.

Actually, black adds it automatically in this case.

Simon

[PATCHv2 5/5] gdb: add gdbarch::displaced_step_buffer_length

[Andrew Burgess]

The gdbarch::max_insn_length field is used mostly to support displaced
stepping; it controls the size of the buffers allocated for the
displaced-step instruction, and is also used when first copying the
instruction, and later, when fixing up the instruction, in order to
read in and parse the instruction being stepped.

However, it has started to be used in other places in GDB, for
example, it's used in the Python disassembler API, and it is used on
amd64 as part of branch-tracing instruction classification.

The problem is that the value assigned to max_insn_length is not
always the maximum instruction length, but sometimes is a multiple of
that length, as required to support displaced stepping, see rs600,
ARM, and AArch64 for examples of this.

It seems to me that we are overloading the meaning of the
max_insn_length field, and I think that could potentially lead to
confusion.

I propose that we add a new gdbarch field,
gdbarch::displaced_step_buffer_length, this new field will do
exactly what it says on the tin; represent the required displaced step
buffer size.  The max_insn_length field can then do exactly what it
claims to do; represent the maximum length of a single instruction.

As some architectures (e.g. i386, and amd64) only require their
displaced step buffers to be a single instruction in size, I propose
that the default for displaced_step_buffer_length will be the
value of max_insn_length.  Architectures than need more buffer space
can then override this default as needed.

I've updated all architectures to setup the new field if appropriate,
and I've audited all calls to gdbarch_max_insn_length and switched to
gdbarch_displaced_step_buffer_length where appropriate.

There should be no user visible changes after this commit.

Approved-By: Simon Marchi <simon.marchi@efficios.com>
---
 gdb/aarch64-linux-tdep.c  |  4 +++-
 gdb/arm-tdep.c            |  4 +++-
 gdb/displaced-stepping.c  |  6 +++---
 gdb/gdbarch-gen.h         | 12 ++++++++++--
 gdb/gdbarch.c             | 26 ++++++++++++++++++++++++++
 gdb/gdbarch_components.py | 18 ++++++++++++++++--
 gdb/linux-tdep.c          |  2 +-
 gdb/rs6000-tdep.c         |  6 ++++--
 8 files changed, 66 insertions(+), 12 deletions(-)

diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index 0000b498f89..3eaf4e1131f 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -2240,7 +2240,9 @@ aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   set_gdbarch_get_syscall_number (gdbarch, aarch64_linux_get_syscall_number);
 
   /* Displaced stepping.  */
-  set_gdbarch_max_insn_length (gdbarch, 4 * AARCH64_DISPLACED_MODIFIED_INSNS);
+  set_gdbarch_max_insn_length (gdbarch, 4);
+  set_gdbarch_displaced_step_max_buffer_length
+    (gdbarch, 4 * AARCH64_DISPLACED_MODIFIED_INSNS);
   set_gdbarch_displaced_step_copy_insn (gdbarch,
 					aarch64_displaced_step_copy_insn);
   set_gdbarch_displaced_step_fixup (gdbarch, aarch64_displaced_step_fixup);
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index b64c21ce68f..d1941992c77 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -10662,7 +10662,9 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   /* Note: for displaced stepping, this includes the breakpoint, and one word
      of additional scratch space.  This setting isn't used for anything beside
      displaced stepping at present.  */
-  set_gdbarch_max_insn_length (gdbarch, 4 * ARM_DISPLACED_MODIFIED_INSNS);
+  set_gdbarch_displaced_step_max_buffer_length
+    (gdbarch, 4 * ARM_DISPLACED_MODIFIED_INSNS);
+  set_gdbarch_max_insn_length (gdbarch, 4);
 
   /* This should be low enough for everything.  */
   tdep->lowest_pc = 0x20;
diff --git a/gdb/displaced-stepping.c b/gdb/displaced-stepping.c
index 06b32a80f6a..72b6366390f 100644
--- a/gdb/displaced-stepping.c
+++ b/gdb/displaced-stepping.c
@@ -55,7 +55,7 @@ displaced_step_buffers::prepare (thread_info *thread, CORE_ADDR &displaced_pc)
   regcache *regcache = get_thread_regcache (thread);
   const address_space *aspace = regcache->aspace ();
   gdbarch *arch = regcache->arch ();
-  ULONGEST len = gdbarch_max_insn_length (arch);
+  ULONGEST len = gdbarch_displaced_step_max_buffer_length (arch);
 
   /* Search for an unused buffer.  */
   displaced_step_buffer *buffer = nullptr;
@@ -243,7 +243,7 @@ displaced_step_buffers::finish (gdbarch *arch, thread_info *thread,
      below.  */
   thread->inf->displaced_step_state.unavailable = false;
 
-  ULONGEST len = gdbarch_max_insn_length (arch);
+  ULONGEST len = gdbarch_displaced_step_max_buffer_length (arch);
 
   /* Restore memory of the buffer.  */
   write_memory_ptid (thread->ptid, buffer->addr,
@@ -302,7 +302,7 @@ displaced_step_buffers::restore_in_ptid (ptid_t ptid)
 
       regcache *regcache = get_thread_regcache (buffer.current_thread);
       gdbarch *arch = regcache->arch ();
-      ULONGEST len = gdbarch_max_insn_length (arch);
+      ULONGEST len = gdbarch_displaced_step_max_buffer_length (arch);
 
       write_memory_ptid (ptid, buffer.addr, buffer.saved_copy.data (), len);
 
diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
index ddb97f60315..76d12a15317 100644
--- a/gdb/gdbarch-gen.h
+++ b/gdb/gdbarch-gen.h
@@ -1039,8 +1039,8 @@ extern void set_gdbarch_max_insn_length (struct gdbarch *gdbarch, ULONGEST max_i
    see the comments in infrun.c.
 
    The TO area is only guaranteed to have space for
-   gdbarch_max_insn_length (arch) bytes, so this function must not
-   write more bytes than that to that area.
+   gdbarch_displaced_step_buffer_length (arch) octets, so this
+   function must not write more octets than that to this area.
 
    If you do not provide this function, GDB assumes that the
    architecture does not support displaced stepping.
@@ -1122,6 +1122,14 @@ typedef void (gdbarch_displaced_step_restore_all_in_ptid_ftype) (inferior *paren
 extern void gdbarch_displaced_step_restore_all_in_ptid (struct gdbarch *gdbarch, inferior *parent_inf, ptid_t child_ptid);
 extern void set_gdbarch_displaced_step_restore_all_in_ptid (struct gdbarch *gdbarch, gdbarch_displaced_step_restore_all_in_ptid_ftype *displaced_step_restore_all_in_ptid);
 
+/* The maximum length in octets required for a displaced-step instruction
+   buffer.  By default this will be the same as gdbarch::max_insn_length,
+   but should be overridden for architectures that might expand a
+   displaced-step instruction to multiple replacement instructions. */
+
+extern ULONGEST gdbarch_displaced_step_buffer_length (struct gdbarch *gdbarch);
+extern void set_gdbarch_displaced_step_buffer_length (struct gdbarch *gdbarch, ULONGEST displaced_step_buffer_length);
+
 /* Relocate an instruction to execute at a different address.  OLDLOC
    is the address in the inferior memory where the instruction to
    relocate is currently at.  On input, TO points to the destination
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index efd111eeabc..8cb307d39c9 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -192,6 +192,7 @@ struct gdbarch
   gdbarch_displaced_step_finish_ftype *displaced_step_finish = NULL;
   gdbarch_displaced_step_copy_insn_closure_by_addr_ftype *displaced_step_copy_insn_closure_by_addr = nullptr;
   gdbarch_displaced_step_restore_all_in_ptid_ftype *displaced_step_restore_all_in_ptid = nullptr;
+  ULONGEST displaced_step_buffer_length = 0;
   gdbarch_relocate_instruction_ftype *relocate_instruction = NULL;
   gdbarch_overlay_update_ftype *overlay_update = nullptr;
   gdbarch_core_read_description_ftype *core_read_description = nullptr;
@@ -463,6 +464,10 @@ verify_gdbarch (struct gdbarch *gdbarch)
     log.puts ("\n\tdisplaced_step_finish");
   /* Skip verify of displaced_step_copy_insn_closure_by_addr, has predicate.  */
   /* Skip verify of displaced_step_restore_all_in_ptid, invalid_p == 0 */
+  if (gdbarch->displaced_step_buffer_length == 0)
+    gdbarch->displaced_step_buffer_length = gdbarch->max_insn_length;
+  if (gdbarch->displaced_step_buffer_length < gdbarch->max_insn_length)
+    log.puts ("\n\tdisplaced_step_buffer_length");
   /* Skip verify of relocate_instruction, has predicate.  */
   /* Skip verify of overlay_update, has predicate.  */
   /* Skip verify of core_read_description, has predicate.  */
@@ -1121,6 +1126,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
   gdb_printf (file,
 	      "gdbarch_dump: displaced_step_restore_all_in_ptid = <%s>\n",
 	      host_address_to_string (gdbarch->displaced_step_restore_all_in_ptid));
+  gdb_printf (file,
+	      "gdbarch_dump: displaced_step_buffer_length = %s\n",
+	      plongest (gdbarch->displaced_step_buffer_length));
   gdb_printf (file,
 	      "gdbarch_dump: gdbarch_relocate_instruction_p() = %d\n",
 	      gdbarch_relocate_instruction_p (gdbarch));
@@ -4169,6 +4177,24 @@ set_gdbarch_displaced_step_restore_all_in_ptid (struct gdbarch *gdbarch,
   gdbarch->displaced_step_restore_all_in_ptid = displaced_step_restore_all_in_ptid;
 }
 
+ULONGEST
+gdbarch_displaced_step_buffer_length (struct gdbarch *gdbarch)
+{
+  gdb_assert (gdbarch != NULL);
+  /* Check variable is valid.  */
+  gdb_assert (!(gdbarch->displaced_step_buffer_length < gdbarch->max_insn_length));
+  if (gdbarch_debug >= 2)
+    gdb_printf (gdb_stdlog, "gdbarch_displaced_step_buffer_length called\n");
+  return gdbarch->displaced_step_buffer_length;
+}
+
+void
+set_gdbarch_displaced_step_buffer_length (struct gdbarch *gdbarch,
+					  ULONGEST displaced_step_buffer_length)
+{
+  gdbarch->displaced_step_buffer_length = displaced_step_buffer_length;
+}
+
 bool
 gdbarch_relocate_instruction_p (struct gdbarch *gdbarch)
 {
diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
index 1eef2fb584e..58fba1aea71 100644
--- a/gdb/gdbarch_components.py
+++ b/gdb/gdbarch_components.py
@@ -1739,8 +1739,8 @@ For a general explanation of displaced stepping and how GDB uses it,
 see the comments in infrun.c.
 
 The TO area is only guaranteed to have space for
-gdbarch_max_insn_length (arch) bytes, so this function must not
-write more bytes than that to that area.
+gdbarch_displaced_step_buffer_length (arch) octets, so this
+function must not write more octets than that to this area.
 
 If you do not provide this function, GDB assumes that the
 architecture does not support displaced stepping.
@@ -1848,6 +1848,20 @@ contents of all displaced step buffers in the child's address space.
     invalid=False,
 )
 
+Value(
+    comment="""
+The maximum length in octets required for a displaced-step instruction
+buffer.  By default this will be the same as gdbarch::max_insn_length,
+but should be overridden for architectures that might expand a
+displaced-step instruction to multiple replacement instructions.
+""",
+    type="ULONGEST",
+    name="displaced_step_buffer_length",
+    predefault="0",
+    postdefault="gdbarch->max_insn_length",
+    invalid="gdbarch->displaced_step_buffer_length < gdbarch->max_insn_length",
+)
+
 Method(
     comment="""
 Relocate an instruction to execute at a different address.  OLDLOC
diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index e6ce13a1c67..ce48515d8ca 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -2603,7 +2603,7 @@ linux_displaced_step_prepare (gdbarch *arch, thread_info *thread,
 	 at DISP_STEP_BUF_ADDR.  They are all of size BUF_LEN.  */
       CORE_ADDR disp_step_buf_addr
 	= linux_displaced_step_location (thread->inf->gdbarch);
-      int buf_len = gdbarch_max_insn_length (arch);
+      int buf_len = gdbarch_displaced_step_max_buffer_length (arch);
 
       linux_gdbarch_data *gdbarch_data = get_linux_gdbarch_data (arch);
       gdb_assert (gdbarch_data->num_disp_step_buffers > 0);
diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 104515de030..9fc4574bb41 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -889,7 +889,8 @@ ppc_displaced_step_copy_insn (struct gdbarch *gdbarch,
 			      CORE_ADDR from, CORE_ADDR to,
 			      struct regcache *regs)
 {
-  size_t len = gdbarch_max_insn_length (gdbarch);
+  size_t len = gdbarch_displaced_step_max_buffer_length (gdbarch);
+  gdb_assert (len > PPC_INSN_SIZE);
   std::unique_ptr<ppc_displaced_step_copy_insn_closure> closure
     (new ppc_displaced_step_copy_insn_closure (len));
   gdb_byte *buf = closure->buf.data ();
@@ -8363,8 +8364,9 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_displaced_step_finish (gdbarch, ppc_displaced_step_finish);
   set_gdbarch_displaced_step_restore_all_in_ptid
     (gdbarch, ppc_displaced_step_restore_all_in_ptid);
+  set_gdbarch_displaced_step_max_buffer_length (gdbarch, 2 * PPC_INSN_SIZE);
 
-  set_gdbarch_max_insn_length (gdbarch, 2 * PPC_INSN_SIZE);
+  set_gdbarch_max_insn_length (gdbarch, PPC_INSN_SIZE);
 
   /* Hook in ABI-specific overrides, if they have been registered.  */
   info.target_desc = tdesc;
-- 
2.25.4

Re: [PATCHv2 5/5] gdb: add gdbarch::displaced_step_buffer_length

[Simon Marchi]

> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index e6ce13a1c67..ce48515d8ca 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -2603,7 +2603,7 @@ linux_displaced_step_prepare (gdbarch *arch, thread_info *thread,
>  	 at DISP_STEP_BUF_ADDR.  They are all of size BUF_LEN.  */
>        CORE_ADDR disp_step_buf_addr
>  	= linux_displaced_step_location (thread->inf->gdbarch);
> -      int buf_len = gdbarch_max_insn_length (arch);
> +      int buf_len = gdbarch_displaced_step_max_buffer_length (arch);

There are a few places that still use the old name.

Simon

[PATCHv3 0/9] Add new gdbarch::displaced_step_buffer_length field

[Andrew Burgess]

Changes in V3:

  - Have now run 'black' on each patch in this series, so everything
    should be formatted correctly,

  - Have fixed the out of date names in the last patch of this series.
    Everything buildds and runs fine for me now,

  - Have removed the out of date text from the commit message in patch
    #3,

  - Have fixed the missing comma Tom spotted in patch #4,

  - I have NOT removed the comments Simon pointed out in patch #4.
    Removing these would require changing the generated code for more
    Components than I already change in this commit.  And so, I think,
    if those comments are to go that would require a separate patch.

    That said, we do generate validation code within the getters for
    many components, so I think having a comment in the components
    where we don't generate validation makes sense.  So for me, I
    think the comments do add value, I'd suggest we should keep them.

  - And the big one.  I've changed the 'invalid' default from False to
    True.  I know Simon suggested that False was the correct choice,
    but I actually think that True makes more sense.  I'd rather we
    assume we should generate the validity checks and require than new
    components explicitly choose to not have that check, rather than
    just assuming we shouldn't check.

    However, in order to get to the default True state I ended up
    having to fix some other issues.  And, so, incase people really
    would rather see False as the default, I've left the patch which
    changes to default True as the penultimate patch in the series.
    If you feel really strongly that False is best, I can just drop
    the patch that switches over to use True.  Just let me know.

Let me know what you think,

Thanks,
Andrew


---

Changes in v2:

  - The final patch (#5), actually adding displaced_step_buffer_length
    has been updated inline with Simon's feedback.  The changes are
    pretty minor so I've added the Approved-By tag.

  - Patches #1 and #2 are new in this version.  These two patches are
    removing fields from gdbarch_components.py that have no effect on
    the generated source.  Removing these fields makes it easier to
    refactor the gdbarch.py code in later commits.

  - Patch #3 is from the v1 series and hasn't changed significantly.
    I dropped the comment change on max_insn_length.  Simon was
    correct in his feedback, the changes I made to this comment made
    little sense.  The rest of the patch is basically unchagned.

  - Patch #4 is new in this version.  This makes the changes Simon
    wanted to see about the 'invalid' field no longer being able to
    take the value 'None', and also we can now have both a predicate
    and a string 'invalid' field.

    I did play with splitting these two changes (invalid can't be None
    and predicate+string invalid), but this made little sense, and was
    really forced.  Allowing the second seems to fall naturally out of
    refactoring the 'invalid' field to stop it taking the value 'None'.

---

Andrew Burgess (9):
  gdb/gdbarch: remove unused 'invalid=True' from gdbarch_components.py
  gdb/gdbarch: remove yet more 'invalid=True' from gdbarch_components.py
  gdb/gdbarch: split postdefault setup from invalid check in gdbarch.py
  gdb/gdbarch: remove the 'invalid=None' state from
    gdbarch_components.py
  gdbarch: use predefault for more value components within gdbarch
  gdbarch: improve generation of validation in gdbarch getters
  gdbarch: remove some unneeded predefault="0" from
    gdbarch_components.py
  gdbarch: make invalid=True the default for all Components
  gdb: add gdbarch::displaced_step_buffer_length

 gdb/aarch64-linux-tdep.c  |   4 +-
 gdb/arm-tdep.c            |   4 +-
 gdb/displaced-stepping.c  |   6 +-
 gdb/gdbarch-gen.h         |  12 ++-
 gdb/gdbarch.c             |  94 +++++++++++++-------
 gdb/gdbarch.py            |  66 +++++++-------
 gdb/gdbarch_components.py | 182 +++++++++++---------------------------
 gdb/gdbarch_types.py      |   7 +-
 gdb/linux-tdep.c          |   2 +-
 gdb/rs6000-tdep.c         |   6 +-
 10 files changed, 175 insertions(+), 208 deletions(-)


base-commit: 6a208145d24c47912c8beb4f1f4b9abeb8d51134
-- 
2.25.4

[PATCHv3 1/9] gdb/gdbarch: remove unused 'invalid=True' from gdbarch_components.py

[Andrew Burgess]

Due to the algorithm used to generate verify_gdbarch in gdbarch.py, if
a component has a predicate, then a validation check will never be
generated.

There are a bunch of components that are declared with both a
predicate AND have 'invalid=True' set.  The 'invalid=True' has no
effect.

In this commit I clean things up by removing all these additional
'invalid=True' lines.  There's no change in any of the generated files
after this commit.
---
 gdb/gdbarch_components.py | 64 ---------------------------------------
 1 file changed, 64 deletions(-)

diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
index caa65c334ec..656c6a9f905 100644
--- a/gdb/gdbarch_components.py
+++ b/gdb/gdbarch_components.py
@@ -374,7 +374,6 @@ Function(
     name="read_pc",
     params=[("readable_regcache *", "regcache")],
     predicate=True,
-    invalid=True,
 )
 
 Function(
@@ -382,7 +381,6 @@ Function(
     name="write_pc",
     params=[("struct regcache *", "regcache"), ("CORE_ADDR", "val")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -411,7 +409,6 @@ Method(
         ("gdb_byte *", "buf"),
     ],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -425,7 +422,6 @@ never be called.
     name="pseudo_register_read_value",
     params=[("readable_regcache *", "regcache"), ("int", "cookednum")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -437,7 +433,6 @@ Method(
         ("const gdb_byte *", "buf"),
     ],
     predicate=True,
-    invalid=True,
 )
 
 Value(
@@ -469,7 +464,6 @@ Return -1 if something goes wrong, 0 otherwise.
     name="ax_pseudo_register_collect",
     params=[("struct agent_expr *", "ax"), ("int", "reg")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -482,7 +476,6 @@ Return -1 if something goes wrong, 0 otherwise.
     name="ax_pseudo_register_push_stack",
     params=[("struct agent_expr *", "ax"), ("int", "reg")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -495,7 +488,6 @@ UIOUT is the output stream where the handler will place information.
     name="report_signal_info",
     params=[("struct ui_out *", "uiout"), ("enum gdb_signal", "siggnal")],
     predicate=True,
-    invalid=True,
 )
 
 Value(
@@ -649,7 +641,6 @@ Method(
         ("CORE_ADDR", "struct_addr"),
     ],
     predicate=True,
-    invalid=True,
 )
 
 Value(
@@ -673,7 +664,6 @@ Method(
         ("struct regcache *", "regcache"),
     ],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -721,7 +711,6 @@ Method(
         ("const char *", "args"),
     ],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -763,7 +752,6 @@ FRAME corresponds to the longjmp frame.
     name="get_longjmp_target",
     params=[("frame_info_ptr", "frame"), ("CORE_ADDR *", "pc")],
     predicate=True,
-    invalid=True,
 )
 
 Value(
@@ -845,7 +833,6 @@ Method(
     name="integer_to_address",
     params=[("struct type *", "type"), ("const gdb_byte *", "buf")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -952,7 +939,6 @@ Method(
     name="skip_main_prologue",
     params=[("CORE_ADDR", "ip")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -973,7 +959,6 @@ is not used.
     name="skip_entrypoint",
     params=[("CORE_ADDR", "ip")],
     predicate=True,
-    invalid=True,
 )
 
 Function(
@@ -1034,7 +1019,6 @@ Method(
     name="adjust_breakpoint_address",
     params=[("CORE_ADDR", "bpaddr")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1094,7 +1078,6 @@ Fetch the target specific address used to represent a load module.
     name="fetch_tls_load_module_address",
     params=[("struct objfile *", "objfile")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1109,7 +1092,6 @@ be zero for statically linked multithreaded inferiors.
     name="get_thread_local_address",
     params=[("ptid_t", "ptid"), ("CORE_ADDR", "lm_addr"), ("CORE_ADDR", "offset")],
     predicate=True,
-    invalid=True,
 )
 
 Value(
@@ -1143,7 +1125,6 @@ frame-base.  Enable frame-base before frame-unwind.
     name="frame_num_args",
     params=[("frame_info_ptr", "frame")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1151,7 +1132,6 @@ Method(
     name="frame_align",
     params=[("CORE_ADDR", "address")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1314,7 +1294,6 @@ past a conditional branch to self.
     name="software_single_step",
     params=[("struct regcache *", "regcache")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1326,7 +1305,6 @@ further single-step is needed before the instruction finishes.
     name="single_step_through_delay",
     params=[("frame_info_ptr", "frame")],
     predicate=True,
-    invalid=True,
 )
 
 Function(
@@ -1425,7 +1403,6 @@ the main symbol table and DWARF-2 records.
     name="elf_make_msymbol_special",
     params=[("asymbol *", "sym"), ("struct minimal_symbol *", "msym")],
     predicate=True,
-    invalid=True,
 )
 
 Function(
@@ -1513,7 +1490,6 @@ Function(
     name="address_class_type_flags",
     params=[("int", "byte_size"), ("int", "dwarf2_addr_class")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1521,7 +1497,6 @@ Method(
     name="address_class_type_flags_to_name",
     params=[("type_instance_flags", "type_flags")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1546,7 +1521,6 @@ type_flags was set, false otherwise.
     name="address_class_name_to_type_flags",
     params=[("const char *", "name"), ("type_instance_flags *", "type_flags_ptr")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1572,7 +1546,6 @@ Fetch the pointer to the ith function argument.
         ("struct type *", "type"),
     ],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1592,7 +1565,6 @@ sections.
         ("const struct regcache *", "regcache"),
     ],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1603,7 +1575,6 @@ Create core file notes
     name="make_corefile_notes",
     params=[("bfd *", "obfd"), ("int *", "note_size")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1614,7 +1585,6 @@ Find core file memory regions
     name="find_memory_regions",
     params=[("find_memory_region_ftype", "func"), ("void *", "data")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1625,7 +1595,6 @@ Given a bfd OBFD, segment ADDRESS and SIZE, create a memory tag section to be du
     name="create_memtag_section",
     params=[("bfd *", "obfd"), ("CORE_ADDR", "address"), ("size_t", "size")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1636,7 +1605,6 @@ Given a memory tag section OSEC, fill OSEC's contents with the appropriate tag d
     name="fill_memtag_section",
     params=[("asection *", "osec")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1654,7 +1622,6 @@ If no tags were found, return an empty vector.
         ("size_t", "length"),
     ],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1668,7 +1635,6 @@ failed, otherwise, return the red length of READBUF.
     name="core_xfer_shared_libraries",
     params=[("gdb_byte *", "readbuf"), ("ULONGEST", "offset"), ("ULONGEST", "len")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1681,7 +1647,6 @@ Return the number of bytes read (zero indicates failure).
     name="core_xfer_shared_libraries_aix",
     params=[("gdb_byte *", "readbuf"), ("ULONGEST", "offset"), ("ULONGEST", "len")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1692,7 +1657,6 @@ How the core target converts a PTID from a core file to a string.
     name="core_pid_to_str",
     params=[("ptid_t", "ptid")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1703,7 +1667,6 @@ How the core target extracts the name of a thread from a core file.
     name="core_thread_name",
     params=[("struct thread_info *", "thr")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1716,7 +1679,6 @@ of bytes read (zero indicates EOF, a negative value indicates failure).
     name="core_xfer_siginfo",
     params=[("gdb_byte *", "readbuf"), ("ULONGEST", "offset"), ("ULONGEST", "len")],
     predicate=True,
-    invalid=True,
 )
 
 Value(
@@ -1727,7 +1689,6 @@ BFD target to use when generating a core file.
     name="gcore_bfd_target",
     predicate=True,
     predefault="0",
-    invalid=True,
     printer="pstring (gdbarch->gcore_bfd_target)",
 )
 
@@ -1773,7 +1734,6 @@ The maximum length of an instruction on this architecture in bytes.
     name="max_insn_length",
     predicate=True,
     predefault="0",
-    invalid=True,
 )
 
 Method(
@@ -1806,7 +1766,6 @@ that case.
     name="displaced_step_copy_insn",
     params=[("CORE_ADDR", "from"), ("CORE_ADDR", "to"), ("struct regcache *", "regs")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1856,7 +1815,6 @@ see the comments in infrun.c.
     ],
     predicate=True,
     predefault="NULL",
-    invalid=True,
 )
 
 Method(
@@ -1869,7 +1827,6 @@ Throw an exception if any unexpected error happens.
     name="displaced_step_prepare",
     params=[("thread_info *", "thread"), ("CORE_ADDR &", "displaced_pc")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1891,7 +1848,6 @@ Return the closure associated to the displaced step buffer that is at ADDR.
     name="displaced_step_copy_insn_closure_by_addr",
     params=[("inferior *", "inf"), ("CORE_ADDR", "addr")],
     predicate=True,
-    invalid=True,
 )
 
 Function(
@@ -1924,7 +1880,6 @@ offset adjusted; etc.
     params=[("CORE_ADDR *", "to"), ("CORE_ADDR", "from")],
     predicate=True,
     predefault="NULL",
-    invalid=True,
 )
 
 Function(
@@ -1935,7 +1890,6 @@ Refresh overlay mapped state for section OSECT.
     name="overlay_update",
     params=[("struct obj_section *", "osect")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1943,7 +1897,6 @@ Method(
     name="core_read_description",
     params=[("struct target_ops *", "target"), ("bfd *", "abfd")],
     predicate=True,
-    invalid=True,
 )
 
 Value(
@@ -1967,7 +1920,6 @@ Return -1 if something goes wrong, 0 otherwise.
     name="process_record",
     params=[("struct regcache *", "regcache"), ("CORE_ADDR", "addr")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1979,7 +1931,6 @@ Return -1 if something goes wrong, 0 otherwise.
     name="process_record_signal",
     params=[("struct regcache *", "regcache"), ("enum gdb_signal", "signal")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -1996,7 +1947,6 @@ headers.  This is mainly used when cross-debugging core files ---
     name="gdb_signal_from_target",
     params=[("int", "signo")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -2013,7 +1963,6 @@ signal number is invalid.
     name="gdb_signal_to_target",
     params=[("enum gdb_signal", "signal")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -2026,7 +1975,6 @@ Return a type suitable to inspect extra signal information.
     name="get_siginfo_type",
     params=[],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -2037,7 +1985,6 @@ Record architecture-specific information from the symbol table.
     name="record_special_symbol",
     params=[("struct objfile *", "objfile"), ("asymbol *", "sym")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -2049,7 +1996,6 @@ Get architecture-specific system calls information from registers.
     name="get_syscall_number",
     params=[("thread_info *", "thread")],
     predicate=True,
-    invalid=True,
 )
 
 Value(
@@ -2219,7 +2165,6 @@ something like `(%', do not match just the `('.
     name="stap_is_single_operand",
     params=[("const char *", "s")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -2250,7 +2195,6 @@ parser), and should advance the buffer pointer (p->arg).
     name="stap_parse_special_token",
     params=[("struct stap_parse_info *", "p")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -2287,7 +2231,6 @@ The rationale for this can be found at PR breakpoints/24541.
         ("int", "regnum"),
     ],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -2300,7 +2243,6 @@ NARG must be >= 0.
     name="dtrace_parse_probe_argument",
     params=[("int", "narg")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -2312,7 +2254,6 @@ corresponding to a disabled DTrace is-enabled probe.
     name="dtrace_probe_is_enabled",
     params=[("CORE_ADDR", "addr")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -2323,7 +2264,6 @@ Enable a DTrace is-enabled probe at ADDR.
     name="dtrace_enable_probe",
     params=[("CORE_ADDR", "addr")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -2334,7 +2274,6 @@ Disable a DTrace is-enabled probe at ADDR.
     name="dtrace_disable_probe",
     params=[("CORE_ADDR", "addr")],
     predicate=True,
-    invalid=True,
 )
 
 Value(
@@ -2476,7 +2415,6 @@ Implement the "info proc" command.
     name="info_proc",
     params=[("const char *", "args"), ("enum info_proc_what", "what")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -2489,7 +2427,6 @@ one for live targets.
     name="core_info_proc",
     params=[("const char *", "args"), ("enum info_proc_what", "what")],
     predicate=True,
-    invalid=True,
 )
 
 Method(
@@ -2585,7 +2522,6 @@ Return 1 if an entry was read into *TYPEP and *VALP.
         ("CORE_ADDR *", "valp"),
     ],
     predicate=True,
-    invalid=True,
 )
 
 Method(
-- 
2.25.4

[PATCHv3 2/9] gdb/gdbarch: remove yet more 'invalid=True' from gdbarch_components.py

[Andrew Burgess]

Following on from the previous commit, this commit removes yet more
'invalid=True' lines from gdbarch_components.py where the invalid
setting has no effect.

Due to the algorithm used in gdbarch.py for generated verify_gdbarch,
if a component has a postdefault value then no invalid check will ever
be generated for the component, as such setting 'invalid=True' on the
component is pointless.  This commit removes the setting of invalid.

There is no change in the generated code after this commit.
---
 gdb/gdbarch_components.py | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
index 656c6a9f905..d3dfcfc806f 100644
--- a/gdb/gdbarch_components.py
+++ b/gdb/gdbarch_components.py
@@ -206,7 +206,6 @@ Value(
     type="const struct floatformat **",
     name="bfloat16_format",
     postdefault="floatformats_bfloat16",
-    invalid=True,
     printer="pformat (gdbarch, gdbarch->bfloat16_format)",
 )
 
@@ -221,7 +220,6 @@ Value(
     type="const struct floatformat **",
     name="half_format",
     postdefault="floatformats_ieee_half",
-    invalid=True,
     printer="pformat (gdbarch, gdbarch->half_format)",
 )
 
@@ -236,7 +234,6 @@ Value(
     type="const struct floatformat **",
     name="float_format",
     postdefault="floatformats_ieee_single",
-    invalid=True,
     printer="pformat (gdbarch, gdbarch->float_format)",
 )
 
@@ -251,7 +248,6 @@ Value(
     type="const struct floatformat **",
     name="double_format",
     postdefault="floatformats_ieee_double",
-    invalid=True,
     printer="pformat (gdbarch, gdbarch->double_format)",
 )
 
@@ -266,7 +262,6 @@ Value(
     type="const struct floatformat **",
     name="long_double_format",
     postdefault="floatformats_ieee_double",
-    invalid=True,
     printer="pformat (gdbarch, gdbarch->long_double_format)",
 )
 
@@ -289,7 +284,6 @@ One if `wchar_t' is signed, zero if unsigned.
     name="wchar_signed",
     predefault="-1",
     postdefault="1",
-    invalid=True,
 )
 
 Method(
@@ -332,7 +326,6 @@ addr_bit is the size of a target address as represented in gdb
     name="addr_bit",
     predefault="0",
     postdefault="gdbarch_ptr_bit (gdbarch)",
-    invalid=True,
 )
 
 Value(
@@ -355,7 +348,6 @@ and if Dwarf versions < 4 need to be supported.
     name="dwarf2_addr_size",
     predefault="0",
     postdefault="gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT",
-    invalid=True,
 )
 
 Value(
@@ -366,7 +358,6 @@ One if `char' acts like `signed char', zero if `unsigned char'.
     name="char_signed",
     predefault="-1",
     postdefault="1",
-    invalid=True,
 )
 
 Function(
-- 
2.25.4

[PATCHv3 3/9] gdb/gdbarch: split postdefault setup from invalid check in gdbarch.py

[Andrew Burgess]

Restructure how gdbarch.py generates the verify_gdbarch function.
Previously the postdefault handling was bundled together with the
validation.  This means that a field can't have both a postdefault,
and set its invalid attribute to a string.

This doesn't seem reasonable to me, I see no reason why a field can't
have both a postdefault (used when the tdep doesn't set the field),
and an invalid expression, which can be used to validate the value
that a tdep might set.

In this commit I restructure the verify_gdbarch generation code to
allow the above, there is no change in the actual generated code in
this commit, that will come in later commit.
---
 gdb/gdbarch.py            | 42 +++++++++++++++++++++++++--------------
 gdb/gdbarch_components.py | 40 ++++++++++++++++---------------------
 2 files changed, 44 insertions(+), 38 deletions(-)

diff --git a/gdb/gdbarch.py b/gdb/gdbarch.py
index 93b1e8bf84e..5437c827f34 100755
--- a/gdb/gdbarch.py
+++ b/gdb/gdbarch.py
@@ -203,35 +203,47 @@ with open("gdbarch.c", "w") as f:
         file=f,
     )
     for c in filter(not_info, components):
+        # An opportunity to write in the 'postdefault' value.  We
+        # change field's value to the postdefault if its current value
+        # is not different to the initial value of the field.
+        if c.postdefault is not None:
+            init_value = c.predefault or "0"
+            print(f"  if (gdbarch->{c.name} == {init_value})", file=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 isinstance(c.invalid, str) and c.postdefault is not None:
-            print(f"  if ({c.invalid})", file=f)
-            print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
-        elif c.predefault is not None and c.postdefault is not None:
-            print(f"  if (gdbarch->{c.name} == {c.predefault})", file=f)
-            print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
-        elif c.postdefault is not None:
-            print(f"  if (gdbarch->{c.name} == 0)", file=f)
-            print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
+        elif c.invalid is None:
+            # No validation has been requested for this component.
+            pass
         elif isinstance(c.invalid, str):
             print(f"  if ({c.invalid})", file=f)
             print(f"""    log.puts ("\\n\\t{c.name}");""", file=f)
-        elif c.predefault is not None:
-            print(f"  if (gdbarch->{c.name} == {c.predefault})", file=f)
-            print(f"""    log.puts ("\\n\\t{c.name}");""", file=f)
         elif c.invalid is True:
-            print(f"  if (gdbarch->{c.name} == 0)", file=f)
-            print(f"""    log.puts ("\\n\\t{c.name}");""", file=f)
+            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
+                # postdefault will have been applied above, so this field
+                # will not have a zero value.
+                raise Exception(
+                    f"component {c.name} has postdefault and invalid set to True"
+                )
+            else:
+                init_value = c.predefault or "0"
+                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("unhandled case when generating gdbarch validation")
+            raise Exception(
+                f"unhandled case when generating gdbarch validation: {c.name}"
+            )
     print("  if (!log.empty ())", file=f)
     print(
         """    internal_error (_("verify_gdbarch: the following are invalid ...%s"),""",
diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
index d3dfcfc806f..1f217123216 100644
--- a/gdb/gdbarch_components.py
+++ b/gdb/gdbarch_components.py
@@ -63,34 +63,28 @@
 # * "predefault", "postdefault", and "invalid" - These are used for
 # the initialization and verification steps:
 #
-# A gdbarch is zero-initialized.  Then, if a field has a pre-default,
-# the field is set to that value.  After initialization is complete
-# (that is, after the tdep code has a chance to change the settings),
-# the post-initialization step is done.
+# A gdbarch is zero-initialized.  Then, if a field has a "predefault",
+# the field is set to that value.  This becomes the field's initial
+# value.
 #
-# There is a generic algorithm to generate a "validation function" for
-# all fields.  If the field has an "invalid" attribute with a string
-# value, then this string is the expression (note that a string-valued
-# "invalid" and "predicate" are mutually exclusive; and the case where
-# invalid is True means to ignore this field and instead use the
-# default checking that is about to be described).  Otherwise, if
-# there is a "predefault", then the field is valid if it differs from
-# the predefault.  Otherwise, the check is done against 0 (really NULL
-# for function pointers, but same idea).
-#
-# In post-initialization / validation, there are several cases.
+# After initialization is complete (that is, after the tdep code has a
+# chance to change the settings), the post-initialization step is
+# done.
 #
-# * If "invalid" is False, or if the field specifies "predicate",
-# validation is skipped.  Otherwise, a validation step is emitted.
+# If the field still has its initial value (see above), and the field
+# has a "postdefault", then the field is set to this value.
 #
-# * Otherwise, the validity is checked using the usual validation
-# function (see above).  If the field is considered valid, nothing is
-# done.
+# After the possible "postdefault" assignment, validation is
+# performed for fields that don't have a "predicate".
 #
-# * Otherwise, the field's value is invalid.  If there is a
-# "postdefault", then the field is assigned that value.
+# If the field has an "invalid" attribute with a string value, then
+# this string is the expression that should evaluate to true when the
+# field is invalid.
 #
-# * Otherwise, the gdbarch will fail validation and gdb will crash.
+# Otherwise, if "invalid" is True, then the generic validation
+# function is used: the field is considered invalid it still contains
+# its default value.  This validation is what is used within the _p
+# predicate function if the field has "predicate" set to True.
 #
 # Function and Method share:
 #
-- 
2.25.4

[PATCHv3 4/9] gdb/gdbarch: remove the 'invalid=None' state from gdbarch_components.py

[Andrew Burgess]

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,
-- 
2.25.4

[PATCHv3 5/9] gdbarch: use predefault for more value components within gdbarch

[Andrew Burgess]

For some reason the following value components of gdbarch:

  bfloat16_format
  half_format
  float_format
  double_format
  long_double_format
  so_ops

All use a postdefault but no predefault to set the default value for
the component.

As the postdefault values for these components are all constant
pointers that don't depend on other fields within the gdbarch, then I
don't see any reason why we couldn't use a predefault instead.

So lets do that.
---
 gdb/gdbarch.c             | 24 ++++++------------------
 gdb/gdbarch_components.py | 12 ++++++------
 2 files changed, 12 insertions(+), 24 deletions(-)

diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index efd111eeabc..064afd7c226 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -53,15 +53,15 @@ struct gdbarch
   int long_bit = 4*TARGET_CHAR_BIT;
   int long_long_bit = 2*4*TARGET_CHAR_BIT;
   int bfloat16_bit = 2*TARGET_CHAR_BIT;
-  const struct floatformat ** bfloat16_format = 0;
+  const struct floatformat ** bfloat16_format = floatformats_bfloat16;
   int half_bit = 2*TARGET_CHAR_BIT;
-  const struct floatformat ** half_format = 0;
+  const struct floatformat ** half_format = floatformats_ieee_half;
   int float_bit = 4*TARGET_CHAR_BIT;
-  const struct floatformat ** float_format = 0;
+  const struct floatformat ** float_format = floatformats_ieee_single;
   int double_bit = 8*TARGET_CHAR_BIT;
-  const struct floatformat ** double_format = 0;
+  const struct floatformat ** double_format = floatformats_ieee_double;
   int long_double_bit = 8*TARGET_CHAR_BIT;
-  const struct floatformat ** long_double_format = 0;
+  const struct floatformat ** long_double_format = floatformats_ieee_double;
   int wchar_bit = 4*TARGET_CHAR_BIT;
   int wchar_signed = -1;
   gdbarch_floatformat_for_type_ftype *floatformat_for_type = default_floatformat_for_type;
@@ -151,7 +151,7 @@ struct gdbarch
   gdbarch_single_step_through_delay_ftype *single_step_through_delay = nullptr;
   gdbarch_print_insn_ftype *print_insn = default_print_insn;
   gdbarch_skip_trampoline_code_ftype *skip_trampoline_code = generic_skip_trampoline_code;
-  const struct target_so_ops * so_ops = 0;
+  const struct target_so_ops * so_ops = &solib_target_so_ops;
   gdbarch_skip_solib_resolver_ftype *skip_solib_resolver = generic_skip_solib_resolver;
   gdbarch_in_solib_return_trampoline_ftype *in_solib_return_trampoline = generic_in_solib_return_trampoline;
   gdbarch_in_indirect_branch_thunk_ftype *in_indirect_branch_thunk = default_in_indirect_branch_thunk;
@@ -296,24 +296,14 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of long_bit, invalid_p == 0 */
   /* Skip verify of long_long_bit, invalid_p == 0 */
   /* 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)
@@ -419,8 +409,6 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of single_step_through_delay, has predicate.  */
   /* Skip verify of print_insn, invalid_p == 0 */
   /* 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 */
diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
index 8576ccfaf2b..2cb700309fd 100644
--- a/gdb/gdbarch_components.py
+++ b/gdb/gdbarch_components.py
@@ -199,7 +199,7 @@ useful).
 Value(
     type="const struct floatformat **",
     name="bfloat16_format",
-    postdefault="floatformats_bfloat16",
+    predefault="floatformats_bfloat16",
     printer="pformat (gdbarch, gdbarch->bfloat16_format)",
 )
 
@@ -213,7 +213,7 @@ Value(
 Value(
     type="const struct floatformat **",
     name="half_format",
-    postdefault="floatformats_ieee_half",
+    predefault="floatformats_ieee_half",
     printer="pformat (gdbarch, gdbarch->half_format)",
 )
 
@@ -227,7 +227,7 @@ Value(
 Value(
     type="const struct floatformat **",
     name="float_format",
-    postdefault="floatformats_ieee_single",
+    predefault="floatformats_ieee_single",
     printer="pformat (gdbarch, gdbarch->float_format)",
 )
 
@@ -241,7 +241,7 @@ Value(
 Value(
     type="const struct floatformat **",
     name="double_format",
-    postdefault="floatformats_ieee_double",
+    predefault="floatformats_ieee_double",
     printer="pformat (gdbarch, gdbarch->double_format)",
 )
 
@@ -255,7 +255,7 @@ Value(
 Value(
     type="const struct floatformat **",
     name="long_double_format",
-    postdefault="floatformats_ieee_double",
+    predefault="floatformats_ieee_double",
     printer="pformat (gdbarch, gdbarch->long_double_format)",
 )
 
@@ -1316,7 +1316,7 @@ Value(
     comment="Vtable of solib operations functions.",
     type="const struct target_so_ops *",
     name="so_ops",
-    postdefault="&solib_target_so_ops",
+    predefault="&solib_target_so_ops",
     printer="host_address_to_string (gdbarch->so_ops)",
 )
 
-- 
2.25.4

[PATCHv3 6/9] gdbarch: improve generation of validation in gdbarch getters

[Andrew Burgess]

We currently generate some validation code within the gdbarch getter
methods.

This commit adjusts the algorithm used to generate this validation
slightly to make the gdbarch.py code (I think) clearer; there's no
significant changes to what is generated.

The validation algorithm for gdbarch values is now:

  - If the Value has an 'invalid' field that is a string, use that for
    validation,

  - If the Value has its 'predicate' field set to true, then check the
    predicate returns true, this ensures the predicate has been
    called,

  - If the Value has its 'invalid' field set to True, or the Value has
    'postdefault' field, then check the fields has changed from its
    initial value,

  - Otherwise no validation is performed.

The only changes after this commit are:

  - Some comments change slightly, and

  - For 'gcore_bfd_target' and 'max_insn_length' we now validate by
    calling the predicate rather than checking the field value
    directly, the underlying check being performed is unchanged
    though.

There should be no user visible changes after this commit.
---
 gdb/gdbarch.c  | 18 +++++++++---------
 gdb/gdbarch.py | 16 +++++++---------
 2 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 064afd7c226..6018c632f91 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -1658,7 +1658,7 @@ int
 gdbarch_wchar_signed (struct gdbarch *gdbarch)
 {
   gdb_assert (gdbarch != NULL);
-  /* Check variable changed from pre-default.  */
+  /* Check variable changed from its initial value.  */
   gdb_assert (gdbarch->wchar_signed != -1);
   if (gdbarch_debug >= 2)
     gdb_printf (gdb_stdlog, "gdbarch_wchar_signed called\n");
@@ -1710,7 +1710,7 @@ int
 gdbarch_addr_bit (struct gdbarch *gdbarch)
 {
   gdb_assert (gdbarch != NULL);
-  /* Check variable changed from pre-default.  */
+  /* Check variable changed from its initial value.  */
   gdb_assert (gdbarch->addr_bit != 0);
   if (gdbarch_debug >= 2)
     gdb_printf (gdb_stdlog, "gdbarch_addr_bit called\n");
@@ -1728,7 +1728,7 @@ int
 gdbarch_dwarf2_addr_size (struct gdbarch *gdbarch)
 {
   gdb_assert (gdbarch != NULL);
-  /* Check variable changed from pre-default.  */
+  /* Check variable changed from its initial value.  */
   gdb_assert (gdbarch->dwarf2_addr_size != 0);
   if (gdbarch_debug >= 2)
     gdb_printf (gdb_stdlog, "gdbarch_dwarf2_addr_size called\n");
@@ -1746,7 +1746,7 @@ int
 gdbarch_char_signed (struct gdbarch *gdbarch)
 {
   gdb_assert (gdbarch != NULL);
-  /* Check variable changed from pre-default.  */
+  /* Check variable changed from its initial value.  */
   gdb_assert (gdbarch->char_signed != -1);
   if (gdbarch_debug >= 2)
     gdb_printf (gdb_stdlog, "gdbarch_char_signed called\n");
@@ -1901,7 +1901,7 @@ int
 gdbarch_num_regs (struct gdbarch *gdbarch)
 {
   gdb_assert (gdbarch != NULL);
-  /* Check variable changed from pre-default.  */
+  /* Check variable changed from its initial value.  */
   gdb_assert (gdbarch->num_regs != -1);
   if (gdbarch_debug >= 2)
     gdb_printf (gdb_stdlog, "gdbarch_num_regs called\n");
@@ -3919,8 +3919,8 @@ const char *
 gdbarch_gcore_bfd_target (struct gdbarch *gdbarch)
 {
   gdb_assert (gdbarch != NULL);
-  /* Check variable changed from pre-default.  */
-  gdb_assert (gdbarch->gcore_bfd_target != 0);
+  /* Check predicate was used.  */
+  gdb_assert (gdbarch_gcore_bfd_target_p (gdbarch));
   if (gdbarch_debug >= 2)
     gdb_printf (gdb_stdlog, "gdbarch_gcore_bfd_target called\n");
   return gdbarch->gcore_bfd_target;
@@ -3995,8 +3995,8 @@ ULONGEST
 gdbarch_max_insn_length (struct gdbarch *gdbarch)
 {
   gdb_assert (gdbarch != NULL);
-  /* Check variable changed from pre-default.  */
-  gdb_assert (gdbarch->max_insn_length != 0);
+  /* Check predicate was used.  */
+  gdb_assert (gdbarch_max_insn_length_p (gdbarch));
   if (gdbarch_debug >= 2)
     gdb_printf (gdb_stdlog, "gdbarch_max_insn_length called\n");
   return gdbarch->max_insn_length;
diff --git a/gdb/gdbarch.py b/gdb/gdbarch.py
index 7ff2cabe2e8..62592c1b13e 100755
--- a/gdb/gdbarch.py
+++ b/gdb/gdbarch.py
@@ -351,15 +351,13 @@ with open("gdbarch.c", "w") as f:
             if isinstance(c.invalid, str):
                 print("  /* Check variable is valid.  */", file=f)
                 print(f"  gdb_assert (!({c.invalid}));", file=f)
-            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
-                    )
+            elif c.predicate:
+                print("  /* Check predicate was used.  */", file=f)
+                print(f"  gdb_assert (gdbarch_{c.name}_p (gdbarch));", file=f)
+            elif c.invalid or c.postdefault is not None:
+                init_value = c.predefault or "0"
+                print("  /* Check variable changed from its initial value.  */", file=f)
+                print(f"  gdb_assert (gdbarch->{c.name} != {init_value});", file=f)
             else:
                 print(f"  /* Skip verify of {c.name}, invalid_p == 0 */", file=f)
             print("  if (gdbarch_debug >= 2)", file=f)
-- 
2.25.4

Re: [PATCHv3 6/9] gdbarch: improve generation of validation in gdbarch getters

[Simon Marchi]

> diff --git a/gdb/gdbarch.py b/gdb/gdbarch.py
> index 7ff2cabe2e8..62592c1b13e 100755
> --- a/gdb/gdbarch.py
> +++ b/gdb/gdbarch.py
> @@ -351,15 +351,13 @@ with open("gdbarch.c", "w") as f:
>              if isinstance(c.invalid, str):
>                  print("  /* Check variable is valid.  */", file=f)
>                  print(f"  gdb_assert (!({c.invalid}));", file=f)
> -            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
> -                    )
> +            elif c.predicate:
> +                print("  /* Check predicate was used.  */", file=f)
> +                print(f"  gdb_assert (gdbarch_{c.name}_p (gdbarch));", file=f)
> +            elif c.invalid or c.postdefault is not None:
> +                init_value = c.predefault or "0"
> +                print("  /* Check variable changed from its initial value.  */", file=f)
> +                print(f"  gdb_assert (gdbarch->{c.name} != {init_value});", file=f)
>              else:
>                  print(f"  /* Skip verify of {c.name}, invalid_p == 0 */", file=f)
>              print("  if (gdbarch_debug >= 2)", file=f)

I still have some trouble reading these snippets of if/elseif/.../else
and explaining the logic to myself.  But given that the generated
file doesn't change in a significant way shows that it's a safe change
in any case.

Simon

[PATCHv3 7/9] gdbarch: remove some unneeded predefault="0" from gdbarch_components.py

[Andrew Burgess]

I noticed that there are a bunch of 'predefault="0"' lines in
gdbarch_components.py, and that some (just some, not all) of these are
not needed.

The gdbarch is already zero initialized, but these lines seem to
exists so that we can know when to compare against "0" and when to
compare against "NULL".  At least, this seems to be useful in some
places in the generated code.

Specifically, if we remove the predefault="0" line from the
max_insn_length component then we end up generating a line like:

  gdb_assert (gdbarch->max_insn_length != NULL);

which doesn't compile as we compare a ULONGEST to NULL.

In this commit I remove all the predefault="0" lines that I claim are
obviously not needed.  These are lines for components that are not
Values (i.e. the component holds a function pointer anyway), or for
Value components that hold a pointer type, in which case using NULL is
fine.

The only changes after this commit are some fields that have nullptr
as their initial value, and gcore_bfd_target now compares to NULL not
0 in gdbarch_gcore_bfd_target_p, which, given the field is of type
'const char *', seems like an improvement.
---
 gdb/gdbarch.c             | 10 +++++-----
 gdb/gdbarch_components.py | 22 +---------------------
 2 files changed, 6 insertions(+), 26 deletions(-)

diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 6018c632f91..84f6a481885 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -88,7 +88,7 @@ struct gdbarch
   gdbarch_ecoff_reg_to_regnum_ftype *ecoff_reg_to_regnum = no_op_reg_to_regnum;
   gdbarch_sdb_reg_to_regnum_ftype *sdb_reg_to_regnum = no_op_reg_to_regnum;
   gdbarch_dwarf2_reg_to_regnum_ftype *dwarf2_reg_to_regnum = no_op_reg_to_regnum;
-  gdbarch_register_name_ftype *register_name = 0;
+  gdbarch_register_name_ftype *register_name = nullptr;
   gdbarch_register_type_ftype *register_type = nullptr;
   gdbarch_dummy_id_ftype *dummy_id = default_dummy_id;
   int deprecated_fp_regnum = -1;
@@ -115,12 +115,12 @@ struct gdbarch
   gdbarch_return_value_as_value_ftype *return_value_as_value = default_gdbarch_return_value;
   gdbarch_get_return_buf_addr_ftype *get_return_buf_addr = default_get_return_buf_addr;
   gdbarch_return_in_first_hidden_param_p_ftype *return_in_first_hidden_param_p = default_return_in_first_hidden_param_p;
-  gdbarch_skip_prologue_ftype *skip_prologue = 0;
+  gdbarch_skip_prologue_ftype *skip_prologue = nullptr;
   gdbarch_skip_main_prologue_ftype *skip_main_prologue = nullptr;
   gdbarch_skip_entrypoint_ftype *skip_entrypoint = nullptr;
-  gdbarch_inner_than_ftype *inner_than = 0;
+  gdbarch_inner_than_ftype *inner_than = nullptr;
   gdbarch_breakpoint_from_pc_ftype *breakpoint_from_pc = default_breakpoint_from_pc;
-  gdbarch_breakpoint_kind_from_pc_ftype *breakpoint_kind_from_pc = 0;
+  gdbarch_breakpoint_kind_from_pc_ftype *breakpoint_kind_from_pc = nullptr;
   gdbarch_sw_breakpoint_from_kind_ftype *sw_breakpoint_from_kind = NULL;
   gdbarch_breakpoint_kind_from_current_state_ftype *breakpoint_kind_from_current_state = default_breakpoint_kind_from_current_state;
   gdbarch_adjust_breakpoint_address_ftype *adjust_breakpoint_address = nullptr;
@@ -3912,7 +3912,7 @@ bool
 gdbarch_gcore_bfd_target_p (struct gdbarch *gdbarch)
 {
   gdb_assert (gdbarch != NULL);
-  return gdbarch->gcore_bfd_target != 0;
+  return gdbarch->gcore_bfd_target != NULL;
 }
 
 const char *
diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
index 2cb700309fd..57bbf8eb7b6 100644
--- a/gdb/gdbarch_components.py
+++ b/gdb/gdbarch_components.py
@@ -340,7 +340,6 @@ and if Dwarf versions < 4 need to be supported.
 """,
     type="int",
     name="dwarf2_addr_size",
-    predefault="0",
     postdefault="gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT",
 )
 
@@ -567,7 +566,6 @@ should never return nullptr.
     params=[("int", "regnr")],
     param_checks=["regnr >= 0", "regnr < gdbarch_num_cooked_regs (gdbarch)"],
     result_checks=["result != nullptr"],
-    predefault="0",
     invalid=True,
 )
 
@@ -915,7 +913,6 @@ Method(
     type="CORE_ADDR",
     name="skip_prologue",
     params=[("CORE_ADDR", "ip")],
-    predefault="0",
     invalid=True,
 )
 
@@ -950,7 +947,6 @@ Function(
     type="int",
     name="inner_than",
     params=[("CORE_ADDR", "lhs"), ("CORE_ADDR", "rhs")],
-    predefault="0",
     invalid=True,
 )
 
@@ -969,7 +965,6 @@ Return the breakpoint kind for this target based on *PCPTR.
     type="int",
     name="breakpoint_kind_from_pc",
     params=[("CORE_ADDR *", "pcptr")],
-    predefault="0",
     invalid=True,
 )
 
@@ -1673,7 +1668,6 @@ BFD target to use when generating a core file.
     type="const char *",
     name="gcore_bfd_target",
     predicate=True,
-    predefault="0",
     printer="pstring (gdbarch->gcore_bfd_target)",
     invalid=True,
 )
@@ -1697,7 +1691,6 @@ significant bit of the pfn for pointers to virtual member functions.
 """,
     type="int",
     name="vbit_in_delta",
-    predefault="0",
     invalid=False,
 )
 
@@ -1718,8 +1711,8 @@ The maximum length of an instruction on this architecture in bytes.
 """,
     type="ULONGEST",
     name="max_insn_length",
-    predicate=True,
     predefault="0",
+    predicate=True,
     invalid=True,
 )
 
@@ -1991,7 +1984,6 @@ The filename of the XML syscall for this architecture.
 """,
     type="const char *",
     name="xml_syscall_file",
-    predefault="0",
     invalid=False,
     printer="pstring (gdbarch->xml_syscall_file)",
 )
@@ -2002,7 +1994,6 @@ Information about system calls from this architecture
 """,
     type="struct syscalls_info *",
     name="syscalls_info",
-    predefault="0",
     invalid=False,
     printer="host_address_to_string (gdbarch->syscalls_info)",
 )
@@ -2020,7 +2011,6 @@ in this case, this prefix would be the character `$'.
 """,
     type="const char *const *",
     name="stap_integer_prefixes",
-    predefault="0",
     invalid=False,
     printer="pstring_list (gdbarch->stap_integer_prefixes)",
 )
@@ -2032,7 +2022,6 @@ on the architecture's assembly.
 """,
     type="const char *const *",
     name="stap_integer_suffixes",
-    predefault="0",
     invalid=False,
     printer="pstring_list (gdbarch->stap_integer_suffixes)",
 )
@@ -2049,7 +2038,6 @@ in this case, this prefix would be the character `%'.
 """,
     type="const char *const *",
     name="stap_register_prefixes",
-    predefault="0",
     invalid=False,
     printer="pstring_list (gdbarch->stap_register_prefixes)",
 )
@@ -2061,7 +2049,6 @@ the architecture's assembly.
 """,
     type="const char *const *",
     name="stap_register_suffixes",
-    predefault="0",
     invalid=False,
     printer="pstring_list (gdbarch->stap_register_suffixes)",
 )
@@ -2081,7 +2068,6 @@ displacement, e.g., `4(%eax)' on x86.
 """,
     type="const char *const *",
     name="stap_register_indirection_prefixes",
-    predefault="0",
     invalid=False,
     printer="pstring_list (gdbarch->stap_register_indirection_prefixes)",
 )
@@ -2101,7 +2087,6 @@ displacement, e.g., `4(%eax)' on x86.
 """,
     type="const char *const *",
     name="stap_register_indirection_suffixes",
-    predefault="0",
     invalid=False,
     printer="pstring_list (gdbarch->stap_register_indirection_suffixes)",
 )
@@ -2117,7 +2102,6 @@ register would be represented as `r10' internally.
 """,
     type="const char *",
     name="stap_gdb_register_prefix",
-    predefault="0",
     invalid=False,
     printer="pstring (gdbarch->stap_gdb_register_prefix)",
 )
@@ -2128,7 +2112,6 @@ Suffix used to name a register using GDB's nomenclature.
 """,
     type="const char *",
     name="stap_gdb_register_suffix",
-    predefault="0",
     invalid=False,
     printer="pstring (gdbarch->stap_gdb_register_suffix)",
 )
@@ -2610,7 +2593,6 @@ Functions for allowing a target to modify its disassembler options.
 """,
     type="const char *",
     name="disassembler_options_implicit",
-    predefault="0",
     invalid=False,
     printer="pstring (gdbarch->disassembler_options_implicit)",
 )
@@ -2618,7 +2600,6 @@ Functions for allowing a target to modify its disassembler options.
 Value(
     type="char **",
     name="disassembler_options",
-    predefault="0",
     invalid=False,
     printer="pstring_ptr (gdbarch->disassembler_options)",
 )
@@ -2626,7 +2607,6 @@ Value(
 Value(
     type="const disasm_options_and_args_t *",
     name="valid_disassembler_options",
-    predefault="0",
     invalid=False,
     printer="host_address_to_string (gdbarch->valid_disassembler_options)",
 )
-- 
2.25.4

[PATCHv3 8/9] gdbarch: make invalid=True the default for all Components

[Andrew Burgess]

This commit switches the default value for the 'invalid' field from
False to True.  All components that previous set the invalid field to
True explicitly have had the field removed.

I think that True is a good choice for the default, this means that we
now get the validity checks by default, and if anyone adds a new
Component they need to make a choice to add an 'invalid=False' line
and disable the validation.

The flip side of this is that 'invalid=False' seems to be far more
common than 'invalid=True'.  But I don't see a huge problem with this,
we shouldn't be aiming to reduce our typing, rather we should choose
based on which is least likely to introduce bugs.  I think assuming
that we should do a validity check will achieve that.

Some additional components need to have an 'invalid=False' line added
to their definition, these are components that have a predefault
value, which is sufficient; the tdep code doesn't need to replace this
value if it doesn't want to.

Without adding the 'invalid=False' these components would be
considered to be invalid if they have not changed from their
predefault value -- but the predefault is fine.

There's no change in the generated code after this commit, so there
will be no user visible changes after this commit.
---
 gdb/gdbarch_components.py | 27 +++++++++++++++------------
 gdb/gdbarch_types.py      |  6 +++---
 2 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
index 57bbf8eb7b6..6cdf15b3910 100644
--- a/gdb/gdbarch_components.py
+++ b/gdb/gdbarch_components.py
@@ -81,10 +81,11 @@
 # this string is the expression that should evaluate to true when the
 # field is invalid.
 #
-# Otherwise, if "invalid" is True, then the generic validation
-# function is used: the field is considered invalid it still contains
-# its default value.  This validation is what is used within the _p
-# predicate function if the field has "predicate" set to True.
+# Otherwise, if "invalid" is True (the default), then the generic
+# validation function is used: the field is considered invalid it
+# still contains its default value.  This validation is what is used
+# within the _p predicate function if the field has "predicate" set to
+# True.
 #
 # Function and Method share:
 #
@@ -201,6 +202,7 @@ Value(
     name="bfloat16_format",
     predefault="floatformats_bfloat16",
     printer="pformat (gdbarch, gdbarch->bfloat16_format)",
+    invalid=False,
 )
 
 Value(
@@ -215,6 +217,7 @@ Value(
     name="half_format",
     predefault="floatformats_ieee_half",
     printer="pformat (gdbarch, gdbarch->half_format)",
+    invalid=False,
 )
 
 Value(
@@ -229,6 +232,7 @@ Value(
     name="float_format",
     predefault="floatformats_ieee_single",
     printer="pformat (gdbarch, gdbarch->float_format)",
+    invalid=False,
 )
 
 Value(
@@ -243,6 +247,7 @@ Value(
     name="double_format",
     predefault="floatformats_ieee_double",
     printer="pformat (gdbarch, gdbarch->double_format)",
+    invalid=False,
 )
 
 Value(
@@ -257,6 +262,7 @@ Value(
     name="long_double_format",
     predefault="floatformats_ieee_double",
     printer="pformat (gdbarch, gdbarch->long_double_format)",
+    invalid=False,
 )
 
 Value(
@@ -278,6 +284,7 @@ One if `wchar_t' is signed, zero if unsigned.
     name="wchar_signed",
     predefault="-1",
     postdefault="1",
+    invalid=False,
 )
 
 Method(
@@ -320,6 +327,7 @@ addr_bit is the size of a target address as represented in gdb
     name="addr_bit",
     predefault="0",
     postdefault="gdbarch_ptr_bit (gdbarch)",
+    invalid=False,
 )
 
 Value(
@@ -341,6 +349,7 @@ and if Dwarf versions < 4 need to be supported.
     type="int",
     name="dwarf2_addr_size",
     postdefault="gdbarch_ptr_bit (gdbarch) / TARGET_CHAR_BIT",
+    invalid=False,
 )
 
 Value(
@@ -351,6 +360,7 @@ One if `char' acts like `signed char', zero if `unsigned char'.
     name="char_signed",
     predefault="-1",
     postdefault="1",
+    invalid=False,
 )
 
 Function(
@@ -423,7 +433,6 @@ Value(
     type="int",
     name="num_regs",
     predefault="-1",
-    invalid=True,
 )
 
 Value(
@@ -566,7 +575,6 @@ should never return nullptr.
     params=[("int", "regnr")],
     param_checks=["regnr >= 0", "regnr < gdbarch_num_cooked_regs (gdbarch)"],
     result_checks=["result != nullptr"],
-    invalid=True,
 )
 
 Method(
@@ -578,7 +586,6 @@ use "register_type".
     type="struct type *",
     name="register_type",
     params=[("int", "reg_nr")],
-    invalid=True,
 )
 
 Method(
@@ -913,7 +920,6 @@ Method(
     type="CORE_ADDR",
     name="skip_prologue",
     params=[("CORE_ADDR", "ip")],
-    invalid=True,
 )
 
 Method(
@@ -947,7 +953,6 @@ Function(
     type="int",
     name="inner_than",
     params=[("CORE_ADDR", "lhs"), ("CORE_ADDR", "rhs")],
-    invalid=True,
 )
 
 Method(
@@ -965,7 +970,6 @@ Return the breakpoint kind for this target based on *PCPTR.
     type="int",
     name="breakpoint_kind_from_pc",
     params=[("CORE_ADDR *", "pcptr")],
-    invalid=True,
 )
 
 Method(
@@ -1313,6 +1317,7 @@ Value(
     name="so_ops",
     predefault="&solib_target_so_ops",
     printer="host_address_to_string (gdbarch->so_ops)",
+    invalid=False,
 )
 
 Method(
@@ -1669,7 +1674,6 @@ BFD target to use when generating a core file.
     name="gcore_bfd_target",
     predicate=True,
     printer="pstring (gdbarch->gcore_bfd_target)",
-    invalid=True,
 )
 
 Value(
@@ -1713,7 +1717,6 @@ The maximum length of an instruction on this architecture in bytes.
     name="max_insn_length",
     predefault="0",
     predicate=True,
-    invalid=True,
 )
 
 Method(
diff --git a/gdb/gdbarch_types.py b/gdb/gdbarch_types.py
index d40851d127f..910ec641bdc 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: Union[bool, str] = False,
+        invalid: Union[bool, str] = True,
         params: Optional[List[Tuple[str, str]]] = None,
         param_checks: Optional[List[str]] = None,
         result_checks: Optional[List[str]] = None,
@@ -97,7 +97,7 @@ class Value(Component):
         predicate: bool = False,
         predefault: Optional[str] = None,
         postdefault: Optional[str] = None,
-        invalid: Union[bool, str] = False,
+        invalid: Union[bool, str] = True,
         printer: Optional[str] = None,
     ):
         super().__init__(
@@ -125,7 +125,7 @@ class Function(Component):
         predicate: bool = False,
         predefault: Optional[str] = None,
         postdefault: Optional[str] = None,
-        invalid: Union[bool, str] = False,
+        invalid: Union[bool, str] = True,
         printer: Optional[str] = None,
         param_checks: Optional[List[str]] = None,
         result_checks: Optional[List[str]] = None,
-- 
2.25.4

[PATCHv3 9/9] gdb: add gdbarch::displaced_step_buffer_length

[Andrew Burgess]

The gdbarch::max_insn_length field is used mostly to support displaced
stepping; it controls the size of the buffers allocated for the
displaced-step instruction, and is also used when first copying the
instruction, and later, when fixing up the instruction, in order to
read in and parse the instruction being stepped.

However, it has started to be used in other places in GDB, for
example, it's used in the Python disassembler API, and it is used on
amd64 as part of branch-tracing instruction classification.

The problem is that the value assigned to max_insn_length is not
always the maximum instruction length, but sometimes is a multiple of
that length, as required to support displaced stepping, see rs600,
ARM, and AArch64 for examples of this.

It seems to me that we are overloading the meaning of the
max_insn_length field, and I think that could potentially lead to
confusion.

I propose that we add a new gdbarch field,
gdbarch::displaced_step_buffer_length, this new field will do
exactly what it says on the tin; represent the required displaced step
buffer size.  The max_insn_length field can then do exactly what it
claims to do; represent the maximum length of a single instruction.

As some architectures (e.g. i386, and amd64) only require their
displaced step buffers to be a single instruction in size, I propose
that the default for displaced_step_buffer_length will be the
value of max_insn_length.  Architectures than need more buffer space
can then override this default as needed.

I've updated all architectures to setup the new field if appropriate,
and I've audited all calls to gdbarch_max_insn_length and switched to
gdbarch_displaced_step_buffer_length where appropriate.

There should be no user visible changes after this commit.

Approved-By: Simon Marchi <simon.marchi@efficios.com>
---
 gdb/aarch64-linux-tdep.c  |  4 +++-
 gdb/arm-tdep.c            |  4 +++-
 gdb/displaced-stepping.c  |  6 +++---
 gdb/gdbarch-gen.h         | 12 ++++++++++--
 gdb/gdbarch.c             | 26 ++++++++++++++++++++++++++
 gdb/gdbarch_components.py | 18 ++++++++++++++++--
 gdb/linux-tdep.c          |  2 +-
 gdb/rs6000-tdep.c         |  6 ++++--
 8 files changed, 66 insertions(+), 12 deletions(-)

diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index 0000b498f89..b183a3c9a38 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -2240,7 +2240,9 @@ aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   set_gdbarch_get_syscall_number (gdbarch, aarch64_linux_get_syscall_number);
 
   /* Displaced stepping.  */
-  set_gdbarch_max_insn_length (gdbarch, 4 * AARCH64_DISPLACED_MODIFIED_INSNS);
+  set_gdbarch_max_insn_length (gdbarch, 4);
+  set_gdbarch_displaced_step_buffer_length
+    (gdbarch, 4 * AARCH64_DISPLACED_MODIFIED_INSNS);
   set_gdbarch_displaced_step_copy_insn (gdbarch,
 					aarch64_displaced_step_copy_insn);
   set_gdbarch_displaced_step_fixup (gdbarch, aarch64_displaced_step_fixup);
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index b64c21ce68f..3a018e85983 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -10662,7 +10662,9 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   /* Note: for displaced stepping, this includes the breakpoint, and one word
      of additional scratch space.  This setting isn't used for anything beside
      displaced stepping at present.  */
-  set_gdbarch_max_insn_length (gdbarch, 4 * ARM_DISPLACED_MODIFIED_INSNS);
+  set_gdbarch_displaced_step_buffer_length
+    (gdbarch, 4 * ARM_DISPLACED_MODIFIED_INSNS);
+  set_gdbarch_max_insn_length (gdbarch, 4);
 
   /* This should be low enough for everything.  */
   tdep->lowest_pc = 0x20;
diff --git a/gdb/displaced-stepping.c b/gdb/displaced-stepping.c
index 06b32a80f6a..9f98ea8c35b 100644
--- a/gdb/displaced-stepping.c
+++ b/gdb/displaced-stepping.c
@@ -55,7 +55,7 @@ displaced_step_buffers::prepare (thread_info *thread, CORE_ADDR &displaced_pc)
   regcache *regcache = get_thread_regcache (thread);
   const address_space *aspace = regcache->aspace ();
   gdbarch *arch = regcache->arch ();
-  ULONGEST len = gdbarch_max_insn_length (arch);
+  ULONGEST len = gdbarch_displaced_step_buffer_length (arch);
 
   /* Search for an unused buffer.  */
   displaced_step_buffer *buffer = nullptr;
@@ -243,7 +243,7 @@ displaced_step_buffers::finish (gdbarch *arch, thread_info *thread,
      below.  */
   thread->inf->displaced_step_state.unavailable = false;
 
-  ULONGEST len = gdbarch_max_insn_length (arch);
+  ULONGEST len = gdbarch_displaced_step_buffer_length (arch);
 
   /* Restore memory of the buffer.  */
   write_memory_ptid (thread->ptid, buffer->addr,
@@ -302,7 +302,7 @@ displaced_step_buffers::restore_in_ptid (ptid_t ptid)
 
       regcache *regcache = get_thread_regcache (buffer.current_thread);
       gdbarch *arch = regcache->arch ();
-      ULONGEST len = gdbarch_max_insn_length (arch);
+      ULONGEST len = gdbarch_displaced_step_buffer_length (arch);
 
       write_memory_ptid (ptid, buffer.addr, buffer.saved_copy.data (), len);
 
diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
index ddb97f60315..76d12a15317 100644
--- a/gdb/gdbarch-gen.h
+++ b/gdb/gdbarch-gen.h
@@ -1039,8 +1039,8 @@ extern void set_gdbarch_max_insn_length (struct gdbarch *gdbarch, ULONGEST max_i
    see the comments in infrun.c.
 
    The TO area is only guaranteed to have space for
-   gdbarch_max_insn_length (arch) bytes, so this function must not
-   write more bytes than that to that area.
+   gdbarch_displaced_step_buffer_length (arch) octets, so this
+   function must not write more octets than that to this area.
 
    If you do not provide this function, GDB assumes that the
    architecture does not support displaced stepping.
@@ -1122,6 +1122,14 @@ typedef void (gdbarch_displaced_step_restore_all_in_ptid_ftype) (inferior *paren
 extern void gdbarch_displaced_step_restore_all_in_ptid (struct gdbarch *gdbarch, inferior *parent_inf, ptid_t child_ptid);
 extern void set_gdbarch_displaced_step_restore_all_in_ptid (struct gdbarch *gdbarch, gdbarch_displaced_step_restore_all_in_ptid_ftype *displaced_step_restore_all_in_ptid);
 
+/* The maximum length in octets required for a displaced-step instruction
+   buffer.  By default this will be the same as gdbarch::max_insn_length,
+   but should be overridden for architectures that might expand a
+   displaced-step instruction to multiple replacement instructions. */
+
+extern ULONGEST gdbarch_displaced_step_buffer_length (struct gdbarch *gdbarch);
+extern void set_gdbarch_displaced_step_buffer_length (struct gdbarch *gdbarch, ULONGEST displaced_step_buffer_length);
+
 /* Relocate an instruction to execute at a different address.  OLDLOC
    is the address in the inferior memory where the instruction to
    relocate is currently at.  On input, TO points to the destination
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 84f6a481885..b4763aa6bf4 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -192,6 +192,7 @@ struct gdbarch
   gdbarch_displaced_step_finish_ftype *displaced_step_finish = NULL;
   gdbarch_displaced_step_copy_insn_closure_by_addr_ftype *displaced_step_copy_insn_closure_by_addr = nullptr;
   gdbarch_displaced_step_restore_all_in_ptid_ftype *displaced_step_restore_all_in_ptid = nullptr;
+  ULONGEST displaced_step_buffer_length = 0;
   gdbarch_relocate_instruction_ftype *relocate_instruction = NULL;
   gdbarch_overlay_update_ftype *overlay_update = nullptr;
   gdbarch_core_read_description_ftype *core_read_description = nullptr;
@@ -451,6 +452,10 @@ verify_gdbarch (struct gdbarch *gdbarch)
     log.puts ("\n\tdisplaced_step_finish");
   /* Skip verify of displaced_step_copy_insn_closure_by_addr, has predicate.  */
   /* Skip verify of displaced_step_restore_all_in_ptid, invalid_p == 0 */
+  if (gdbarch->displaced_step_buffer_length == 0)
+    gdbarch->displaced_step_buffer_length = gdbarch->max_insn_length;
+  if (gdbarch->displaced_step_buffer_length < gdbarch->max_insn_length)
+    log.puts ("\n\tdisplaced_step_buffer_length");
   /* Skip verify of relocate_instruction, has predicate.  */
   /* Skip verify of overlay_update, has predicate.  */
   /* Skip verify of core_read_description, has predicate.  */
@@ -1109,6 +1114,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
   gdb_printf (file,
 	      "gdbarch_dump: displaced_step_restore_all_in_ptid = <%s>\n",
 	      host_address_to_string (gdbarch->displaced_step_restore_all_in_ptid));
+  gdb_printf (file,
+	      "gdbarch_dump: displaced_step_buffer_length = %s\n",
+	      plongest (gdbarch->displaced_step_buffer_length));
   gdb_printf (file,
 	      "gdbarch_dump: gdbarch_relocate_instruction_p() = %d\n",
 	      gdbarch_relocate_instruction_p (gdbarch));
@@ -4157,6 +4165,24 @@ set_gdbarch_displaced_step_restore_all_in_ptid (struct gdbarch *gdbarch,
   gdbarch->displaced_step_restore_all_in_ptid = displaced_step_restore_all_in_ptid;
 }
 
+ULONGEST
+gdbarch_displaced_step_buffer_length (struct gdbarch *gdbarch)
+{
+  gdb_assert (gdbarch != NULL);
+  /* Check variable is valid.  */
+  gdb_assert (!(gdbarch->displaced_step_buffer_length < gdbarch->max_insn_length));
+  if (gdbarch_debug >= 2)
+    gdb_printf (gdb_stdlog, "gdbarch_displaced_step_buffer_length called\n");
+  return gdbarch->displaced_step_buffer_length;
+}
+
+void
+set_gdbarch_displaced_step_buffer_length (struct gdbarch *gdbarch,
+					  ULONGEST displaced_step_buffer_length)
+{
+  gdbarch->displaced_step_buffer_length = displaced_step_buffer_length;
+}
+
 bool
 gdbarch_relocate_instruction_p (struct gdbarch *gdbarch)
 {
diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
index 6cdf15b3910..92c501d2a72 100644
--- a/gdb/gdbarch_components.py
+++ b/gdb/gdbarch_components.py
@@ -1735,8 +1735,8 @@ For a general explanation of displaced stepping and how GDB uses it,
 see the comments in infrun.c.
 
 The TO area is only guaranteed to have space for
-gdbarch_max_insn_length (arch) bytes, so this function must not
-write more bytes than that to that area.
+gdbarch_displaced_step_buffer_length (arch) octets, so this
+function must not write more octets than that to this area.
 
 If you do not provide this function, GDB assumes that the
 architecture does not support displaced stepping.
@@ -1844,6 +1844,20 @@ contents of all displaced step buffers in the child's address space.
     invalid=False,
 )
 
+Value(
+    comment="""
+The maximum length in octets required for a displaced-step instruction
+buffer.  By default this will be the same as gdbarch::max_insn_length,
+but should be overridden for architectures that might expand a
+displaced-step instruction to multiple replacement instructions.
+""",
+    type="ULONGEST",
+    name="displaced_step_buffer_length",
+    predefault="0",
+    postdefault="gdbarch->max_insn_length",
+    invalid="gdbarch->displaced_step_buffer_length < gdbarch->max_insn_length",
+)
+
 Method(
     comment="""
 Relocate an instruction to execute at a different address.  OLDLOC
diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index e6ce13a1c67..3eaa5f33584 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -2603,7 +2603,7 @@ linux_displaced_step_prepare (gdbarch *arch, thread_info *thread,
 	 at DISP_STEP_BUF_ADDR.  They are all of size BUF_LEN.  */
       CORE_ADDR disp_step_buf_addr
 	= linux_displaced_step_location (thread->inf->gdbarch);
-      int buf_len = gdbarch_max_insn_length (arch);
+      int buf_len = gdbarch_displaced_step_buffer_length (arch);
 
       linux_gdbarch_data *gdbarch_data = get_linux_gdbarch_data (arch);
       gdb_assert (gdbarch_data->num_disp_step_buffers > 0);
diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 104515de030..8468bb53cec 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -889,7 +889,8 @@ ppc_displaced_step_copy_insn (struct gdbarch *gdbarch,
 			      CORE_ADDR from, CORE_ADDR to,
 			      struct regcache *regs)
 {
-  size_t len = gdbarch_max_insn_length (gdbarch);
+  size_t len = gdbarch_displaced_step_buffer_length (gdbarch);
+  gdb_assert (len > PPC_INSN_SIZE);
   std::unique_ptr<ppc_displaced_step_copy_insn_closure> closure
     (new ppc_displaced_step_copy_insn_closure (len));
   gdb_byte *buf = closure->buf.data ();
@@ -8363,8 +8364,9 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_displaced_step_finish (gdbarch, ppc_displaced_step_finish);
   set_gdbarch_displaced_step_restore_all_in_ptid
     (gdbarch, ppc_displaced_step_restore_all_in_ptid);
+  set_gdbarch_displaced_step_buffer_length (gdbarch, 2 * PPC_INSN_SIZE);
 
-  set_gdbarch_max_insn_length (gdbarch, 2 * PPC_INSN_SIZE);
+  set_gdbarch_max_insn_length (gdbarch, PPC_INSN_SIZE);
 
   /* Hook in ABI-specific overrides, if they have been registered.  */
   info.target_desc = tdesc;
-- 
2.25.4

Re: [PATCHv3 0/9] Add new gdbarch::displaced_step_buffer_length field

[Simon Marchi]

On 3/10/23 13:43, Andrew Burgess via Gdb-patches wrote:
> Changes in V3:
> 
>   - Have now run 'black' on each patch in this series, so everything
>     should be formatted correctly,
> 
>   - Have fixed the out of date names in the last patch of this series.
>     Everything buildds and runs fine for me now,
> 
>   - Have removed the out of date text from the commit message in patch
>     #3,
> 
>   - Have fixed the missing comma Tom spotted in patch #4,
> 
>   - I have NOT removed the comments Simon pointed out in patch #4.
>     Removing these would require changing the generated code for more
>     Components than I already change in this commit.  And so, I think,
>     if those comments are to go that would require a separate patch.
> 
>     That said, we do generate validation code within the getters for
>     many components, so I think having a comment in the components
>     where we don't generate validation makes sense.  So for me, I
>     think the comments do add value, I'd suggest we should keep them.
> 
>   - And the big one.  I've changed the 'invalid' default from False to
>     True.  I know Simon suggested that False was the correct choice,
>     but I actually think that True makes more sense.  I'd rather we
>     assume we should generate the validity checks and require than new
>     components explicitly choose to not have that check, rather than
>     just assuming we shouldn't check.
> 
>     However, in order to get to the default True state I ended up
>     having to fix some other issues.  And, so, incase people really
>     would rather see False as the default, I've left the patch which
>     changes to default True as the penultimate patch in the series.
>     If you feel really strongly that False is best, I can just drop
>     the patch that switches over to use True.  Just let me know.
> 
> Let me know what you think,
> 
> Thanks,
> Andrew

Thanks Andrew, it all LGTM:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

Re: [PATCHv3 0/9] Add new gdbarch::displaced_step_buffer_length field

[Andrew Burgess]

Simon Marchi <simark@simark.ca> writes:

> On 3/10/23 13:43, Andrew Burgess via Gdb-patches wrote:
>> Changes in V3:
>> 
>>   - Have now run 'black' on each patch in this series, so everything
>>     should be formatted correctly,
>> 
>>   - Have fixed the out of date names in the last patch of this series.
>>     Everything buildds and runs fine for me now,
>> 
>>   - Have removed the out of date text from the commit message in patch
>>     #3,
>> 
>>   - Have fixed the missing comma Tom spotted in patch #4,
>> 
>>   - I have NOT removed the comments Simon pointed out in patch #4.
>>     Removing these would require changing the generated code for more
>>     Components than I already change in this commit.  And so, I think,
>>     if those comments are to go that would require a separate patch.
>> 
>>     That said, we do generate validation code within the getters for
>>     many components, so I think having a comment in the components
>>     where we don't generate validation makes sense.  So for me, I
>>     think the comments do add value, I'd suggest we should keep them.
>> 
>>   - And the big one.  I've changed the 'invalid' default from False to
>>     True.  I know Simon suggested that False was the correct choice,
>>     but I actually think that True makes more sense.  I'd rather we
>>     assume we should generate the validity checks and require than new
>>     components explicitly choose to not have that check, rather than
>>     just assuming we shouldn't check.
>> 
>>     However, in order to get to the default True state I ended up
>>     having to fix some other issues.  And, so, incase people really
>>     would rather see False as the default, I've left the patch which
>>     changes to default True as the penultimate patch in the series.
>>     If you feel really strongly that False is best, I can just drop
>>     the patch that switches over to use True.  Just let me know.
>> 
>> Let me know what you think,
>> 
>> Thanks,
>> Andrew
>
> Thanks Andrew, it all LGTM:
>
> Approved-By: Simon Marchi <simon.marchi@efficios.com>

Pushed.

Thanks,
Andrew