[Patch 2/2] MIPS hardware watchpoint support.

David Daney ddaney@caviumnetworks.com
Thu Apr 16 17:08:00 GMT 2009 

Pedro Alves wrote:
> 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)
> 
> 

Done.  All these years and there is still more to learn about the coding 
conventions.

>> +/* Assuming usable watch registers, return the irw_mask.  */
>> +static uint32_t
> 
> Please add an empty line between comment and function.  Here
> and elsewhere.

Done.

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

Done.

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

Comment deleted, and types casted to (unsigned long long).


>> +  for (i = 0; i < MAX_DEBUG_REGISTER; i++)
>> +    {
> 
> Unneeded braces.  You have a couple more in the patch.

Fixed.

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

>> +       {
>> +         perror_with_name (_("Couldn't write debug register"));
>> +         return -1;
> 
> perror doesn't return, no need for `return -1'.

Fixed.


>> +
>> +  watch_mirror = watch_readback;
>> +  populate_regs_from_watches (&watch_mirror);
>> +
>> +  retval = write_watchpoint_regs();
> 
>                                    ^ missing space.


All instances fixed.


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

Done.


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

I slightly modified the patch so that it makes more conservative 
estimates about how many hardware watches it can handle.  This fixes 
these failures.


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

I was mistaken, it is earlier, not later.  With hardware watch points, 
$pc points to the faulting instruction, with software watch points $pc 
points to the following instruction.  In a couple of tests, this results 
in the reported line number being different than the expected value.



Here is the revised patch and its test results:

--- pre/gdb.sum	2009-04-15 12:12:33.000000000 -0700
+++ post/gdb.sum	2009-04-15 10:22:37.000000000 -0700
@@ -1,4 +1,4 @@
-Test Run By root on Wed Apr 15 10:42:35 2009
+Test Run By root on Tue Apr 14 16:28:52 2009
  Native configuration is mips64-unknown-linux-gnu

  		=== gdb tests ===
@@ -5684,7 +5684,7 @@
  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
+FAIL: gdb.base/recurse.exp: continue to first instance watchpoint, 
second time

Different line number as explained above.


  PASS: gdb.base/recurse.exp: first instance watchpoint deleted when 
leaving scope
  Running ../../../src/gdb/testsuite/gdb.base/regs.exp ...
  Running ../../../src/gdb/testsuite/gdb.base/relational.exp ...
@@ -8272,26 +8272,26 @@
  PASS: gdb.base/watch_thread_num.exp: Disable breakpoint 2
  PASS: gdb.base/watch_thread_num.exp: Watchpoint on shared variable
  PASS: gdb.base/watch_thread_num.exp: info breakpoint 3
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 1 
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered 
iteration 1 (timeout)
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 2 
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered 
iteration 2 (timeout)
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 3 
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered 
iteration 3 (timeout)
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 4 
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered 
iteration 4 (timeout)
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 5 
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered 
iteration 5 (timeout)
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 6 
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered 
iteration 6 (timeout)
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 7 
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered 
iteration 7 (timeout)
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 8 
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered 
iteration 8 (timeout)
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 9 
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered 
iteration 9 (timeout)
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 10 
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered 
iteration 10 (timeout)
+PASS: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 1
+PASS: gdb.base/watch_thread_num.exp: Check thread that triggered 
iteration 1
+FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 2
+FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered 
iteration 2
+FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 3
+PASS: gdb.base/watch_thread_num.exp: Check thread that triggered 
iteration 3
+FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 4
+FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered 
iteration 4
+FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 5
+FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered 
iteration 5
+FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 6
+FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered 
iteration 6
+FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 7
+FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered 
iteration 7
+FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 8
+PASS: gdb.base/watch_thread_num.exp: Check thread that triggered 
iteration 8
+FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 9
+FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered 
iteration 9
+FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 10
+FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered 
iteration 10
  Running ../../../src/gdb/testsuite/gdb.base/watchpoint-hw.exp ...
  Running ../../../src/gdb/testsuite/gdb.base/watchpoint-solib.exp ...
  PASS: gdb.base/watchpoint-solib.exp: set pending breakpoint
@@ -8476,7 +8476,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 ../../../src/gdb/testsuite/gdb.cp/annota3.exp ...
  PASS: gdb.cp/annota3.exp: breakpoint main
@@ -8488,7 +8488,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

The two annotation failures are explained in the

  PASS: gdb.cp/annota3.exp: annotate-quit
  Running ../../../src/gdb/testsuite/gdb.cp/anon-union.exp ...
  PASS: gdb.cp/anon-union.exp: next 1
@@ -11976,7 +11976,7 @@
  PASS: gdb.mi/mi-watch.exp: hw: mi runto callee4
  PASS: gdb.mi/mi-watch.exp: hw: break-watch operation
  PASS: gdb.mi/mi-watch.exp: hw: list of watchpoints
-PASS: gdb.mi/mi-watch.exp: hw: watchpoint trigger
+FAIL: gdb.mi/mi-watch.exp: hw: watchpoint trigger (unknown output after 
running)

Different line number as explained above.

  PASS: gdb.mi/mi-watch.exp: hw: watchpoint trigger
  Running ../../../src/gdb/testsuite/gdb.mi/mi2-basics.exp ...
  PASS: gdb.mi/mi2-basics.exp: acceptance of MI operations
@@ -12609,7 +12609,7 @@
  PASS: gdb.mi/mi2-watch.exp: hw: mi runto callee4
  PASS: gdb.mi/mi2-watch.exp: hw: break-watch operation
  PASS: gdb.mi/mi2-watch.exp: hw: list of watchpoints
-PASS: gdb.mi/mi2-watch.exp: hw: watchpoint trigger
+FAIL: gdb.mi/mi2-watch.exp: hw: watchpoint trigger (unknown output 
after running)

Different line number as explained above.

  PASS: gdb.mi/mi2-watch.exp: hw: watchpoint trigger
  Running ../../../src/gdb/testsuite/gdb.modula2/unbounded-array.exp ...
  PASS: gdb.modula2/unbounded-array.exp: switch to modula-2
@@ -13351,10 +13351,10 @@
  PASS: gdb.threads/tls.exp: info address a_thread_local
  Running ../../../src/gdb/testsuite/gdb.threads/watchthreads.exp ...
  PASS: gdb.threads/watchthreads.exp: successfully compiled posix 
threads test case
-FAIL: gdb.threads/watchthreads.exp: watch args[0]
+PASS: gdb.threads/watchthreads.exp: watch args[0]
  FAIL: gdb.threads/watchthreads.exp: watch args[1]
  FAIL: gdb.threads/watchthreads.exp: threaded watch loop
-FAIL: gdb.threads/watchthreads.exp: first watchpoint on args[0] hit
+PASS: gdb.threads/watchthreads.exp: first watchpoint on args[0] hit
  FAIL: gdb.threads/watchthreads.exp: first watchpoint on args[1] hit
  FAIL: gdb.threads/watchthreads.exp: watchpoint on args[0] hit in thread
  FAIL: gdb.threads/watchthreads.exp: watchpoint on args[1] hit in thread



OK to commit?

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: mips-watch.patch
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20090416/36bf0461/attachment.ksh>

More information about the Gdb-patches mailing list