This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch 2/2] Implement support for PowerPC BookE masked and ranged watchpoints
- From: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- To: Thiago Jung Bauermann <bauerman at br dot ibm dot com>
- Cc: Eli Zaretskii <eliz at gnu dot org>, gdb-patches at sourceware dot org
- Date: Tue, 16 Nov 2010 05:01:16 +0100
- Subject: Re: [patch 2/2] Implement support for PowerPC BookE masked and ranged watchpoints
- References: <1282074110.2606.703.camel@hactar> <1287807761.10521.423.camel@hactar> <838w1p8egs.fsf@gnu.org> <1288403952.2598.58.camel@hactar> <83wrp05f2f.fsf@gnu.org> <1288648026.3377.7.camel@hactar> <83d3qo5qkj.fsf@gnu.org> <1288905125.14606.4.camel@hactar>
# 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