Re: [patch 2/3] Implement support for PowerPC BookE ranged watchpoints

[Pedro Alves <pedro at codesourcery dot com>]

I think this looks great.  Thanks a million for following through!

The resource accounting bits, I still say that they should all just
go away (at some point), and gdb should just try to insert the
watchpoint immediately, and see if the target refuses.  E.g., how could
one sanely implement the accounting for remote targets?  The target
is free to do all sorts of smart merging, and resource reusing, and
in fact, x86 gdbserver does so (which is why the x86 and most
other ports don't actually make use of the resource accounting
interfaces, they just always accept watchpoints).

A few directed comments only:

On Thursday 23 December 2010 18:49:42, Thiago Jung Bauermann wrote:
> +int
> +is_scalar_type_recursive (struct type *t)
> +{
> +  CHECK_TYPEDEF (t);
> +
> +  if (is_scalar_type (t))
> +    return 1;
> +  else if ((TYPE_CODE (t) == TYPE_CODE_ARRAY
> +           || TYPE_CODE (t) == TYPE_CODE_STRING) && TYPE_NFIELDS (t) == 1
> +          && TYPE_CODE (TYPE_INDEX_TYPE (t)) == TYPE_CODE_RANGE)
> +    {
> +      LONGEST low_bound, high_bound;
> +      struct type *elt_type = check_typedef (TYPE_TARGET_TYPE (t));
> +
> +      get_discrete_bounds (TYPE_INDEX_TYPE (t), &low_bound, &high_bound);
> +
> +      return high_bound == low_bound && is_scalar_type_recursive (elt_type);
> +    }

What does the "TYPE_NFIELDS (t) == 1" do ?

I think you're missing at least pointers and references.  These should
also get the "exact" treatment, no?  Take a look at valprint.c:scalar_type_p.

Could you please add small describing comments above these new
functions?  ("return true if type T is a blah, blah".)

> +static void
> +show_powerpc_exact_watchpoints (struct ui_file *file, int from_tty,
> +                               struct cmd_list_element *c,
> +                               const char *value)
> +{
> +  fprintf_filtered (file, _("\
> +Use of just one debug register per watchpoint is %s.\n"), value);
> +}
> +

Should be "Use of exact watchpoints is ...", I think?  Thus getting rid
of the small lie that not all watchpoints (in the user perspective) will
use just one debug register, leaving the just one debug register
issue explanation to the help string (as you already have).

-- 
Pedro Alves