* [PATCH] Failure to stop at duplicate breakpoints @ 2012-09-20 15:10 Andrew Burgess 2012-09-20 18:40 ` Sergio Durigan Junior 0 siblings, 1 reply; 7+ messages in thread From: Andrew Burgess @ 2012-09-20 15:10 UTC (permalink / raw) To: gdb-patches When two breakpoints are created at the same location, one of them is marked as a duplicate. When gdb inserts the breakpoints we only really insert one breakpoint at any unique location. By creating duplicate breakpoints and deleting or disabling them in the right order it is possible to get into a state where gdb has a single breakpoint with a single location, but that location is marked duplicate and so is never inserted, with the result we don't stop at the breakpoint. Patch and test included below. OK to apply? Thanks Andrew gdb/ChangeLog 2012-09-20 Andrew Burgess <aburgess@broadcom.com> * 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-20 Andrew Burgess <aburgess@broadcom.com> * 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..50145b4 --- /dev/null +++ b/gdb/testsuite/gdb.base/duplicate-bp.c @@ -0,0 +1,23 @@ +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..021aa30 --- /dev/null +++ b/gdb/testsuite/gdb.base/duplicate-bp.exp @@ -0,0 +1,137 @@ +# 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 <http://www.gnu.org/licenses/>. + +if { [prepare_for_testing duplicate-bp.exp "duplicate-bp" {duplicate-bp.c} {debug nowarnings}] } { + return -1 +} +set srcfile "duplicate-bp.c" + +# Setup for the test, create COUNT breakpoints at the function BREAKPT. +proc test_setup { count } { + upvar srcfile 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\]+.*" + + return 1 +} + + +# Test 1: Create two breakpoints at BREAKPT. Delete #1 and expect to stop +# at #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. +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. +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. +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 #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 #1" + +# Test 6: Create three breakpoints at BREAKPT. Disable #3, delete #1, +# expect to stop at #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. +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 6: Create three breakpoints at BREAKPT. Disable #3, delete #2, +# expect to stop at #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" ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Failure to stop at duplicate breakpoints 2012-09-20 15:10 [PATCH] Failure to stop at duplicate breakpoints Andrew Burgess @ 2012-09-20 18:40 ` Sergio Durigan Junior 2012-09-20 23:21 ` Andrew Burgess 0 siblings, 1 reply; 7+ messages in thread From: Sergio Durigan Junior @ 2012-09-20 18:40 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches On Thursday, September 20 2012, Andrew Burgess wrote: > When two breakpoints are created at the same location, one of them is > marked as a duplicate. When gdb inserts the breakpoints we only > really insert one breakpoint at any unique location. > > By creating duplicate breakpoints and deleting or disabling them in > the right order it is possible to get into a state where gdb has a > single breakpoint with a single location, but that location is marked > duplicate and so is never inserted, with the result we don't stop at > the breakpoint. > > Patch and test included below. Thanks for the patches, but your mail client messed with them, probably. They are malformed in several places. -- Sergio ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Failure to stop at duplicate breakpoints 2012-09-20 18:40 ` Sergio Durigan Junior @ 2012-09-20 23:21 ` Andrew Burgess 2012-09-25 16:16 ` Pedro Alves 0 siblings, 1 reply; 7+ messages in thread From: Andrew Burgess @ 2012-09-20 23:21 UTC (permalink / raw) To: gdb-patches; +Cc: Sergio Durigan Junior On 20/09/2012 7:40 PM, Sergio Durigan Junior wrote: > On Thursday, September 20 2012, Andrew Burgess wrote: > >> When two breakpoints are created at the same location, one of them is >> marked as a duplicate. When gdb inserts the breakpoints we only >> really insert one breakpoint at any unique location. >> >> By creating duplicate breakpoints and deleting or disabling them in >> the right order it is possible to get into a state where gdb has a >> single breakpoint with a single location, but that location is marked >> duplicate and so is never inserted, with the result we don't stop at >> the breakpoint. >> >> Patch and test included below. > > Thanks for the patches, but your mail client messed with them, probably. > They are malformed in several places. Sorry for the badly formatted patch, I believe I've fixed the issue, and the version below should be more readable. OK to apply? Thanks, Andrew gdb/ChangeLog 2012-09-20 Andrew Burgess <aburgess@broadcom.com> * 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-20 Andrew Burgess <aburgess@broadcom.com> * 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..50145b4 --- /dev/null +++ b/gdb/testsuite/gdb.base/duplicate-bp.c @@ -0,0 +1,23 @@ +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..021aa30 --- /dev/null +++ b/gdb/testsuite/gdb.base/duplicate-bp.exp @@ -0,0 +1,137 @@ +# 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 <http://www.gnu.org/licenses/>. + +if { [prepare_for_testing duplicate-bp.exp "duplicate-bp" {duplicate-bp.c} {debug nowarnings}] } { + return -1 +} +set srcfile "duplicate-bp.c" + +# Setup for the test, create COUNT breakpoints at the function BREAKPT. +proc test_setup { count } { + upvar srcfile 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\]+.*" + + return 1 +} + + +# Test 1: Create two breakpoints at BREAKPT. Delete #1 and expect to stop +# at #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. +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. +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. +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 #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 #1" + +# Test 6: Create three breakpoints at BREAKPT. Disable #3, delete #1, +# expect to stop at #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. +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 6: Create three breakpoints at BREAKPT. Disable #3, delete #2, +# expect to stop at #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" ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Failure to stop at duplicate breakpoints 2012-09-20 23:21 ` Andrew Burgess @ 2012-09-25 16:16 ` Pedro Alves 2012-09-25 21:48 ` Andrew Burgess 0 siblings, 1 reply; 7+ messages in thread From: Pedro Alves @ 2012-09-25 16:16 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches, Sergio Durigan Junior Hi Andrew, On 09/21/2012 12:20 AM, Andrew Burgess wrote: > 2012-09-20 Andrew Burgess <aburgess@broadcom.com> > > * breakpoint.c (update_global_location_list): Ignore previous > duplicate status of a breakpoint when starting a new scan for > duplicate breakpoints. This one's okay. > --- /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. > @@ -0,0 +1,23 @@ > +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..021aa30 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/duplicate-bp.exp > @@ -0,0 +1,137 @@ > +# 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 <http://www.gnu.org/licenses/>. > + > +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}] } { > + return -1 > +} > +set srcfile "duplicate-bp.c" This won't be needed then. While at it, is "nowarnings" really necessary? > + > +# 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". > + > + 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\]+.*" > + > + return 1 > +} > + > + > +# Test 1: Create two breakpoints at BREAKPT. Delete #1 and expect to stop > +# at #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. > +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. > +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. > +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 #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 #1" > + > +# Test 6: Create three breakpoints at BREAKPT. Disable #3, delete #1, > +# expect to stop at #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. > +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 6: Create three breakpoints at BREAKPT. Disable #3, delete #2, > +# expect to stop at #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" > > There are many duplicate test output messages in the .sum file: $ cat testsuite/gdb.sum | grep PASS | sort | uniq -c | sort -n 1 PASS: gdb.base/duplicate-bp.exp: delete #1, stop at #2 1 PASS: gdb.base/duplicate-bp.exp: delete #2, stop at #1 1 PASS: gdb.base/duplicate-bp.exp: disable #1, delete #2, stop at #3 1 PASS: gdb.base/duplicate-bp.exp: disable #1, delete #3, stop at #1 1 PASS: gdb.base/duplicate-bp.exp: disable #2, delete #1, stop at #3 1 PASS: gdb.base/duplicate-bp.exp: disable #2, delete #3, stop at #1 1 PASS: gdb.base/duplicate-bp.exp: disable #3, delete #1, stop at #2 1 PASS: gdb.base/duplicate-bp.exp: disable #3, delete #2, stop at #1 2 PASS: gdb.base/duplicate-bp.exp: delete $bp_num_3 2 PASS: gdb.base/duplicate-bp.exp: disable $bp_num_1 2 PASS: gdb.base/duplicate-bp.exp: disable $bp_num_2 2 PASS: gdb.base/duplicate-bp.exp: disable $bp_num_3 3 PASS: gdb.base/duplicate-bp.exp: delete $bp_num_1 3 PASS: gdb.base/duplicate-bp.exp: delete $bp_num_2 6 PASS: gdb.base/duplicate-bp.exp: set $bp_num_3 = $bpnum 8 PASS: gdb.base/duplicate-bp.exp: set $bp_num_1 = $bpnum 8 PASS: gdb.base/duplicate-bp.exp: set $bp_num_2 = $bpnum 14 PASS: gdb.base/duplicate-bp.exp: step Could you please make them all unique? http://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique You can use with_test_prefix to group tests, for example. Also, please get rid of the trailing whitespace the patch is adding: $ git am ~/unduplicate.mbox Applying: Failure to stop at duplicate breakpoints /home/pedro/gdb/mygit/src/.git/rebase-apply/patch:48: trailing whitespace. /home/pedro/gdb/mygit/src/.git/rebase-apply/patch:83: trailing whitespace. if ![runto_main] then { /home/pedro/gdb/mygit/src/.git/rebase-apply/patch:84: trailing whitespace. fail "can't run to main" /home/pedro/gdb/mygit/src/.git/rebase-apply/patch:94: trailing whitespace. /home/pedro/gdb/mygit/src/.git/rebase-apply/patch:110: trailing whitespace. test_setup 2 warning: 5 lines add whitespace errors. Thanks! -- Pedro Alves ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Failure to stop at duplicate breakpoints 2012-09-25 16:16 ` Pedro Alves @ 2012-09-25 21:48 ` Andrew Burgess 2012-09-26 15:06 ` Pedro Alves 0 siblings, 1 reply; 7+ messages in thread From: Andrew Burgess @ 2012-09-25 21:48 UTC (permalink / raw) To: gdb-patches 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 <aburgess@broadcom.com> * 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 <aburgess@broadcom.com> * 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 <http://www.gnu.org/licenses/>. */ + +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 <http://www.gnu.org/licenses/>. + +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" +} ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Failure to stop at duplicate breakpoints 2012-09-25 21:48 ` Andrew Burgess @ 2012-09-26 15:06 ` Pedro Alves 2012-09-26 16:37 ` Andrew Burgess 0 siblings, 1 reply; 7+ messages in thread From: Pedro Alves @ 2012-09-26 15:06 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches On 09/25/2012 10:48 PM, Andrew Burgess wrote: > Pedro, > > Thanks for taking the time to review my patch. I've addressed the issues you raised, and Great, thanks. Just one little more I missed before: > +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 global binfile and > + clean_restart duplicate-bp instead: clean_restart $binfile Okay with that change. Thanks again. -- Pedro Alves ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Failure to stop at duplicate breakpoints 2012-09-26 15:06 ` Pedro Alves @ 2012-09-26 16:37 ` Andrew Burgess 0 siblings, 0 replies; 7+ messages in thread From: Andrew Burgess @ 2012-09-26 16:37 UTC (permalink / raw) To: gdb-patches; +Cc: Pedro Alves On 26/09/2012 4:06 PM, Pedro Alves wrote: > On 09/25/2012 10:48 PM, Andrew Burgess wrote: >> Pedro, >> >> Thanks for taking the time to review my patch. I've addressed the issues you raised, and > > Great, thanks. Just one little more I missed before: > >> +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 > > global binfile > > and > >> + clean_restart duplicate-bp > > instead: > > clean_restart $binfile > > Okay with that change. Thanks again. Have committed with that small fix. Thanks for your review. Andrew ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-09-26 16:37 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-09-20 15:10 [PATCH] Failure to stop at duplicate breakpoints Andrew Burgess 2012-09-20 18:40 ` Sergio Durigan Junior 2012-09-20 23:21 ` Andrew Burgess 2012-09-25 16:16 ` Pedro Alves 2012-09-25 21:48 ` Andrew Burgess 2012-09-26 15:06 ` Pedro Alves 2012-09-26 16:37 ` Andrew Burgess
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox