Re: [PATCH] Failure to stop at duplicate breakpoints

[Pedro Alves <palves at redhat dot com>]

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