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

Jan Kratochvil
2010-11-16T04:01:41Z

# 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