Re: [RFA] Implement support for PowerPC BookE masked watchpoints

Ulrich Weigand
2011-02-17T14:40:31Z

Thiago Jung Bauermann wrote:

> I'm almost done with the series adding suport for PowerPC BookE
> features... There's only this patch adding masked watchpoints, and
> another one adding ranged hardware breakpoints (to be posted later). So
> please bear with me a little longer. :-)

I have one basic issue with this patch: the semantics of the new masked
watchpoint commands seems not quite fully defined.

The semantics of a GDB watchpoint usually is: stop execution whenever
the value of an expression changes. To implement this, GDB will parse
the expression, and create a set of low-level watchpoints to be
implemented by hardware (which are usually along the lines of: stop
execution whenever a store to a contiguous region of memory happens).

Now, the "masked breakpoints" feature is at its core defined at that
lower level: stop execution whenever a store to a memory region
happens, where that region of memory is defined in a more complex
manner (via masked address matching).

The problem with the patch in its current form seems to me that it
confuses the two levels. On the GDB command line, the user specifies
an expression, just like with normal watchpoints, and a mask, which
is passed directly to the hardware feature.

The effect is that GDB will parse the expression, create the list of
memory regions it needs to watch to implement its high-level semantics.
Then, it will just add the mask to each of the addresses generated
in that manner. It seems to me that in general, the effects of what
will actually happen now are hard to predict, and will generally not
be what the user expects.

Of course, the feature *can* be used in ways the user understands,
like in the example you show in the documentation:
watch *0xdeadbeef mask 0xffffff00

But if used in a more complex way, like
watch p->x mask 0xffffff00
for some local variable p, this doesn't seem to have very useful
(or even well-defined) semantics ...

Now, I'm not sure how exactly the GDB command line syntax ought to
be modified to implement this. But most fundamentally, we should
go away from specifying "the value of an expression", and rather
go to specifying a memory region via address and mask.

We've recently added the -location feature that already prevents
GDB from fully tracking the expression, but rather just evaluate
it one time for its address. Maybe one simple fix could be to
restrict the "mask" feature to watch -location (or have the
presence of "mask" automatically imply -location, or maybe have
instead a new keyword -masked-location or so).

However, even so there are still differences: even with -location,
GDB watchpoint code will still treat a watchpoint as refering to
one value, and track it (e.g. remember the value we saw last time;
re-evaluate it each time execution stops etc.). This is really
not appropriate for the masked watchpoint case, because we do not
*have* any contiguous region that would define any single value.
Therefore, it seems to me that all this code ought to be disabled
for masked watchpoints.

A couple of more specific comments on the patch:

> @@ -1518,10 +1522,26 @@ update_watchpoint (struct breakpoint *b, int reparse)
> target_resources_ok = target_can_use_hardware_watchpoint
> (bp_hardware_watchpoint, i, other_type_used);
> if (target_resources_ok <= 0)
> - b->type = bp_watchpoint;
> + {
> + if (orig_type == bp_hardware_watchpoint
> + && b->ops && b->ops->works_in_software_mode
> + && !b->ops->works_in_software_mode (b))
> + error (_("This watchpoint cannot be used "
> + "in software mode."));
> + else
> + b->type = bp_watchpoint;
> + }
> }
> else
> - b->type = bp_watchpoint;
> + {
> + if (b->type == bp_hardware_watchpoint
> + && b->ops && b->ops->works_in_software_mode
> + && !b->ops->works_in_software_mode (b))
> + error (_("This watchpoint cannot be used "
> + "in software mode."));
> + else
> + b->type = bp_watchpoint;
> + }

What is the point of checking the original type? If works_in_software_mode
returns false, the original type couldn't have been a software watchpoint
anyway, could it?

> print_it_typical. */
> /* FIXME: how breakpoint can ever be NULL here? */
> if (b != NULL && b->ops != NULL && b->ops->print_it != NULL)
> - return b->ops->print_it (b);
> + return b->ops->print_it (b, bs->old_val);
> else
> return print_it_typical (bs);
> }

I don't understand this change. You add the "old_val" argument to the
print_it routine, and the new instance of print_it for masked watchpoints
then uses it (for what purpose?) ... But for masked watchpoints, the
whole concept of its "value", new or old, doesn't make sense to start
with ...

> @@ -3761,6 +3796,11 @@ watchpoint_check (void *p)
> b->val_valid = 1;
> return WP_VALUE_CHANGED;
> }
> + else if (is_masked_watchpoint (b))
> + /* Since we don't know the exact trigger address (from
> + stopped_data_address), just tell the user we've triggered
> + a mask watchpoint. */
> + return WP_VALUE_CHANGED;

This goes back to the issue discussed above: this should not even
attempt to "compute the old value" which doesn't make sense for a
masked watchpoint. Instead, it should simply report a stop to the
user whenever the hardware reports a hit.

> +static void
> +print_one_detail_masked_watchpoint (const struct breakpoint *b,
> + struct ui_out *uiout)
> +{
> + ui_out_text (uiout, "\tmask ");
> +
> + /* I don't know whether it's possible to get here without a b->loc,
> + but we can handle the situation. */
> + if (b->loc)
> + ui_out_field_core_addr (uiout, "mask", b->loc->gdbarch, b->hw_wp_mask);
> + else
> + ui_out_field_string (uiout, "mask", core_addr_to_string (b->hw_wp_mask));

I think we should enforce that there is always exactly one location on
masked watchpoints, just like we do e.g. for catchpoints.

> +/* Implement the "print_recreate" breakpoint_ops method for
> + masked hardware watchpoints. */
> +
> +static void
> +print_recreate_masked_watchpoint (struct breakpoint *b, struct ui_file *fp)
> +{
> + char tmp[40];
> +
> + switch (b->type)
> + {
> + case bp_hardware_watchpoint:
> + fprintf_unfiltered (fp, "watch");
> + break;
> + case bp_read_watchpoint:
> + fprintf_unfiltered (fp, "rwatch");
> + break;
> + case bp_access_watchpoint:
> + fprintf_unfiltered (fp, "awatch");
> + break;
> + default:
> + internal_error (__FILE__, __LINE__,
> + _("Invalid hardware watchpoint type."));
> + }
> +
> + sprintf_vma (tmp, b->hw_wp_mask);
> + fprintf_unfiltered (fp, " %s mask 0x%s", b->exp_string, tmp);

Depending on the discussion on command syntax this may need to change anyway,
but even with the current syntax, it ignores any -location flag that may have
been given initially ...

> @@ -8540,7 +8832,14 @@ 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;
> - b->ops = &watchpoint_breakpoint_ops;
> +
> + if (use_mask)
> + {
> + b->hw_wp_mask = hw_wp_mask;
> + b->ops = &masked_watchpoint_breakpoint_ops;
> + }
> + else
> + b->ops = &watchpoint_breakpoint_ops;

We shouldn't compute an intial value or set val_valid for masked watchpoints.

> +/* Insert a new masked watchpoint at ADDR using the mask MASK.
> + RW may be hw_read for a read watchpoint, hw_write for a write watchpoint
> + or hw_access for an access watchpoint. Returns 0 on success and throws
> + an error on failure. */
> +
> +static int
> +ppc_linux_insert_mask_watchpoint (struct target_ops *ops, CORE_ADDR addr,
> + CORE_ADDR mask, int rw)
> +{
> + ptid_t ptid;
> + struct lwp_info *lp;
> + struct ppc_hw_breakpoint p;
> +
> + gdb_assert (have_ptrace_booke_interface ());
> +
> + if ((mask & 0xC0000000) != 0xC0000000)
> + error (_("\
> +The given mask covers kernel address space and cannot be used.\n\
> +You have to delete the masked watchpoint to continue the debugging session."));

Huh, this interface seems a bit odd. Shouldn't such watchpoints be rejected
right from the start, using something like the region_ok_for_watchpoint
callback?

> +/* Return the number of registers needed for a masked hardware watchpoint. */
> +
> +static int
> +ppc_linux_masked_watch_num_registers (struct target_ops *target)
> +{
> + return ((have_ptrace_booke_interface ()
> + && booke_debug_info.features & PPC_DEBUG_FEATURE_DATA_BP_MASK)?
> + 2 : -1);
> +}

So maybe this interface ought to get parameters to be able to check
whether specific watchpoints are valid?

> INHERIT (to_insert_watchpoint, t);
> INHERIT (to_remove_watchpoint, t);
> + /* Do not inherit to_insert_mask_watchpoint. */
> + /* Do not inherit to_remove_mask_watchpoint. */

Why should this differ between regular and masked watchpoints? Shouldn't
they either both use inheritance or none?

Bye,
Ulrich

--
Dr. Ulrich Weigand
GNU Toolchain for Linux on System z and Cell BE
Ulrich.Weigand@de.ibm.com