[PATCH 0/4] Use checked_static_cast for breakpoint casts

[PATCH 0/4] Use checked_static_cast for breakpoint casts

[Tom Tromey]

While reviewing a patch, I noticed some unchecked casts to watchpoint.
This series replaces all the unchecked breakpoint casts I could find
with checked casts.

Regression tested on x86-64 Fedora 36.

---
Tom Tromey (4):
      Use gdb::checked_static_cast for watchpoints
      Use gdb::checked_static_cast for tracepoints
      Use gdb::checked_static_cast for code_breakpoint
      Use gdb::checked_static_cast for catchpoints

 gdb/ada-lang.c             |   3 +-
 gdb/break-catch-sig.c      |   8 +-
 gdb/break-catch-syscall.c  |   4 +-
 gdb/breakpoint.c           | 337 +++++++++++++++++++++++----------------------
 gdb/elfread.c              |   2 +-
 gdb/guile/scm-breakpoint.c |   2 +-
 gdb/mi/mi-cmd-break.c      |  10 +-
 gdb/python/py-breakpoint.c |   2 +-
 gdb/remote.c               |  11 +-
 gdb/target-debug.h         |   2 +
 gdb/target-delegates.c     |  16 +--
 gdb/target.c               |   2 +-
 gdb/target.h               |   4 +-
 gdb/tracefile-tfile.c      |   4 +-
 gdb/tracepoint.c           |  28 ++--
 gdb/tracepoint.h           |   4 +-
 16 files changed, 232 insertions(+), 207 deletions(-)
---
base-commit: 0e0edacca5f34621f85e93a22ee31c6810b5941c
change-id: 20230915-watchpoint-casts-d43c55a7b13e

Best regards,
-- 
Tom Tromey <tromey@adacore.com>

[PATCH 1/4] Use gdb::checked_static_cast for watchpoints

[Tom Tromey]

This replaces some casts to 'watchpoint *' with checked_static_cast.
In one spot, an unnecessary block is also removed.
---
 gdb/breakpoint.c           | 298 +++++++++++++++++++++++----------------------
 gdb/guile/scm-breakpoint.c |   2 +-
 gdb/python/py-breakpoint.c |   2 +-
 3 files changed, 153 insertions(+), 149 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index c429af455ff..c0d86f38c73 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5249,7 +5249,7 @@ watchpoint_check (bpstat *bs)
 
   /* BS is built from an existing struct breakpoint.  */
   gdb_assert (bs->breakpoint_at != NULL);
-  b = (struct watchpoint *) bs->breakpoint_at;
+  b = gdb::checked_static_cast<struct watchpoint *> (bs->breakpoint_at);
 
   /* If this is a local watchpoint, we only want to check if the
      watchpoint frame is in scope if the current thread is the thread
@@ -5410,158 +5410,155 @@ bpstat_check_watchpoint (bpstat *bs)
   /* BS is built for existing struct breakpoint.  */
   bl = bs->bp_location_at.get ();
   gdb_assert (bl != NULL);
-  b = (struct watchpoint *) bs->breakpoint_at;
-  gdb_assert (b != NULL);
-
-    {
-      bool must_check_value = false;
-
-      if (b->type == bp_watchpoint)
-	/* For a software watchpoint, we must always check the
-	   watched value.  */
-	must_check_value = true;
-      else if (b->watchpoint_triggered == watch_triggered_yes)
-	/* We have a hardware watchpoint (read, write, or access)
-	   and the target earlier reported an address watched by
-	   this watchpoint.  */
-	must_check_value = true;
-      else if (b->watchpoint_triggered == watch_triggered_unknown
-	       && b->type == bp_hardware_watchpoint)
-	/* We were stopped by a hardware watchpoint, but the target could
-	   not report the data address.  We must check the watchpoint's
-	   value.  Access and read watchpoints are out of luck; without
-	   a data address, we can't figure it out.  */
-	must_check_value = true;
+  b = gdb::checked_static_cast<struct watchpoint *> (bs->breakpoint_at);
+
+  bool must_check_value = false;
+
+  if (b->type == bp_watchpoint)
+    /* For a software watchpoint, we must always check the
+       watched value.  */
+    must_check_value = true;
+  else if (b->watchpoint_triggered == watch_triggered_yes)
+    /* We have a hardware watchpoint (read, write, or access)
+       and the target earlier reported an address watched by
+       this watchpoint.  */
+    must_check_value = true;
+  else if (b->watchpoint_triggered == watch_triggered_unknown
+	   && b->type == bp_hardware_watchpoint)
+    /* We were stopped by a hardware watchpoint, but the target could
+       not report the data address.  We must check the watchpoint's
+       value.  Access and read watchpoints are out of luck; without
+       a data address, we can't figure it out.  */
+    must_check_value = true;
+
+  if (must_check_value)
+    {
+      wp_check_result e;
 
-      if (must_check_value)
+      try
+	{
+	  e = watchpoint_check (bs);
+	}
+      catch (const gdb_exception_error &ex)
 	{
-	  wp_check_result e;
+	  exception_fprintf (gdb_stderr, ex,
+			     "Error evaluating expression "
+			     "for watchpoint %d\n",
+			     b->number);
 
-	  try
-	    {
-	      e = watchpoint_check (bs);
-	    }
-	  catch (const gdb_exception_error &ex)
+	  SWITCH_THRU_ALL_UIS ()
 	    {
-	      exception_fprintf (gdb_stderr, ex,
-				 "Error evaluating expression "
-				 "for watchpoint %d\n",
-				 b->number);
-
-	      SWITCH_THRU_ALL_UIS ()
-		{
-		  gdb_printf (_("Watchpoint %d deleted.\n"),
-			      b->number);
-		}
-	      watchpoint_del_at_next_stop (b);
-	      e = WP_DELETED;
+	      gdb_printf (_("Watchpoint %d deleted.\n"),
+			  b->number);
 	    }
+	  watchpoint_del_at_next_stop (b);
+	  e = WP_DELETED;
+	}
 
-	  switch (e)
+      switch (e)
+	{
+	case WP_DELETED:
+	  /* We've already printed what needs to be printed.  */
+	  bs->print_it = print_it_done;
+	  /* Stop.  */
+	  break;
+	case WP_IGNORE:
+	  bs->print_it = print_it_noop;
+	  bs->stop = false;
+	  break;
+	case WP_VALUE_CHANGED:
+	  if (b->type == bp_read_watchpoint)
 	    {
-	    case WP_DELETED:
-	      /* We've already printed what needs to be printed.  */
-	      bs->print_it = print_it_done;
-	      /* Stop.  */
-	      break;
-	    case WP_IGNORE:
-	      bs->print_it = print_it_noop;
-	      bs->stop = false;
-	      break;
-	    case WP_VALUE_CHANGED:
-	      if (b->type == bp_read_watchpoint)
+	      /* There are two cases to consider here:
+
+		 1. We're watching the triggered memory for reads.
+		 In that case, trust the target, and always report
+		 the watchpoint hit to the user.  Even though
+		 reads don't cause value changes, the value may
+		 have changed since the last time it was read, and
+		 since we're not trapping writes, we will not see
+		 those, and as such we should ignore our notion of
+		 old value.
+
+		 2. We're watching the triggered memory for both
+		 reads and writes.  There are two ways this may
+		 happen:
+
+		 2.1. This is a target that can't break on data
+		 reads only, but can break on accesses (reads or
+		 writes), such as e.g., x86.  We detect this case
+		 at the time we try to insert read watchpoints.
+
+		 2.2. Otherwise, the target supports read
+		 watchpoints, but, the user set an access or write
+		 watchpoint watching the same memory as this read
+		 watchpoint.
+
+		 If we're watching memory writes as well as reads,
+		 ignore watchpoint hits when we find that the
+		 value hasn't changed, as reads don't cause
+		 changes.  This still gives false positives when
+		 the program writes the same value to memory as
+		 what there was already in memory (we will confuse
+		 it for a read), but it's much better than
+		 nothing.  */
+
+	      int other_write_watchpoint = 0;
+
+	      if (bl->watchpoint_type == hw_read)
 		{
-		  /* There are two cases to consider here:
-
-		     1. We're watching the triggered memory for reads.
-		     In that case, trust the target, and always report
-		     the watchpoint hit to the user.  Even though
-		     reads don't cause value changes, the value may
-		     have changed since the last time it was read, and
-		     since we're not trapping writes, we will not see
-		     those, and as such we should ignore our notion of
-		     old value.
-
-		     2. We're watching the triggered memory for both
-		     reads and writes.  There are two ways this may
-		     happen:
-
-		     2.1. This is a target that can't break on data
-		     reads only, but can break on accesses (reads or
-		     writes), such as e.g., x86.  We detect this case
-		     at the time we try to insert read watchpoints.
-
-		     2.2. Otherwise, the target supports read
-		     watchpoints, but, the user set an access or write
-		     watchpoint watching the same memory as this read
-		     watchpoint.
-
-		     If we're watching memory writes as well as reads,
-		     ignore watchpoint hits when we find that the
-		     value hasn't changed, as reads don't cause
-		     changes.  This still gives false positives when
-		     the program writes the same value to memory as
-		     what there was already in memory (we will confuse
-		     it for a read), but it's much better than
-		     nothing.  */
-
-		  int other_write_watchpoint = 0;
-
-		  if (bl->watchpoint_type == hw_read)
-		    {
-		      for (breakpoint &other_b : all_breakpoints ())
-			if (other_b.type == bp_hardware_watchpoint
-			    || other_b.type == bp_access_watchpoint)
+		  for (breakpoint &other_b : all_breakpoints ())
+		    if (other_b.type == bp_hardware_watchpoint
+			|| other_b.type == bp_access_watchpoint)
+		      {
+			watchpoint &other_w =
+			  gdb::checked_static_cast<watchpoint &> (other_b);
+
+			if (other_w.watchpoint_triggered
+			    == watch_triggered_yes)
 			  {
-			    watchpoint &other_w =
-			      gdb::checked_static_cast<watchpoint &> (other_b);
-
-			    if (other_w.watchpoint_triggered
-				== watch_triggered_yes)
-			      {
-				other_write_watchpoint = 1;
-				break;
-			      }
+			    other_write_watchpoint = 1;
+			    break;
 			  }
-		    }
-
-		  if (other_write_watchpoint
-		      || bl->watchpoint_type == hw_access)
-		    {
-		      /* We're watching the same memory for writes,
-			 and the value changed since the last time we
-			 updated it, so this trap must be for a write.
-			 Ignore it.  */
-		      bs->print_it = print_it_noop;
-		      bs->stop = false;
-		    }
+		      }
 		}
-	      break;
-	    case WP_VALUE_NOT_CHANGED:
-	      if (b->type == bp_hardware_watchpoint
-		  || b->type == bp_watchpoint)
+
+	      if (other_write_watchpoint
+		  || bl->watchpoint_type == hw_access)
 		{
-		  /* Don't stop: write watchpoints shouldn't fire if
-		     the value hasn't changed.  */
+		  /* We're watching the same memory for writes,
+		     and the value changed since the last time we
+		     updated it, so this trap must be for a write.
+		     Ignore it.  */
 		  bs->print_it = print_it_noop;
 		  bs->stop = false;
 		}
-	      /* Stop.  */
-	      break;
-	    default:
-	      /* Can't happen.  */
-	      break;
 	    }
+	  break;
+	case WP_VALUE_NOT_CHANGED:
+	  if (b->type == bp_hardware_watchpoint
+	      || b->type == bp_watchpoint)
+	    {
+	      /* Don't stop: write watchpoints shouldn't fire if
+		 the value hasn't changed.  */
+	      bs->print_it = print_it_noop;
+	      bs->stop = false;
+	    }
+	  /* Stop.  */
+	  break;
+	default:
+	  /* Can't happen.  */
+	  break;
 	}
-      else	/* !must_check_value */
-	{
-	  /* This is a case where some watchpoint(s) triggered, but
-	     not at the address of this watchpoint, or else no
-	     watchpoint triggered after all.  So don't print
-	     anything for this watchpoint.  */
-	  bs->print_it = print_it_noop;
-	  bs->stop = false;
-	}
+    }
+  else	/* !must_check_value */
+    {
+      /* This is a case where some watchpoint(s) triggered, but
+	 not at the address of this watchpoint, or else no
+	 watchpoint triggered after all.  So don't print
+	 anything for this watchpoint.  */
+      bs->print_it = print_it_noop;
+      bs->stop = false;
     }
 }
 
@@ -5625,7 +5622,7 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread)
 
   if (is_watchpoint (b))
     {
-      struct watchpoint *w = (struct watchpoint *) b;
+      struct watchpoint *w = gdb::checked_static_cast<struct watchpoint *> (b);
 
       cond = w->cond_exp.get ();
     }
@@ -5644,7 +5641,7 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread)
       scoped_value_mark mark;
 
       if (is_watchpoint (b))
-	w = (struct watchpoint *) b;
+	w = gdb::checked_static_cast<struct watchpoint *> (b);
       else
 	w = NULL;
 
@@ -5794,7 +5791,8 @@ build_bpstat_chain (const address_space *aspace, CORE_ADDR bp_addr,
 	     iteration.  */
 	  if (b.type == bp_watchpoint_scope && b.related_breakpoint != &b)
 	    {
-	      struct watchpoint *w = (struct watchpoint *) b.related_breakpoint;
+	      struct watchpoint *w
+		= gdb::checked_static_cast<struct watchpoint *> (b.related_breakpoint);
 
 	      w->watchpoint_triggered = watch_triggered_yes;
 	    }
@@ -5918,7 +5916,8 @@ bpstat_stop_status (const address_space *aspace,
 	  && bs->breakpoint_at
 	  && is_hardware_watchpoint (bs->breakpoint_at))
 	{
-	  struct watchpoint *w = (struct watchpoint *) bs->breakpoint_at;
+	  struct watchpoint *w
+	    = gdb::checked_static_cast<struct watchpoint *> (bs->breakpoint_at);
 
 	  update_watchpoint (w, false /* don't reparse.  */);
 	  need_remove_insert = 1;
@@ -6568,7 +6567,8 @@ print_one_breakpoint_location (struct breakpoint *b,
     {
       if (is_watchpoint (b))
 	{
-	  struct watchpoint *w = (struct watchpoint *) b;
+	  struct watchpoint *w
+	    = gdb::checked_static_cast<struct watchpoint *> (b);
 
 	  /* Field 4, the address, is omitted (which makes the columns
 	     not line up too nicely with the headers, but the effect
@@ -6828,7 +6828,8 @@ print_one_breakpoint_location (struct breakpoint *b,
     {
       if (is_watchpoint (b))
 	{
-	  struct watchpoint *w = (struct watchpoint *) b;
+	  struct watchpoint *w
+	    = gdb::checked_static_cast<struct watchpoint *> (b);
 
 	  uiout->field_string ("original-location", w->exp_string.get ());
 	}
@@ -7257,8 +7258,10 @@ static bool
 watchpoint_locations_match (const struct bp_location *loc1,
 			    const struct bp_location *loc2)
 {
-  struct watchpoint *w1 = (struct watchpoint *) loc1->owner;
-  struct watchpoint *w2 = (struct watchpoint *) loc2->owner;
+  struct watchpoint *w1
+    = gdb::checked_static_cast<struct watchpoint *> (loc1->owner);
+  struct watchpoint *w2
+    = gdb::checked_static_cast<struct watchpoint *> (loc2->owner);
 
   /* Both of them must exist.  */
   gdb_assert (w1 != NULL);
@@ -12604,9 +12607,9 @@ delete_breakpoint (struct breakpoint *bpt)
       struct watchpoint *w;
 
       if (bpt->type == bp_watchpoint_scope)
-	w = (struct watchpoint *) bpt->related_breakpoint;
+	w = gdb::checked_static_cast<struct watchpoint *> (bpt->related_breakpoint);
       else if (bpt->related_breakpoint->type == bp_watchpoint_scope)
-	w = (struct watchpoint *) bpt;
+	w = gdb::checked_static_cast<struct watchpoint *> (bpt);
       else
 	w = NULL;
       if (w != NULL)
@@ -13770,7 +13773,8 @@ enable_breakpoint_disp (struct breakpoint *bpt, enum bpdisp disposition,
 
       try
 	{
-	  struct watchpoint *w = (struct watchpoint *) bpt;
+	  struct watchpoint *w
+	    = gdb::checked_static_cast<struct watchpoint *> (bpt);
 
 	  orig_enable_state = bpt->enable_state;
 	  bpt->enable_state = bp_enabled;
diff --git a/gdb/guile/scm-breakpoint.c b/gdb/guile/scm-breakpoint.c
index 59254646bcc..541e1037a5e 100644
--- a/gdb/guile/scm-breakpoint.c
+++ b/gdb/guile/scm-breakpoint.c
@@ -895,7 +895,7 @@ gdbscm_breakpoint_expression (SCM self)
   if (!is_watchpoint (bp_smob->bp))
     return SCM_BOOL_F;
 
-  wp = (struct watchpoint *) bp_smob->bp;
+  wp = gdb::checked_static_cast<struct watchpoint *> (bp_smob->bp);
 
   const char *str = wp->exp_string.get ();
   if (! str)
diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index cb064515690..c6d23b65768 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -556,7 +556,7 @@ bppy_get_expression (PyObject *self, void *closure)
   if (!is_watchpoint (obj->bp))
     Py_RETURN_NONE;
 
-  wp = (struct watchpoint *) obj->bp;
+  wp = gdb::checked_static_cast<struct watchpoint *> (obj->bp);
 
   str = wp->exp_string.get ();
   if (! str)

-- 
2.40.1

[PATCH 2/4] Use gdb::checked_static_cast for tracepoints

[Tom Tromey]

This replaces some casts to 'tracepoint *' with checked_static_cast.
Some functions are changed to accept a 'tracepoint *' now, for better
type safety.
---
 gdb/breakpoint.c       | 33 +++++++++++++++++++--------------
 gdb/mi/mi-cmd-break.c  | 10 +++++++---
 gdb/remote.c           | 11 +++++------
 gdb/target-debug.h     |  2 ++
 gdb/target-delegates.c | 16 ++++++++--------
 gdb/target.c           |  2 +-
 gdb/target.h           |  4 ++--
 gdb/tracefile-tfile.c  |  4 ++--
 gdb/tracepoint.c       | 28 +++++++++++++++++-----------
 gdb/tracepoint.h       |  4 ++--
 10 files changed, 65 insertions(+), 49 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index c0d86f38c73..134ad76ce9a 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1433,7 +1433,7 @@ validate_commands_for_breakpoint (struct breakpoint *b,
 {
   if (is_tracepoint (b))
     {
-      struct tracepoint *t = (struct tracepoint *) b;
+      struct tracepoint *t = gdb::checked_static_cast<struct tracepoint *> (b);
       struct command_line *c;
       struct command_line *while_stepping = 0;
 
@@ -1471,7 +1471,7 @@ validate_commands_for_breakpoint (struct breakpoint *b,
 		while_stepping = c;
 	    }
 
-	  validate_actionline (c->line, b);
+	  validate_actionline (c->line, t);
 	}
       if (while_stepping)
 	{
@@ -1645,7 +1645,9 @@ commands_command_1 (const char *arg, int from_tty,
 
 	       auto do_validate = [=] (const char *line)
 				  {
-				    validate_actionline (line, b);
+				    struct tracepoint *t
+				      = gdb::checked_static_cast<struct tracepoint *> (b);
+				    validate_actionline (line, t);
 				  };
 	       gdb::function_view<void (const char *)> validator;
 	       if (is_tracepoint (b))
@@ -6762,7 +6764,7 @@ print_one_breakpoint_location (struct breakpoint *b,
 
   if (!part_of_multiple && is_tracepoint (b))
     {
-      struct tracepoint *tp = (struct tracepoint *) b;
+      struct tracepoint *tp = gdb::checked_static_cast<struct tracepoint *> (b);
 
       if (tp->traceframe_usage)
 	{
@@ -6794,7 +6796,7 @@ print_one_breakpoint_location (struct breakpoint *b,
 
   if (is_tracepoint (b))
     {
-      struct tracepoint *t = (struct tracepoint *) b;
+      struct tracepoint *t = gdb::checked_static_cast<struct tracepoint *> (b);
 
       if (!part_of_multiple && t->pass_count)
 	{
@@ -12811,9 +12813,8 @@ ambiguous_names_p (const bp_location_range &locs)
    precisely because it confuses tools).  */
 
 static struct symtab_and_line
-update_static_tracepoint (struct breakpoint *b, struct symtab_and_line sal)
+update_static_tracepoint (struct tracepoint *tp, struct symtab_and_line sal)
 {
-  struct tracepoint *tp = (struct tracepoint *) b;
   struct static_tracepoint_marker marker;
   CORE_ADDR pc;
 
@@ -12825,7 +12826,7 @@ update_static_tracepoint (struct breakpoint *b, struct symtab_and_line sal)
     {
       if (tp->static_trace_marker_id != marker.str_id)
 	warning (_("static tracepoint %d changed probed marker from %s to %s"),
-		 b->number, tp->static_trace_marker_id.c_str (),
+		 tp->number, tp->static_trace_marker_id.c_str (),
 		 marker.str_id.c_str ());
 
       tp->static_trace_marker_id = std::move (marker.str_id);
@@ -12856,7 +12857,7 @@ update_static_tracepoint (struct breakpoint *b, struct symtab_and_line sal)
 
 	  warning (_("marker for static tracepoint %d (%s) not "
 		     "found at previous line number"),
-		   b->number, tp->static_trace_marker_id.c_str ());
+		   tp->number, tp->static_trace_marker_id.c_str ());
 
 	  symtab_and_line sal2 = find_pc_line (tpmarker->address, 0);
 	  sym = find_pc_sect_function (tpmarker->address, NULL);
@@ -12882,17 +12883,17 @@ update_static_tracepoint (struct breakpoint *b, struct symtab_and_line sal)
 	  uiout->field_signed ("line", sal2.line);
 	  uiout->text ("\n");
 
-	  b->first_loc ().line_number = sal2.line;
-	  b->first_loc ().symtab = sym != NULL ? sal2.symtab : NULL;
+	  tp->first_loc ().line_number = sal2.line;
+	  tp->first_loc ().symtab = sym != NULL ? sal2.symtab : NULL;
 
 	  std::unique_ptr<explicit_location_spec> els
 	    (new explicit_location_spec ());
 	  els->source_filename
 	    = xstrdup (symtab_to_filename_for_display (sal2.symtab));
-	  els->line_offset.offset = b->first_loc ().line_number;
+	  els->line_offset.offset = tp->first_loc ().line_number;
 	  els->line_offset.sign = LINE_OFFSET_NONE;
 
-	  b->locspec = std::move (els);
+	  tp->locspec = std::move (els);
 
 	  /* Might be nice to check if function changed, and warn if
 	     so.  */
@@ -13153,7 +13154,11 @@ code_breakpoint::location_spec_to_sals (location_spec *locspec,
 	}
 
       if (type == bp_static_tracepoint)
-	sals[0] = update_static_tracepoint (this, sals[0]);
+	{
+	  struct tracepoint *t
+	    = gdb::checked_static_cast<struct tracepoint *> (this);
+	  sals[0] = update_static_tracepoint (t, sals[0]);
+	}
 
       *found = 1;
     }
diff --git a/gdb/mi/mi-cmd-break.c b/gdb/mi/mi-cmd-break.c
index 975bc1c5dde..2294ea3723b 100644
--- a/gdb/mi/mi-cmd-break.c
+++ b/gdb/mi/mi-cmd-break.c
@@ -585,11 +585,15 @@ mi_cmd_break_commands (const char *command, const char *const *argv, int argc)
       };
 
   if (is_tracepoint (b))
-    break_command = read_command_lines_1 (reader, 1,
-					  [=] (const char *line)
+    {
+      struct tracepoint *t
+	= gdb::checked_static_cast<struct tracepoint *> (b);
+      break_command = read_command_lines_1 (reader, 1,
+					    [=] (const char *line)
 					    {
-					      validate_actionline (line, b);
+					      validate_actionline (line, t);
 					    });
+    }
   else
     break_command = read_command_lines_1 (reader, 1, 0);
 
diff --git a/gdb/remote.c b/gdb/remote.c
index ba81c5b0b6f..38b4145be30 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -911,7 +911,7 @@ class remote_target : public process_stratum_target
 
   int get_trace_status (struct trace_status *ts) override;
 
-  void get_tracepoint_status (struct breakpoint *tp, struct uploaded_tp *utp)
+  void get_tracepoint_status (struct tracepoint *tp, struct uploaded_tp *utp)
     override;
 
   void trace_stop () override;
@@ -13256,7 +13256,7 @@ remote_target::download_tracepoint (struct bp_location *loc)
   std::vector<std::string> stepping_actions;
   char *pkt;
   struct breakpoint *b = loc->owner;
-  struct tracepoint *t = (struct tracepoint *) b;
+  struct tracepoint *t = gdb::checked_static_cast<struct tracepoint *> (b);
   struct remote_state *rs = get_remote_state ();
   int ret;
   const char *err_msg = _("Tracepoint packet too large for target.");
@@ -13675,12 +13675,11 @@ remote_target::get_trace_status (struct trace_status *ts)
 }
 
 void
-remote_target::get_tracepoint_status (struct breakpoint *bp,
+remote_target::get_tracepoint_status (struct tracepoint *tp,
 				      struct uploaded_tp *utp)
 {
   struct remote_state *rs = get_remote_state ();
   char *reply;
-  struct tracepoint *tp = (struct tracepoint *) bp;
   size_t size = get_remote_packet_size ();
 
   if (tp)
@@ -13700,7 +13699,7 @@ remote_target::get_tracepoint_status (struct breakpoint *bp,
 	  if (reply && *reply)
 	    {
 	      if (*reply == 'V')
-		parse_tracepoint_status (reply + 1, bp, utp);
+		parse_tracepoint_status (reply + 1, tp, utp);
 	    }
 	}
     }
@@ -13715,7 +13714,7 @@ remote_target::get_tracepoint_status (struct breakpoint *bp,
       if (reply && *reply)
 	{
 	  if (*reply == 'V')
-	    parse_tracepoint_status (reply + 1, bp, utp);
+	    parse_tracepoint_status (reply + 1, tp, utp);
 	}
     }
 }
diff --git a/gdb/target-debug.h b/gdb/target-debug.h
index a028d2ba5f4..2eccc7840d6 100644
--- a/gdb/target-debug.h
+++ b/gdb/target-debug.h
@@ -126,6 +126,8 @@
   target_debug_do_print (host_address_to_string (X))
 #define target_debug_print_struct_breakpoint_p(X)	\
   target_debug_do_print (host_address_to_string (X))
+#define target_debug_print_struct_tracepoint_p(X)	\
+  target_debug_do_print (host_address_to_string (X))
 #define target_debug_print_struct_uploaded_tp_p(X)	\
   target_debug_do_print (host_address_to_string (X))
 #define target_debug_print_struct_uploaded_tp_pp(X)	\
diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index 8b066249624..909f2725035 100644
--- a/gdb/target-delegates.c
+++ b/gdb/target-delegates.c
@@ -143,7 +143,7 @@ struct dummy_target : public target_ops
   void trace_set_readonly_regions () override;
   void trace_start () override;
   int get_trace_status (struct trace_status *arg0) override;
-  void get_tracepoint_status (struct breakpoint *arg0, struct uploaded_tp *arg1) override;
+  void get_tracepoint_status (struct tracepoint *arg0, struct uploaded_tp *arg1) override;
   void trace_stop () override;
   int trace_find (enum trace_find_type arg0, int arg1, CORE_ADDR arg2, CORE_ADDR arg3, int *arg4) override;
   bool get_trace_state_variable_value (int arg0, LONGEST *arg1) override;
@@ -318,7 +318,7 @@ struct debug_target : public target_ops
   void trace_set_readonly_regions () override;
   void trace_start () override;
   int get_trace_status (struct trace_status *arg0) override;
-  void get_tracepoint_status (struct breakpoint *arg0, struct uploaded_tp *arg1) override;
+  void get_tracepoint_status (struct tracepoint *arg0, struct uploaded_tp *arg1) override;
   void trace_stop () override;
   int trace_find (enum trace_find_type arg0, int arg1, CORE_ADDR arg2, CORE_ADDR arg3, int *arg4) override;
   bool get_trace_state_variable_value (int arg0, LONGEST *arg1) override;
@@ -3216,24 +3216,24 @@ debug_target::get_trace_status (struct trace_status *arg0)
 }
 
 void
-target_ops::get_tracepoint_status (struct breakpoint *arg0, struct uploaded_tp *arg1)
+target_ops::get_tracepoint_status (struct tracepoint *arg0, struct uploaded_tp *arg1)
 {
   this->beneath ()->get_tracepoint_status (arg0, arg1);
 }
 
 void
-dummy_target::get_tracepoint_status (struct breakpoint *arg0, struct uploaded_tp *arg1)
+dummy_target::get_tracepoint_status (struct tracepoint *arg0, struct uploaded_tp *arg1)
 {
   tcomplain ();
 }
 
 void
-debug_target::get_tracepoint_status (struct breakpoint *arg0, struct uploaded_tp *arg1)
+debug_target::get_tracepoint_status (struct tracepoint *arg0, struct uploaded_tp *arg1)
 {
   gdb_printf (gdb_stdlog, "-> %s->get_tracepoint_status (...)\n", this->beneath ()->shortname ());
   this->beneath ()->get_tracepoint_status (arg0, arg1);
   gdb_printf (gdb_stdlog, "<- %s->get_tracepoint_status (", this->beneath ()->shortname ());
-  target_debug_print_struct_breakpoint_p (arg0);
+  target_debug_print_struct_tracepoint_p (arg0);
   gdb_puts (", ", gdb_stdlog);
   target_debug_print_struct_uploaded_tp_p (arg1);
   gdb_puts (")\n", gdb_stdlog);
@@ -4553,9 +4553,9 @@ dummy_target::fetch_x86_xsave_layout ()
 x86_xsave_layout
 debug_target::fetch_x86_xsave_layout ()
 {
-  x86_xsave_layout result;
   gdb_printf (gdb_stdlog, "-> %s->fetch_x86_xsave_layout (...)\n", this->beneath ()->shortname ());
-  result = this->beneath ()->fetch_x86_xsave_layout ();
+  x86_xsave_layout result
+    = this->beneath ()->fetch_x86_xsave_layout ();
   gdb_printf (gdb_stdlog, "<- %s->fetch_x86_xsave_layout (", this->beneath ()->shortname ());
   gdb_puts (") = ", gdb_stdlog);
   target_debug_print_x86_xsave_layout (result);
diff --git a/gdb/target.c b/gdb/target.c
index 68d7fe2bb56..8cb4fa1736d 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -667,7 +667,7 @@ target_get_trace_status (trace_status *ts)
 }
 
 void
-target_get_tracepoint_status (breakpoint *tp, uploaded_tp *utp)
+target_get_tracepoint_status (tracepoint *tp, uploaded_tp *utp)
 {
   return current_inferior ()->top_target ()->get_tracepoint_status (tp, utp);
 }
diff --git a/gdb/target.h b/gdb/target.h
index aa07075f9f2..18388a32d19 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1043,7 +1043,7 @@ struct target_ops
     virtual int get_trace_status (struct trace_status *ts)
       TARGET_DEFAULT_RETURN (-1);
 
-    virtual void get_tracepoint_status (struct breakpoint *tp,
+    virtual void get_tracepoint_status (struct tracepoint *tp,
 					struct uploaded_tp *utp)
       TARGET_DEFAULT_NORETURN (tcomplain ());
 
@@ -2255,7 +2255,7 @@ extern void target_trace_set_readonly_regions ();
 
 extern int target_get_trace_status (trace_status *ts);
 
-extern void target_get_tracepoint_status (breakpoint *tp, uploaded_tp *utp);
+extern void target_get_tracepoint_status (tracepoint *tp, uploaded_tp *utp);
 
 extern void target_trace_stop ();
 
diff --git a/gdb/tracefile-tfile.c b/gdb/tracefile-tfile.c
index 3440f375021..1158a8beeb2 100644
--- a/gdb/tracefile-tfile.c
+++ b/gdb/tracefile-tfile.c
@@ -67,7 +67,7 @@ class tfile_target final : public tracefile_target
   bool get_trace_state_variable_value (int tsv, LONGEST *val) override;
   traceframe_info_up traceframe_info () override;
 
-  void get_tracepoint_status (struct breakpoint *tp,
+  void get_tracepoint_status (struct tracepoint *tp,
 			      struct uploaded_tp *utp) override;
 };
 
@@ -633,7 +633,7 @@ tfile_target::files_info ()
 }
 
 void
-tfile_target::get_tracepoint_status (struct breakpoint *tp, struct uploaded_tp *utp)
+tfile_target::get_tracepoint_status (struct tracepoint *tp, struct uploaded_tp *utp)
 {
   /* Other bits of trace status were collected as part of opening the
      trace files, so nothing to do here.  */
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 205380476b3..9cec91f0fd4 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -157,7 +157,7 @@ static std::string trace_stop_notes;
 
 struct collection_list;
 
-static counted_command_line all_tracepoint_actions (struct breakpoint *);
+static counted_command_line all_tracepoint_actions (struct tracepoint *);
 
 static struct trace_status trace_status;
 
@@ -626,12 +626,11 @@ finalize_tracepoint_aexpr (struct agent_expr *aexpr)
 
 /* worker function */
 void
-validate_actionline (const char *line, struct breakpoint *b)
+validate_actionline (const char *line, struct tracepoint *t)
 {
   struct cmd_list_element *c;
   const char *tmp_p;
   const char *p;
-  struct tracepoint *t = (struct tracepoint *) b;
 
   /* If EOF is typed, *line is NULL.  */
   if (line == NULL)
@@ -1479,7 +1478,9 @@ encode_actions (struct bp_location *tloc,
   gdbarch_virtual_frame_pointer (tloc->gdbarch,
 				 tloc->address, &frame_reg, &frame_offset);
 
-  counted_command_line actions = all_tracepoint_actions (tloc->owner);
+  struct tracepoint *t
+    = gdb::checked_static_cast<struct tracepoint *> (tloc->owner);
+  counted_command_line actions = all_tracepoint_actions (t);
   encode_actions_1 (actions.get (), tloc, frame_reg, frame_offset,
 		    tracepoint_list, stepping_list);
   encode_actions_1 (breakpoint_commands (tloc->owner), tloc,
@@ -1884,8 +1885,12 @@ tstatus_command (const char *args, int from_tty)
 		(long int) (ts->stop_time % 1000000));
 
   /* Now report any per-tracepoint status available.  */
-  for (breakpoint &t : all_tracepoints ())
-    target_get_tracepoint_status (&t, NULL);
+  for (breakpoint &b : all_tracepoints ())
+    {
+      struct tracepoint *t
+	= gdb::checked_static_cast<struct tracepoint *> (&b);
+      target_get_tracepoint_status (t, nullptr);
+    }
 }
 
 /* Report the trace status to uiout, in a way suitable for MI, and not
@@ -2751,7 +2756,7 @@ get_traceframe_location (int *stepping_frame_p)
 /* Return the default collect actions of a tracepoint T.  */
 
 static counted_command_line
-all_tracepoint_actions (struct breakpoint *t)
+all_tracepoint_actions (struct tracepoint *t)
 {
   counted_command_line actions (nullptr, command_lines_deleter ());
 
@@ -2794,7 +2799,9 @@ tdump_command (const char *args, int from_tty)
 
   select_frame (get_current_frame ());
 
-  counted_command_line actions = all_tracepoint_actions (loc->owner);
+  struct tracepoint *t
+    = gdb::checked_static_cast<struct tracepoint *> (loc->owner);
+  counted_command_line actions = all_tracepoint_actions (t);
 
   trace_dump_actions (actions.get (), 0, stepping_frame, from_tty);
   trace_dump_actions (breakpoint_commands (loc->owner), 0, stepping_frame,
@@ -3059,7 +3066,7 @@ merge_uploaded_tracepoints (struct uploaded_tp **uploaded_tps)
 
 	  /* Mark this location as already inserted.  */
 	  loc->inserted = 1;
-	  t = (struct tracepoint *) loc->owner;
+	  t = gdb::checked_static_cast<struct tracepoint *> (loc->owner);
 	  gdb_printf (_("Assuming tracepoint %d is same "
 			"as target's tracepoint %d at %s.\n"),
 		      loc->owner->number, utp->number,
@@ -3368,11 +3375,10 @@ Status line: '%s'\n"), p, line);
 }
 
 void
-parse_tracepoint_status (const char *p, struct breakpoint *bp,
+parse_tracepoint_status (const char *p, struct tracepoint *tp,
 			 struct uploaded_tp *utp)
 {
   ULONGEST uval;
-  struct tracepoint *tp = (struct tracepoint *) bp;
 
   p = unpack_varlen_hex (p, &uval);
   if (tp)
diff --git a/gdb/tracepoint.h b/gdb/tracepoint.h
index a637d6b71d3..60b51390002 100644
--- a/gdb/tracepoint.h
+++ b/gdb/tracepoint.h
@@ -354,7 +354,7 @@ extern void encode_actions_rsp (struct bp_location *tloc,
 				std::vector<std::string> *tdp_actions,
 				std::vector<std::string> *stepping_actions);
 
-extern void validate_actionline (const char *, struct breakpoint *);
+extern void validate_actionline (const char *, struct tracepoint *);
 extern void validate_trace_state_variable_name (const char *name);
 
 extern struct trace_state_variable *find_trace_state_variable (const char *name);
@@ -369,7 +369,7 @@ extern int encode_source_string (int num, ULONGEST addr,
 
 extern void parse_trace_status (const char *line, struct trace_status *ts);
 
-extern void parse_tracepoint_status (const char *p, struct breakpoint *tp,
+extern void parse_tracepoint_status (const char *p, struct tracepoint *tp,
 				     struct uploaded_tp *utp);
 
 extern void parse_tracepoint_definition (const char *line,

-- 
2.40.1

[PATCH 3/4] Use gdb::checked_static_cast for code_breakpoint

[Tom Tromey]

This replaces some casts to 'code_breakpoint *' with
checked_static_cast.
---
 gdb/breakpoint.c | 6 ++++--
 gdb/elfread.c    | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 134ad76ce9a..5d87b17a58d 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -6174,10 +6174,12 @@ bpstat_run_callbacks (bpstat *bs_head)
 	  handle_jit_event (bs->bp_location_at->address);
 	  break;
 	case bp_gnu_ifunc_resolver:
-	  gnu_ifunc_resolver_stop ((code_breakpoint *) b);
+	  gnu_ifunc_resolver_stop
+	    (gdb::checked_static_cast<code_breakpoint *> (b));
 	  break;
 	case bp_gnu_ifunc_resolver_return:
-	  gnu_ifunc_resolver_return_stop ((code_breakpoint *) b);
+	  gnu_ifunc_resolver_return_stop
+	    (gdb::checked_static_cast<code_breakpoint *> (b));
 	  break;
 	}
     }
diff --git a/gdb/elfread.c b/gdb/elfread.c
index 55e3e47ceca..8704b52f35b 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -1033,7 +1033,7 @@ elf_gnu_ifunc_resolver_return_stop (code_breakpoint *b)
 			    "gnu-indirect-function breakpoint type %d"),
 			  (int) b->type);
 	}
-      b = (code_breakpoint *) b_next;
+      b = gdb::checked_static_cast<code_breakpoint *> (b_next);
     }
   gdb_assert (b->type == bp_gnu_ifunc_resolver);
   gdb_assert (b->has_single_location ());

-- 
2.40.1

[PATCH 4/4] Use gdb::checked_static_cast for catchpoints

[Tom Tromey]

This replaces some casts to various kinds of catchpoint with
checked_static_cast.
---
 gdb/ada-lang.c            | 3 ++-
 gdb/break-catch-sig.c     | 8 +++++---
 gdb/break-catch-syscall.c | 4 ++--
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 2496c099f19..b1de58cce6f 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -12203,7 +12203,8 @@ ada_catchpoint::allocate_location ()
 bool
 ada_catchpoint::should_stop_exception (const struct bp_location *bl) const
 {
-  struct ada_catchpoint *c = (struct ada_catchpoint *) bl->owner;
+  struct ada_catchpoint *c
+    = gdb::checked_static_cast<ada_catchpoint *> (bl->owner);
   const struct ada_catchpoint_location *ada_loc
     = (const struct ada_catchpoint_location *) bl;
   bool stop;
diff --git a/gdb/break-catch-sig.c b/gdb/break-catch-sig.c
index 10c8b81f8e8..cc4f525e3a3 100644
--- a/gdb/break-catch-sig.c
+++ b/gdb/break-catch-sig.c
@@ -103,7 +103,8 @@ signal_to_name_or_int (enum gdb_signal sig)
 int
 signal_catchpoint::insert_location (struct bp_location *bl)
 {
-  struct signal_catchpoint *c = (struct signal_catchpoint *) bl->owner;
+  struct signal_catchpoint *c
+    = gdb::checked_static_cast<signal_catchpoint *> (bl->owner);
 
   if (!c->signals_to_be_caught.empty ())
     {
@@ -130,7 +131,8 @@ int
 signal_catchpoint::remove_location (struct bp_location *bl,
 				    enum remove_bp_reason reason)
 {
-  struct signal_catchpoint *c = (struct signal_catchpoint *) bl->owner;
+  struct signal_catchpoint *c
+    = gdb::checked_static_cast<signal_catchpoint *> (bl->owner);
 
   if (!c->signals_to_be_caught.empty ())
     {
@@ -166,7 +168,7 @@ signal_catchpoint::breakpoint_hit (const struct bp_location *bl,
 				   const target_waitstatus &ws)
 {
   const struct signal_catchpoint *c
-    = (const struct signal_catchpoint *) bl->owner;
+    = gdb::checked_static_cast<const signal_catchpoint *> (bl->owner);
   gdb_signal signal_number;
 
   if (ws.kind () != TARGET_WAITKIND_STOPPED)
diff --git a/gdb/break-catch-syscall.c b/gdb/break-catch-syscall.c
index 9abf8183984..313ac44562c 100644
--- a/gdb/break-catch-syscall.c
+++ b/gdb/break-catch-syscall.c
@@ -480,10 +480,10 @@ catch_syscall_enabled (void)
 static bool
 catching_syscall_number_1 (struct breakpoint *b, int syscall_number)
 {
-
   if (is_syscall_catchpoint_enabled (b))
     {
-      struct syscall_catchpoint *c = (struct syscall_catchpoint *) b;
+      struct syscall_catchpoint *c
+	= gdb::checked_static_cast<syscall_catchpoint *> (b);
 
       if (!c->syscalls_to_be_caught.empty ())
 	{

-- 
2.40.1

Re: [PATCH 1/4] Use gdb::checked_static_cast for watchpoints

[Simon Marchi]

On 2023-09-15 14:36, Tom Tromey via Gdb-patches wrote:
> This replaces some casts to 'watchpoint *' with checked_static_cast.
> In one spot, an unnecessary block is also removed.
> ---
>  gdb/breakpoint.c           | 298 +++++++++++++++++++++++----------------------
>  gdb/guile/scm-breakpoint.c |   2 +-
>  gdb/python/py-breakpoint.c |   2 +-
>  3 files changed, 153 insertions(+), 149 deletions(-)
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index c429af455ff..c0d86f38c73 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -5249,7 +5249,7 @@ watchpoint_check (bpstat *bs)
>  
>    /* BS is built from an existing struct breakpoint.  */
>    gdb_assert (bs->breakpoint_at != NULL);
> -  b = (struct watchpoint *) bs->breakpoint_at;
> +  b = gdb::checked_static_cast<struct watchpoint *> (bs->breakpoint_at);

Not mandatory, but when doing these refactorings, I find it's a good
time to bring the variable declaration down.  And also, get rid of the
struct keyword, if possible.  So:

  watchpoint *b = gdb::checked_static_cast<watchpoint *> (bs->breakpoint_at);

Simon

Re: [PATCH 0/4] Use checked_static_cast for breakpoint casts

[Simon Marchi]

On 2023-09-15 14:36, Tom Tromey via Gdb-patches wrote:
> While reviewing a patch, I noticed some unchecked casts to watchpoint.
> This series replaces all the unchecked breakpoint casts I could find
> with checked casts.
> 
> Regression tested on x86-64 Fedora 36.

I sent a minor comment on patch 1.  Getting rid of the struct keyword
when possible might allow avoid line wrapping in a bunch of places.
But otherwise:

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

Simon

Re: [PATCH 1/4] Use gdb::checked_static_cast for watchpoints

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> Not mandatory, but when doing these refactorings, I find it's a good
Simon> time to bring the variable declaration down.  And also, get rid of the
Simon> struct keyword, if possible.  So:

Simon>   watchpoint *b = gdb::checked_static_cast<watchpoint *> (bs->breakpoint_at);

I made these changes in v2.

Tom