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

Message ID 20230915-watchpoint-casts-v1-1-a4ff35c9644e@adacore.com
State New
Headers
Series Use checked_static_cast for breakpoint casts |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed

Commit Message

Tom Tromey Sept. 15, 2023, 6:36 p.m. UTC
  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(-)
  

Comments

Simon Marchi Sept. 18, 2023, 12:40 a.m. UTC | #1
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
  
Tom Tromey Sept. 18, 2023, 2:20 p.m. UTC | #2
>>>>> "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
  

Patch

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)