* [PATCH 1/2] Support the new PPC476 processor -- Arch Independent
@ 2009-12-16 20:49 Sérgio Durigan Júnior
2009-12-18 11:28 ` Eli Zaretskii
0 siblings, 1 reply; 9+ messages in thread
From: Sérgio Durigan Júnior @ 2009-12-16 20:49 UTC (permalink / raw)
To: gdb-patches; +Cc: Thiago Jung Bauermann, Luis Machado, Matt Tyrlik
[-- Attachment #1: Type: text/plain, Size: 4240 bytes --]
Hello guys,
This is the patch that modifies the architecture-independent files. It is
responsible for the implementation of the new commands (such as hbreak-range
and watch-range), and the logic that handles the new types of hardware
breakpoints/watchpoints.
Regards,
--
Sérgio Durigan Júnior
Linux on Power Toolchain - Software Engineer
Linux Technology Center - LTC
IBM Brazil
gdb/ChangeLog:
2009-16-12 Sergio Durigan Junior <sergiodj@linux.vnet.ibm.com>
* breakpoint.c (breakpoint_address_match_range): New function.
(update_watchpoint): Added variables to save/restore new fields
for the new types of hardware breakpoints/watchpoints. Disabled
watchpoints before checking the used count. Handled the case when
the hardware supports the acceleration of the watchpoint's condition
evaluation.
(insert_bp_location): Calling the correct insertion method according
to the type of the hardware watchpoint.
(remove_breakpoint_1): Calling the correct deletion method according
to the type of the hardware watchpoint.
(print_it_typical): Handling the various cases when the new types of
hardware breakpoint/watchpoint are going to be printed.
(watchpoints_triggered): Handling the triggering of a hardware masked
watchpoint.
(value_equal_watchpoint): New function.
(watchpoint_check): Calling the correct comparison function for
watchpoints. Handling the case of a hardware mased watchpoint
trigger.
(bpstat_check_location): Handling the case of a hardware ranged
watchpoint.
(bpstat_check_breakpoint_conditions): Handling the case of a hardware
watchpoint with a condition begin hardware-accelerated.
(print_one_breakpoint_location): Handling the case of a hardware
ranged watchpoint.
(breakpoint_1): Ditto.
(set_raw_breakpoint_without_location): Setting the flag to zero.
(set_raw_breakpoint): Setting the range to zero.
(hw_breakpoint_used_count): Added a call to target-specific function
that will tell how many extra slots a hardware breakpoint needs.
(hw_watchpoint_used_count): Ditto, for hardware watchpoints.
(mention): Handling the case when we need to mention special types
of hardware breakpoints/watchpoints.
(watch_command_1): Added a routine to check for the existence of the
`mask' parameter when the user creates a watchpoint. Handling the
creation of special types of hardware watchpoints (masked and
hardware-accelerated condition).
(can_use_hardware_watchpoint): Added code to check if the memory
that is going to be watched is big, i.e., it needs a hardware ranged
watchpoint.
(watch_range_command_1): New function.
(watch_range_command): Ditto.
(awatch_range_command): Ditto.
(rwatch_range_command): Ditto.
(hbreak_range_command): Ditto.
(update_breakpoint_locations): Handling the case of a hardware
watchpoint with a condition begin hardware-accelerated.
(exp_is_address_p): New function.
(exp_is_var_p): Ditto.
(cond_is_address_equal_literal_p): Ditto.
(cond_is_var_equal_literal_p): Ditto.
(get_var_address): Ditto.
(default_watch_address_if_var_equal_literal_p): Ditto.
(default_watch_var_if_address_equal_literal_p): Ditto.
(default_watch_var_if_var_equal_literal_p): Ditto.
(default_watch_address_if_address_equal_literal_p): Ditto.
* breakpoint.h (struct bp_target_info) <length>: New field.
(struct bp_location) <cond_hw_accel>, <cond_hw_addr>,
<ranged_hw_bp_length>, <hw_wp_mask>: New fields.
(enum hw_point_flag): New.
(struct breakpoint) <hw_point_flag>: New field.
(default_watch_address_if_address_equal_literal_p): Declaring.
(default_watch_var_if_var_equal_literal_p): Ditto.
(default_watch_address_if_var_equal_literal_p): Ditto.
(default_watch_var_if_address_equal_literal_p): Ditto.
* findcmd.c (parse_addr_range): New function.
(parse_find_args): Calling `parse_addr_range'.
* ui-out.c (ui_out_field_range_core_addr): New function.
* ui-out.h (ui_out_field_range_core_addr): Declaring.
* value.h (parse_addr_range): Declaring.
gdb/testsuite/ChangeLog:
2009-16-12 Sergio Durigan Junior <sergiodj@linux.vnet.ibm.com>
* gdb.base/watchpoint.c (buf): Changing the vector size from 10
to 30.
(func3): Assigning a value to `buf'.
* gdb.base/watchpoint.exp (test_watchpoint_in_big_blob): New
procedure.
[-- Attachment #2: ppc476-arch-indep.patch --]
[-- Type: text/x-patch, Size: 57888 bytes --]
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 5394ae4..42f04a3 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -127,6 +127,12 @@ static int breakpoint_address_match (struct address_space *aspace1,
struct address_space *aspace2,
CORE_ADDR addr2);
+static int breakpoint_address_match_range (struct address_space *aspace1,
+ CORE_ADDR addr1,
+ CORE_ADDR len1,
+ struct address_space *aspace2,
+ CORE_ADDR addr2);
+
static void breakpoints_info (char *, int);
static void breakpoint_1 (int, int);
@@ -1059,6 +1065,8 @@ update_watchpoint (struct breakpoint *b, int reparse)
struct bp_location *loc;
int frame_saved;
bpstat bs;
+ CORE_ADDR mask = 0;
+ ULONGEST range = 0;
/* If this is a local watchpoint, we only want to check if the
watchpoint frame is in scope if the current thread is the thread
@@ -1066,7 +1074,19 @@ update_watchpoint (struct breakpoint *b, int reparse)
if (!watchpoint_in_thread_scope (b))
return;
- /* We don't free locations. They are stored in bp_location array and
+ /* We've got to save some special fields before updating
+ this watchpoint. */
+ switch (b->hw_point_flag)
+ {
+ case HW_POINT_MASKED_WATCH:
+ mask = b->loc->hw_wp_mask;
+ break;
+ case HW_POINT_RANGED_WATCH:
+ range = b->loc->ranged_hw_bp_length;
+ break;
+ }
+
+ /* We don't free locations. They are stored in bp_location_chain and
update_global_locations will eventually delete them and remove
breakpoints if needed. */
b->loc = NULL;
@@ -1100,6 +1120,7 @@ update_watchpoint (struct breakpoint *b, int reparse)
if (within_current_scope && reparse)
{
char *s;
+
if (b->exp)
{
xfree (b->exp);
@@ -1151,6 +1172,13 @@ update_watchpoint (struct breakpoint *b, int reparse)
&& reparse)
{
int i, mem_cnt, other_type_used;
+ 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;
i = hw_watchpoint_used_count (bp_hardware_watchpoint,
&other_type_used);
@@ -1167,6 +1195,8 @@ update_watchpoint (struct breakpoint *b, int reparse)
else
b->type = bp_hardware_watchpoint;
}
+ /* Restoring the original state. */
+ b->enable_state = e;
}
frame_pspace = get_frame_program_space (get_selected_frame (NULL));
@@ -1229,6 +1259,17 @@ update_watchpoint (struct breakpoint *b, int reparse)
{
char *s = b->cond_string;
b->loc->cond = parse_exp_1 (&s, b->exp_valid_block, 0);
+
+ if (b->type == bp_hardware_watchpoint
+ && TARGET_CAN_USE_SPECIAL_HW_POINT_P (HW_POINT_COND_HW_ACCEL)
+ && TARGET_CAN_USE_WATCHPOINT_COND_ACCEL_P (b->loc))
+ {
+ TARGET_GET_WATCHPOINT_COND_ACCEL_ADDR (b->loc,
+ &b->loc->cond_hw_addr);
+ b->hw_point_flag = HW_POINT_COND_HW_ACCEL;
+ }
+ else
+ b->hw_point_flag = HW_POINT_NONE;
}
}
else if (!within_current_scope)
@@ -1242,6 +1283,13 @@ in which its expression is valid.\n"),
b->disposition = disp_del_at_next_stop;
}
+ /* Restoring some special fields. */
+ if (b->loc)
+ {
+ b->loc->hw_wp_mask = mask;
+ b->loc->ranged_hw_bp_length = range;
+ }
+
/* Restore the selected frame. */
if (frame_saved)
select_frame (frame_find_by_id (saved_frame_id));
@@ -1302,6 +1350,7 @@ insert_bp_location (struct bp_location *bpt,
memset (&bpt->target_info, 0, sizeof (bpt->target_info));
bpt->target_info.placed_address = bpt->address;
bpt->target_info.placed_address_space = bpt->pspace->aspace;
+ bpt->target_info.length = bpt->ranged_hw_bp_length;
if (bpt->loc_type == bp_loc_software_breakpoint
|| bpt->loc_type == bp_loc_hardware_breakpoint)
@@ -1472,9 +1521,20 @@ Note: automatically using hardware breakpoints for read-only addresses.\n"));
watchpoints. It's not clear that it's necessary... */
&& bpt->owner->disposition != disp_del_at_next_stop)
{
- val = target_insert_watchpoint (bpt->address,
- bpt->length,
- bpt->watchpoint_type);
+ if (bpt->owner->hw_point_flag == HW_POINT_MASKED_WATCH)
+ val = target_insert_mask_watchpoint (bpt->address,
+ bpt->length,
+ bpt->watchpoint_type,
+ bpt->hw_wp_mask);
+ else if (bpt->owner->hw_point_flag == HW_POINT_COND_HW_ACCEL)
+ val = target_insert_cond_accel_watchpoint (bpt->address,
+ bpt->length,
+ bpt->watchpoint_type,
+ bpt->cond_hw_addr);
+ else
+ val = target_insert_watchpoint (bpt->address,
+ bpt->length,
+ bpt->watchpoint_type);
bpt->inserted = (val != -1);
}
@@ -1613,6 +1673,7 @@ insert_breakpoint_locations (void)
val = insert_bp_location (b, tmp_error_stream,
&disabled_breaks,
&hw_breakpoint_error);
+
if (val)
error = val;
}
@@ -2100,8 +2161,17 @@ remove_breakpoint_1 (struct bp_location *b, insertion_state_t is)
struct value *n;
b->inserted = (is == mark_inserted);
- val = target_remove_watchpoint (b->address, b->length,
- b->watchpoint_type);
+ if (b->owner->hw_point_flag == HW_POINT_MASKED_WATCH)
+ val = target_remove_mask_watchpoint (b->address, b->length,
+ b->watchpoint_type,
+ b->hw_wp_mask);
+ else if (b->owner->hw_point_flag == HW_POINT_COND_HW_ACCEL)
+ val = target_remove_cond_accel_watchpoint (b->address, b->length,
+ b->watchpoint_type,
+ b->cond_hw_addr);
+ else
+ val = target_remove_watchpoint (b->address, b->length,
+ b->watchpoint_type);
/* Failure to remove any of the hardware watchpoints comes here. */
if ((is == mark_uninserted) && (b->inserted))
@@ -2267,8 +2337,15 @@ breakpoint_here_p (struct address_space *aspace, CORE_ADDR pc)
if ((breakpoint_enabled (bpt->owner)
|| bpt->owner->enable_state == bp_permanent)
- && 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))))
{
if (overlay_debugging
&& section_is_overlay (bpt->section)
@@ -2839,6 +2916,11 @@ print_it_typical (bpstat bs)
annotate_breakpoint (b->number);
if (bp_temp)
ui_out_text (uiout, "\nTemporary breakpoint ");
+ else if (bl && bl->owner
+ && bl->owner->hw_point_flag == HW_POINT_RANGED_BREAK)
+ ui_out_text (uiout, "\nRange hardware assisted breakpoint ");
+ else if (b->type == bp_hardware_breakpoint)
+ ui_out_text (uiout, "\nHardware assisted breakpoint ");
else
ui_out_text (uiout, "\nBreakpoint ");
if (ui_out_is_mi_like_p (uiout))
@@ -2887,13 +2969,19 @@ print_it_typical (bpstat bs)
(uiout, "reason",
async_reason_lookup (EXEC_ASYNC_WATCHPOINT_TRIGGER));
mention (b);
- make_cleanup_ui_out_tuple_begin_end (uiout, "value");
- ui_out_text (uiout, "\nOld value = ");
- watchpoint_value_print (bs->old_val, stb->stream);
- ui_out_field_stream (uiout, "old", stb);
- ui_out_text (uiout, "\nNew value = ");
- watchpoint_value_print (b->val, stb->stream);
- ui_out_field_stream (uiout, "new", stb);
+ if (b->hw_point_flag == HW_POINT_MASKED_WATCH)
+ ui_out_text (uiout, "\nCheck the underlying instruction \
+at PC for address and value related to this watchpoint trigger.\n");
+ else
+ {
+ make_cleanup_ui_out_tuple_begin_end (uiout, "value");
+ ui_out_text (uiout, "\nOld value = ");
+ watchpoint_value_print (bs->old_val, stb->stream);
+ ui_out_field_stream (uiout, "old", stb);
+ ui_out_text (uiout, "\nNew value = ");
+ watchpoint_value_print (b->val, stb->stream);
+ ui_out_field_stream (uiout, "new", stb);
+ }
ui_out_text (uiout, "\n");
/* More than one watchpoint may have been triggered. */
result = PRINT_UNKNOWN;
@@ -2905,10 +2993,18 @@ print_it_typical (bpstat bs)
(uiout, "reason",
async_reason_lookup (EXEC_ASYNC_READ_WATCHPOINT_TRIGGER));
mention (b);
- make_cleanup_ui_out_tuple_begin_end (uiout, "value");
- ui_out_text (uiout, "\nValue = ");
- watchpoint_value_print (b->val, stb->stream);
- ui_out_field_stream (uiout, "value", stb);
+
+ if (b->hw_point_flag == HW_POINT_MASKED_WATCH)
+ ui_out_text (uiout, "\nCheck the underlying instruction\
+ at PC for address and value related to this watchpoint trigger.\n");
+ else
+ {
+ make_cleanup_ui_out_tuple_begin_end (uiout, "value");
+ ui_out_text (uiout, "\nValue = ");
+ watchpoint_value_print (b->val, stb->stream);
+ ui_out_field_stream (uiout, "value", stb);
+ }
+
ui_out_text (uiout, "\n");
result = PRINT_UNKNOWN;
break;
@@ -2922,24 +3018,39 @@ print_it_typical (bpstat bs)
(uiout, "reason",
async_reason_lookup (EXEC_ASYNC_ACCESS_WATCHPOINT_TRIGGER));
mention (b);
- make_cleanup_ui_out_tuple_begin_end (uiout, "value");
- ui_out_text (uiout, "\nOld value = ");
- watchpoint_value_print (bs->old_val, stb->stream);
- ui_out_field_stream (uiout, "old", stb);
- ui_out_text (uiout, "\nNew value = ");
+
+ if (b->hw_point_flag != HW_POINT_MASKED_WATCH)
+ {
+ make_cleanup_ui_out_tuple_begin_end (uiout, "value");
+ ui_out_text (uiout, "\nOld value = ");
+ watchpoint_value_print (bs->old_val, stb->stream);
+ ui_out_field_stream (uiout, "old", stb);
+ ui_out_text (uiout, "\nNew value = ");
+ }
}
else
{
mention (b);
- if (ui_out_is_mi_like_p (uiout))
- ui_out_field_string
- (uiout, "reason",
- async_reason_lookup (EXEC_ASYNC_ACCESS_WATCHPOINT_TRIGGER));
- make_cleanup_ui_out_tuple_begin_end (uiout, "value");
- ui_out_text (uiout, "\nValue = ");
+
+ if (b->hw_point_flag != HW_POINT_MASKED_WATCH)
+ {
+ if (ui_out_is_mi_like_p (uiout))
+ ui_out_field_string
+ (uiout, "reason",
+ async_reason_lookup (EXEC_ASYNC_ACCESS_WATCHPOINT_TRIGGER));
+ make_cleanup_ui_out_tuple_begin_end (uiout, "value");
+ ui_out_text (uiout, "\nValue = ");
+ }
}
- watchpoint_value_print (b->val, stb->stream);
- ui_out_field_stream (uiout, "new", stb);
+ if (b->hw_point_flag == HW_POINT_MASKED_WATCH)
+ ui_out_text (uiout, "\nCheck the underlying instruction\
+ at PC for address and value related to this watchpoint trigger.\n");
+ else
+ {
+ watchpoint_value_print (b->val, stb->stream);
+ ui_out_field_stream (uiout, "new", stb);
+ }
+
ui_out_text (uiout, "\n");
result = PRINT_UNKNOWN;
break;
@@ -3148,15 +3259,30 @@ watchpoints_triggered (struct target_waitstatus *ws)
b->watchpoint_triggered = watch_triggered_no;
for (loc = b->loc; loc; loc = loc->next)
- /* Exact match not required. Within range is
- sufficient. */
- if (target_watchpoint_addr_within_range (¤t_target,
- addr, loc->address,
- loc->length))
- {
- b->watchpoint_triggered = watch_triggered_yes;
- break;
- }
+ {
+ CORE_ADDR newaddr, start;
+
+ if (loc->owner->hw_point_flag == HW_POINT_MASKED_WATCH)
+ {
+ newaddr = addr & loc->hw_wp_mask;
+ start = loc->address & loc->hw_wp_mask;
+ }
+ else
+ {
+ newaddr = addr;
+ start = loc->address;
+ }
+
+ /* Exact match not required. Within range is
+ sufficient. */
+ if (target_watchpoint_addr_within_range (¤t_target,
+ newaddr, start,
+ loc->length))
+ {
+ b->watchpoint_triggered = watch_triggered_yes;
+ break;
+ }
+ }
}
return 1;
@@ -3174,7 +3300,23 @@ watchpoints_triggered (struct target_waitstatus *ws)
#define BP_TEMPFLAG 1
#define BP_HARDWAREFLAG 2
-/* Check watchpoint condition. */
+/* Check watchpoint condition. We can't use value_equal because it coerces
+ an array to a pointer, thus comparing just the address of the array instead
+ of its contents. This is not what we want. */
+
+static int
+value_equal_watchpoint (struct value *arg1, struct value *arg2)
+{
+ struct type *type1, *type2;
+
+ type1 = check_typedef (value_type (arg1));
+ type2 = check_typedef (value_type (arg2));
+
+ return TYPE_CODE (type1) == TYPE_CODE (type2)
+ && TYPE_LENGTH (type1) == TYPE_LENGTH (type2)
+ && memcmp (value_contents (arg1), value_contents (arg2),
+ TYPE_LENGTH (type1)) == 0;
+}
static int
watchpoint_check (void *p)
@@ -3246,7 +3388,7 @@ watchpoint_check (void *p)
fetch_watchpoint_value (b->exp, &new_val, NULL, NULL);
if ((b->val != NULL) != (new_val != NULL)
- || (b->val != NULL && !value_equal (b->val, new_val)))
+ || (b->val != NULL && !value_equal_watchpoint (b->val, new_val)))
{
if (new_val != NULL)
{
@@ -3259,6 +3401,10 @@ watchpoint_check (void *p)
/* We will stop here */
return WP_VALUE_CHANGED;
}
+ 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;
else
{
/* Nothing changed, don't do anything. */
@@ -3337,7 +3483,10 @@ bpstat_check_location (const struct bp_location *bl,
if (b->type == bp_hardware_breakpoint)
{
- if (bl->address != bp_addr)
+ if ((b->hw_point_flag == HW_POINT_NONE && bl->address != bp_addr)
+ || (b->hw_point_flag == HW_POINT_RANGED_BREAK
+ && (bp_addr < bl->address
+ || (bp_addr >= bl->address + bl->ranged_hw_bp_length))))
return 0;
if (overlay_debugging /* unmapped overlay section */
&& section_is_overlay (bl->section)
@@ -3465,6 +3614,11 @@ bpstat_check_breakpoint_conditions (bpstat bs, ptid_t ptid)
if (frame_id_p (b->frame_id)
&& !frame_id_eq (b->frame_id, get_stack_frame_id (get_current_frame ())))
bs->stop = 0;
+ else if (b->hw_point_flag == HW_POINT_COND_HW_ACCEL)
+ /* The condition is going to be hardware-accelerated, which means
+ that we don't have to evaluate it because the hardware already
+ did this. Just return. */
+ return;
else if (bs->stop)
{
int value_is_zero = 0;
@@ -4097,7 +4251,13 @@ print_one_breakpoint_location (struct breakpoint *b,
if (opts.addressprint)
{
if (print_address_bits <= 32)
- strcat (wrap_indent, " ");
+ {
+ if (loc && loc->owner
+ && loc->owner->hw_point_flag == HW_POINT_RANGED_BREAK)
+ strcat (wrap_indent, " ");
+ else
+ strcat (wrap_indent, " ");
+ }
else
strcat (wrap_indent, " ");
}
@@ -4155,8 +4315,14 @@ print_one_breakpoint_location (struct breakpoint *b,
else if (b->loc == NULL || loc->shlib_disabled)
ui_out_field_string (uiout, "addr", "<PENDING>");
else
- ui_out_field_core_addr (uiout, "addr",
- loc->gdbarch, loc->address);
+ if (loc && loc->owner
+ && loc->owner->hw_point_flag == HW_POINT_RANGED_BREAK)
+ ui_out_field_range_core_addr (uiout, "addr", loc->gdbarch,
+ loc->address,
+ loc->address + loc->ranged_hw_bp_length);
+ else
+ ui_out_field_core_addr (uiout, "addr",
+ loc->gdbarch, loc->address);
}
annotate_field (5);
if (!header_of_multiple)
@@ -4492,7 +4658,10 @@ breakpoint_1 (int bnum, int allflag)
if (nr_printable_breakpoints > 0)
annotate_field (4);
if (print_address_bits <= 32)
- ui_out_table_header (uiout, 10, ui_left, "addr", "Address");/* 5 */
+ if (b && b->hw_point_flag == HW_POINT_RANGED_BREAK)
+ ui_out_table_header (uiout, 18, ui_left, "addr", "Address");
+ else
+ ui_out_table_header (uiout, 10, ui_left, "addr", "Address");/* 5 */
else
ui_out_table_header (uiout, 18, ui_left, "addr", "Address");/* 5 */
}
@@ -4681,6 +4850,23 @@ breakpoint_address_match (struct address_space *aspace1, CORE_ADDR addr1,
&& addr1 == addr2);
}
+/* Returns true if {ASPACE1,ADDR1} and {ASPACE2,ADDR2} represent the
+ same breakpoint location, but (ADDR1 <= ADDR2 <= ADDR1 + LEN1).
+ In most targets, this can only be true if ASPACE1 matches ASPACE2.
+ On targets that have global breakpoints, the address space
+ doesn't really matter. */
+
+static int
+breakpoint_address_match_range (struct address_space *aspace1, CORE_ADDR addr1,
+ CORE_ADDR len1,
+ struct address_space *aspace2, CORE_ADDR addr2)
+{
+ return ((gdbarch_has_global_breakpoints (target_gdbarch)
+ || aspace1 == aspace2)
+ && addr2 >= addr1
+ && addr2 <= addr1 + len1);
+}
+
/* Assuming LOC1 and LOC2's types' have meaningful target addresses
(breakpoint_address_is_meaningful), returns true if LOC1 and LOC2
represent the same location. */
@@ -4850,6 +5036,7 @@ set_raw_breakpoint_without_location (struct gdbarch *gdbarch,
b->syscalls_to_be_caught = NULL;
b->ops = NULL;
b->condition_not_parsed = 0;
+ b->hw_point_flag = HW_POINT_NONE;
/* Add this breakpoint to the end of the chain
so that a list of breakpoints will come out in order
@@ -4941,6 +5128,7 @@ set_raw_breakpoint (struct gdbarch *gdbarch,
/* Store the program space that was used to set the breakpoint, for
breakpoint resetting. */
b->pspace = sal.pspace;
+ b->loc->ranged_hw_bp_length = 0;
if (sal.symtab == NULL)
b->source_file = NULL;
@@ -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);
+ }
}
return i;
@@ -5789,10 +5982,15 @@ hw_watchpoint_used_count (enum bptype type, int *other_type_used)
if (breakpoint_enabled (b))
{
if (b->type == type)
- i++;
- else if ((b->type == bp_hardware_watchpoint
- || b->type == bp_read_watchpoint
- || b->type == bp_access_watchpoint))
+ {
+ i++;
+ /* Special types of hardware watchpoints can use more
+ than one slot. */
+ i += target_hw_point_extra_slot_count (b->hw_point_flag);
+ }
+ else if ((b->type == bp_hardware_watchpoint ||
+ b->type == bp_read_watchpoint ||
+ b->type == bp_access_watchpoint))
*other_type_used = 1;
}
}
@@ -6009,6 +6207,10 @@ mention (struct breakpoint *b)
do_cleanups (ui_out_chain);
break;
case bp_hardware_watchpoint:
+ if (b->hw_point_flag == HW_POINT_MASKED_WATCH)
+ ui_out_text (uiout, "Mask ");
+ else if (b->hw_point_flag == HW_POINT_RANGED_WATCH)
+ ui_out_text (uiout, "Range ");
ui_out_text (uiout, "Hardware watchpoint ");
ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, "wpt");
ui_out_field_int (uiout, "number", b->number);
@@ -6017,6 +6219,10 @@ mention (struct breakpoint *b)
do_cleanups (ui_out_chain);
break;
case bp_read_watchpoint:
+ if (b->hw_point_flag == HW_POINT_MASKED_WATCH)
+ ui_out_text (uiout, "Mask ");
+ else if (b->hw_point_flag == HW_POINT_RANGED_WATCH)
+ ui_out_text (uiout, "Range ");
ui_out_text (uiout, "Hardware read watchpoint ");
ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, "hw-rwpt");
ui_out_field_int (uiout, "number", b->number);
@@ -6025,6 +6231,10 @@ mention (struct breakpoint *b)
do_cleanups (ui_out_chain);
break;
case bp_access_watchpoint:
+ if (b->hw_point_flag == HW_POINT_MASKED_WATCH)
+ ui_out_text (uiout, "Mask ");
+ else if (b->hw_point_flag == HW_POINT_RANGED_WATCH)
+ ui_out_text (uiout, "Range ");
ui_out_text (uiout, "Hardware access (read/write) watchpoint ");
ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, "hw-awpt");
ui_out_field_int (uiout, "number", b->number);
@@ -6051,6 +6261,9 @@ mention (struct breakpoint *b)
say_where = 0;
break;
}
+
+ if (b->hw_point_flag == HW_POINT_RANGED_BREAK)
+ printf_filtered (_("Range "));
printf_filtered (_("Hardware assisted breakpoint %d"), b->number);
say_where = 1;
break;
@@ -7089,74 +7302,143 @@ watch_command_1 (char *arg, int accessflag, int from_tty)
char *exp_start = NULL;
char *exp_end = NULL;
char *tok, *id_tok_start, *end_tok;
- int toklen;
+ int toklen = -1;
char *cond_start = NULL;
char *cond_end = NULL;
int i, other_type_used, target_resources_ok = 0;
enum bptype bp_type;
int mem_cnt = 0;
int thread = -1;
+ /* Flag to indicate whether we are going to use masks for
+ the hardware watchpoint. */
+ int use_mask = 0;
+ CORE_ADDR hw_wp_mask = 0;
+
+ do {
+ /* Make sure that we actually have parameters to parse. */
+ if (arg != NULL && arg[0] != '\0')
+ {
+ toklen = strlen (arg); /* Size of argument list. */
- /* Make sure that we actually have parameters to parse. */
- if (arg != NULL && arg[0] != '\0')
- {
- toklen = strlen (arg); /* Size of argument list. */
-
- /* Points tok to the end of the argument list. */
- tok = arg + toklen - 1;
+ /* Points tok to the end of the argument list. */
+ tok = arg + toklen - 1;
- /* Go backwards in the parameters list. Skip the last parameter.
- If we're expecting a 'thread <thread_num>' parameter, this should
- be the thread identifier. */
- while (tok > arg && (*tok == ' ' || *tok == '\t'))
- tok--;
- while (tok > arg && (*tok != ' ' && *tok != '\t'))
- tok--;
+ /* Go backwards in the parameters list. Skip the last parameter.
+ If we're expecting a 'thread <thread_num>' parameter, this should
+ be the thread identifier. */
+ while (tok > arg && (*tok == ' ' || *tok == '\t'))
+ tok--;
+ while (tok > arg && (*tok != ' ' && *tok != '\t'))
+ tok--;
- /* Points end_tok to the beginning of the last token. */
- id_tok_start = tok + 1;
+ /* Points end_tok to the beginning of the last token. */
+ id_tok_start = tok + 1;
- /* Go backwards in the parameters list. Skip one more parameter.
- If we're expecting a 'thread <thread_num>' parameter, we should
- reach a "thread" token. */
- while (tok > arg && (*tok == ' ' || *tok == '\t'))
- tok--;
+ /* Go backwards in the parameters list. Skip one more parameter.
+ If we're expecting a 'thread <thread_num>' parameter, we should
+ reach a "thread" token. */
+ while (tok > arg && (*tok == ' ' || *tok == '\t'))
+ tok--;
- end_tok = tok;
+ end_tok = tok;
- while (tok > arg && (*tok != ' ' && *tok != '\t'))
- tok--;
+ while (tok > arg && (*tok != ' ' && *tok != '\t'))
+ tok--;
- /* Move the pointer forward to skip the whitespace and
- calculate the length of the token. */
- tok++;
- toklen = end_tok - tok;
+ /* Move the pointer forward to skip the whitespace and
+ calculate the length of the token. */
+ tok++;
+ toklen = end_tok - tok;
- if (toklen >= 1 && strncmp (tok, "thread", toklen) == 0)
- {
- /* At this point we've found a "thread" token, which means
- the user is trying to set a watchpoint that triggers
- only in a specific thread. */
- char *endp;
-
- /* Extract the thread ID from the next token. */
- thread = strtol (id_tok_start, &endp, 0);
-
- /* Check if the user provided a valid numeric value for the
- thread ID. */
- if (*endp != ' ' && *endp != '\t' && *endp != '\0')
- error (_("Invalid thread ID specification %s."), id_tok_start);
-
- /* Check if the thread actually exists. */
- if (!valid_thread_id (thread))
- error (_("Unknown thread %d."), thread);
-
- /* Truncate the string and get rid of the thread <thread_num>
- parameter before the parameter list is parsed by the
- evaluate_expression() function. */
- *tok = '\0';
- }
- }
+ if (toklen >= 1 && strncmp (tok, "thread", toklen) == 0)
+ {
+ /* At this point we've found a "thread" token, which means
+ the user is trying to set a watchpoint that triggers
+ only in a specific thread. */
+ char *endp;
+
+ /* Extract the thread ID from the next token. */
+ thread = strtol (id_tok_start, &endp, 0);
+
+ /* Check if the user provided a valid numeric value for the
+ thread ID. */
+ if (*endp != ' ' && *endp != '\t' && *endp != '\0')
+ error (_("Invalid thread ID specification %s."),
+ id_tok_start);
+
+ /* Check if the thread actually exists. */
+ if (!valid_thread_id (thread))
+ error (_("Unknown thread %d."), thread);
+
+ /* Truncate the string and get rid of the thread <thread_num>
+ parameter before the parameter list is parsed by the
+ evaluate_expression() function. */
+ *tok = '\0';
+ }
+ else if (toklen >= 1 && strncmp (tok, "mask", toklen) == 0)
+ {
+ /* We've found a "mask" token, which means the user wants to
+ create a hardware watchpoint that is going to have the mask
+ facility. */
+ char *tokp = tok;
+ char *addr_p;
+ char *endp;
+ char *mask_addr;
+ int len_addr;
+ struct value *mask_value;
+ struct cleanup *clean_mask_addr;
+
+ /* Does the target support masked watchpoints? */
+ if (!TARGET_CAN_USE_SPECIAL_HW_POINT_P (HW_POINT_MASKED_WATCH))
+ error (_("This target does not support the usage of masks \
+with hardware watchpoints."));
+
+ use_mask = 1;
+
+ /* Skipping the token "mask", and possible spaces. */
+ while (!isspace (*tokp) && *tokp != '\0')
+ tokp++;
+ while (isspace (*tokp))
+ tokp++;
+
+ if (*tokp == '\0')
+ error (_("You must supply the address that is going to be\n\
+used in the mask."));
+ addr_p = tokp;
+
+ while (!isspace (*addr_p) && *addr_p != '\0')
+ addr_p++;
+
+ 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."));
+
+ mask_addr = xstrndup (tokp, len_addr);
+ clean_mask_addr = make_cleanup (xfree, mask_addr);
+
+ mask_value = parse_to_comma_and_eval (&mask_addr);
+ hw_wp_mask = value_as_address (mask_value);
+
+ /* Truncate the string and get rid of the "mask <address>"
+ parameter before the parameter list is parsed by the
+ evaluate_expression() function. */
+ *tok = '\0';
+ do_cleanups (clean_mask_addr);
+ }
+ else if (toklen >= 1)
+ /* There is a token, but it is not recognized. We should
+ stop here. */
+ break;
+ }
+ } while (toklen >= 1);
/* Parse the rest of the arguments. */
innermost_block = NULL;
@@ -7208,6 +7490,11 @@ watch_command_1 (char *arg, int accessflag, int from_tty)
error (_("Expression cannot be implemented with read/access watchpoint."));
if (mem_cnt != 0)
{
+ /* If we are going to use masks, then we may need more
+ slots in order to use the hardware watchpoint. */
+ if (use_mask)
+ mem_cnt += target_hw_point_extra_slot_count (HW_POINT_MASKED_WATCH);
+
i = hw_watchpoint_used_count (bp_type, &other_type_used);
target_resources_ok =
target_can_use_hardware_watchpoint (bp_type, i + mem_cnt,
@@ -7222,7 +7509,14 @@ watch_command_1 (char *arg, int accessflag, int from_tty)
/* Change the type of breakpoint to an ordinary watchpoint if a hardware
watchpoint could not be set. */
if (!mem_cnt || target_resources_ok <= 0)
- bp_type = bp_watchpoint;
+ {
+ if (use_mask)
+ /* No way to implement a watchpoint that uses mask without
+ hardware support. */
+ error (_("Cannot use masks without hardware watchpoints."));
+ else
+ bp_type = bp_watchpoint;
+ }
frame = block_innermost_frame (exp_valid_block);
@@ -7270,6 +7564,12 @@ watch_command_1 (char *arg, int accessflag, int from_tty)
b->exp_string = savestring (exp_start, exp_end - exp_start);
b->val = val;
b->val_valid = 1;
+ if (use_mask)
+ {
+ b->loc->hw_wp_mask = hw_wp_mask;
+ b->hw_point_flag = HW_POINT_MASKED_WATCH;
+ }
+
if (cond_start)
b->cond_string = savestring (cond_start, cond_end - cond_start);
else
@@ -7300,6 +7600,10 @@ watch_command_1 (char *arg, int accessflag, int from_tty)
that should be inserted. */
update_watchpoint (b, 1);
+ if (b->hw_point_flag == HW_POINT_COND_HW_ACCEL)
+ printf_filtered (_("This watchpoint will have its condition evaluation \
+assisted by hardware.\n"));
+
mention (b);
update_global_location_list (1);
}
@@ -7351,21 +7655,25 @@ can_use_hardware_watchpoint (struct value *v)
/* Ahh, memory we actually used! Check if we can cover
it with hardware watchpoints. */
struct type *vtype = check_typedef (value_type (v));
+ /* Is the value a struct or array? */
+ int is_big_blob = TYPE_CODE (vtype) == TYPE_CODE_STRUCT
+ || TYPE_CODE (vtype) == TYPE_CODE_ARRAY;
/* We only watch structs and arrays if user asked for it
explicitly, never if they just happen to appear in a
middle of some value chain. */
- if (v == head
- || (TYPE_CODE (vtype) != TYPE_CODE_STRUCT
- && TYPE_CODE (vtype) != TYPE_CODE_ARRAY))
+ if (v == head || !is_big_blob)
{
CORE_ADDR vaddr = value_address (v);
int len = TYPE_LENGTH (value_type (v));
+ int ret;
- if (!target_region_ok_for_hw_watchpoint (vaddr, len))
+ ret = TARGET_REGION_OK_FOR_HW_WATCHPOINT (vaddr, len,
+ is_big_blob);
+ if (!ret)
return 0;
else
- found_memory_cnt++;
+ found_memory_cnt += ret;
}
}
}
@@ -7416,6 +7724,230 @@ awatch_command (char *arg, int from_tty)
{
watch_command_1 (arg, hw_access, from_tty);
}
+
+static void
+watch_range_command_1 (char *arg, int accessflag, int from_tty)
+{
+ char *exp_string, *string_p;
+ struct gdbarch *gdbarch = get_current_arch ();
+ int wp_count, other_type_used, can_use_wp, ret, err, mem_cnt;
+ CORE_ADDR start_addr;
+ ULONGEST length;
+ struct breakpoint *b;
+ struct expression *exp;
+ struct symtab_and_line sal;
+ struct value *val;
+ struct cleanup *cleanups;
+ enum bptype type;
+
+ /* Do we support ranged hardware watchpoints? */
+ if (!TARGET_CAN_USE_SPECIAL_HW_POINT_P (HW_POINT_RANGED_WATCH))
+ error (_("This target does not support ranged hardware watchpoints."));
+
+ parse_addr_range (&arg, &start_addr, &length);
+
+ exp_string = string_p = xstrprintf ("{char[%s]} %s",
+ phex (length, sizeof (length)),
+ paddress (gdbarch, start_addr));
+ exp = parse_exp_1 (&string_p, 0, 0);
+ fetch_watchpoint_value (exp, &val, NULL, NULL);
+ if (val != NULL)
+ release_value (val);
+ cleanups = make_cleanup (xfree, exp_string);
+
+ mem_cnt = can_use_hardware_watchpoint (val);
+ if (mem_cnt < 0)
+ error (_("Provided range can't be watched."));
+
+ if (accessflag == hw_read)
+ type = bp_read_watchpoint;
+ else if (accessflag == hw_access)
+ type = bp_access_watchpoint;
+ else
+ type = bp_hardware_watchpoint;
+
+ wp_count = hw_watchpoint_used_count (type,
+ &other_type_used);
+ can_use_wp = target_can_use_hardware_watchpoint (type,
+ wp_count + mem_cnt,
+ other_type_used);
+ if (can_use_wp < 0)
+ error (_("Not enough available hardware watchpoints."));
+
+ init_sal (&sal); /* initialize to zeroes */
+ sal.pspace = current_program_space;
+
+ /* Now set up the breakpoint. */
+ b = set_raw_breakpoint (gdbarch, sal, accessflag);
+ set_breakpoint_count (breakpoint_count + 1);
+ b->number = breakpoint_count;
+ b->thread = -1;
+ b->disposition = disp_donttouch;
+ b->exp = exp;
+ b->exp_string = exp_string;
+ b->hw_point_flag = HW_POINT_RANGED_WATCH;
+ if (val)
+ {
+ b->val = val;
+ b->val_valid = 1;
+ }
+ b->watchpoint_frame = null_frame_id;
+
+ mention (b);
+ update_global_location_list (1);
+
+ discard_cleanups (cleanups);
+}
+
+static void
+watch_range_command (char *arg, int from_tty)
+{
+ watch_range_command_1 (arg, bp_hardware_watchpoint, from_tty);
+}
+
+static void
+awatch_range_command (char *arg, int from_tty)
+{
+ watch_range_command_1 (arg, bp_access_watchpoint, from_tty);
+}
+
+static void
+rwatch_range_command (char *arg, int from_tty)
+{
+ watch_range_command_1 (arg, bp_read_watchpoint, from_tty);
+}
+
+static void
+hbreak_range_command (char *arg, int from_tty)
+{
+ int bp_count, can_use_bp, ret, err;
+ int parse_line = 0;
+ CORE_ADDR start_addr;
+ ULONGEST length;
+ struct breakpoint *b;
+ struct symtab_and_line sal;
+ struct gdbarch *gdbarch = get_current_arch ();
+
+ /* Do we support ranged hardware breakpoints? */
+ if (!TARGET_CAN_USE_SPECIAL_HW_POINT_P (HW_POINT_RANGED_BREAK))
+ error (_("This target does not support ranged hardware breakpoints."));
+
+ if (arg != NULL && *arg != '\0')
+ {
+ while (isspace (*arg))
+ arg++;
+
+ /* Is this an address (of the form `0x...'), or a line? */
+ if (arg[0] == '0' && arg[1] == 'x')
+ parse_line = 0;
+ else
+ parse_line = 1;
+ }
+
+ if (parse_line)
+ {
+ char *l1 = arg, *l2;
+ char *line1, *line2;
+ int size;
+ struct symtabs_and_lines sals1, sals2;
+ struct symtab_and_line sal1, sal2;
+
+ l2 = arg;
+ while (*l2 != ',')
+ l2++;
+
+ size = l2 - l1;
+
+ *l2 = '\0';
+ l2++;
+ while (isspace (*l2))
+ l2++;
+
+ line1 = l1;
+ line2 = l2;
+
+ /* Parsing first line. */
+ sals1 = decode_line_1 (&line1, 1, NULL, 0, NULL, NULL);
+
+ if (sals1.nelts != 1)
+ error (_("Could not get information on specified line `%s'."),
+ line1);
+ sal1 = sals1.sals[0];
+ xfree (sals1.sals);
+
+ if (*line1)
+ error (_("Junk at end of arguments."));
+
+ resolve_sal_pc (&sal1);
+
+ /* Parsing second line. */
+ sals2 = decode_line_1 (&line2, 1, NULL, 0, NULL, NULL);
+
+ if (sals2.nelts != 1)
+ error (_("Could not get information on specified line `%s'."),
+ line2);
+ sal2 = sals2.sals[0];
+ xfree (sals2.sals);
+
+ if (*line2)
+ error (_("Junk at end of arguments."));
+
+ resolve_sal_pc (&sal2);
+
+ /* 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."));
+
+ start_addr = sal1.pc;
+ length = sal2.pc - sal1.pc;
+
+ 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"));
+ hbreak_command (l1, 1);
+ return;
+ }
+ }
+ else
+ parse_addr_range (&arg, &start_addr, &length);
+
+ bp_count = hw_breakpoint_used_count ();
+ /* For ranged hardware breakpoints, we may have to use 2 slots
+ instead of 1. */
+ bp_count += target_hw_point_extra_slot_count (HW_POINT_RANGED_BREAK);
+ can_use_bp = target_can_use_hardware_watchpoint (bp_hardware_breakpoint,
+ bp_count + 1,
+ 0);
+ if (can_use_bp < 0)
+ error (_("Not enough available hardware breakpoints."));
+
+ sal = find_pc_line (start_addr, 0);
+ sal.pc = start_addr;
+ sal.section = find_pc_overlay (start_addr);
+ sal.explicit_pc = 1;
+ sal.pspace = current_program_space;
+
+ /* Now set up the breakpoint. */
+ b = set_raw_breakpoint (gdbarch, sal, bp_hardware_breakpoint);
+ set_breakpoint_count (breakpoint_count + 1);
+ b->number = breakpoint_count;
+ b->thread = -1;
+ b->disposition = disp_donttouch;
+ b->exp = NULL;
+ b->exp_string = NULL;
+ b->val = NULL;
+ b->val_valid = 0;
+ b->hw_point_flag = HW_POINT_RANGED_BREAK;
+ b->loc->ranged_hw_bp_length = length;
+
+ mention (b);
+ update_global_location_list (1);
+}
\f
/* Helper routines for the until_command routine in infcmd.c. Here
@@ -8854,6 +9386,18 @@ update_breakpoint_locations (struct breakpoint *b,
b->number, e.message);
new_loc->enabled = 0;
}
+
+ if (b->type == bp_hardware_watchpoint
+ && new_loc->cond
+ && TARGET_CAN_USE_SPECIAL_HW_POINT_P (HW_POINT_COND_HW_ACCEL)
+ && TARGET_CAN_USE_WATCHPOINT_COND_ACCEL_P (new_loc))
+ {
+ TARGET_GET_WATCHPOINT_COND_ACCEL_ADDR (new_loc,
+ &new_loc->cond_hw_addr);
+ new_loc->owner->hw_point_flag = HW_POINT_COND_HW_ACCEL;
+ }
+ else
+ new_loc->owner->hw_point_flag = HW_POINT_NONE;
}
if (b->source_file != NULL)
@@ -10019,6 +10563,230 @@ tracepoint_save_command (char *args, int from_tty)
return;
}
+/* This function checks if the expression associated
+ with the breakpoint `b' is of the form "*<address>".
+ It returns 1 if it is, 0 otherwise. */
+static int
+exp_is_address_p (struct breakpoint *b)
+{
+ /* Check that the associated tree corresponds to that expression,
+ that is 5 elements, first a UNOP_IND, and then an OP_LONG. */
+ if (b->exp->nelts != 5
+ || b->exp->elts[0].opcode != UNOP_IND
+ || b->exp->elts[1].opcode != OP_LONG)
+ return 0;
+ return 1;
+}
+
+/* This function checks if the expression associated
+ with the breakpoint `b' is of the form "<var>".
+ It returns 1 if it is, 0 otherwise. */
+static int
+exp_is_var_p (struct breakpoint *b)
+{
+ /* Check that the associated tree corresponds to that expression,
+ that is 4 elements, first a OP_VAR_VALUE. */
+ if (b->exp->nelts != 4
+ || b->exp->elts[0].opcode != OP_VAR_VALUE)
+ return 0;
+ return 1;
+}
+
+/* This function checks if the condition associated
+ with the bp_location `b' is of the form "*<address> == LITERAL".
+ It returns 1 if it is, 0 otherwise. */
+static int
+cond_is_address_equal_literal_p (struct bp_location *b)
+{
+ /* Check the watchpoint condition expression. It should be
+ of the form "*<address> EQUAL <litteral>", where EQUAL is the
+ equality binary operator. The associated tree should have
+ exactly 10 elements in it, all in a very specific order. */
+ if (b->cond->nelts != 10
+ || b->cond->elts[0].opcode != BINOP_EQUAL
+ || b->cond->elts[1].opcode != UNOP_IND
+ || b->cond->elts[2].opcode != OP_LONG
+ || b->cond->elts[6].opcode != OP_LONG)
+ return 0;
+ return 1;
+}
+
+/* This function checks if the condition associated
+ with the bp_location `b' is of the form "<var> == LITERAL".
+ It returns 1 if it is, 0 otherwise. */
+static int
+cond_is_var_equal_literal_p (struct bp_location *b)
+{
+ /* Check the watchpoint condition expression. It should be
+ of the form "<variable> EQUAL <litteral>", where EQUAL is the
+ equality binary operator. The associated tree should have
+ exactly 9 elements in it, all in a very specific order. */
+ if (b->cond->nelts != 9
+ || b->cond->elts[0].opcode != BINOP_EQUAL
+ || b->cond->elts[1].opcode != OP_VAR_VALUE
+ || b->cond->elts[5].opcode != OP_LONG)
+ return 0;
+ return 1;
+}
+
+/* This function returns the address of a variable VAR which
+ resides on the block B. */
+static CORE_ADDR
+get_var_address (struct symbol *var, struct block *b)
+{
+ struct value *v = address_of_variable (var, b);
+ return value_as_address (v);
+}
+
+/* This function is used to determine whether the condition associated
+ with bp_location B is of the form:
+
+ watch *<ADDRESS> if <VAR> == <LITERAL>
+
+ If it is, then it sets DATA_VALUE to LITERAL and returns 1.
+ Otherwise, it returns 0. */
+int
+default_watch_address_if_var_equal_literal_p (struct bp_location *b,
+ CORE_ADDR *data_value)
+{
+ CORE_ADDR exp_address,
+ cond_address;
+ struct breakpoint *bp = b->owner;
+
+ if (!exp_is_address_p (bp)
+ || !cond_is_var_equal_literal_p (b))
+ return 0;
+
+ exp_address = bp->exp->elts[3].longconst;
+ cond_address = get_var_address (b->cond->elts[3].symbol,
+ b->cond->elts[2].block);
+
+ /* 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;
+ }
+
+ /* At this point, all verifications were positive, so we can use
+ hardware-assisted data-matching. Set the data value, and return
+ non-zero. */
+ *data_value = b->cond->elts[7].longconst;
+
+ return 1;
+}
+
+/* This function is used to determine whether the condition associated
+ with bp_location B is of the form:
+
+ watch <VAR> if *<ADDRESS> == <LITERAL>
+
+ If it is, then it sets DATA_VALUE to LITERAL and returns 1.
+ Otherwise, it returns 0. */
+int
+default_watch_var_if_address_equal_literal_p (struct bp_location *b,
+ CORE_ADDR *data_value)
+{
+ CORE_ADDR exp_address,
+ cond_address;
+ struct breakpoint *bp = b->owner;
+
+ if (!exp_is_var_p (bp)
+ || !cond_is_address_equal_literal_p (b))
+ return 0;
+
+ exp_address = get_var_address (bp->exp->elts[2].symbol,
+ bp->exp->elts[1].block);
+ cond_address = b->cond->elts[4].longconst;
+
+ /* 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;
+ }
+
+ /* At this point, all verifications were positive, so we can use
+ hardware-assisted data-matching. Set the data value, and return
+ non-zero. */
+ *data_value = b->cond->elts[8].longconst;
+
+ return 1;
+}
+
+/* This function is used to determine whether the condition associated
+ with bp_location B is of the form:
+
+ watch <VAR> if <VAR> == <LITERAL>
+
+ If it is, then it sets DATA_VALUE to LITERAL and returns 1.
+ Otherwise, it returns 0. */
+int
+default_watch_var_if_var_equal_literal_p (struct bp_location *b,
+ CORE_ADDR *data_value)
+{
+ char *name_exp, *name_cond;
+ struct breakpoint *bp = b->owner;
+
+ if (!exp_is_var_p (bp)
+ || !cond_is_var_equal_literal_p (b))
+ return 0;
+
+ name_exp = bp->exp->elts[2].symbol->ginfo.name;
+ name_cond = b->cond->elts[3].symbol->ginfo.name;
+
+ /* 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;
+ }
+
+ /* At this point, all verifications were positive, so we can use
+ hardware-assisted data-matching. Set the data value, and return
+ non-zero. */
+ *data_value = b->cond->elts[7].longconst;
+
+ return 1;
+}
+
+/* This function is used to determine whether the condition associated
+ with bp_location B is of the form:
+
+ watch *<ADDRESS> if *<ADDRESS> == <LITERAL>
+
+ If it is, then it sets DATA_VALUE to LITERAL and returns 1.
+ Otherwise, it returns 0. */
+int
+default_watch_address_if_address_equal_literal_p (struct bp_location *b,
+ CORE_ADDR *data_value)
+{
+ CORE_ADDR exp_address,
+ cond_address;
+ struct breakpoint *bp = b->owner;
+
+ if (!exp_is_address_p (bp)
+ || !cond_is_address_equal_literal_p (b))
+ return 0;
+
+ exp_address = bp->exp->elts[3].longconst;
+ cond_address = b->cond->elts[4].longconst;
+
+ /* 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;
+ }
+
+ /* At this point, all verifications were positive, so we can use
+ hardware-assisted data-matching. Set the data value, and return
+ non-zero. */
+ *data_value = b->cond->elts[8].longconst;
+
+ return 1;
+}
+
/* Create a vector of all tracepoints. */
VEC(breakpoint_p) *
@@ -10568,7 +11336,51 @@ inferior in all-stop mode, gdb behaves as if always-inserted mode is off."),
&show_always_inserted_mode,
&breakpoint_set_cmdlist,
&breakpoint_show_cmdlist);
-
+
+ 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."));
+ set_cmd_completer (c, expression_completer);
+
+ c = add_com ("awatch-range", class_breakpoint, awatch_range_command, _("\
+Set an ACCESS 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\
+accesses (reads from or writes to) any address within that range."));
+ set_cmd_completer (c, expression_completer);
+
+ c = add_com ("rwatch-range", class_breakpoint, rwatch_range_command, _("\
+Set a READ 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\
+reads any address within that range."));
+ set_cmd_completer (c, expression_completer);
+
+ add_com ("hbreak-range", class_breakpoint, hbreak_range_command, _("\
+Set a hardware assisted breakpoint 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\
+ start-line, end-line\n\
+\n\
+The breakpoint will stop execution of your program whenever the inferior\n\
+executes any address within that range."));
+
automatic_hardware_breakpoints = 1;
observer_attach_about_to_proceed (breakpoint_about_to_proceed);
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 5ebd36c..4e49473 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -186,6 +186,11 @@ struct bp_target_info
is used to determine the type of breakpoint to insert. */
CORE_ADDR placed_address;
+ /* If this is a ranged hardware breakpoint, then we can use this
+ field in order to store the length of the range that will be
+ watched for execution. */
+ ULONGEST length;
+
/* If the breakpoint lives in memory and reading that memory would
give back the breakpoint, instead of the original contents, then
the original contents are cached here. Only SHADOW_LEN bytes of
@@ -246,6 +251,25 @@ struct bp_location
different locations. */
struct expression *cond;
+ /* Flag to indicate if the condition is going to be accelerated
+ by hardware. If its value is non-zero, then GDB checks the
+ condition using hardware acceleration; otherwise it uses the
+ regular software-based checking. */
+ int cond_hw_accel : 1;
+
+ /* If the condition can be hardware-accelerated, then we must
+ get the condition's variable address so that GDB can
+ properly set the evaluation via hardware. */
+ CORE_ADDR cond_hw_addr;
+
+ /* If we are inserting a ranged hardware breakpoint, then we must
+ set its length here. */
+ ULONGEST ranged_hw_bp_length;
+
+ /* The mask address for this hardware watchpoint. It is valid only
+ if `hw_wp_use_mask' is 1. */
+ CORE_ADDR hw_wp_mask;
+
/* This location's address is in an unloaded solib, and so this
location should not be inserted. It will be automatically
enabled when that solib is loaded. */
@@ -373,6 +397,16 @@ DEF_VEC_I(int);
typedef struct bp_location *bp_location_p;
DEF_VEC_P(bp_location_p);
+/* Special flags for hardware breakpoints/watchpoints. */
+enum hw_point_flag {
+ HW_POINT_NONE = 0,
+ HW_POINT_RANGED_WATCH, /* Hardware ranged watchpoint. */
+ HW_POINT_RANGED_BREAK, /* Hardware ranged breakpoint. */
+ HW_POINT_MASKED_WATCH, /* Hardware masked watchpoint. */
+ HW_POINT_COND_HW_ACCEL, /* Hardware watchpoint with condition
+ hardware-accelerated. */
+};
+
/* Note that the ->silent field is not currently used by any commands
(though the code is in there if it was to be, and set_raw_breakpoint
does set it to 0). I implemented it because I thought it would be
@@ -512,6 +546,9 @@ struct breakpoint
/* Chain of action lines to execute when this tracepoint is hit. */
struct action_line *actions;
+
+ /* Special flags. */
+ enum hw_point_flag hw_point_flag;
};
typedef struct breakpoint *breakpoint_p;
@@ -992,4 +1029,20 @@ extern struct breakpoint *get_tracepoint_by_number (char **arg, int multi_p,
is newly allocated; the caller should free when done with it. */
extern VEC(breakpoint_p) *all_tracepoints (void);
+/* Return greater than zero if the condition associated with
+ the watchpoint `b' can be treated by the hardware; zero otherwise.
+
+ Also stores the data value in `data_value'. */
+extern int default_watch_address_if_address_equal_literal_p (struct bp_location *b,
+ CORE_ADDR *data_value);
+
+extern int default_watch_var_if_var_equal_literal_p (struct bp_location *b,
+ CORE_ADDR *data_value);
+
+extern int default_watch_address_if_var_equal_literal_p (struct bp_location *b,
+ CORE_ADDR *data_value);
+
+extern int default_watch_var_if_address_equal_literal_p (struct bp_location *b,
+ CORE_ADDR *data_value);
+
#endif /* !defined (BREAKPOINT_H) */
diff --git a/gdb/findcmd.c b/gdb/findcmd.c
index 1d28914..85f229c 100644
--- a/gdb/findcmd.c
+++ b/gdb/findcmd.c
@@ -45,6 +45,75 @@ put_bits (bfd_uint64_t data, char *buf, int bits, bfd_boolean big_p)
}
}
+/* Reads an address range, in one of the following formats:
+
+ start-address, end-address
+ start-address, +length
+
+ ARGS will be set to the first character after the end-address or length,
+ or if that character is a comma, the character following it. If a parser
+ error occurs, an exception is thrown and none of the arguments is
+ touched. */
+
+void
+parse_addr_range (char **args, CORE_ADDR *start_addrp,
+ ULONGEST *search_space_lenp)
+{
+ char *s = *args;
+ CORE_ADDR start_addr;
+ ULONGEST search_space_len;
+ struct value *v;
+
+ v = parse_to_comma_and_eval (&s);
+ start_addr = value_as_address (v);
+
+ if (*s == ',')
+ ++s;
+ while (isspace (*s))
+ ++s;
+
+ if (*s == '+')
+ {
+ LONGEST len;
+ ++s;
+ v = parse_to_comma_and_eval (&s);
+ len = value_as_long (v);
+
+ if (len == 0)
+ error (_("Empty search range."));
+ else if (len < 0)
+ error (_("Invalid length."));
+ /* Watch for overflows. */
+ else if (len > CORE_ADDR_MAX
+ || (start_addr + len - 1) < start_addr)
+ error (_("Search space too large."));
+
+ search_space_len = len;
+ }
+ else
+ {
+ CORE_ADDR end_addr;
+
+ v = parse_to_comma_and_eval (&s);
+ end_addr = value_as_address (v);
+ if (start_addr > end_addr)
+ error (_("Invalid search space, end preceeds start."));
+ search_space_len = end_addr - start_addr + 1;
+ /* We don't support searching all of memory
+ (i.e. start=0, end = 0xff..ff).
+ Bail to avoid overflows later on. */
+ if (search_space_len == 0)
+ error (_("Overflow in address range computation, choose smaller range."));
+ }
+
+ if (*s == ',')
+ ++s;
+
+ *args = s;
+ *start_addrp = start_addr;
+ *search_space_lenp = search_space_len;
+}
+
/* Subroutine of find_command to simplify it.
Parse the arguments of the "find" command. */
@@ -114,51 +183,7 @@ parse_find_args (char *args, ULONGEST *max_countp,
}
/* Get the search range. */
-
- v = parse_to_comma_and_eval (&s);
- start_addr = value_as_address (v);
-
- if (*s == ',')
- ++s;
- while (isspace (*s))
- ++s;
-
- if (*s == '+')
- {
- LONGEST len;
- ++s;
- v = parse_to_comma_and_eval (&s);
- len = value_as_long (v);
- if (len == 0)
- {
- printf_filtered (_("Empty search range.\n"));
- return;
- }
- if (len < 0)
- error (_("Invalid length."));
- /* Watch for overflows. */
- if (len > CORE_ADDR_MAX
- || (start_addr + len - 1) < start_addr)
- error (_("Search space too large."));
- search_space_len = len;
- }
- else
- {
- CORE_ADDR end_addr;
- v = parse_to_comma_and_eval (&s);
- end_addr = value_as_address (v);
- if (start_addr > end_addr)
- error (_("Invalid search space, end preceeds start."));
- search_space_len = end_addr - start_addr + 1;
- /* We don't support searching all of memory
- (i.e. start=0, end = 0xff..ff).
- Bail to avoid overflows later on. */
- if (search_space_len == 0)
- error (_("Overflow in address range computation, choose smaller range."));
- }
-
- if (*s == ',')
- ++s;
+ parse_addr_range (&s, &start_addr, &search_space_len);
/* Fetch the search string. */
diff --git a/gdb/testsuite/gdb.base/watchpoint.c b/gdb/testsuite/gdb.base/watchpoint.c
index bba97fa..1e95400 100644
--- a/gdb/testsuite/gdb.base/watchpoint.c
+++ b/gdb/testsuite/gdb.base/watchpoint.c
@@ -30,7 +30,7 @@ int ival2 = -1;
int ival3 = -1;
int ival4 = -1;
int ival5 = -1;
-char buf[10];
+char buf[30];
struct foo
{
int val;
@@ -95,6 +95,7 @@ func3 ()
x = 1; /* second x assignment */
y = 1;
y = 2;
+ buf[26] = 3;
}
int
diff --git a/gdb/testsuite/gdb.base/watchpoint.exp b/gdb/testsuite/gdb.base/watchpoint.exp
index 9fee73b..ee55e76 100644
--- a/gdb/testsuite/gdb.base/watchpoint.exp
+++ b/gdb/testsuite/gdb.base/watchpoint.exp
@@ -678,6 +678,24 @@ proc test_inaccessible_watchpoint {} {
}
}
+proc test_watchpoint_in_big_blob {} {
+ global gdb_prompt
+
+ gdb_test "watch buf" ".*atchpoint \[0-9\]+: buf"
+ send_gdb "cont\n"
+ gdb_expect {
+ -re "Continuing.*\[Ww\]atchpoint.*buf.*Old value = .*$gdb_prompt $" {
+ pass "watchpoint on buf hit"
+ }
+ -re "Continuing.*$gdb_prompt $" {
+ fail "watchpoint on buf hit"
+ }
+ -re ".*$gdb_prompt $" { fail "watchpoint on buf hit" ; return }
+ timeout { fail "watchpoint on buf hit (timeout)" ; return }
+ eof { fail "watchpoint on buf hit (eof)" ; return }
+ }
+}
+
# Start with a fresh gdb.
gdb_exit
@@ -842,6 +860,8 @@ if [initialize] then {
}
test_watchpoint_and_breakpoint
+
+ test_watchpoint_in_big_blob
}
# Restore old timeout
diff --git a/gdb/ui-out.c b/gdb/ui-out.c
index 19a4644..86e7c82 100644
--- a/gdb/ui-out.c
+++ b/gdb/ui-out.c
@@ -486,6 +486,45 @@ ui_out_field_fmt_int (struct ui_out *uiout,
}
void
+ui_out_field_range_core_addr (struct ui_out *uiout,
+ const char *fldname,
+ struct gdbarch *gdbarch,
+ CORE_ADDR address_start,
+ CORE_ADDR address_end)
+{
+ char addstr[40];
+ int addr_bit = gdbarch_addr_bit (gdbarch);
+
+ if (addr_bit < (sizeof (CORE_ADDR) * HOST_CHAR_BIT))
+ {
+ 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)
+ {
+ strcpy (addstr, "[");
+ strcat (addstr, hex_string_custom (address_start, 8));
+ strcat (addstr, ",");
+ strcat (addstr, hex_string_custom (address_end, 8));
+ strcat (addstr, "]");
+ }
+ else
+ {
+ strcpy (addstr, "[");
+ strcat (addstr, hex_string_custom (address_start, 16));
+ strcat (addstr, ",");
+ strcat (addstr, hex_string_custom (address_end, 16));
+ strcat (addstr, "]");
+ }
+
+ ui_out_field_string (uiout, fldname, addstr);
+}
+
+void
ui_out_field_core_addr (struct ui_out *uiout,
const char *fldname,
struct gdbarch *gdbarch,
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index 4f3b7a4..43336c4 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -114,6 +114,11 @@ 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,
+ CORE_ADDR address_start,
+ CORE_ADDR address_end);
+
extern void ui_out_field_core_addr (struct ui_out *uiout, const char *fldname,
struct gdbarch *gdbarch, CORE_ADDR address);
diff --git a/gdb/value.h b/gdb/value.h
index 993f05b..d9cf12d 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -521,6 +521,9 @@ extern CORE_ADDR parse_and_eval_address_1 (char **expptr);
extern LONGEST parse_and_eval_long (char *exp);
+void parse_addr_range (char **args, CORE_ADDR *start_addrp,
+ ULONGEST *search_space_lenp);
+
extern void unop_promote (const struct language_defn *language,
struct gdbarch *gdbarch,
struct value **arg1);
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/2] Support the new PPC476 processor -- Arch Independent
2009-12-16 20:49 [PATCH 1/2] Support the new PPC476 processor -- Arch Independent Sérgio Durigan Júnior
@ 2009-12-18 11:28 ` Eli Zaretskii
2009-12-29 18:55 ` Luis Machado
2009-12-30 2:00 ` Thiago Jung Bauermann
0 siblings, 2 replies; 9+ messages in thread
From: Eli Zaretskii @ 2009-12-18 11:28 UTC (permalink / raw)
To: Sérgio Durigan Júnior; +Cc: gdb-patches, bauerman, luisgpm, tyrlik
> From: Sérgio_Durigan_Júnior <sergiodj@linux.vnet.ibm.com>
> Date: Wed, 16 Dec 2009 18:47:19 -0200
> Cc: Thiago Jung Bauermann <bauerman@br.ibm.com>, Luis Machado <luisgpm@linux.vnet.ibm.com>, Matt Tyrlik <tyrlik@us.ibm.com>
>
> This is the patch that modifies the architecture-independent files. It is
> responsible for the implementation of the new commands (such as hbreak-range
> and watch-range), and the logic that handles the new types of hardware
> breakpoints/watchpoints.
Thanks.
> + /* We've got to save some special fields before updating
> + this watchpoint. */
> + switch (b->hw_point_flag)
> + {
> + case HW_POINT_MASKED_WATCH:
> + mask = b->loc->hw_wp_mask;
> + break;
> + case HW_POINT_RANGED_WATCH:
> + range = b->loc->ranged_hw_bp_length;
> + break;
What's the need for saving and later restoring these fields?
> + 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?
> + if (b->type == bp_hardware_watchpoint
> + && TARGET_CAN_USE_SPECIAL_HW_POINT_P (HW_POINT_COND_HW_ACCEL)
> + && TARGET_CAN_USE_WATCHPOINT_COND_ACCEL_P (b->loc))
> + {
> + TARGET_GET_WATCHPOINT_COND_ACCEL_ADDR (b->loc,
> + &b->loc->cond_hw_addr);
> + b->hw_point_flag = HW_POINT_COND_HW_ACCEL;
> + }
> + else
> + b->hw_point_flag = HW_POINT_NONE;
Instead of having all this target-dependent logic here, can't we have
it inside target-specific code?
> - val = target_insert_watchpoint (bpt->address,
> - bpt->length,
> - bpt->watchpoint_type);
> + if (bpt->owner->hw_point_flag == HW_POINT_MASKED_WATCH)
> + val = target_insert_mask_watchpoint (bpt->address,
> + bpt->length,
> + bpt->watchpoint_type,
> + bpt->hw_wp_mask);
> + else if (bpt->owner->hw_point_flag == HW_POINT_COND_HW_ACCEL)
> + val = target_insert_cond_accel_watchpoint (bpt->address,
> + bpt->length,
> + bpt->watchpoint_type,
> + bpt->cond_hw_addr);
> + else
> + val = target_insert_watchpoint (bpt->address,
> + bpt->length,
> + bpt->watchpoint_type);
Again, why cannot this be on the target side?
> - && 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?
> + if (b->hw_point_flag == HW_POINT_MASKED_WATCH)
> + ui_out_text (uiout, "\nCheck the underlying instruction \
> +at PC for address and value related to this watchpoint trigger.\n");
Sorry, I don't understand what does this message mean. Can you
explain?
> -/* Check watchpoint condition. */
> +/* Check watchpoint condition. We can't use value_equal because it coerces
> + an array to a pointer, thus comparing just the address of the array instead
> + of its contents. This is not what we want. */
> +
> +static int
> +value_equal_watchpoint (struct value *arg1, struct value *arg2)
> +{
> + struct type *type1, *type2;
> +
> + type1 = check_typedef (value_type (arg1));
> + type2 = check_typedef (value_type (arg2));
> +
> + return TYPE_CODE (type1) == TYPE_CODE (type2)
> + && TYPE_LENGTH (type1) == TYPE_LENGTH (type2)
> + && memcmp (value_contents (arg1), value_contents (arg2),
> + TYPE_LENGTH (type1)) == 0;
> +}
>
> static int
> watchpoint_check (void *p)
> @@ -3246,7 +3388,7 @@ watchpoint_check (void *p)
>
> fetch_watchpoint_value (b->exp, &new_val, NULL, NULL);
> if ((b->val != NULL) != (new_val != NULL)
> - || (b->val != NULL && !value_equal (b->val, new_val)))
> + || (b->val != NULL && !value_equal_watchpoint (b->val, new_val)))
Can you elaborate the need for this change? It seems to change the
semantics of watchpoint_check, so I wonder why it is done.
> + 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"?).
> @@ -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?
> @@ -5789,10 +5982,15 @@ hw_watchpoint_used_count (enum bptype type, int *other_type_used)
> if (breakpoint_enabled (b))
> {
> if (b->type == type)
> - i++;
> - else if ((b->type == bp_hardware_watchpoint
> - || b->type == bp_read_watchpoint
> - || b->type == bp_access_watchpoint))
> + {
> + i++;
> + /* Special types of hardware watchpoints can use more
> + than one slot. */
> + i += target_hw_point_extra_slot_count (b->hw_point_flag);
Same here.
> + /* Does the target support masked watchpoints? */
> + if (!TARGET_CAN_USE_SPECIAL_HW_POINT_P (HW_POINT_MASKED_WATCH))
> + error (_("This target does not support the usage of masks \
> +with hardware watchpoints."));
Can't the functionality be emulated by GDB's application code?
> + 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?
> + 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?
> + if (can_use_wp < 0)
> + error (_("Not enough available hardware watchpoints."));
"Not enough hardware resources for specified watchpoints" is more
accurate, I think.
> + /* 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?
> + 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.
> + if (can_use_bp < 0)
> + error (_("Not enough available hardware breakpoints."));
See the comment above about a similar message.
> + /* 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 _().)
> + /* 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.
> + /* 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?
> + 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.
Anyway, this command (and all the other command changes and additions)
need a suitable change to the user manual (although I would suggest to
wait with that until we resolve the UI issues I raised in my previous
message in this thread.)
> + if (start_addr > end_addr)
> + error (_("Invalid search space, end preceeds start."));
See the comment above about a similar message.
Thanks for working on this.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/2] Support the new PPC476 processor -- Arch Independent
2009-12-18 11:28 ` Eli Zaretskii
@ 2009-12-29 18:55 ` Luis Machado
2009-12-29 18:59 ` Luis Machado
2009-12-30 2:00 ` Thiago Jung Bauermann
1 sibling, 1 reply; 9+ messages in thread
From: Luis Machado @ 2009-12-29 18:55 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: gdb-patches, bauerman, tyrlik
Hi,
On Fri, 2009-12-18 at 13:30 +0200, Eli Zaretskii wrote:
> > From: Sérgio_Durigan_Júnior <sergiodj@linux.vnet.ibm.com>
> > Date: Wed, 16 Dec 2009 18:47:19 -0200
> > Cc: Thiago Jung Bauermann <bauerman@br.ibm.com>, Luis Machado <luisgpm@linux.vnet.ibm.com>, Matt Tyrlik <tyrlik@us.ibm.com>
> >
> > This is the patch that modifies the architecture-independent files. It is
> > responsible for the implementation of the new commands (such as hbreak-range
> > and watch-range), and the logic that handles the new types of hardware
> > breakpoints/watchpoints.
>
> Thanks.
>
> > + /* We've got to save some special fields before updating
> > + this watchpoint. */
> > + switch (b->hw_point_flag)
> > + {
> > + case HW_POINT_MASKED_WATCH:
> > + mask = b->loc->hw_wp_mask;
> > + break;
> > + case HW_POINT_RANGED_WATCH:
> > + range = b->loc->ranged_hw_bp_length;
> > + break;
>
> What's the need for saving and later restoring these fields?
Ok, there's a bit of history behind the need for this...
Since GDB's expression parsing is already too complex, no other syntaxes
can be added to it (like the ones used in range/mask expressions)
without having to deal with a bunch of problems and corner cases.
So the code for range/mask breaks/watches parses the expression
represented in our own syntax and stores that data inside fields in the
location structure. But, GDB's breakpoint infrastructure deletes the
whole breakpoint on some occasions, and the data needs to be restored
from the watchpoint/breakpoint expression.
So we need a way to recover the data without having to re-parse the
expression (since we use our own syntax and not GDB's existing parser).
In the end, this is the solution we sticked with.
May not be the best solution, but we're open to suggestions.
> > + 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?
>
It doesn't look too clear to me, i'll have to double check it.
> > + if (b->type == bp_hardware_watchpoint
> > + && TARGET_CAN_USE_SPECIAL_HW_POINT_P (HW_POINT_COND_HW_ACCEL)
> > + && TARGET_CAN_USE_WATCHPOINT_COND_ACCEL_P (b->loc))
> > + {
> > + TARGET_GET_WATCHPOINT_COND_ACCEL_ADDR (b->loc,
> > + &b->loc->cond_hw_addr);
> > + b->hw_point_flag = HW_POINT_COND_HW_ACCEL;
> > + }
> > + else
> > + b->hw_point_flag = HW_POINT_NONE;
>
> Instead of having all this target-dependent logic here, can't we have
> it inside target-specific code?
The target-dependent code doesn't have access to all the details from
the breakpoint structure. Some functions receive only the address of a
breakpoint, or the address and the type of watchpoint
(read/write/access), and not the entire breakpoint structure . We may
need to re-work GDB's infrastructure in order to move that code away
from target-independent locations like this one.
>
> > - val = target_insert_watchpoint (bpt->address,
> > - bpt->length,
> > - bpt->watchpoint_type);
> > + if (bpt->owner->hw_point_flag == HW_POINT_MASKED_WATCH)
> > + val = target_insert_mask_watchpoint (bpt->address,
> > + bpt->length,
> > + bpt->watchpoint_type,
> > + bpt->hw_wp_mask);
> > + else if (bpt->owner->hw_point_flag == HW_POINT_COND_HW_ACCEL)
> > + val = target_insert_cond_accel_watchpoint (bpt->address,
> > + bpt->length,
> > + bpt->watchpoint_type,
> > + bpt->cond_hw_addr);
> > + else
> > + val = target_insert_watchpoint (bpt->address,
> > + bpt->length,
> > + bpt->watchpoint_type);
>
> Again, why cannot this be on the target side?
>
Same as above... There's lack of information about the breakpoint in the
target-specific code.
> > - && 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?
>
If the target-specific code knows what kind of breakpoint/watchpoints
it's dealing with precisely, we can do that.
> > + if (b->hw_point_flag == HW_POINT_MASKED_WATCH)
> > + ui_out_text (uiout, "\nCheck the underlying instruction \
> > +at PC for address and value related to this watchpoint trigger.\n");
>
> Sorry, I don't understand what does this message mean. Can you
> explain?
>
The history behind this is that the kernel is unable to provide us the
precise data address for the watchpoint trigger (stopped_data_address).
Mask watchpoints can monitor non-continguous address ranges, so it's
hard to monitor whether the value in those addresses changed or not,
because we don't know what the trigger address is. We could try doing
this check without the trigger address, but it would be slow.
So, with the above in mind, we leave it to the user to check whether the
value changed or not, since the user has access to the instruction that
caused the trigger (the previous instruction). That's what the message
tries to pass to the user.
> > -/* Check watchpoint condition. */
> > +/* Check watchpoint condition. We can't use value_equal because it coerces
> > + an array to a pointer, thus comparing just the address of the array instead
> > + of its contents. This is not what we want. */
> > +
> > +static int
> > +value_equal_watchpoint (struct value *arg1, struct value *arg2)
> > +{
> > + struct type *type1, *type2;
> > +
> > + type1 = check_typedef (value_type (arg1));
> > + type2 = check_typedef (value_type (arg2));
> > +
> > + return TYPE_CODE (type1) == TYPE_CODE (type2)
> > + && TYPE_LENGTH (type1) == TYPE_LENGTH (type2)
> > + && memcmp (value_contents (arg1), value_contents (arg2),
> > + TYPE_LENGTH (type1)) == 0;
> > +}
> >
> > static int
> > watchpoint_check (void *p)
> > @@ -3246,7 +3388,7 @@ watchpoint_check (void *p)
> >
> > fetch_watchpoint_value (b->exp, &new_val, NULL, NULL);
> > if ((b->val != NULL) != (new_val != NULL)
> > - || (b->val != NULL && !value_equal (b->val, new_val)))
> > + || (b->val != NULL && !value_equal_watchpoint (b->val, new_val)))
>
> Can you elaborate the need for this change? It seems to change the
> semantics of watchpoint_check, so I wonder why it is done.
>
It's done so we can detect triggers due to a range watchpoint. Since we
don't have the data address that caused the trigger, we need to go
through the entire range of a range watchpoint in order to tell if
something has changed. That's why we have a specific function to check
that (value_equal_watchpoint (b->val, new_val)).
> > + 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"?).
>
Yes, this is a typo. It should read "Since we don't know the exact
trigger address".
> > @@ -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?
>
I think so. Thanks for the suggestion.
> > @@ -5789,10 +5982,15 @@ hw_watchpoint_used_count (enum bptype type, int *other_type_used)
> > if (breakpoint_enabled (b))
> > {
> > if (b->type == type)
> > - i++;
> > - else if ((b->type == bp_hardware_watchpoint
> > - || b->type == bp_read_watchpoint
> > - || b->type == bp_access_watchpoint))
> > + {
> > + i++;
> > + /* Special types of hardware watchpoints can use more
> > + than one slot. */
> > + i += target_hw_point_extra_slot_count (b->hw_point_flag);
>
> Same here.
>
> > + /* Does the target support masked watchpoints? */
> > + if (!TARGET_CAN_USE_SPECIAL_HW_POINT_P (HW_POINT_MASKED_WATCH))
> > + error (_("This target does not support the usage of masks \
> > +with hardware watchpoints."));
>
> Can't the functionality be emulated by GDB's application code?
>
I think it could be, but it would also be terribly slow on some cases
due to the large range of addresses it would have to watch. I guess it
would be interesting for completion purposes (to have a soft mask
watch).
> > + 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?
>
I guess something like "watch *address mask -1", something stupid.
> > + 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?
>
Agreed. The reason is not explained. Should be fixed in the next
iteration.
> > + if (can_use_wp < 0)
> > + error (_("Not enough available hardware watchpoints."));
>
> "Not enough hardware resources for specified watchpoints" is more
> accurate, I think.
>
Agreed.
> > + /* 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?
>
Agreed. I think that's a sane approach.
> > + 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.
>
Makes sense to me.
> > + if (can_use_bp < 0)
> > + error (_("Not enough available hardware breakpoints."));
>
> See the comment above about a similar message.
>
> > + /* 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 _().)
>
Maybe work-around or error out on such condition.
> > + /* 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.
>
Same as above.
> > + /* 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?
>
Same as above?
> > + 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.
>
It always works on a range-inclusive basis on ppc, but it could be
different between targets. Maybe we need to fetch that information from
the targets?
> Anyway, this command (and all the other command changes and additions)
> need a suitable change to the user manual (although I would suggest to
> wait with that until we resolve the UI issues I raised in my previous
> message in this thread.)
>
Those bits will sure be provided when we've reached a more stable
version of the patch.
> > + if (start_addr > end_addr)
> > + error (_("Invalid search space, end preceeds start."));
>
> See the comment above about a similar message.
>
> Thanks for working on this.
Thanks for reviewing.
Regards,
Luis
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/2] Support the new PPC476 processor -- Arch Independent
2009-12-29 18:55 ` Luis Machado
@ 2009-12-29 18:59 ` Luis Machado
0 siblings, 0 replies; 9+ messages in thread
From: Luis Machado @ 2009-12-29 18:59 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: gdb-patches, bauerman, tyrlik
Hey,
On Tue, 2009-12-29 at 16:55 -0200, Luis Machado wrote:
> > > -/* Check watchpoint condition. */
> > > +/* Check watchpoint condition. We can't use value_equal because it coerces
> > > + an array to a pointer, thus comparing just the address of the array instead
> > > + of its contents. This is not what we want. */
> > > +
> > > +static int
> > > +value_equal_watchpoint (struct value *arg1, struct value *arg2)
> > > +{
> > > + struct type *type1, *type2;
> > > +
> > > + type1 = check_typedef (value_type (arg1));
> > > + type2 = check_typedef (value_type (arg2));
> > > +
> > > + return TYPE_CODE (type1) == TYPE_CODE (type2)
> > > + && TYPE_LENGTH (type1) == TYPE_LENGTH (type2)
> > > + && memcmp (value_contents (arg1), value_contents (arg2),
> > > + TYPE_LENGTH (type1)) == 0;
> > > +}
> > >
> > > static int
> > > watchpoint_check (void *p)
> > > @@ -3246,7 +3388,7 @@ watchpoint_check (void *p)
> > >
> > > fetch_watchpoint_value (b->exp, &new_val, NULL, NULL);
> > > if ((b->val != NULL) != (new_val != NULL)
> > > - || (b->val != NULL && !value_equal (b->val, new_val)))
> > > + || (b->val != NULL && !value_equal_watchpoint (b->val, new_val)))
> >
> > Can you elaborate the need for this change? It seems to change the
> > semantics of watchpoint_check, so I wonder why it is done.
> >
>
> It's done so we can detect triggers due to a range watchpoint. Since we
> don't have the data address that caused the trigger, we need to go
> through the entire range of a range watchpoint in order to tell if
> something has changed. That's why we have a specific function to check
> that (value_equal_watchpoint (b->val, new_val)).
Just ignore this bit since Thiago already dealt with this one in his
recent patch.
Luis
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] Support the new PPC476 processor -- Arch Independent
2009-12-18 11:28 ` Eli Zaretskii
2009-12-29 18:55 ` Luis Machado
@ 2009-12-30 2:00 ` Thiago Jung Bauermann
2009-12-30 5:02 ` Joel Brobecker
` (2 more replies)
1 sibling, 3 replies; 9+ messages in thread
From: Thiago Jung Bauermann @ 2009-12-30 2:00 UTC (permalink / raw)
To: Eli Zaretskii, gdb-patches; +Cc: luisgpm, tyrlik
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/2] Support the new PPC476 processor -- Arch Independent
2009-12-30 2:00 ` Thiago Jung Bauermann
@ 2009-12-30 5:02 ` Joel Brobecker
2009-12-30 5:11 ` Joel Brobecker
2009-12-30 20:44 ` Eli Zaretskii
2 siblings, 0 replies; 9+ messages in thread
From: Joel Brobecker @ 2009-12-30 5:02 UTC (permalink / raw)
To: Thiago Jung Bauermann; +Cc: Eli Zaretskii, gdb-patches, luisgpm, tyrlik
> > > + /* 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.
Please do! :-)
I just noticed that bug yesterday, and came up with yet another fix
(which is the worse of all suggestions made so far, so I won't send it,
after all [had everything ready to go, I was just waiting for our nightly
build results to come back and some of them take more than 24h]). You
and I are probably the only ones who care, because most of the
watchpoint implementations I looked at were just ignoring the count
and always returning either 1 (ok) or 0 (no more resource).
--
Joel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] Support the new PPC476 processor -- Arch Independent
2009-12-30 2:00 ` Thiago Jung Bauermann
2009-12-30 5:02 ` Joel Brobecker
@ 2009-12-30 5:11 ` Joel Brobecker
2009-12-30 17:41 ` Thiago Jung Bauermann
2009-12-30 20:44 ` Eli Zaretskii
2 siblings, 1 reply; 9+ messages in thread
From: Joel Brobecker @ 2009-12-30 5:11 UTC (permalink / raw)
To: Thiago Jung Bauermann; +Cc: Eli Zaretskii, gdb-patches, luisgpm, tyrlik
> 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\")."));
This is total nit-picking, but I suggest that we avoid the use of
contractions in error messages. I think this is too casual.
"can't" -> "cannot"
"don't" -> "do not"
> 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.
I agree with that (consistency). I don't think I'd be fustrated at GDB
if I inserted the addresses in the wrong order and GDB errored out.
--
Joel
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/2] Support the new PPC476 processor -- Arch Independent
2009-12-30 5:11 ` Joel Brobecker
@ 2009-12-30 17:41 ` Thiago Jung Bauermann
0 siblings, 0 replies; 9+ messages in thread
From: Thiago Jung Bauermann @ 2009-12-30 17:41 UTC (permalink / raw)
To: Joel Brobecker; +Cc: Eli Zaretskii, gdb-patches, luisgpm, tyrlik
On Wed 30 Dec 2009 03:11:20 Joel Brobecker wrote:
> > 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\")."));
>
> This is total nit-picking, but I suggest that we avoid the use of
> contractions in error messages. I think this is too casual.
>
> "can't" -> "cannot"
> "don't" -> "do not"
No problem. Changed.
--
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] Support the new PPC476 processor -- Arch Independent
2009-12-30 2:00 ` Thiago Jung Bauermann
2009-12-30 5:02 ` Joel Brobecker
2009-12-30 5:11 ` Joel Brobecker
@ 2009-12-30 20:44 ` Eli Zaretskii
2 siblings, 0 replies; 9+ messages in thread
From: Eli Zaretskii @ 2009-12-30 20:44 UTC (permalink / raw)
To: Thiago Jung Bauermann; +Cc: gdb-patches, luisgpm, tyrlik
> From: Thiago Jung Bauermann <bauerman@br.ibm.com>
> Date: Tue, 29 Dec 2009 23:59:32 -0200
> Cc: luisgpm@linux.vnet.ibm.com, tyrlik@us.ibm.com
>
> Ok?
Yes, thanks.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2009-12-30 20:44 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-12-16 20:49 [PATCH 1/2] Support the new PPC476 processor -- Arch Independent Sérgio Durigan Júnior
2009-12-18 11:28 ` Eli Zaretskii
2009-12-29 18:55 ` Luis Machado
2009-12-29 18:59 ` Luis Machado
2009-12-30 2:00 ` Thiago Jung Bauermann
2009-12-30 5:02 ` Joel Brobecker
2009-12-30 5:11 ` Joel Brobecker
2009-12-30 17:41 ` Thiago Jung Bauermann
2009-12-30 20:44 ` Eli Zaretskii
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox