Re: [Patch 2/2] MIPS hardware watchpoint support.

Pedro Alves
2009-04-11T17:16:49Z

On Monday 06 April 2009 08:27:07, David Daney wrote:
> Support for hardware watchpoints for mips-linux was merged into the
> Linux kernel for version 2.6.28, with some bug fixes in 2.6.29. This
> patch adds the corresponding gdb support. It has been tested on both
> mipsel-linux (32-bit kernel) and mips64-linux (64-bit kernel).

Thanks! I'll pretend to know a thing about mips from here on, and
hope that it doesn't show too much that I don't.

In general, this looks very good to me, although I didn't
make an effort to understand all the bit juggling going on. Minor
comments to the code below. Then comments to the tests follow that.

> 2009-04-05 David Daney <ddaney@caviumnetworks.com>

> (get_irw_mask, get_reg_mask, get_num_valid, get_watchlo,
> set_watchlo, get_watchhi, set_watchhi, mips_show_dr,

Please split these context lines like so instead, as per the
coding conventions:

> (get_irw_mask, get_reg_mask, get_num_valid, get_watchlo)
> (set_watchlo, get_watchhi, set_watchhi, mips_show_dr)

> +/* Assuming usable watch registers, return the irw_mask. */
> +static uint32_t

Please add an empty line between comment and function. Here
and elsewhere.

> +get_irw_mask (struct pt_watch_regs *regs, int set)
> +{
> + switch (regs->style)
> + {
> + case pt_watch_style_mips32:
> + return regs->mips32.watch_masks[set] & IRW_MASK;
> + case pt_watch_style_mips64:
> + return regs->mips64.watch_masks[set] & IRW_MASK;
> + default:
> + gdb_assert (0);

Please don't write 'gdb_assert (0)', instead make that a
more bit more friendly `internal_error'. Here and
elsewhere.

> +static void
> +mips_show_dr (const char *func, CORE_ADDR addr,
> + int len, enum target_hw_bp_type type)
> +{
> + int i;
> +
> + puts_unfiltered (func);
> + if (addr || len)
> + printf_unfiltered (" (addr=%lx, len=%d, type=%s)",
> + /* This code is for mips, so casting CORE_ADDR
> + to unsigned long should be okay. */
> + (unsigned long)addr, len,

Why bother to explain that instead of using `paddr'? If there's
a reason, then it would be nice if it was spelled out in the
comment.

> + type == hw_write ? "data-write"
> + : (type == hw_read ? "data-read"
> + : (type == hw_access ? "data-read/write"
> + : (type == hw_execute ? "instruction-execute"
> + : "??unknown??"))));
> + puts_unfiltered (":\n");
> +
> + for (i = 0; i < MAX_DEBUG_REGISTER; i++)
> + {

Unneeded braces. You have a couple more in the patch.

> + printf_unfiltered ("\tDR%d: lo=0x%s, hi=0x%s\n",
> + i, paddr (get_watchlo (&watch_mirror, i)),
> + paddr (get_watchhi (&watch_mirror, i)));
> + }
> +}
> +

> +/* Write the mirrored watch register values for each thread. */
> +static int
> +write_watchpoint_regs (void)
> +{
> + struct lwp_info *lp;
> + ptid_t ptid;
> + int tid;
> +
> + ALL_LWPS (lp, ptid)
> + {
> + tid = ptid_get_lwp (ptid);
> + if (ptrace (PTRACE_SET_WATCH_REGS, tid, &watch_mirror) == -1)
> + {
> + perror_with_name (_("Couldn't write debug register"));
> + return -1;

perror doesn't return, no need for `return -1'.

> + }
> + }
> + return 0;
> +}
> +

> +
> +/* Target to_insert_watchpoint implementation. Try to insert a new
> + watch. Return zero on success. */
> +static int
> +mips_linux_insert_watchpoint (CORE_ADDR addr, int len, int type)
> +{

> +
> + watch_mirror = regs;
> + retval = write_watchpoint_regs();

^ missing space.

> +/* Target to_remove_watchpoint implementation. Try to remove a watch.
> + Return zero on success. */
> +static int
> +mips_linux_remove_watchpoint (CORE_ADDR addr, int len, int type)
> +{

> +
> + watch_mirror = watch_readback;
> + populate_regs_from_watches (&watch_mirror);
> +
> + retval = write_watchpoint_regs();

^ missing space.

>
> void
> _initialize_mips_linux_nat (void)
> {
> - struct target_ops *t = linux_trad_target (mips_linux_register_u_offset);
> + struct target_ops *t;
> +
> + deprecated_add_set_cmd ("show-debug-regs", class_maintenance,
> + var_boolean, (char *) &maint_show_dr, _("\
> +Set whether to show variables that mirror the mips debug registers.\n\
> +Use \"on\" to enable, \"off\" to disable.\n\
> +If enabled, the debug registers values are shown when GDB inserts\n\
> +or removes a hardware breakpoint or watchpoint, and when the inferior\n\
> +triggers a breakpoint or watchpoint."),
> + &maintenancelist);

Argh. I've posted a patch that removes deprecated_add_set_cmd, but
never got around to committing it.

http://sourceware.org/ml/gdb-patches/2009-01/msg00119.html

When I get around to finish off that patch, I'll adjust this
file too --- I assume that is OK with you.

However, reusing the x86 command does avoid needing to document
it, but, could you remove the reference to x86 in the current
docs?

@kindex maint show-debug-regs
@cindex x86 hardware debug registers
@item maint show-debug-regs
Control whether to show variables that mirror the x86 hardware debug
registers. Use @code{ON} to enable, @code{OFF} to disable. If
enabled, the debug registers values are shown when @value{GDBN} inserts or
removes a hardware breakpoint or watchpoint, and when the inferior
triggers a hardware-assisted breakpoint or watchpoint.

... since it now applies to mips as well.

> For this submission I tested on a mipsel-linux system with these results
> (with comments):
>
> --- ./gdb/testsuite/gdb.sum 2009-04-05 20:53:20.000000000 -0700
> +++ gdb.sum 2009-04-05 17:53:05.000000000 -0700
> @@ -1,4 +1,4 @@
> -Test Run By root on Sun Apr 5 18:08:05 2009
> +Test Run By root on Sun Apr 5 14:45:02 2009
> Native configuration is mipsel-unknown-linux-gnu
>
> === gdb tests ===
> @@ -5717,15 +5717,15 @@
> PASS: gdb.base/recurse.exp: continue to recurse (a = 5)
> PASS: gdb.base/recurse.exp: next over b = 0 in second instance
> PASS: gdb.base/recurse.exp: set second instance watchpoint
> -PASS: gdb.base/recurse.exp: continue to second instance watchpoint,
> first time
> -PASS: gdb.base/recurse.exp: continue to recurse (a = 4)
> -PASS: gdb.base/recurse.exp: continue to recurse (a = 3)
> -PASS: gdb.base/recurse.exp: continue to recurse (a = 2)
> -PASS: gdb.base/recurse.exp: continue to recurse (a = 1)
> -PASS: gdb.base/recurse.exp: continue to second instance watchpoint,
> second time
> -PASS: gdb.base/recurse.exp: second instance watchpoint deleted when
> leaving scope
> -PASS: gdb.base/recurse.exp: continue to first instance watchpoint,
> second time
> -PASS: gdb.base/recurse.exp: first instance watchpoint deleted when
> leaving scope
> +FAIL: gdb.base/recurse.exp: continue to second instance watchpoint,
> first time
> +FAIL: gdb.base/recurse.exp: continue to recurse (a = 4)
> +FAIL: gdb.base/recurse.exp: continue to recurse (a = 3)
> +FAIL: gdb.base/recurse.exp: continue to recurse (a = 2)
> +FAIL: gdb.base/recurse.exp: continue to recurse (a = 1)
> +FAIL: gdb.base/recurse.exp: continue to second instance watchpoint,
> second time
> +FAIL: gdb.base/recurse.exp: second instance watchpoint deleted when
> leaving scope
> +FAIL: gdb.base/recurse.exp: continue to first instance watchpoint,
> second time
> +FAIL: gdb.base/recurse.exp: first instance watchpoint deleted when
> leaving scope
>
> These failures are caused by the fact that the test assumes more than a
> the single watch register supported by the test system. Although this
> patch supports up to eight registers, the test platform only has one.
> We return true from mips_linux_can_use_hw_breakpoint for multiple
> requests as it is not possible to know how many watchpoints are
> enabled. Later gdb tries to insert more than one watchpoint and all but
> the first fail.

Could we detect that, and fail the rest of the test gracefully?
What error message do you get?

> /home/daney/gdbcvs/src/gdb/testsuite/gdb.base/watchpoint-solib.exp ...
> PASS: gdb.base/watchpoint-solib.exp: set pending breakpoint
> @@ -8516,7 +8516,7 @@
> PASS: gdb.cp/annota2.exp: breakpoint at main
> PASS: gdb.cp/annota2.exp: run until main breakpoint
> PASS: gdb.cp/annota2.exp: set watch on a.x
> -PASS: gdb.cp/annota2.exp: watch triggered on a.x
> +FAIL: gdb.cp/annota2.exp: watch triggered on a.x
> PASS: gdb.cp/annota2.exp: annotate-quit
> Running /home/daney/gdbcvs/src/gdb/testsuite/gdb.cp/annota3.exp ...
> PASS: gdb.cp/annota3.exp: breakpoint main
> @@ -8528,7 +8528,7 @@
> PASS: gdb.cp/annota3.exp: break at main
> PASS: gdb.cp/annota3.exp: second run until main breakpoint
> PASS: gdb.cp/annota3.exp: set watch on a.x
> -PASS: gdb.cp/annota3.exp: watch triggered on a.x
> +FAIL: gdb.cp/annota3.exp: watch triggered on a.x
>
> These two regressions are due to an extra:
> frame-address
> 0x00400860
> frame-address-end
>
> Sequence emitted by gdb with 'set annotate 2'. The watchpoints are
> triggering but the output is slightly different than the test expects.
> I chalk it up to a defect in the test.

This annotation tests are a nuisance, particularly, since we want to
stop supporting annotations at some point. Ideally, we'd just adjust
the test ...

> -PASS: gdb.mi/mi-watch.exp: hw: watchpoint trigger
> +FAIL: gdb.mi/mi-watch.exp: hw: watchpoint trigger (unknown output after
> running)
>
> Reported instruction location of the watchpoint trigger is one
> instruction later and one line later.

Why is that?

--
Pedro Alves