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