Re: [PATCH 1/2] Support the new PPC476 processor -- Arch Independent

[Eli Zaretskii <eliz at gnu dot org>]

> From: Sérgio_Durigan_Júnior <sergiodj@linux.vnet.ibm.com>
> Date: Wed, 16 Dec 2009 18:47:19 -0200
> Cc: Thiago Jung Bauermann <bauerman@br.ibm.com>,         Luis Machado <luisgpm@linux.vnet.ibm.com>,         Matt Tyrlik <tyrlik@us.ibm.com>
> 
> This is the patch that modifies the architecture-independent files.  It is 
> responsible for the implementation of the new commands (such as hbreak-range 
> and watch-range), and the logic that handles the new types of hardware 
> breakpoints/watchpoints.

Thanks.

> +  /* We've got to save some special fields before updating
> +     this watchpoint.  */
> +  switch (b->hw_point_flag)
> +    {
> +    case HW_POINT_MASKED_WATCH:
> +      mask = b->loc->hw_wp_mask;
> +      break;
> +    case HW_POINT_RANGED_WATCH:
> +      range = b->loc->ranged_hw_bp_length;
> +      break;

What's the need for saving and later restoring these fields?

> +	    enum enable_state e;
> +
> +	    /* We have to temporary disable this watchpoint, otherwise
> +	       we will count it twice (once as being inserted, and once
> +	       as a watchpoint that we want to insert).  */
> +	    e = b->enable_state;
> +	    b->enable_state = bp_disabled;

What's the story behind the need to temporarily disabling this
watchpoint?

> +	  if (b->type == bp_hardware_watchpoint
> +	      && TARGET_CAN_USE_SPECIAL_HW_POINT_P (HW_POINT_COND_HW_ACCEL)
> +	      && TARGET_CAN_USE_WATCHPOINT_COND_ACCEL_P (b->loc))
> +	    {
> +	      TARGET_GET_WATCHPOINT_COND_ACCEL_ADDR (b->loc,
> +						     &b->loc->cond_hw_addr);
> +	      b->hw_point_flag = HW_POINT_COND_HW_ACCEL;
> +	    }
> +	  else
> +	    b->hw_point_flag = HW_POINT_NONE;

Instead of having all this target-dependent logic here, can't we have
it inside target-specific code?

> -      val = target_insert_watchpoint (bpt->address, 
> -				      bpt->length,
> -				      bpt->watchpoint_type);
> +      if (bpt->owner->hw_point_flag == HW_POINT_MASKED_WATCH)
> +	val = target_insert_mask_watchpoint (bpt->address,
> +					     bpt->length,
> +					     bpt->watchpoint_type,
> +					     bpt->hw_wp_mask);
> +      else if (bpt->owner->hw_point_flag == HW_POINT_COND_HW_ACCEL)
> +	val = target_insert_cond_accel_watchpoint (bpt->address,
> +						   bpt->length,
> +						   bpt->watchpoint_type,
> +						   bpt->cond_hw_addr);
> +      else
> +	val = target_insert_watchpoint (bpt->address,
> +					bpt->length,
> +					bpt->watchpoint_type);

Again, why cannot this be on the target side?

> -	  && breakpoint_address_match (bpt->pspace->aspace, bpt->address,
> -				       aspace, pc))
> +	  && ((breakpoint_address_match (bpt->pspace->aspace, bpt->address,
> +					aspace, pc) && ((bpt->address == pc)))
> +	       || (bpt->owner->hw_point_flag == HW_POINT_RANGED_BREAK
> +		   && breakpoint_address_match_range (bpt->pspace->aspace,
> +						      bpt->address,
> +						      bpt->ranged_hw_bp_length,
> +						      aspace, pc)
> +		   && pc >= bpt->address
> +		   && pc < (bpt->address + bpt->ranged_hw_bp_length))))

Wouldn't it be better to just extend the current
breakpoint_address_match function, so that it supports ranges as well?

> +      if (b->hw_point_flag == HW_POINT_MASKED_WATCH)
> +	ui_out_text (uiout, "\nCheck the underlying instruction \
> +at PC for address and value related to this watchpoint trigger.\n");

Sorry, I don't understand what does this message mean.  Can you
explain?

> -/* Check watchpoint condition.  */
> +/* Check watchpoint condition.  We can't use value_equal because it coerces
> +   an array to a pointer, thus comparing just the address of the array instead
> +   of its contents.  This is not what we want.  */
> +
> +static int
> +value_equal_watchpoint (struct value *arg1, struct value *arg2)
> +{
> +  struct type *type1, *type2;
> +
> +  type1 = check_typedef (value_type (arg1));
> +  type2 = check_typedef (value_type (arg2));
> +
> +  return TYPE_CODE (type1) == TYPE_CODE (type2)
> +    && TYPE_LENGTH (type1) == TYPE_LENGTH (type2)
> +    && memcmp (value_contents (arg1), value_contents (arg2),
> +	       TYPE_LENGTH (type1)) == 0;
> +}
>  
>  static int
>  watchpoint_check (void *p)
> @@ -3246,7 +3388,7 @@ watchpoint_check (void *p)
>  
>        fetch_watchpoint_value (b->exp, &new_val, NULL, NULL);
>        if ((b->val != NULL) != (new_val != NULL)
> -	  || (b->val != NULL && !value_equal (b->val, new_val)))
> +	  || (b->val != NULL && !value_equal_watchpoint (b->val, new_val)))

Can you elaborate the need for this change?  It seems to change the
semantics of watchpoint_check, so I wonder why it is done.

> +      else if (b->hw_point_flag == HW_POINT_MASKED_WATCH)
> +	/* Since we don't the exact trigger address (from stopped_data_address)
> +	   Just tell the user we've triggered a mask watchpoint.  */
> +	return WP_VALUE_CHANGED;

The sentence in the comment is missing something ("know" after "Since
we"?).

> @@ -5771,7 +5959,12 @@ hw_breakpoint_used_count (void)
>    ALL_BREAKPOINTS (b)
>    {
>      if (b->type == bp_hardware_breakpoint && breakpoint_enabled (b))
> -      i++;
> +      {
> +	i++;
> +	/* Special types of hardware breakpoints can use more than
> +	   one slot.  */
> +	i += target_hw_point_extra_slot_count (b->hw_point_flag);
> +      }

Wouldn't it be more elegant to have target_hw_point_slot_count instead
that would return the number of used slots, instead of incrementing by
one and then call target_hw_point_extra_slot_count to get the extra
slots?

> @@ -5789,10 +5982,15 @@ hw_watchpoint_used_count (enum bptype type, int *other_type_used)
>      if (breakpoint_enabled (b))
>        {
>  	if (b->type == type)
> -	  i++;
> -	else if ((b->type == bp_hardware_watchpoint
> -		  || b->type == bp_read_watchpoint
> -		  || b->type == bp_access_watchpoint))
> +	  {
> +	    i++;
> +	    /* Special types of hardware watchpoints can use more
> +	       than one slot.  */
> +	    i += target_hw_point_extra_slot_count (b->hw_point_flag);

Same here.

> +	      /* Does the target support masked watchpoints?  */
> +	      if (!TARGET_CAN_USE_SPECIAL_HW_POINT_P (HW_POINT_MASKED_WATCH))
> +		error (_("This target does not support the usage of masks \
> +with hardware watchpoints."));

Can't the functionality be emulated by GDB's application code?

> +	      len_addr = addr_p - tokp;
> +
> +	      strtol (tokp, &endp, 16);
> +	      /* Check if the user provided a valid numeric value for the
> +		 mask address.  */
> +	      if (*endp != ' ' && *endp != '\t' && *endp != '\0')
> +		error (_("Invalid mask address specification `%s'."),
> +		       tokp);
> +
> +	      if (len_addr <= 0)
> +		error (_("The mask address is invalid."));

How can the last error condition happen?  What would the luser need to
type to trigger that?

> +  mem_cnt = can_use_hardware_watchpoint (val);
> +  if (mem_cnt < 0)
> +    error (_("Provided range can't be watched."));

This error message does not tell anything about the reason.  Can we
tell the user something more specific here about what can she do to
alleviate the problem?

> +  if (can_use_wp < 0)
> +    error (_("Not enough available hardware watchpoints."));

"Not enough hardware resources for specified watchpoints" is more
accurate, I think.

> +      /* Verifying if the lines make sense.  We need to check if
> +	 the first address in the range is smaller than the second,
> +	 and also compute the length.  */
> +      if (sal1.pc > sal2.pc)
> +	error (_("Invalid search space, end preceeds start."));

First, the message should better be something like

  "Invalid range: start address is greater than end address."

More importantly, would it make sense to reverse the addresses in this
case, instead of erroring out?

> +      if (length == 0)
> +	{
> +	  /* This range is simple enough to be treated by
> +	     the `hbreak' command.  */
> +	  printf_unfiltered (_("Range is too small, using `hbreak' \
> +instead.\n"));

And why do we need to announce that?  Perhaps do that only under
verbose operation.

> +  if (can_use_bp < 0)
> +    error (_("Not enough available hardware breakpoints."));

See the comment above about a similar message.

> +  /* Make sure that the two addresses are the same.  */
> +  if (exp_address != cond_address)
> +    {
> +      printf_filtered ("Addresses in location and condition are different.\n");
> +      return 0;
> +    }

Why do we need this message?  (If we need it, please put it in _().)

> +  /* Make sure that the two addresses are the same.  */
> +  if (exp_address != cond_address)
> +    {
> +      printf_filtered ("Addresses in location and condition are different.\n");
> +      return 0;
> +    }

Same here.  And also, the user could have typed a variable, not an
address, so the message text, if it is needed, might mislead.

> +  /* Make sure that the two variables' names are the same.  */
> +  if (strcmp (name_cond, name_exp) != 0)
> +    {
> +      printf_filtered ("Addresses in location and condition are different.\n");
> +      return 0;
> +    }

And here.  Btw, two different names can eventually evaluate to the
same addresses, no?

> +  c = add_com ("watch-range", class_breakpoint, watch_range_command, _("\
> +Set a WRITE hardware watchpoint for an address range.\n\
> +The address range should be specified in one of the following formats:\n\
> +\n\
> +   start-address, end-address\n\
> +   start-address, +length\n\
> +\n\
> +The watchpoint will stop execution of your program whenever the inferior\n\
> +writes to any address within that range."));

It would be good to tell if the address is inclusive or exclusive.

Anyway, this command (and all the other command changes and additions)
need a suitable change to the user manual (although I would suggest to
wait with that until we resolve the UI issues I raised in my previous
message in this thread.)

> +      if (start_addr > end_addr)
> +	error (_("Invalid search space, end preceeds start."));

See the comment above about a similar message.

Thanks for working on this.