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

[Thiago Jung Bauermann <bauerman at br dot ibm dot com>]

Hi Eli,

Thanks for your review! Sorry for the delay in answering this. I was
improving some things in the patch, as well as incorporating your
suggestions.

Luis already answered some of your comments, so I'll skip them here.

On Fri 18 Dec 2009 09:30:06 Eli Zaretskii wrote:
> > +	    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?

This is a fix for an actual bug in the current GDB code. Jan suggested an
improved fix. I'll post this chunk as a separate patch, and explain the
issue there.

> > -	  && 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?

We could, by adding a length argument to breakpoint_address_match. But then
we'd have to update the 10 other call sites to make them call the function
passing 0 for length. Do you think that would be better?

> > +      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"?).

Fixed.

> > @@ -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?

In this chunk, yes. But the function is also used in watch_command_1 and 
hbreak_range_command in a slightly different way. If we changed to 
target_hw_point_slot_count, then we'd have to decrement its return value
by one in those places.

> > +	      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?

It seems that len_addr can never be <= 0. That would happen if the user
didn't provide an argument after the "mask" token, but that was already
verified earlier. I'll remove this if.

> > +  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?

I changed it to:

  if (mem_cnt < 0)
    error (_("\
Provided range can't be watched.  Either the target watchpoint resources\n\
don't support it, or hardware watchpoints are disabled in GDB\n\
(see \"set can-use-hw-watchpoints\")."));

What do you think?

> > +  if (can_use_wp < 0)
> > +    error (_("Not enough available hardware watchpoints."));
> 
> "Not enough hardware resources for specified watchpoints" is more
> accurate, I think.

Ok, changed.

> > +      /* 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?

Here we are just imitating the behaviour (and error message) of the find
command. Since the syntax is the same, we thought that it'd be confusing
if GDB behaved differently in different commands with the same syntax.

> > +      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.

Right. I removed the printf.

> > +  if (can_use_bp < 0)
> > +    error (_("Not enough available hardware breakpoints."));
> 
> See the comment above about a similar message.

Changed too.

> > +  /* 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 _().)

This message is necessary because the hardware assisted watchpoint
condition in BookE can be used only when the watched address and the
address used in the condition are the same. I changed the message to:

      printf_filtered (_("\
Memory location for the watchpoint expression and its condition need\n\
to be the same.\n"));   

WDYT?

> > +  /* 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.

The new error message doesn't mention addresses or variables anymore.

> > +  /* 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?

Right. The current code doesn't address that. I tried tinkering a bit to
solve the problem but couldn't, yet. I'll have to look more into it.

> > +  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.

Changed to:

The watchpoint will stop execution of your program whenever the inferior\n\
writes to any address within the [start-address, end-address] interval."));

Ok?
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center