This is the mail archive of the gdb-patches@sourceware.cygnus.com mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]

Hardware breakpoints and watchpoints: patches


The following patches make watchpoints work much better for me.  The
original code would occasionally trigger incorrect watchpoints or
leave watchpoints unremoved in the debug registers, which would then
cause SIGTRAP and othere calamities.

I wonder if the patches that prevent rwatch from breaking if the
watched value has changed are general enough.  I think x86 needs it,
since there are no hardware-assisted read-only watchpoints there; but
what about other platforms?  Should I introduce some architecture-
dependent macro here?

1999-08-14  Eli Zaretskii  <eliz@is.elta.co.il>

	* breakpoint.c (bpstat_stop_status): Handle hardware watchpoints
	like we do with access and read watchpoints.  Accept triggered
	addresses anywhere inside the region occupied by a watched value
	as a sign that the watchpoint fired.
	(TARGET_REGION_OK_FOR_HW_WATCHPOINT): Supply a default definition.
	(can_use_hardware_watchpoint): Call TARGET_REGION_OK_FOR_HW_WATCHPOINT;
	if it returns zero, return zero immediately.
	(insert_breakpoints): Try to insert watchpoints for all the values
	on the value chain, even if some of them fail to insert.  Remove
	the breakpoint if parts of its value chain couldn't be inserted.

	* target.h (TARGET_REGION_OK_FOR_HW_WATCHPOINT): New macro.

1999-08-06  Eli Zaretskii  <eliz@is.elta.co.il>

	* breakpoint.c (bpstat_stop_status): Don't decrement the hit count
	if not going to stop, leave it as it was before.  Don't stop if
	target_stopped_data_address() returns zero.  Don't stop if a read
	watchpoint fired, but the value has changed.  Don't stop if some
	watchpoint was triggered, but its address doesn't match the
	address of this watchpoint.


*** gdb/breakpoint.c~0	Fri Mar 26 06:23:50 1999
--- gdb/breakpoint.c	Sat Aug 14 17:52:58 1999
*************** insert_breakpoints ()
*** 796,803 ****
  		      val = target_insert_watchpoint (addr, len, type);
  		      if (val == -1)
  			{
  			  b->inserted = 0;
- 			  break;
  			}
  		      val = 0;
  		    }
--- 796,807 ----
  		      val = target_insert_watchpoint (addr, len, type);
  		      if (val == -1)
  			{
+ 			  /* Don't exit the loop, try to insert every
+ 			     value on the value chain.  That's because
+ 			     we will be removing all the watches below,
+ 			     and removing a watchpoint we didn't insert
+ 			     could have adverse effects.  */
  			  b->inserted = 0;
  			}
  		      val = 0;
  		    }
*************** insert_breakpoints ()
*** 805,812 ****
  	      /* Failure to insert a watchpoint on any memory value in the
  		 value chain brings us here.  */
  	      if (!b->inserted)
! 		warning ("Hardware watchpoint %d: Could not insert watchpoint\n",
! 			 b->number);
  	    }
  	  else
  	    {
--- 809,825 ----
  	      /* Failure to insert a watchpoint on any memory value in the
  		 value chain brings us here.  */
  	      if (!b->inserted)
! 		{
! 		  warning ("\
! Hardware watchpoint %d: Could not insert watchpoint\n", b->number);
! 		  /* This watchpoint couldn't be inserted, but some of
! 		     the values on its value chain might have their
! 		     watchpoints inserted.  We must remove them,
! 		     otherwise the resources they use will remain
! 		     occupied.  */
! 		  remove_breakpoint (b, mark_uninserted);
! 		  val = -1;	/* to return failure indication */
! 		}
  	    }
  	  else
  	    {
*************** bpstat_stop_status (pc, not_a_breakpoint
*** 2124,2130 ****
        bs->print = 1;
  
        sprintf (message, message1, b->number);
!       if (b->type == bp_watchpoint || b->type == bp_hardware_watchpoint)
  	{
  	  switch (catch_errors (watchpoint_check, bs, message, RETURN_MASK_ALL))
  	    {
--- 2137,2143 ----
        bs->print = 1;
  
        sprintf (message, message1, b->number);
!       if (b->type == bp_watchpoint)
  	{
  	  switch (catch_errors (watchpoint_check, bs, message, RETURN_MASK_ALL))
  	    {
*************** bpstat_stop_status (pc, not_a_breakpoint
*** 2141,2148 ****
  	      /* Don't stop.  */
  	      bs->print_it = print_it_noop;
  	      bs->stop = 0;
- 	      /* Don't consider this a hit.  */
- 	      --(b->hit_count);
  	      continue;
  	    default:
  	      /* Can't happen.  */
--- 2154,2159 ----
*************** bpstat_stop_status (pc, not_a_breakpoint
*** 2160,2173 ****
  	      break;
  	    }
  	}
!       else if (b->type == bp_read_watchpoint || b->type == bp_access_watchpoint)
          {
  	  CORE_ADDR addr;
  	  value_ptr v;
            int found = 0;
  
  	  addr = target_stopped_data_address();
! 	  if (addr == 0) continue;
            for (v = b->val_chain; v; v = v->next)
              {
                if (v->lval == lval_memory)
--- 2171,2192 ----
  	      break;
  	    }
  	}
!       else if (b->type == bp_read_watchpoint
! 	       || b->type == bp_access_watchpoint
! 	       || b->type == bp_hardware_watchpoint)
          {
  	  CORE_ADDR addr;
  	  value_ptr v;
            int found = 0;
  
  	  addr = target_stopped_data_address();
! 	  if (addr == 0)
! 	    {
! 	      /* Don't stop.  */
! 	      bs->print_it = print_it_noop;
! 	      bs->stop = 0;
! 	      continue;
! 	    }
            for (v = b->val_chain; v; v = v->next)
              {
                if (v->lval == lval_memory)
*************** bpstat_stop_status (pc, not_a_breakpoint
*** 2175,2181 ****
                    CORE_ADDR vaddr;
  
                    vaddr = VALUE_ADDRESS (v) + VALUE_OFFSET (v);
! 	          if (addr == vaddr)
  	            found = 1;
                  }
              }
--- 2194,2205 ----
                    CORE_ADDR vaddr;
  
                    vaddr = VALUE_ADDRESS (v) + VALUE_OFFSET (v);
! 		  /* If VADDR is watched as a sequence of several
! 		     addresses (e.g., with several debug registers),
! 		     the address that triggered might be anywhere
! 		     in the region that V defines.  */
! 	          if (addr >= vaddr
! 		      && addr < vaddr + TYPE_LENGTH (VALUE_TYPE (v)))
  	            found = 1;
                  }
              }
*************** bpstat_stop_status (pc, not_a_breakpoint
*** 2188,2199 ****
                    /* Stop.  */
                    break;
                  case WP_VALUE_CHANGED:
                  case WP_VALUE_NOT_CHANGED:
!                   /* Stop.  */
  		  ++(b->hit_count);
                    break;
                  default:
                    /* Can't happen.  */
                  case 0:
                    /* Error from catch_errors.  */
                    printf_filtered ("Watchpoint %d deleted.\n", b->number);
--- 2212,2251 ----
                    /* Stop.  */
                    break;
                  case WP_VALUE_CHANGED:
+ 		  if (b->type == bp_read_watchpoint)
+ 		    {
+ 		      /* Don't stop: read watchpoints shouldn't fire if
+ 			 the value has changed.  This is for targets
+ 			 which cannot set read-only watchpoints.  */
+ 		      bs->print_it = print_it_noop;
+ 		      bs->stop = 0;
+ 		      continue;
+ 		    }
+ 		  ++(b->hit_count);
+ 		  break;
                  case WP_VALUE_NOT_CHANGED:
!                   /* Stop, but not if this is a write-only watchpoint.
! 		     FIXME: this causes GDB to not report watchpoints
! 		     if the memory was written with the same value.
! 		     However, to correct this, we need maintain a
! 		     one-to-one correspeondence between the watchpoint
! 		     and the target resources used to implement it, and
! 		     let the target tell us which watchpoint(s) fired.
! 		     With the current design, if we don't check that the
! 		     value has changed, we might erroneously say that a
! 		     read watchpoint triggered if it watches the same
! 		     address as some other write watchpoint.  */
! 		  if (b->type == bp_hardware_watchpoint)
! 		    {
! 		      bs->print_it = print_it_noop;
! 		      bs->stop = 0;
! 		      continue;
! 		    }
  		  ++(b->hit_count);
                    break;
                  default:
                    /* Can't happen.  */
+ 		  /* FALLTHROUGH */
                  case 0:
                    /* Error from catch_errors.  */
                    printf_filtered ("Watchpoint %d deleted.\n", b->number);
*************** bpstat_stop_status (pc, not_a_breakpoint
*** 2204,2209 ****
--- 2256,2271 ----
                    bs->print_it = print_it_done;
                    break;
  	      }
+ 	  else
+ 	    {
+ 	      /* This is a case where some watchpoint(s) triggered,
+ 		 but not at the address of this watchpoint (FOUND
+ 		 was left zero).  So don't print anything for this
+ 		 watchpoint.  */
+ 	      bs->print_it = print_it_noop;
+ 	      bs->stop = 0;
+ 	      continue;
+ 	    }
          }
        else 
          {
*************** watch_command_1 (arg, accessflag, from_t
*** 4393,4398 ****
--- 4455,4465 ----
      ((byte_size) <= (REGISTER_SIZE))
  #endif
  
+ #if !defined(TARGET_REGION_OK_FOR_HW_WATCHPOINT)
+ #define TARGET_REGION_OK_FOR_HW_WATCHPOINT(addr,len) \
+ 	TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(len)
+ #endif
+ 
  static int
  can_use_hardware_watchpoint (v)
       struct value *v;
*************** can_use_hardware_watchpoint (v)
*** 4411,4418 ****
      {
        if (v->lval == lval_memory)
  	{
! 	  if (TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT (TYPE_LENGTH (VALUE_TYPE (v))))
! 	    found_memory_cnt++;
          }
        else if (v->lval != not_lval && v->modifiable == 0)
  	return 0;
--- 4478,4489 ----
      {
        if (v->lval == lval_memory)
  	{
! 	  int vaddr = VALUE_ADDRESS (v) + VALUE_OFFSET (v);
! 	  int len = TYPE_LENGTH (VALUE_TYPE (v));
! 
! 	  if (!TARGET_REGION_OK_FOR_HW_WATCHPOINT (vaddr, len))
! 	    return 0;
! 	  found_memory_cnt++;
          }
        else if (v->lval != not_lval && v->modifiable == 0)
  	return 0;
*************** delete_breakpoint (bpt)
*** 5667,5673 ****
  	    && b->enable != call_disabled)
  	  {
  	    int val;
! 	    val = target_insert_breakpoint (b->address, b->shadow_contents);
  	    if (val != 0)
  	      {
  		target_terminal_ours_for_output ();
--- 5738,5746 ----
  	    && b->enable != call_disabled)
  	  {
  	    int val;
! 	    val = b->type == bp_hardware_breakpoint
! 	      ? target_insert_hw_breakpoint (b->address, b->shadow_contents)
! 	      : target_insert_breakpoint (b->address, b->shadow_contents);
  	    if (val != 0)
  	      {
  		target_terminal_ours_for_output ();
*** gdb/target.h~0	Fri Mar 26 06:26:02 1999
--- gdb/target.h	Sat Aug 14 14:46:04 1999
*************** extern char *normal_pid_to_str PARAMS ((
*** 1046,1055 ****
  
  #ifndef TARGET_HAS_HARDWARE_WATCHPOINTS
  
! /* Returns non-zero if we can set a hardware watchpoint of type TYPE.  TYPE is
!    one of bp_hardware_watchpoint, bp_read_watchpoint, bp_write_watchpoint, or
!    bp_hardware_breakpoint.  CNT is the number of such watchpoints used so far
!    (including this one?).  OTHERTYPE is who knows what...  */
  
  #define TARGET_CAN_USE_HARDWARE_WATCHPOINT(TYPE,CNT,OTHERTYPE) 0
  
--- 1046,1061 ----
  
  #ifndef TARGET_HAS_HARDWARE_WATCHPOINTS
  
! /* Returns the number of hardware watchpoints of type TYPE that we can
!    set.  Value is positive if we can set CNT watchpoints, zero if
!    setting watchpoints of type TYPE is not supported, and negative if
!    CNT is more than the maximum number of watchpoints of type TYPE
!    that we can support.  TYPE is one of bp_hardware_watchpoint,
!    bp_read_watchpoint, bp_write_watchpoint, or bp_hardware_breakpoint.
!    CNT is the number of such watchpoints used so far (including this
!    one).  OTHERTYPE is non-zero if other types of watchpoints are
!    currently enabled (this is for targets where all watchpoints need to
!    be of the same type, for example).  */
  
  #define TARGET_CAN_USE_HARDWARE_WATCHPOINT(TYPE,CNT,OTHERTYPE) 0
  
*************** extern char *normal_pid_to_str PARAMS ((
*** 1058,1063 ****
--- 1064,1076 ----
          (LONGEST)(byte_count) <= REGISTER_SIZE
  #endif
  
+ /* Returns non-zero if we can use hardware watchpoints to watch a region
+    whose address is ADDR and whose length is LEN.  */
+ #if !defined(TARGET_REGION_OK_FOR_HW_WATCHPOINT)
+ #define TARGET_REGION_OK_FOR_HW_WATCHPOINT(addr,len) \
+ 	TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(len)
+ #endif
+ 
  /* However, some addresses may not be profitable to use hardware to watch,
     or may be difficult to understand when the addressed object is out of
     scope, and hence should be unwatched.  On some targets, this may have

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]