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

[Pedro Alves <pedro at codesourcery dot com>]

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