[PATCH 0/2] Some changes to the gdbarch*.py scripts

[PATCH 0/2] Some changes to the gdbarch*.py scripts

[Andrew Burgess]

A couple of changes to the gdb/gdbarch*.py script:

  #1: Remove an unused predicate function, and

  #2: Extend the verify_gdbarch function to cover more cases.

Thanks,
Andrew

---

Andrew Burgess (2):
  gdb/gdbarch: remove the predicate function for gdbarch_register_type
  gdb/gdbarch: compare some fields against 0 verify_gdbarch

 gdb/gdbarch-components.py | 13 ++++++-------
 gdb/gdbarch-gen.h         |  2 --
 gdb/gdbarch.c             | 22 +++++++++++-----------
 gdb/gdbarch.py            | 10 ++++++++++
 4 files changed, 27 insertions(+), 20 deletions(-)

-- 
2.25.4

[PATCH 1/2] gdb/gdbarch: remove the predicate function for gdbarch_register_type

[Andrew Burgess]

I don't believe that the gdbarch_register_type_p predicate is called
anywhere in GDB, and the gdbarch_register_type function is called
without checking the gdbarch_register_type_p predicate function
everywhere it is used, for example in
init_regcache_descr (regcache.c).

My claim is that the gdbarch_register_type function is required for
every architecture, and GDB will not work if this function is not
supplied.

And so, in this commit, I remove the 'predicate=True' from
gdbarch-components.py for the 'register_type' field, and regenerate
the gdbarch files.

There should be no user visible changes after this commit.
---
 gdb/gdbarch-components.py |  1 -
 gdb/gdbarch-gen.h         |  2 --
 gdb/gdbarch.c             | 11 -----------
 3 files changed, 14 deletions(-)

diff --git a/gdb/gdbarch-components.py b/gdb/gdbarch-components.py
index bc09d35f9de..aa0b3682593 100644
--- a/gdb/gdbarch-components.py
+++ b/gdb/gdbarch-components.py
@@ -574,7 +574,6 @@ use "register_type".
     type="struct type *",
     name="register_type",
     params=[("int", "reg_nr")],
-    predicate=True,
     invalid=True,
 )
 
diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
index b7beb73d36d..7a8721328ab 100644
--- a/gdb/gdbarch-gen.h
+++ b/gdb/gdbarch-gen.h
@@ -296,8 +296,6 @@ extern void set_gdbarch_register_name (struct gdbarch *gdbarch, gdbarch_register
    the register cache should call this function directly; others should
    use "register_type". */
 
-extern bool gdbarch_register_type_p (struct gdbarch *gdbarch);
-
 typedef struct type * (gdbarch_register_type_ftype) (struct gdbarch *gdbarch, int reg_nr);
 extern struct type * gdbarch_register_type (struct gdbarch *gdbarch, int reg_nr);
 extern void set_gdbarch_register_type (struct gdbarch *gdbarch, gdbarch_register_type_ftype *register_type);
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 55dd602b27f..28e1fbc2c71 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -443,7 +443,6 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of dwarf2_reg_to_regnum, invalid_p == 0 */
   if (gdbarch->register_name == 0)
     log.puts ("\n\tregister_name");
-  /* Skip verify of register_type, has predicate.  */
   /* Skip verify of dummy_id, invalid_p == 0 */
   /* Skip verify of deprecated_fp_regnum, invalid_p == 0 */
   /* Skip verify of push_dummy_call, has predicate.  */
@@ -780,9 +779,6 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
   fprintf_filtered (file,
                       "gdbarch_dump: register_name = <%s>\n",
                       host_address_to_string (gdbarch->register_name));
-  fprintf_filtered (file,
-                      "gdbarch_dump: gdbarch_register_type_p() = %d\n",
-                      gdbarch_register_type_p (gdbarch));
   fprintf_filtered (file,
                       "gdbarch_dump: register_type = <%s>\n",
                       host_address_to_string (gdbarch->register_type));
@@ -2219,13 +2215,6 @@ set_gdbarch_register_name (struct gdbarch *gdbarch,
   gdbarch->register_name = register_name;
 }
 
-bool
-gdbarch_register_type_p (struct gdbarch *gdbarch)
-{
-  gdb_assert (gdbarch != NULL);
-  return gdbarch->register_type != NULL;
-}
-
 struct type *
 gdbarch_register_type (struct gdbarch *gdbarch, int reg_nr)
 {
-- 
2.25.4

Re: [PATCH 1/2] gdb/gdbarch: remove the predicate function for gdbarch_register_type

[Tom Tromey]

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

Andrew> I don't believe that the gdbarch_register_type_p predicate is called
Andrew> anywhere in GDB, and the gdbarch_register_type function is called
Andrew> without checking the gdbarch_register_type_p predicate function
Andrew> everywhere it is used, for example in
Andrew> init_regcache_descr (regcache.c).

Andrew> My claim is that the gdbarch_register_type function is required for
Andrew> every architecture, and GDB will not work if this function is not
Andrew> supplied.

Andrew> And so, in this commit, I remove the 'predicate=True' from
Andrew> gdbarch-components.py for the 'register_type' field, and regenerate
Andrew> the gdbarch files.

Looks ok to me.

thanks,
Tom

[PATCH 2/2] gdb/gdbarch: compare some fields against 0 verify_gdbarch

[Andrew Burgess]

After the previous commit, which removes the predicate function
gdbarch_register_type_p, I noticed that the gdbarch->register_type
field was not checked at in the verify_gdbarch function.

More than not being checked, the field wasn't mentioned at all.

I find this strange, I would expect that every field would at least be
mentioned - we already generate comments for some fields saying that
this field is _not_ being checked, so the fact that this field isn't
being checked looks (to me), like this field is somehow slipping
through the cracks.

The comment at the top of gdbarch-components.py tries to explain how
the validation is done.  I didn't understand this comment completely,
but, I think this final sentence:

  "Otherwise, the check is done against 0 (really NULL for function
  pointers, but same idea)."

Means that, if non of the other cases apply, then the field should be
checked against 0, with 0 indicating that the field is invalid (was
not set by the tdep code).  However, this is clearly not being done.

Looking in gdbarch.py at the code to generate verify_gdbarch we do
find that there is a case that is not handled, the case where the
'invalid' field is set true True, but non of the other cases apply.

In this commit I propose two changes:

 1. Handle the case where the 'invalid' field of a property is set to
 True, this should perform a check for the field of gdbarch still
 being set to 0, and

 2. If the if/else series that generates verify_gdbarch doesn't handle
 a property then we should raise an exception.  This means that if a
 property is added which isn't handled, we should no longer silently
 ignore it.

After doing this, I re-generated the gdbarch files and saw that the
following gdbarch fields now had new validation checks:

  register_type
  believe_pcc_promotion
  register_to_value
  value_to_register
  frame_red_zone_size
  displaced_step_restore_all_in_ptid
  solib_symbols_extension

Looking at how these are currently set in the various -tdep.c files, I
believe the only one of these that is required to be set for all
architectures is the register_type field.

And so, for all of the other fields, I've changed the property
definition on gdbarch-components.py, setting the 'invalid' field to
False.

Now, after re-generation, the register_type field is checked against
0, thus an architecture that doesn't set_gdbarch_register_type will
now fail during validation.  For all the other fields we skip the
validation, in which case, it is find for an architecture to not set
this field.

My expectation is that there should be no user visible changes after
this commit.  Certainly for all fields except register_type, all I've
really done is cause some extra comments to be generated, so I think
that's clearly fine.

For the register_type field, my claim is that any architecture that
didn't provide this would fail when creating its register cache, and I
couldn't spot an architecture that doesn't provide this hook.  As
such, I think this change should be fine too.
---
 gdb/gdbarch-components.py | 12 ++++++------
 gdb/gdbarch.c             | 11 +++++++++++
 gdb/gdbarch.py            | 10 ++++++++++
 3 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/gdb/gdbarch-components.py b/gdb/gdbarch-components.py
index aa0b3682593..c820ddae764 100644
--- a/gdb/gdbarch-components.py
+++ b/gdb/gdbarch-components.py
@@ -740,7 +740,7 @@ FRAME corresponds to the longjmp frame.
 Value(
     type="int",
     name="believe_pcc_promotion",
-    invalid=True,
+    invalid=False,
 )
 
 Method(
@@ -762,7 +762,7 @@ Function(
         ("int *", "optimizedp"),
         ("int *", "unavailablep"),
     ],
-    invalid=True,
+    invalid=False,
 )
 
 Function(
@@ -774,7 +774,7 @@ Function(
         ("struct type *", "type"),
         ("const gdb_byte *", "buf"),
     ],
-    invalid=True,
+    invalid=False,
 )
 
 Method(
@@ -1086,7 +1086,7 @@ Method(
 Value(
     type="int",
     name="frame_red_zone_size",
-    invalid=True,
+    invalid=False,
 )
 
 Method(
@@ -1767,7 +1767,7 @@ contents of all displaced step buffers in the child's address space.
     type="void",
     name="displaced_step_restore_all_in_ptid",
     params=[("inferior *", "parent_inf"), ("ptid_t", "child_ptid")],
-    invalid=True,
+    invalid=False,
 )
 
 Method(
@@ -2298,7 +2298,7 @@ compared to the names of the files GDB should load for debug info.
 """,
     type="const char *",
     name="solib_symbols_extension",
-    invalid=True,
+    invalid=False,
     printer="pstring (gdbarch->solib_symbols_extension)",
 )
 
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 28e1fbc2c71..9fdcf1505fe 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -443,6 +443,8 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of dwarf2_reg_to_regnum, invalid_p == 0 */
   if (gdbarch->register_name == 0)
     log.puts ("\n\tregister_name");
+  if (gdbarch->register_type == 0)
+    log.puts ("\n\tregister_type");
   /* Skip verify of dummy_id, invalid_p == 0 */
   /* Skip verify of deprecated_fp_regnum, invalid_p == 0 */
   /* Skip verify of push_dummy_call, has predicate.  */
@@ -456,7 +458,10 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of cannot_fetch_register, invalid_p == 0 */
   /* Skip verify of cannot_store_register, invalid_p == 0 */
   /* Skip verify of get_longjmp_target, has predicate.  */
+  /* Skip verify of believe_pcc_promotion, invalid_p == 0 */
   /* Skip verify of convert_register_p, invalid_p == 0 */
+  /* Skip verify of register_to_value, invalid_p == 0 */
+  /* Skip verify of value_to_register, invalid_p == 0 */
   /* Skip verify of value_from_register, invalid_p == 0 */
   /* Skip verify of pointer_to_address, invalid_p == 0 */
   /* Skip verify of address_to_pointer, invalid_p == 0 */
@@ -488,6 +493,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of frame_num_args, has predicate.  */
   /* Skip verify of frame_align, has predicate.  */
   /* Skip verify of stabs_argument_has_addr, invalid_p == 0 */
+  /* Skip verify of frame_red_zone_size, invalid_p == 0 */
   /* Skip verify of convert_from_func_ptr_addr, invalid_p == 0 */
   /* Skip verify of addr_bits_remove, invalid_p == 0 */
   /* Skip verify of significant_addr_bit, invalid_p == 0 */
@@ -538,6 +544,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
   if ((! gdbarch->displaced_step_finish) != (! gdbarch->displaced_step_prepare))
     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 */
   /* Skip verify of relocate_instruction, has predicate.  */
   /* Skip verify of overlay_update, has predicate.  */
   /* Skip verify of core_read_description, has predicate.  */
@@ -573,6 +580,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of guess_tracepoint_registers, invalid_p == 0 */
   /* Skip verify of auto_charset, invalid_p == 0 */
   /* Skip verify of auto_wide_charset, invalid_p == 0 */
+  /* Skip verify of solib_symbols_extension, invalid_p == 0 */
   /* Skip verify of has_dos_based_file_system, invalid_p == 0 */
   /* Skip verify of gen_return_address, invalid_p == 0 */
   /* Skip verify of info_proc, has predicate.  */
@@ -2485,6 +2493,7 @@ int
 gdbarch_believe_pcc_promotion (struct gdbarch *gdbarch)
 {
   gdb_assert (gdbarch != NULL);
+  /* Skip verify of believe_pcc_promotion, invalid_p == 0 */
   if (gdbarch_debug >= 2)
     fprintf_unfiltered (gdb_stdlog, "gdbarch_believe_pcc_promotion called\n");
   return gdbarch->believe_pcc_promotion;
@@ -3091,6 +3100,7 @@ int
 gdbarch_frame_red_zone_size (struct gdbarch *gdbarch)
 {
   gdb_assert (gdbarch != NULL);
+  /* Skip verify of frame_red_zone_size, invalid_p == 0 */
   if (gdbarch_debug >= 2)
     fprintf_unfiltered (gdb_stdlog, "gdbarch_frame_red_zone_size called\n");
   return gdbarch->frame_red_zone_size;
@@ -4822,6 +4832,7 @@ const char *
 gdbarch_solib_symbols_extension (struct gdbarch *gdbarch)
 {
   gdb_assert (gdbarch != NULL);
+  /* Skip verify of solib_symbols_extension, invalid_p == 0 */
   if (gdbarch_debug >= 2)
     fprintf_unfiltered (gdb_stdlog, "gdbarch_solib_symbols_extension called\n");
   return gdbarch->solib_symbols_extension;
diff --git a/gdb/gdbarch.py b/gdb/gdbarch.py
index 3bd6400355e..c89d19ccbcf 100755
--- a/gdb/gdbarch.py
+++ b/gdb/gdbarch.py
@@ -360,6 +360,16 @@ with open("gdbarch.c", "w") as 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)
+        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")
     print("  if (!log.empty ())", file=f)
     print("    internal_error (__FILE__, __LINE__,", file=f)
     print("""		    _("verify_gdbarch: the following are invalid ...%s"),""", file=f)
-- 
2.25.4

Re: [PATCH 2/2] gdb/gdbarch: compare some fields against 0 verify_gdbarch

[Tom Tromey]

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

Andrew> The comment at the top of gdbarch-components.py tries to explain how
Andrew> the validation is done.  I didn't understand this comment completely,
Andrew> but, I think this final sentence:

I wrote it and now I don't really understand it either.
I guess I wasn't being as clear as I thought I was.

Andrew>  1. Handle the case where the 'invalid' field of a property is set to
Andrew>  True, this should perform a check for the field of gdbarch still
Andrew>  being set to 0, and

Andrew>  2. If the if/else series that generates verify_gdbarch doesn't handle
Andrew>  a property then we should raise an exception.  This means that if a
Andrew>  property is added which isn't handled, we should no longer silently
Andrew>  ignore it.

Looks good.

thanks,
Tom

Re: [PATCH 2/2] gdb/gdbarch: compare some fields against 0 verify_gdbarch

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Andrew> The comment at the top of gdbarch-components.py tries to explain how
> Andrew> the validation is done.  I didn't understand this comment completely,
> Andrew> but, I think this final sentence:
>
> I wrote it and now I don't really understand it either.
> I guess I wasn't being as clear as I thought I was.
>
> Andrew>  1. Handle the case where the 'invalid' field of a property is set to
> Andrew>  True, this should perform a check for the field of gdbarch still
> Andrew>  being set to 0, and
>
> Andrew>  2. If the if/else series that generates verify_gdbarch doesn't handle
> Andrew>  a property then we should raise an exception.  This means that if a
> Andrew>  property is added which isn't handled, we should no longer silently
> Andrew>  ignore it.
>
> Looks good.

Thanks, I pushed both these patches.

Andrew