From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4406 invoked by alias); 20 Nov 2009 00:31:47 -0000 Received: (qmail 4393 invoked by uid 22791); 20 Nov 2009 00:31:44 -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; Fri, 20 Nov 2009 00:30:54 +0000 Received: (qmail 6669 invoked from network); 20 Nov 2009 00:30:51 -0000 Received: from unknown (HELO orlando) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 20 Nov 2009 00:30:51 -0000 From: Pedro Alves To: Daniel Jacobowitz Subject: Re: Don't delete local watchpoints just because a different thread stopped. Date: Fri, 20 Nov 2009 00:31:00 -0000 User-Agent: KMail/1.9.10 Cc: gdb-patches@sourceware.org References: <200911190207.02432.pedro@codesourcery.com> <20091119174539.GA24336@caradoc.them.org> In-Reply-To: <20091119174539.GA24336@caradoc.them.org> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200911200030.50194.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-11/txt/msg00425.txt.bz2 On Thursday 19 November 2009 17:45:39, Daniel Jacobowitz wrote: > On Thu, Nov 19, 2009 at 02:07:01AM +0000, Pedro Alves wrote: > > One could even come up with a test case where the current > > frame id on this other thread does happen to be in the > > frame chain. Shouldn't be hard to trigger if the threads > > are running the same function --- GDB can then mistakenly > > try to extract a new current watchpoint value on the wrong > > thread context. > > I hope your threads have different stacks :-) So I don't think this > case happens. Urgh, talk about braino! :-P :-) > > > Index: src/gdb/breakpoint.h > > =================================================================== > > --- src.orig/gdb/breakpoint.h 2009-11-19 01:10:05.000000000 +0000 > > +++ src/gdb/breakpoint.h 2009-11-19 01:10:16.000000000 +0000 > > @@ -457,6 +457,10 @@ struct breakpoint > > should be evaluated on the outermost frame. */ > > struct frame_id watchpoint_frame; > > > > + /* Holds the thread which identifies the frame this watchpoint > > + should be considered in scope for, or -1 if don't care. */ > > + int watchpoint_thread; > > + > > /* For hardware watchpoints, the triggered status according to the > > hardware. */ > > enum watchpoint_triggered watchpoint_triggered; > > Why not just use a ptid? If the answer has to do with ptid reuse, > then we ought to delete the watchpoint before that comes up - anyway, > at least deserves a comment of what units this is in. No reason. I started out with a hack using the bkpt->thread field, then decided to keep user visible things distinct from internal things, and just copy/pasted the field --- the int'ness sticked. Indeed a ptid makes things simpler. Thanks. > > @@ -1004,6 +1028,12 @@ update_watchpoint (struct breakpoint *b, > > bpstat bs; > > struct program_space *frame_pspace; > > > > + /* If this is a local watchpoint, we only want to check if the > > + watchpoint frame is in scope if the current thread is the thread > > + that was used to create the watchpoint. */ > > + if (!watchpoint_thread_match (b)) > > + return; > > + > > /* We don't free locations. They are stored in bp_location array and > > update_global_locations will eventually delete them and remove > > breakpoints if needed. */ > > For all-stop, do we want to check whenever the watchpoint's thread is > stopped? Yep (non-stop), good catch. Usually we'd get here with a stopped thread, but there's always say, '(gdb) file' -> breakpoint_re_set -> update_watchpoint. I'm actually thinking that path may need further tweaking if the current thread is running, but I'll defer putting more brain cycles on that until more non-stop watchpoint support is in place. Here's the updated patch. New test unchanged. Tested on x86_64-linux. 2009-11-19 Pedro Alves gdb/ * breakpoint.h (struct breakpoint) : New field. * breakpoint.c (watchpoint_in_thread_scope): New. (update_watchpoint): Skip if the local watchpoint's thread doesn't match the current thread, or if the current thread is running. (watchpoint_check): Ditto. (watch_command_1): Set the watchpoint's watchpoint_thread field. 2009-11-19 Pedro Alves gdb/testsuite/ * local-watch-wrong-thread.c, local-watch-wrong-thread.exp: New files. More comments? -- Pedro Alves 2009-11-19 Pedro Alves gdb/ * breakpoint.h (struct breakpoint) : New field. * breakpoint.c (watchpoint_in_thread_scope): New. (update_watchpoint): Skip if the local watchpoint's thread doesn't match the current thread, or if the current thread is running. (watchpoint_check): Ditto. (watch_command_1): Set the watchpoint's watchpoint_thread field. --- gdb/breakpoint.c | 39 +++++++++++++++++++++++++++++++++++++-- gdb/breakpoint.h | 5 +++++ 2 files changed, 42 insertions(+), 2 deletions(-) Index: src/gdb/breakpoint.h =================================================================== --- src.orig/gdb/breakpoint.h 2009-11-20 00:07:15.000000000 +0000 +++ src/gdb/breakpoint.h 2009-11-20 00:15:39.000000000 +0000 @@ -457,6 +457,11 @@ struct breakpoint should be evaluated on the outermost frame. */ struct frame_id watchpoint_frame; + /* Holds the thread which identifies the frame this watchpoint + should be considered in scope for, or `null_ptid' if the + watchpoint should be evaluated in all threads. */ + ptid_t watchpoint_thread; + /* For hardware watchpoints, the triggered status according to the hardware. */ enum watchpoint_triggered watchpoint_triggered; Index: src/gdb/breakpoint.c =================================================================== --- src.orig/gdb/breakpoint.c 2009-11-20 00:07:15.000000000 +0000 +++ src/gdb/breakpoint.c 2009-11-20 00:26:07.000000000 +0000 @@ -985,6 +985,23 @@ fetch_watchpoint_value (struct expressio } } +/* Assuming that B is a watchpoint: returns true if the current thread + and its running state are safe to evaluate or update watchpoint B. + Watchpoints on local expressions need to evaluated in the context + of the thread that was current when the watchpoint was created, + and, that thread needs to be stopped to be able to select the + correct frame context. Watchpoints on global expressions can be + evaluated on any thread, and in any state. It is presently left to + the target allowing memory accesses when threads are running. */ + +static int +watchpoint_in_thread_scope (struct breakpoint *b) +{ + return (ptid_equal (b->watchpoint_thread, null_ptid) + || (ptid_equal (inferior_ptid, b->watchpoint_thread) + && !is_executing (inferior_ptid))); +} + /* Assuming that B is a watchpoint: - Reparse watchpoint expression, if REPARSE is non-zero - Evaluate expression and store the result in B->val @@ -1004,6 +1021,12 @@ update_watchpoint (struct breakpoint *b, bpstat bs; struct program_space *frame_pspace; + /* If this is a local watchpoint, we only want to check if the + watchpoint frame is in scope if the current thread is the thread + that was used to create the watchpoint. */ + if (!watchpoint_in_thread_scope (b)) + return; + /* We don't free locations. They are stored in bp_location array and update_global_locations will eventually delete them and remove breakpoints if needed. */ @@ -3085,6 +3108,12 @@ watchpoint_check (void *p) b = bs->breakpoint_at->owner; + /* If this is a local watchpoint, we only want to check if the + watchpoint frame is in scope if the current thread is the thread + that was used to create the watchpoint. */ + if (!watchpoint_in_thread_scope (b)) + return WP_VALUE_NOT_CHANGED; + if (b->exp_valid_block == NULL) within_current_scope = 1; else @@ -7151,9 +7180,15 @@ watch_command_1 (char *arg, int accessfl b->cond_string = 0; if (frame) - b->watchpoint_frame = get_frame_id (frame); + { + b->watchpoint_frame = get_frame_id (frame); + b->watchpoint_thread = inferior_ptid; + } else - b->watchpoint_frame = null_frame_id; + { + b->watchpoint_frame = null_frame_id; + b->watchpoint_thread = null_ptid; + } if (scope_breakpoint != NULL) { =================================================================== 2009-11-19 Pedro Alves gdb/testsuite/ * local-watch-wrong-thread.c, local-watch-wrong-thread.exp: New files. --- gdb/testsuite/gdb.threads/local-watch-wrong-thread.c | 80 +++++++++++ gdb/testsuite/gdb.threads/local-watch-wrong-thread.exp | 118 +++++++++++++++++ 2 files changed, 198 insertions(+) Index: src/gdb/testsuite/gdb.threads/local-watch-wrong-thread.c =================================================================== --- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ src/gdb/testsuite/gdb.threads/local-watch-wrong-thread.c 2009-11-19 01:10:15.000000000 +0000 @@ -0,0 +1,80 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2002, 2003, 2004, 2007, 2008, 2009 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . + +*/ + +#include +#include +#include + +unsigned int args[2]; +int trigger = 0; + +void * +thread_function0 (void *arg) +{ + int my_number = (long) arg; + volatile int *myp = (volatile int *) &args[my_number]; + + while (*myp > 0) + { + (*myp) ++; + usleep (1); /* Loop increment 1. */ + } + + return NULL; +} + +void * +thread_function1 (void *arg) +{ + int my_number = (long) arg; + + volatile int *myp = (volatile int *) &args[my_number]; + + while (*myp > 0) + { + (*myp) ++; + usleep (1); /* Loop increment 2. */ + } + + return NULL; +} + +int +main () +{ + int res; + pthread_t threads[2]; + void *thread_result; + long i = 0; + + args[i] = 1; /* Init value. */ + res = pthread_create (&threads[i], NULL, + thread_function0, + (void *) i); + + i++; + args[i] = 1; /* Init value. */ + res = pthread_create(&threads[i], NULL, + thread_function1, + (void *) i); + + pthread_join (threads[0], &thread_result); + pthread_join (threads[1], &thread_result); + exit(EXIT_SUCCESS); +} Index: src/gdb/testsuite/gdb.threads/local-watch-wrong-thread.exp =================================================================== --- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ src/gdb/testsuite/gdb.threads/local-watch-wrong-thread.exp 2009-11-19 02:02:12.000000000 +0000 @@ -0,0 +1,118 @@ +# This testcase is part of GDB, the GNU debugger. + +# Copyright 2004, 2007, 2008, 2009 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + +# Check that GDB can support multiple watchpoints across threads. + +# This test verifies that a local watchpoint isn't deleted when a +# thread other than the thread the local watchpoint was set in stops +# for a breakpoint. +if [target_info exists gdb,no_hardware_watchpoints] { + return 0; +} + +set testfile "local-watch-wrong-thread" +set srcfile ${testfile}.c +set binfile ${objdir}/${subdir}/${testfile} +if {[gdb_compile_pthreads \ + "${srcdir}/${subdir}/${srcfile}" \ + "${binfile}" executable {debug} ] != "" } { + return -1 +} + +gdb_exit +gdb_start +gdb_reinitialize_dir $srcdir/$subdir +gdb_load ${binfile} + +gdb_test "set can-use-hw-watchpoints 1" "" "" + +if ![runto_main] then { + fail "Can't run to main" + return +} + +set inc_line_1 [gdb_get_line_number "Loop increment 1"] +set inc_line_2 [gdb_get_line_number "Loop increment 2"] + +# Run to the loop within thread_function0, so we can set our local +# watchpoint. +gdb_test "break ${srcfile}:${inc_line_1}" \ + "Breakpoint 2 at .*: file .*${srcfile}, line .*" \ + "breakpoint on thread_function0" + +gdb_test "continue" \ + ".*Breakpoint 2.*thread_function0.*" \ + "continue to thread_function0" + +delete_breakpoints + +# Set the local watchpoint, and confirm that it traps as expected. +gdb_test "watch *myp" \ + "Hardware watchpoint 3\: \\*myp.*" \ + "set local watchpoint on *myp" + +gdb_test "continue" \ + "Hardware watchpoint.*Old value.*New value.*thread_function0.*" \ + "local watchpoint triggers" + +delete_breakpoints + +# Recreate the watchpoint, but this time with a condition we know +# won't trigger. This is so the watchpoint is inserted, and the +# target reports events, but GDB doesn't stop for them. We want to +# hit the breakpoints on the other thread, and make sure this +# watchpoint isn't deleted then. +gdb_test "watch *myp if trigger != 0" \ + "Hardware watchpoint 4\: \\*myp.*" \ + "set local watchpoint on *myp, with false conditional" + +# Run to a breakpoint on a different thread. The previous local +# watchpoint should not be deleted with the standard 'the program has +# left the block in which its expression is valid', because the +# thread_function0 thread should still be running in the loop. +gdb_test "break ${srcfile}:${inc_line_2}" \ + "Breakpoint 5 at .*: file .*${srcfile}, line .*" \ + "breakpoint on the other thread" + +gdb_test "continue" \ + "Breakpoint 5, thread_function1.*" \ + "the other thread stopped on breakpoint" + +# Delete the new breakpoint, we don't need it anymore. +gdb_test "delete 5" "" "" + +# Check if the local watchpoint hasn't been deleted (is still listed). +# This is simpler to check than expecting 'the program has left ...', +# and, is immune to string changes in that warning. +gdb_test "info breakpoints" \ + ".*4.*hw watchpoint.*keep.*y.*\\*myp.*" \ + "local watchpoint is still in breakpoint list" + +# Make the watchpoint condition eval true. +gdb_test "set trigger=1" "" "let local watchpoint trigger" + +gdb_test "continue" \ + "Hardware watchpoint.*Old value.*New value.*thread_function0.*" \ + "local watchpoint still triggers" + +# Confirm that the local watchpoint is indeed deleted if +# thread_function0 returns. +gdb_test "set *myp=0" "" "let thread_function0 return" + +gdb_test "continue" \ + ".*Watchpoint.*deleted.*" \ + "local watchpoint automatically deleted"