[binutils-gdb] gdb/gdbarch: compare some fields against 0 verify_gdbarch

[binutils-gdb] gdb/gdbarch: compare some fields against 0 verify_gdbarch

[Andrew Burgess]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=a5118a18db47c8ccaf4995fbb62e2a1eb377fa3e

commit a5118a18db47c8ccaf4995fbb62e2a1eb377fa3e
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Thu Mar 10 11:18:18 2022 +0000

    gdb/gdbarch: compare some fields against 0 verify_gdbarch
    
    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.

Diff:
---
 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)