From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10549 invoked by alias); 11 Dec 2011 23:39:16 -0000 Received: (qmail 10541 invoked by uid 22791); 11 Dec 2011 23:39:14 -0000 X-SWARE-Spam-Status: No, hits=-7.3 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_QE 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; Sun, 11 Dec 2011 23:38:56 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id pBBNce96032252 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Sun, 11 Dec 2011 18:38:41 -0500 Received: from host2.jankratochvil.net (ovpn-116-39.ams2.redhat.com [10.36.116.39]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id pBBNcDJb031515 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Sun, 11 Dec 2011 18:38:23 -0500 Date: Mon, 12 Dec 2011 00:14:00 -0000 From: Jan Kratochvil To: Pedro Alves Cc: Tristan Gingold , gdb-patches@sourceware.org Subject: Re: [PATCH] PR threads/10729: x86 hw watchpoints and non-stop mode Message-ID: <20111211233811.GA27629@host2.jankratochvil.net> References: <201112051601.59664.pedro@codesourcery.com> <20111205202513.GA26929@host2.jankratochvil.net> <201112091630.20916.pedro@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201112091630.20916.pedro@codesourcery.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: 2011-12/txt/msg00337.txt.bz2 Hi Pedro, on simple non-hit (inferior does not touch "j" at all) watchpoint case: strace -o 2 -q ./gdb -nx ./36 -ex start -ex 'watch j' -ex stepi -ex 'set confirm no' -ex q grep 'PTRACE_....USER' 1 >1b; grep 'PTRACE_....USER' 2 >2b It has performance regression of 15 ptrace syscalls -> 27 ptrace syscalls. On Fri, 09 Dec 2011 17:30:20 +0100, Pedro Alves wrote: > @@ -513,22 +499,7 @@ i386_update_inferior_debug_regs (struct i386_debug_reg_state *new_state) > ALL_DEBUG_REGISTERS (i) > { > if (I386_DR_VACANT (new_state, i) != I386_DR_VACANT (&dr_mirror, i)) > - { > - if (!I386_DR_VACANT (new_state, i)) > - { > - i386_dr_low.set_addr (i, new_state->dr_mirror[i]); > - > - /* Only a sanity check for leftover bits (set possibly only > - by inferior). */ > - if (i386_dr_low.unset_status) > - i386_dr_low.unset_status (I386_DR_WATCH_MASK (i)); Deleting this part is a regression. Testcase for that part is attached. > - } > - else > - { > - if (i386_dr_low.reset_addr) > - i386_dr_low.reset_addr (i); > - } > - } > + i386_dr_low.set_addr (i, new_state->dr_mirror[i]); > else > gdb_assert (new_state->dr_mirror[i] == dr_mirror.dr_mirror[i]); > } > @@ -636,11 +607,12 @@ i386_stopped_data_address (struct target_ops *ops, CORE_ADDR *addr_p) > int rc = 0; > unsigned status; > unsigned control; > - struct i386_debug_reg_state *state = &dr_mirror; > > - dr_mirror.dr_status_mirror = i386_dr_low.get_status (); > - status = dr_mirror.dr_status_mirror; > - control = dr_mirror.dr_control_mirror; > + /* Get the current values the inferior has. If the thread was > + running when we last changed watchpoints, the mirror no longer > + represents what was set in this thread's debug registers. */ > + status = i386_dr_low.get_status (); > + control = i386_dr_low.get_control (); > > ALL_DEBUG_REGISTERS(i) > { > @@ -650,12 +622,9 @@ i386_stopped_data_address (struct target_ops *ops, CORE_ADDR *addr_p) > that GDB doesn't call the target_stopped_data_address > method except for data watchpoints. In other words, I'm > being paranoiac. */ > - && I386_DR_GET_RW_LEN (control, i) != 0 > - /* This third condition makes sure DRi is not vacant, this > - avoids false positives in windows-nat.c. */ > - && !I386_DR_VACANT (state, i)) This removal is probably safe as everything gets pre-cleared but after you fix the performance regressions I am not sure if it should not be kept there. > + && I386_DR_GET_RW_LEN (control, i) != 0) > { > - addr = state->dr_mirror[i]; > + addr = i386_dr_low.get_addr (i); Why to do this change? Why we can no longer trust DR_MIRROR? This is a performance regression. > rc = 1; > if (maint_show_dr) > i386_show_dr (&dr_mirror, "watchpoint_hit", addr, -1, hw_write); gdb/testsuite/ 2011-12-11 Jan Kratochvil * gdb.base/watchpoint-hw-pre-set.c: New file. * gdb.base/watchpoint-hw-pre-set.exp: New file. --- /dev/null +++ b/gdb/testsuite/gdb.base/watchpoint-hw-pre-set.c @@ -0,0 +1,155 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2011 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 . */ + +#define _GNU_SOURCE 1 +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include + +#define SET_WATCHPOINT set_watchpoint + +static void +set_watchpoint (pid_t pid, volatile void *addr) +{ + unsigned long dr7; + long l; + + errno = 0; + l = ptrace (PTRACE_POKEUSER, pid, + offsetof (struct user, u_debugreg[0]), (unsigned long) addr); + assert_perror (errno); + assert (l == 0); + + dr7 = (DR_RW_WRITE << DR_CONTROL_SHIFT); + dr7 |= (DR_LEN_4 << DR_CONTROL_SHIFT); + dr7 |= (1UL << DR_LOCAL_ENABLE_SHIFT); + dr7 |= (1UL << DR_GLOBAL_ENABLE_SHIFT); + + l = ptrace (PTRACE_POKEUSER, pid, offsetof (struct user, u_debugreg[7]), dr7); + assert_perror (errno); + assert (l == 0); +} + +static pid_t child; + +static void +cleanup (void) +{ + if (child > 0) + kill (child, SIGKILL); + child = 0; +} + +static void +handler_fail (int signo) +{ + cleanup (); + signal (signo, SIG_DFL); + raise (signo); +} + +static volatile long long check, dummy; + +static void +marker (void) +{ + dummy++; +} + +static int resume; + +int +main (void) +{ + pid_t got_pid; + int i, status, cycles = 0; + long l; + + atexit (cleanup); + signal (SIGABRT, handler_fail); + signal (SIGINT, handler_fail); + + child = fork (); + switch (child) + { + case -1: + assert (0); + case 0: + l = ptrace (PTRACE_TRACEME, 0, NULL, NULL); + assert (l == 0); + + i = raise (SIGUSR1); + assert (i == 0); + + check = 1; + + while (!resume && cycles++ < 600 * 10) + usleep (1000000 / 10); + + marker (); + + i = raise (SIGUSR2); + assert (i == 0); + /* NOTREACHED */ + assert (0); + default: + break; + } + + got_pid = waitpid (child, &status, 0); + assert (got_pid == child); + assert (WIFSTOPPED (status)); + assert (WSTOPSIG (status) == SIGUSR1); + + SET_WATCHPOINT (child, &check); + + errno = 0; + l = ptrace (PTRACE_CONT, child, NULL, NULL); + assert_perror (errno); + assert (l == 0); + + got_pid = waitpid (child, &status, 0); + assert (got_pid == child); + assert (WIFSTOPPED (status)); + if (WSTOPSIG (status) == SIGUSR2) + { + /* We missed the watchpoint - unsupported by hardware? Found on: + + qemu-system-x86_64 0.9.1-6.fc9.x86_64 + + qemu-kvm kvm-65-7.fc9.x86_64 + kernel-2.6.25.9-76.fc9.x86_64. */ + return 2; + } + assert (WSTOPSIG (status) == SIGTRAP); + + errno = 0; + l = ptrace (PTRACE_DETACH, child, NULL, NULL); + assert_perror (errno); + assert (l == 0); + + marker (); + + return 0; +} --- /dev/null +++ b/gdb/testsuite/gdb.base/watchpoint-hw-pre-set.exp @@ -0,0 +1,62 @@ +# Copyright 2011 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 . + +# Test a newly created hardware watchpoint gets cleared its possible pre-set +# hit in the status register. Otherwise a false hit may occur. + +if {[skip_hw_watchpoint_access_tests] + || (![istarget "i?86-*-linux*"] && ![istarget "x86_64-*-linux*"]) + || [is_remote target]} { + return 0 +} + +set test watchpoint-hw-pre-set +set srcfile ${test}.c +if { [prepare_for_testing ${test}.exp ${test} ${srcfile}] } { + return -1 +} + +if ![runto "marker"] { + return -1 +} + +set test "print child" +gdb_test_multiple $test $test { + -re " = (\[0-9\]+)\r\n$gdb_prompt $" { + pass $test + set child $expect_out(1,string) + } +} + +gdb_test "attach $child" "Attaching to program: .*, process $child\r\n.*" \ + "attach" \ + {A program is being debugged already\. Kill it\? \(y or n\) } "y" + +gdb_test_no_output "set variable resume=1" +#gdb_test "maintenance set show-debug-regs on" +gdb_test "awatch check" {Hardware access \(read/write\) watchpoint [0-9]+: check} + +set test "stepi" +gdb_test_multiple $test $test { + -re "\r\nHardware access \\(read/write\\) watchpoint \[0-9\]+: check\r\n.*\r\n$gdb_prompt $" { + fail $test + } + -re "\r\n$gdb_prompt $" { + pass $test + } +} + +gdb_test "kill" "" "kill" \ + {Kill the program being debugged\? \(y or n\) } "y"