Re: [PATCH 1/2] Support the new PPC476 processor -- Arch Independent

Eli Zaretskii
2009-12-18T11:28:20Z

> 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.