From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24070 invoked by alias); 25 Sep 2012 21:48:34 -0000 Received: (qmail 24058 invoked by uid 22791); 25 Sep 2012 21:48:33 -0000 X-SWARE-Spam-Status: No, hits=-3.4 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mms2.broadcom.com (HELO mms2.broadcom.com) (216.31.210.18) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 25 Sep 2012 21:48:17 +0000 Received: from [10.9.200.133] by mms2.broadcom.com with ESMTP (Broadcom SMTP Relay (Email Firewall v6.5)); Tue, 25 Sep 2012 14:46:40 -0700 X-Server-Uuid: 4500596E-606A-40F9-852D-14843D8201B2 Received: from mail-irva-13.broadcom.com (10.11.16.103) by IRVEXCHHUB02.corp.ad.broadcom.com (10.9.200.133) with Microsoft SMTP Server id 8.2.247.2; Tue, 25 Sep 2012 14:47:31 -0700 Received: from [10.177.252.18] (unknown [10.177.252.18]) by mail-irva-13.broadcom.com (Postfix) with ESMTP id 5D6F240FE4 for ; Tue, 25 Sep 2012 14:48:05 -0700 (PDT) Message-ID: <50622695.3040002@broadcom.com> Date: Tue, 25 Sep 2012 21:48:00 -0000 From: "Andrew Burgess" User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:15.0) Gecko/20120907 Thunderbird/15.0.1 MIME-Version: 1.0 To: gdb-patches@sourceware.org Subject: Re: [PATCH] Failure to stop at duplicate breakpoints References: <505B31C2.5010203@broadcom.com> <505BA4D2.7070104@broadcom.com> <5061D8B8.2050209@redhat.com> In-Reply-To: <5061D8B8.2050209@redhat.com> Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 7bit 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-09/txt/msg00564.txt.bz2 Pedro, Thanks for taking the time to review my patch. I've addressed the issues you raised, and On 25/09/2012 5:15 PM, Pedro Alves wrote: > >> --- /dev/null >> +++ b/gdb/testsuite/gdb.base/duplicate-bp.c > > Please add the standard copyright header boilerplate. Even if the > contents of the file don't have that much copyrightable content > now, it's better to always add a header as a rule, than to > police later changes to files and worry about having to add > a header then. Done. >> + >> +if { [prepare_for_testing duplicate-bp.exp "duplicate-bp" {duplicate-bp.c} {debug nowarnings}] } { > > Could you make this use standard_testfile? Like: > > standard_testfile > > if { [prepare_for_testing ${testfile}.exp ${testfile} $srcfile {debug nowarnings}] } { Done, well it turns out "prepare_for_testing ${testfile}.exp ${testfile}" is enough. > > >> + return -1 >> +} >> +set srcfile "duplicate-bp.c" > > This won't be needed then. > > While at it, is "nowarnings" really necessary? Removed. > > >> + >> +# Setup for the test, create COUNT breakpoints at the function BREAKPT. >> +proc test_setup { count } { >> + upvar srcfile srcfile > > It is more idiomatic in our testsuite to use "global srcfile". Fixed. > > There are many duplicate test output messages in the .sum file: > They should all be unique now. > > Also, please get rid of the trailing whitespace the patch is adding: > Done. Again, thanks for taking the time to review this patch. Cheers, Andrew gdb/ChangeLog 2012-09-25 Andrew Burgess * breakpoint.c (update_global_location_list): Ignore previous duplicate status of a breakpoint when starting a new scan for duplicate breakpoints. diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index b841bcd..f771d06 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -12463,7 +12463,7 @@ update_global_location_list (int should_insert) struct bp_location **loc_first_p; b = loc->owner; - if (!should_be_inserted (loc) + if (!unduplicated_should_be_inserted (loc) || !breakpoint_address_is_meaningful (b) /* Don't detect duplicate for tracepoint locations because they are never duplicated. See the comments in field `duplicate' of gdb/testsuite/ChangeLog 2012-09-25 Andrew Burgess * gdb.base/duplicate-bp.c: New file. * gdb.base/duplicate-bp.exp: New file. diff --git a/gdb/testsuite/gdb.base/duplicate-bp.c b/gdb/testsuite/gdb.base/duplicate-bp.c new file mode 100644 index 0000000..662a7c4 --- /dev/null +++ b/gdb/testsuite/gdb.base/duplicate-bp.c @@ -0,0 +1,40 @@ +/* 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 . */ + +void +spacer () +{ + /* Nothing. */ +} + +void +breakpt () +{ + /* Nothing. */ +} + +int +main () +{ + spacer (); + + breakpt (); + + spacer (); + + return 0; +} diff --git a/gdb/testsuite/gdb.base/duplicate-bp.exp b/gdb/testsuite/gdb.base/duplicate-bp.exp new file mode 100644 index 0000000..01279fb --- /dev/null +++ b/gdb/testsuite/gdb.base/duplicate-bp.exp @@ -0,0 +1,156 @@ +# 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 . + +standard_testfile + +if { [prepare_for_testing ${testfile}.exp ${testfile}] } { + return -1 +} + +# Setup for the test, create COUNT breakpoints at the function BREAKPT. +proc test_setup { count } { + global srcfile + + clean_restart duplicate-bp + + if ![runto_main] then { + fail "can't run to main" + return 0 + } + + for {set i 1} {$i <= $count} {incr i} { + gdb_breakpoint "breakpt" + gdb_test_no_output "set \$bp_num_${i} = \$bpnum" + } + + gdb_test "step" \ + "spacer \\(\\) at .*$srcfile:\[0-9\]+.*" \ + "step to place breakpoints" + + return 1 +} + + +# Test 1: Create two breakpoints at BREAKPT. Delete #1 and expect to stop +# at #2. +with_test_prefix "del_1_stop_2" { + test_setup 2 + + gdb_test_no_output {delete $bp_num_1} + + gdb_test continue "Continuing\\..*Breakpoint \[0-9\]+, breakpt \\(\\) at .*$srcfile:\[0-9\]+.*" \ + "delete #1, stop at #2" +} + +# Test 2: Create two breakpoints at BREAKPT. Delete #2 and expect to stop +# at #1. +with_test_prefix "del_2_stop_1" { + test_setup 2 + + gdb_test_no_output {delete $bp_num_2} + + gdb_test continue "Continuing\\..*Breakpoint \[0-9\]+, breakpt \\(\\) at .*$srcfile:\[0-9\]+.*" \ + "delete #2, stop at #1" +} + +# Test 3: Create three breakpoints at BREAKPT. Disable #1, delete #2, +# expect to stop at #3. +with_test_prefix "dis_1_del_2_stop_3" { + test_setup 3 + + gdb_test_no_output {disable $bp_num_1} + + gdb_test "step" ".*" + + gdb_test_no_output {delete $bp_num_2} + + gdb_test continue "Continuing\\..*Breakpoint \[0-9\]+, breakpt \\(\\) at .*$srcfile:\[0-9\]+.*" \ + "disable #1, delete #2, stop at #3" +} + +# Test 4: Create three breakpoints at BREAKPT. Disable #2, delete #1, +# expect to stop at #3. +with_test_prefix "dis_2_del_1_stop_3" { + test_setup 3 + + gdb_test_no_output {disable $bp_num_2} + + gdb_test "step" ".*" + + gdb_test_no_output {delete $bp_num_1} + + gdb_test continue "Continuing\\..*Breakpoint \[0-9\]+, breakpt \\(\\) at .*$srcfile:\[0-9\]+.*" \ + "disable #2, delete #1, stop at #3" +} + +# Test 5: Create three breakpoints at BREAKPT. Disable #1, delete #3, +# expect to stop at #2. +with_test_prefix "dis_1_del_3_stop_1" { + test_setup 3 + + gdb_test_no_output {disable $bp_num_1} + + gdb_test "step" ".*" + + gdb_test_no_output {delete $bp_num_3} + + gdb_test continue "Continuing\\..*Breakpoint \[0-9\]+, breakpt \\(\\) at .*$srcfile:\[0-9\]+.*" \ + "disable #1, delete #3, stop at #2" +} + +# Test 6: Create three breakpoints at BREAKPT. Disable #3, delete #1, +# expect to stop at #2 +with_test_prefix "dis_3_del_1_stop_2" { + test_setup 3 + + gdb_test_no_output {disable $bp_num_3} + + gdb_test "step" ".*" + + gdb_test_no_output {delete $bp_num_1} + + gdb_test continue "Continuing\\..*Breakpoint \[0-9\]+, breakpt \\(\\) at .*$srcfile:\[0-9\]+.*" \ + "disable #3, delete #1, stop at #2" +} + +# Test 7: Create three breakpoints at BREAKPT. Disable #2, delete #3, +# expect to stop at #1. +with_test_prefix "dis_2_del_3_stop_1" { + test_setup 3 + + gdb_test_no_output {disable $bp_num_2} + + gdb_test "step" ".*" + + gdb_test_no_output {delete $bp_num_3} + + gdb_test continue "Continuing\\..*Breakpoint \[0-9\]+, breakpt \\(\\) at .*$srcfile:\[0-9\]+.*" \ + "disable #2, delete #3, stop at #1" +} + +# Test 8: Create three breakpoints at BREAKPT. Disable #3, delete #2, +# expect to stop at #1. +with_test_prefix "dis_3_del_2_stop_1" { + test_setup 3 + + gdb_test_no_output {disable $bp_num_3} + + gdb_test "step" ".*" + + gdb_test_no_output {delete $bp_num_2} + + gdb_test continue "Continuing\\..*Breakpoint \[0-9\]+, breakpt \\(\\) at .*$srcfile:\[0-9\]+.*" \ + "disable #3, delete #2, stop at #1" +}