From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7376 invoked by alias); 18 Feb 2010 01:11:41 -0000 Received: (qmail 7363 invoked by uid 22791); 18 Feb 2010 01:11:40 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 18 Feb 2010 01:11:36 +0000 Received: (qmail 29776 invoked from network); 18 Feb 2010 01:11:34 -0000 Received: from unknown (HELO orlando.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 18 Feb 2010 01:11:34 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: read watchpoints don't work on targets that support read watchpoints Date: Thu, 18 Feb 2010 01:11:00 -0000 User-Agent: KMail/1.12.2 (Linux/2.6.31-19-generic; KDE/4.3.2; x86_64; ; ) MIME-Version: 1.0 Content-Type: Text/Plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Message-Id: <201002180111.31520.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: 2010-02/txt/msg00448.txt.bz2 We got a bug report that read watchpoints weren't working as expected against an ARM target. The target was reporting the read watchpoint hits correctly, but GDB was just always ignoring them like so: ... &"infrun: TARGET_WAITKIND_STOPPED\n" &"infrun: stop_pc = 0xce\n" &"infrun: stopped by watchpoint\n" &"infrun: stopped data address = 0x20007fdc\n" &"infrun: no stepping, continue\n" &"infrun: resume (step=0, signal=0), trap_expected=0\n" ... The reproducer is just something like: void main() { int a = 3; while (1) { a++; } } and rwatching `a'. This compiled as: ~"Dump of assembler code for function main:\n" ~"0x000000c0 :\tpush\t{r7}\n" ~"0x000000c2 :\tsub\tsp, #12\n" ~"0x000000c4 :\tadd\tr7, sp, #0\n" ~"0x000000c6 :\tmov.w\tr3, #3\n" ~"0x000000ca :\tstr\tr3, [r7, #4]\n" ~"0x000000cc :\tldr\tr3, [r7, #4]\n" <<< reads memory, traps read ~"0x000000ce :\tadd.w\tr3, r3, #1\n" ~"0x000000d2 :\tstr\tr3, [r7, #4]\n" <<< writes mem, changes `a', doesn't trap ~"0x000000d4 :\tb.n\t0xcc \n" The problem is that bpstat_check_watchpoint logic for read watchpoints is bogus for targets that do support read watchpoints properly: case WP_VALUE_CHANGED: if (b->type == bp_read_watchpoint) { /* Don't stop: read watchpoints shouldn't fire if the value has changed. This is for targets which cannot set read-only watchpoints. */ bs->print_it = print_it_noop; bs->stop = 0; } If we don't get a memory write trap, then when the next read traps, we'll see that the watched value changes, and consequently we just ignore the watchpoint! This happens to be PR9605 The compromise here is reversed. We should believe the target when it says a read watchpoint trapped. On targets that can't do read-only watchpoints, but lie to the core allowing them to be inserted, read watchpoints will behave as access watchpoints. As Jeremy says on the PR: "over-enthusiastic reporting is less of an issue than under-enthusiastic reporting". The logic of not reporting read watchpoints if the memory changes could be reinstated, if conditionalized on the target telling the core that it can't do read watchpoints, but it can do access watchpoints instead. Or the target itself could do that (filtering read-write traps from gdb if the memory watched doesn't change), which is more efficient, transparent and flexible. Or such targets should just refuse to install read watchpoints, and GDB should try to fallback to trying access watchpoints, and knowing it should apply the "only-if-not-changed" logic then. This is never perfect, because we'll report reads when the program writes the same value as the memory already had, but then again, this is what already happens today. The latter option is a bit problematic with the current interfaces, as we don't know _why_ is the target refusing a read watchpoint. In any case, I think that targets allowing read watchpoints to be inserted, and then trapping on writes should get what they deserve. I think we should apply this, and work from here on if needed. Any objection or better ideas? -- Pedro Alves 2010-02-18 Pedro Alves PR9605 gdb/ * breakpoint.c (bpstat_check_watchpoint): Stop for read watchpoints even if the value changed. --- gdb/breakpoint.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) Index: src/gdb/breakpoint.c =================================================================== --- src.orig/gdb/breakpoint.c 2010-02-17 22:26:47.000000000 +0000 +++ src/gdb/breakpoint.c 2010-02-18 00:27:22.000000000 +0000 @@ -3432,14 +3432,16 @@ bpstat_check_watchpoint (bpstat bs) /* Stop. */ break; case WP_VALUE_CHANGED: - if (b->type == bp_read_watchpoint) - { - /* Don't stop: read watchpoints shouldn't fire if - the value has changed. This is for targets - which cannot set read-only watchpoints. */ - bs->print_it = print_it_noop; - bs->stop = 0; - } + /* For read watchpoints, even though reads don't cause + value changes, the value may have changed since the + last time it was read, as we're not supposedly + trapping memory writes. There are targets that can't + break on data reads only, they have to break on + accesses (reads or writes), even though they still + claim support for read watchpoints. On those, read + watchpoints will behave as access breakpoints, but + that's a better compromise than missing reads on + targets that do support breaking on reads only. */ break; case WP_VALUE_NOT_CHANGED: if (b->type == bp_hardware_watchpoint