From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8359 invoked by alias); 11 Apr 2009 17:16:49 -0000 Received: (qmail 8348 invoked by uid 22791); 11 Apr 2009 17:16:48 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL,BAYES_00,J_CHICKENPOX_33,J_CHICKENPOX_44,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 11 Apr 2009 17:16:43 +0000 Received: (qmail 16674 invoked from network); 11 Apr 2009 17:16:40 -0000 Received: from unknown (HELO orlando.local) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 11 Apr 2009 17:16:40 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [Patch 2/2] MIPS hardware watchpoint support. Date: Sat, 11 Apr 2009 17:16:00 -0000 User-Agent: KMail/1.9.10 Cc: David Daney , David Daney References: <49D9AECB.7040302@gmail.com> In-Reply-To: <49D9AECB.7040302@gmail.com> MIME-Version: 1.0 Content-Disposition: inline Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Message-Id: <200904111817.36236.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2009-04/txt/msg00210.txt.bz2 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) > +/* 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