This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/2] Support the new PPC476 processor -- Arch Independent
- From: Eli Zaretskii <eliz at gnu dot org>
- To: Sérgio Durigan Júnior <sergiodj at linux dot vnet dot ibm dot com>
- Cc: gdb-patches at sourceware dot org, bauerman at br dot ibm dot com, luisgpm at linux dot vnet dot ibm dot com, tyrlik at us dot ibm dot com
- Date: Fri, 18 Dec 2009 13:30:06 +0200
- Subject: Re: [PATCH 1/2] Support the new PPC476 processor -- Arch Independent
- References: <200912161847.19433.sergiodj@linux.vnet.ibm.com>
- Reply-to: Eli Zaretskii <eliz at gnu dot org>
> 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.