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

Luis Machado
2009-12-29T18:55:16Z

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