diff mbox series

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

Message ID e972a5d4ebc3b7b9ba966827923fd46f2acac433.1678473293.git.aburgess@redhat.com
State New
Headers
Series Add new gdbarch::displaced_step_buffer_length field |

Commit Message

Andrew Burgess March 10, 2023, 6:43 p.m. UTC 
  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(-)

Comments

Simon Marchi March 11, 2023, 2:57 a.m. UTC | #1 
> 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
diff mbox series

Patch

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)