[PATCH] Failure to stop at duplicate breakpoints

[PATCH] Failure to stop at duplicate breakpoints

[Andrew Burgess]

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"

Re: [PATCH] Failure to stop at duplicate breakpoints

[Sergio Durigan Junior]

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

Re: [PATCH] Failure to stop at duplicate breakpoints

[Andrew Burgess]

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"

Re: [PATCH] Failure to stop at duplicate breakpoints

[Pedro Alves]

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

Re: [PATCH] Failure to stop at duplicate breakpoints

[Andrew Burgess]

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"
+}

Re: [PATCH] Failure to stop at duplicate breakpoints

[Pedro Alves]

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

Re: [PATCH] Failure to stop at duplicate breakpoints

[Andrew Burgess]

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