Re: patch: fix bug with enabling/disabling breakpoints for duplicated locations

[Pedro Alves <pedro at codesourcery dot com>]

Hello Philippe,

On Sunday 31 July 2011 02:03:32, Philippe Waroquiers wrote:
> 
> When breakpoint always-inserted is on, and there are duplicated
> locations, enabling/disabling breakpoints is giving errors
> (at least with gdbserver, because multiple z or Z packets are being sent
> for the same address).
> The logic to handle duplicated locations in breakpoints.c
> is that in a list of duplicated location that should be inserted:
>     - the first one is not marked as duplicated and is inserted
>     - following ones are marked as duplicated and are not inserted.
> This invariant was not properly maintained by update_global_location_list.
> 
> Tested on f12/x86 in native and gdbserver mode.
> Tested on debian5/amd64 in native and gdbserver mode.
> No regression identified.

I've regtested with always inserted mode always on,
on amd64 gdbserver, and saw no regressions either.

This is okay with the minor issues pointed out below
addressed.  Thanks for fixing this!

> gdb/testsuite/ChangeLog
> 
> 	* gdb/testsuite/gdb.base/break-always.exp: Complete the test
> 	with duplicated breakpoints and enabling/disabling them.

Write that as:

> 	* gdb.base/break-always.exp: Complete the test


>   
> + /* Same as should_be_inserted but does the check assuming
> +    that the location is not duplicated.  */
> + static int

Empty line between comment and function.

> + unduplicated_should_be_inserted  (struct bp_location *bl)

Spurious extra space before open parens.

> + {
> +   int result;
> +   const int save_duplicate = bl->duplicate;
> +   bl->duplicate = 0;
> +   result = should_be_inserted (bl);
> +   bl->duplicate = save_duplicate;
> +   return result;
> + }

Empty line between declarations and code, please.  Here and
everywhere.

>   
> !                           /* loc2 is a duplicated location. We need to check
> !                              if it should be inserted in case it will be
> !                              unduplicated.  */
> ! 			  if (loc2 != old_loc
> !                               && unduplicated_should_be_inserted (loc2))
>   			    {
> ! 			      swap_insertion (old_loc, loc2);

Watch out tabs vs spaces.

>   			      keep_in_target = 1;
>   			      break;
>   			    }
> *************** update_global_location_list (int should_
> *** 10538,10543 ****
> --- 10565,10576 ----
>   	  continue;
>   	}
>   
> + 
> +       /* This and the above ensure the invariant that the first location
> +          is not duplicated, and is the inserted one.
> +          All following are marked as duplicated, and are not inserted.  */
> +       if (loc->inserted)
> +         swap_insertion(loc, *loc_first_p);
>         loc->duplicate = 1;

Missing space before open parens.


>   gdb_test "break bar" "Breakpoint 2.*" "set breakpoint on bar"
> + gdb_test "break bar" "Note: breakpoint 2 also set.*Breakpoint 3.*" "set 2nd breakpoint on bar"
> + gdb_test "break bar" "Note: breakpoints 2 and 3 also set.*Breakpoint 4.*" "set 3rd breakpoint on bar"
> + gdb_test "break bar" "Note: breakpoints 2, 3 and 4 also set.*Breakpoint 5.*" "set 4th breakpoint on bar"
> + gdb_test "info breakpoints" "keep y.*keep y.*keep y.*keep y.*keep y.*" "check breakpoint state"
> + gdb_test_no_output "disable" "disable all breakpoints"
> + gdb_test_no_output "enable" "enable all breakpoints"
> + gdb_test_no_output "disable" "disable all breakpoints"
> + gdb_test_no_output "enable 3" "enable 3 3.A"
> + gdb_test_no_output "disable 3" "disable 3.B"
> + gdb_test_no_output "enable 3" "enable 3.C"
> + gdb_test_no_output "enable 2" "enable 2.D"
> + gdb_test_no_output "disable 2" "disable 2.E"
> + gdb_test_no_output "disable 3" "disable 3.F"
> + gdb_test_no_output "enable 3" "enable 3.G"
> + gdb_test_no_output "enable 2" "enable 2.H"
> + gdb_test_no_output "disable 2" "disable 2.I"
> + gdb_test "info breakpoints" "keep n.*keep n.*keep y.*keep n.*keep n.*" "check breakpoint state"
> + gdb_test_no_output "enable" "enable all breakpoints"
>   gdb_continue_to_breakpoint "bar" ".*break-always.c:$bar_location.*"

You were almost there, but please make it so that all
tests have unique output in gdb.sum:

 $ cat testsuite/gdb.sum  | grep "PASS: " | wc -l
 22
 $ cat testsuite/gdb.sum  | grep "PASS: " | sort -u | wc -l
 19

-- 
Pedro Alves