From a5118a18db47c8ccaf4995fbb62e2a1eb377fa3e Mon Sep 17 00:00:00 2001 From: Andrew Burgess Date: Thu, 10 Mar 2022 11:18:18 +0000 Subject: [PATCH] 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. --- 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.30.2