From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21920 invoked by alias); 20 Jan 2012 21:31:38 -0000 Received: (qmail 21912 invoked by uid 22791); 20 Jan 2012 21:31:36 -0000 X-SWARE-Spam-Status: No, hits=-6.6 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_XZ,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 20 Jan 2012 21:31:21 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q0KLVJXB021339 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 20 Jan 2012 16:31:19 -0500 Received: from host2.jankratochvil.net (ovpn-116-21.ams2.redhat.com [10.36.116.21]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q0KLVB6D013158 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Fri, 20 Jan 2012 16:31:13 -0500 Date: Fri, 20 Jan 2012 21:34:00 -0000 From: Jan Kratochvil To: Pedro Alves Cc: gdb-patches@sourceware.org Subject: [patch 2/2] Fix watchpoints for multi-inferior #2 Message-ID: <20120120213110.GB424@host2.jankratochvil.net> References: <20120102164652.GB10231@host2.jankratochvil.net> <4F02020F.5090906@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4F02020F.5090906@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) 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: 2012-01/txt/msg00750.txt.bz2 On Mon, 02 Jan 2012 20:14:23 +0100, Pedro Alves wrote: > Do we clear inf_data or inf_data's contents anywhere on inferior > exit or startup, so to not leave debug registers stale across runs? > (The cleanup only runs when the inferior is deleted.) Yes, it is already cleared in FSF GDB. Plus I think this issue is unrelated to this multi-inferiorization patch. #0 i386_init_dregs (state=0x24ad330) at i386-nat.c:165 #1 in i386_cleanup_dregs () at i386-nat.c:311 #2 in amd64_linux_child_post_startup_inferior (ptid=...) at amd64-linux-nat.c:502 #3 in inf_ptrace_create_inferior (ops=0x1f4d100, exec_file=0x20d4390 "/home/jkratoch/redhat/archer-jankratochvil-watchpoint3/gdb/gdb", allargs=0x24ad3d0 "", env=0x20b84a0, from_tty=1) at inf-ptrace.c:148 > >--- /dev/null > >+++ b/gdb/testsuite/gdb.multi/watchpoint-multi.exp [...] > >+if [is_remote target] { > >+ # It is KFAIL. > >+ continue > > Did you mean to turn this into a real kfail? What are the > gdbserver problems, btw? It is no longer KFAIL, included gdbserver fixes. The first one is for dead-loop of: Got an event from pending child 10373 (057f) Got a pending child 10373 Got an event from pending child 10373 (057f) Got a pending child 10373 because linux_wait_for_event creates creates status_pending_p and then asks linux_wait_for_event_1 for the next event which apparently returns the newly created status_pending_p so linux_wait_for_event stores it back and so on. The second fix is that despite default `set schedule-multiple off' gdbserver sometimes resumed all the inferiors on GDB "continue". Both cases are visible with the testcase (the first one in ~50% of runs). > >+# Simulate non-stop+target-async which also uses breakpoint always-inserted. > >+gdb_test_no_output "set breakpoint always-inserted on" > >+# displaced-stepping is also needed as other GDB sometimes still removes the > >+# breakpoints, even with always-inserted on. > >+gdb_test_no_output "set displaced-stepping on" > > 'if ![support_displaced_stepping] bail' missing, as well as the usual > hw watchpoints support checks. Fixed. The testcase runs in extended-remote mode only with Pedro's new board I have not seen but it follows the advice: Re: testsuite: native/non-extended/extended modes http://sourceware.org/ml/gdb-patches/2012-01/msg00740.html No regressions on {x86_64,x86_64-m32,i686}-fedorarawhide-linux-gnu and in gdbserver mode and on ppc64-rhel62-linux-gnu. Thanks, Jan gdb/ 2012-01-20 Jan Kratochvil Fix watchpoints to be specific for each inferior. * breakpoint.c (watchpoint_in_thread_scope): Verify also current_program_space. * i386-nat.c (i386_inferior_data_cleanup): New. (i386_inferior_data_get): Replace variable inf_data_local by an inferior_data call. (i386_use_watchpoints): Initialize i386_inferior_data. * linux-nat.c (linux_nat_iterate_watchpoint_lwps): Use INFERIOR_PTID specific iterate_over_lwps. gdb/gdbserver/ 2012-01-20 Jan Kratochvil * linux-low.c (linux_wait_for_event_1): Rename to ... (linux_wait_for_event): ... here and merge it with former linux_wait_for_event - new variable wait_ptid, use it. (linux_wait_for_event): Remove - merge it to linux_wait_for_event_1. (linux_wait_1): Resume only single inferior if CONT_THREAD is set so. Gdb/testsuite/ 2012-01-20 Jan Kratochvil Fix watchpoints to be specific for each inferior. * gdb.multi/watchpoint-multi.c: New file. * gdb.multi/watchpoint-multi.exp: New file. --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -1239,9 +1239,10 @@ is_watchpoint (const struct breakpoint *bpt) static int watchpoint_in_thread_scope (struct watchpoint *b) { - return (ptid_equal (b->watchpoint_thread, null_ptid) - || (ptid_equal (inferior_ptid, b->watchpoint_thread) - && !is_executing (inferior_ptid))); + return (b->base.pspace == current_program_space + && (ptid_equal (b->watchpoint_thread, null_ptid) + || (ptid_equal (inferior_ptid, b->watchpoint_thread) + && !is_executing (inferior_ptid)))); } /* Set watchpoint B to disp_del_at_next_stop, even including its possible --- a/gdb/gdbserver/linux-low.c +++ b/gdb/gdbserver/linux-low.c @@ -1569,9 +1569,10 @@ ptid_t step_over_bkpt; the stopped child otherwise. */ static int -linux_wait_for_event_1 (ptid_t ptid, int *wstat, int options) +linux_wait_for_event (ptid_t ptid, int *wstat, int options) { struct lwp_info *event_child, *requested_child; + ptid_t wait_ptid; event_child = NULL; requested_child = NULL; @@ -1621,13 +1622,24 @@ linux_wait_for_event_1 (ptid_t ptid, int *wstat, int options) return lwpid_of (event_child); } + if (ptid_is_pid (ptid)) + { + /* A request to wait for a specific tgid. This is not possible + with waitpid, so instead, we wait for any child, and leave + children we're not interested in right now with a pending + status to report later. */ + wait_ptid = minus_one_ptid; + } + else + wait_ptid = ptid; + /* We only enter this loop if no process has a pending wait status. Thus any action taken in response to a wait status inside this loop is responding as soon as we detect the status, not after any pending events. */ while (1) { - event_child = linux_wait_for_lwp (ptid, wstat, options); + event_child = linux_wait_for_lwp (wait_ptid, wstat, options); if ((options & WNOHANG) && event_child == NULL) { @@ -1639,6 +1651,19 @@ linux_wait_for_event_1 (ptid_t ptid, int *wstat, int options) if (event_child == NULL) error ("event from unknown child"); + if (ptid_is_pid (ptid) + && ptid_get_pid (ptid) != ptid_get_pid (ptid_of (event_child))) + { + if (! WIFSTOPPED (*wstat)) + mark_lwp_dead (event_child, *wstat); + else + { + event_child->status_pending_p = 1; + event_child->status_pending = *wstat; + } + continue; + } + current_inferior = get_lwp_thread (event_child); /* Check for thread exit. */ @@ -1731,48 +1756,6 @@ linux_wait_for_event_1 (ptid_t ptid, int *wstat, int options) return 0; } -static int -linux_wait_for_event (ptid_t ptid, int *wstat, int options) -{ - ptid_t wait_ptid; - - if (ptid_is_pid (ptid)) - { - /* A request to wait for a specific tgid. This is not possible - with waitpid, so instead, we wait for any child, and leave - children we're not interested in right now with a pending - status to report later. */ - wait_ptid = minus_one_ptid; - } - else - wait_ptid = ptid; - - while (1) - { - int event_pid; - - event_pid = linux_wait_for_event_1 (wait_ptid, wstat, options); - - if (event_pid > 0 - && ptid_is_pid (ptid) && ptid_get_pid (ptid) != event_pid) - { - struct lwp_info *event_child - = find_lwp_pid (pid_to_ptid (event_pid)); - - if (! WIFSTOPPED (*wstat)) - mark_lwp_dead (event_child, *wstat); - else - { - event_child->status_pending_p = 1; - event_child->status_pending = *wstat; - } - } - else - return event_pid; - } -} - - /* Count the LWP's that have had events. */ static int @@ -2107,7 +2090,14 @@ retry: if (thread == NULL) { struct thread_resume resume_info; - resume_info.thread = minus_one_ptid; + + /* Resume only a single process if requested so. */ + if (!ptid_equal (cont_thread, minus_one_ptid) + && ptid_get_lwp (cont_thread) == -1) + resume_info.thread = cont_thread; + else + resume_info.thread = minus_one_ptid; + resume_info.kind = resume_continue; resume_info.sig = 0; linux_resume (&resume_info, 1); --- a/gdb/i386-nat.c +++ b/gdb/i386-nat.c @@ -181,16 +181,31 @@ struct i386_inferior_data struct i386_debug_reg_state state; }; +/* Per-inferior hook for register_inferior_data_with_cleanup. */ + +static void +i386_inferior_data_cleanup (struct inferior *inf, void *arg) +{ + struct i386_inferior_data *inf_data = arg; + + xfree (inf_data); +} + /* Get data specific for INFERIOR_PTID LWP. Return special data area for processes being detached. */ static struct i386_inferior_data * i386_inferior_data_get (void) { - /* Intermediate patch stub. */ - static struct i386_inferior_data inf_data_local; struct inferior *inf = current_inferior (); - struct i386_inferior_data *inf_data = &inf_data_local; + struct i386_inferior_data *inf_data; + + inf_data = inferior_data (inf, i386_inferior_data); + if (inf_data == NULL) + { + inf_data = xzalloc (sizeof (*inf_data)); + set_inferior_data (current_inferior (), i386_inferior_data, inf_data); + } if (inf->pid != ptid_get_pid (inferior_ptid)) { @@ -856,6 +871,10 @@ i386_use_watchpoints (struct target_ops *t) t->to_remove_watchpoint = i386_remove_watchpoint; t->to_insert_hw_breakpoint = i386_insert_hw_breakpoint; t->to_remove_hw_breakpoint = i386_remove_hw_breakpoint; + + if (i386_inferior_data == NULL) + i386_inferior_data + = register_inferior_data_with_cleanup (i386_inferior_data_cleanup); } void --- /dev/null +++ b/gdb/testsuite/gdb.multi/watchpoint-multi.c @@ -0,0 +1,51 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2012 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 + +static volatile int a, b, c; + +static void +marker_exit (void) +{ + a = 1; +} + +static void * +start (void *arg) +{ + b = 2; + c = 3; + + return NULL; +} + +int +main (void) +{ + pthread_t thread; + int i; + + i = pthread_create (&thread, NULL, start, NULL); + assert (i == 0); + i = pthread_join (thread, NULL); + assert (i == 0); + + marker_exit (); + return 0; +} --- /dev/null +++ b/gdb/testsuite/gdb.multi/watchpoint-multi.exp @@ -0,0 +1,91 @@ +# Copyright 2012 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 . + +set testfile "watchpoint-multi" + +# Multiple inferiors are needed, therefore both native and extended gdbserver +# modes are supported. Only non-extended gdbserver is not supported. +if [target_info exists use_gdb_stub] { + untested ${testfile}.exp + return +} + +# Do not use simple hardware watchpoints ("watch") as its false hit may be +# unnoticed by GDB if it reads it still has the same value. +if [skip_hw_watchpoint_access_tests] { + untested "${testfile} (no hardware access watchpoints)" + return +} + +set executable ${testfile} +set srcfile ${testfile}.c +set binfile ${objdir}/${subdir}/${executable} + +if { [gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } { + untested ${testfile}.exp + return -1 +} + +clean_restart $executable + +# Simulate non-stop+target-async which also uses breakpoint always-inserted. +gdb_test_no_output "set breakpoint always-inserted on" +# displaced-stepping is also needed as other GDB sometimes still removes the +# breakpoints, even with always-inserted on. +# Without the support this test just is not as thorough as it could be. +if [support_displaced_stepping] { + gdb_test_no_output "set displaced-stepping on" +} + +# Debugging of this testcase: +#gdb_test_no_output "maintenance set show-debug-regs on" +#gdb_test_no_output "set debug infrun 1" + +gdb_breakpoint main {temporary} +gdb_test "run" "Temporary breakpoint.* main .*" "start to main inferior 1" + +gdb_test "add-inferior" "Added inferior 2" +gdb_test "inferior 2" "witching to inferior 2 .*" +gdb_load $binfile + +gdb_breakpoint main {temporary} +gdb_test "run" "Temporary breakpoint.* main .*" "start to main inferior 2" + +gdb_test "awatch c" "Hardware access \\(read/write\\) watchpoint \[0-9\]+: c" + +gdb_breakpoint "marker_exit" + +gdb_test "inferior 1" "witching to inferior 1 .*" + +if [skip_hw_watchpoint_multi_tests] { + # On single hardware watchpoint at least test the watchpoint in inferior + # 2 is not hit. +} else { + gdb_test "awatch b" "Hardware access \\(read/write\\) watchpoint \[0-9\]+: b" + + gdb_test "inferior 2" "witching to inferior 2 .*" + + # FAIL would be a hit on watchpoint for `b' - that one is for the other + # inferior. + gdb_test "continue" "Hardware access \\(read/write\\) watchpoint \[0-9\]+: c\r\n\r\nOld value = 0\r\nNew value = 3\r\n.*" "catch c" + + gdb_test "continue" "Breakpoint \[0-9\]+, marker_exit .*" "catch marker_exit in inferior 2" + + gdb_test "inferior 1" "witching to inferior 1 .*" + + gdb_test "continue" "Hardware access \\(read/write\\) watchpoint \[0-9\]+: b\r\n\r\nOld value = 0\r\nNew value = 2\r\n.*" "catch b" +} + +gdb_test "continue" "Breakpoint \[0-9\]+, marker_exit .*" "catch marker_exit in inferior 1"