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

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

#       awatch-range 0xbffff8e8,0xbffff8f8


[...]
#       awatch *0xbffff8e8 mask 0xffffff00

What is the difference of this masked range versus?

        awatch-range 0xbffff800, 0xbffff8ff

If there is no difference then the mask address feature is useful only for
masks which are not of the form (-1 << n).


On Thu, 04 Nov 2010 22:12:05 +0100, Thiago Jung Bauermann wrote:
> +watch-range START_ADDR,+LENGTH | START_ADDR, END_ADDR
> +rwatch-range START_ADDR,+LENGTH | START_ADDR, END_ADDR
> +awatch-range START_ADDR,+LENGTH | START_ADDR, END_ADDR
> +  Set a hardware watchpoint for an address range.
> +  The watchpoint will stop execution of your program whenever the inferior
> +  writes, reads, or accesses (respectively for watch-range, awatch-range
> +  and rwatch-range) any address within the specified range.

What is the problem with the syntax?

watch *array@elementcount
OR
watch *(char *)array@bytescount

People already must know the @ operator just for printing arrays.  And some
nifty user friendly is provided by FE (graphical front end) and not CLI.

I may be biased but I would not try to invent new commands unless necessary,
GDB has already enough of them no user knows them all.


> +		target_resources_ok =  target_can_use_hardware_watchpoint

two spaces.


> +static int
> +insert_ranged_watchpoint (struct bp_location *bpt)

Like going to send in the other mail, `bpt' is (mostly) used for
`struct breakpoint'.  For bp_location please use `bl', `bploc' or some other
bp_location-suggesting names used in GDB.


> +  c = add_com ("watch-range", class_breakpoint, watch_range_command, _("\
> +Set a 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 the inferior whenever it\n\
> +writes to any address within the [start-address, end-address] range."));

I would prefer some explicit "both inclusively" statement there.
One could expect `end-address' to be the exclusive one.


> +/* Special types of hardware breakpoints/watchpoints.  */
> +enum hw_point_flag {

Incorrect GNU formatting.

> +	HW_POINT_RANGED_WATCH, /* Hardware ranged watchpoint.  */
> +	HW_POINT_MASKED_WATCH  /* Hardware masked watchpoint.  */
> +};


> +  p.version         = PPC_DEBUG_CURRENT_VERSION;

These extraneous spaces do not conform to GNU formatting.

> +  p.trigger_type    = get_trigger_type (rw);
> +  p.addr_mode       = PPC_BREAKPOINT_MODE_MASK;
> +  p.condition_mode  = PPC_BREAKPOINT_CONDITION_NONE;
> +  p.addr            = (uint64_t) addr;
> +  p.addr2           = (uint64_t) mask;

Probably excessive cast.


> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -601,11 +601,16 @@ update_current_target (void)
>        INHERIT (to_files_info, t);
>        INHERIT (to_insert_breakpoint, t);
>        INHERIT (to_remove_breakpoint, t);
> +      INHERIT (to_can_use_special_hw_point, t);

There are now two target interface styles in use.  This inheriting one and the
runtime-inheriting one (see target_pid_to_str and others).  I was told the
target_pid_to_str style is now preferred and it makes sense to me.  Please
convert the new target vector methods to the new style.


> +  if (addr_bit < (sizeof (CORE_ADDR) * HOST_CHAR_BIT))
> +  {

Wrong GNU formatting.

> +    address_start &= ((CORE_ADDR) 1 << addr_bit) - 1;
> +    address_end &= ((CORE_ADDR) 1 << addr_bit) - 1;
> +  }
> +
> +  /* FIXME: cagney/2002-05-03: Need local_address_string() function
> +     that returns the language localized string formatted to a width
> +     based on gdbarch_addr_bit.  */
> +  if (addr_bit <= 32)
> +  {

Wrong GNU formatting.

> +    strcpy (addstr, "[");
> +    strcat (addstr, hex_string_custom (address_start, 8));
> +    strcat (addstr, ", ");
> +    strcat (addstr, hex_string_custom (address_end, 8));
> +    strcat (addstr, "]");
> +  }
> +  else
> +  {

Wrong GNU formatting.

> --- a/gdb/ui-out.h
> +++ b/gdb/ui-out.h
> @@ -113,6 +113,12 @@ extern void ui_out_field_fmt_int (struct ui_out *uiout, int width,
>  				  enum ui_align align, const char *fldname, 
>  		 		  int value);
>  
> +extern void ui_out_field_range_core_addr (struct ui_out *uiout,
> +					  const char *fldname,
> +                                          struct gdbarch *gdbarch,

Spaces used instead of tabs.

> +					  CORE_ADDR address_start,
> +					  CORE_ADDR length);
> +


Thanks,
Jan