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

["Philippe Waroquiers" <philippe.waroquiers@skynet.be>]

[-- Attachment #1: Type: text/plain, Size: 167 bytes --]


> This is okay with the minor issues pointed out below
> addressed.  Thanks for fixing this!
After fixing the issues, committed the attached patch.

Thanks

Philippe

[-- Attachment #2: committed_fix_enabling_dupl.txt --]
[-- Type: text/plain, Size: 7500 bytes --]

Index: gdb/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.13248
diff -c -p -r1.13248 ChangeLog
*** gdb/ChangeLog	3 Aug 2011 15:17:08 -0000	1.13248
--- gdb/ChangeLog	3 Aug 2011 21:07:34 -0000
***************
*** 1,3 ****
--- 1,12 ----
+ 2011-08-03  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
+ 
+ 	* breakpoint.c (update_global_location_list): Ensure
+ 	invariant 'first loc marked not duplicated and inserted,
+ 	following locs marked duplicated/not inserted' is respected
+ 	for multiple locations at the same address.
+ 	(unduplicated_should_be_inserted) New function.
+ 	(swap_insertion) New function.
+ 
  2011-08-03  Jan Kratochvil  <jan.kratochvil@redhat.com>
  
  	* stack.c (print_frame_arguments_choices): Comment typo fix.
Index: gdb/breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.606
diff -c -p -r1.606 breakpoint.c
*** gdb/breakpoint.c	1 Aug 2011 18:45:49 -0000	1.606
--- gdb/breakpoint.c	3 Aug 2011 21:07:36 -0000
*************** should_be_inserted (struct bp_location *
*** 1571,1576 ****
--- 1571,1591 ----
    return 1;
  }
  
+ /* Same as should_be_inserted but does the check assuming
+    that the location is not duplicated.  */
+ 
+ static int
+ unduplicated_should_be_inserted (struct bp_location *bl)
+ {
+   int result;
+   const int save_duplicate = bl->duplicate;
+ 
+   bl->duplicate = 0;
+   result = should_be_inserted (bl);
+   bl->duplicate = save_duplicate;
+   return result;
+ }
+ 
  /* Insert a low-level "breakpoint" of some type.  BL is the breakpoint
     location.  Any error messages are printed to TMP_ERROR_STREAM; and
     DISABLED_BREAKS, and HW_BREAKPOINT_ERROR are used to report problems.
*************** bp_location_target_extensions_update (vo
*** 10241,10246 ****
--- 10256,10278 ----
      }
  }
  
+ /* Swap the insertion/duplication state between two locations.  */
+ 
+ static void
+ swap_insertion (struct bp_location *left, struct bp_location *right)
+ {
+   const int left_inserted = left->inserted;
+   const int left_duplicate = left->duplicate;
+   const struct bp_target_info left_target_info = left->target_info;
+ 
+   left->inserted = right->inserted;
+   left->duplicate = right->duplicate;
+   left->target_info = right->target_info;
+   right->inserted = left_inserted;
+   right->duplicate = left_duplicate;
+   right->target_info = left_target_info;
+ }
+ 
  /* If SHOULD_INSERT is false, do not insert any breakpoint locations
     into the inferior, only remove already-inserted locations that no
     longer should be inserted.  Functions that delete a breakpoint or
*************** update_global_location_list (int should_
*** 10377,10387 ****
  
  		      if (breakpoint_locations_match (loc2, old_loc))
  			{
- 			  /* For the sake of should_be_inserted.
- 			     Duplicates check below will fix up this
- 			     later.  */
- 			  loc2->duplicate = 0;
- 
  			  /* Read watchpoint locations are switched to
  			     access watchpoints, if the former are not
  			     supported, but the latter are.  */
--- 10409,10414 ----
*************** update_global_location_list (int should_
*** 10391,10400 ****
  			      loc2->watchpoint_type = old_loc->watchpoint_type;
  			    }
  
! 			  if (loc2 != old_loc && should_be_inserted (loc2))
  			    {
! 			      loc2->inserted = 1;
! 			      loc2->target_info = old_loc->target_info;
  			      keep_in_target = 1;
  			      break;
  			    }
--- 10418,10430 ----
  			      loc2->watchpoint_type = old_loc->watchpoint_type;
  			    }
  
! 			  /* 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);
  			      keep_in_target = 1;
  			      break;
  			    }
*************** update_global_location_list (int should_
*** 10541,10546 ****
--- 10571,10582 ----
  	  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;
  
        if ((*loc_first_p)->owner->enable_state == bp_permanent && loc->inserted
Index: gdb/testsuite/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/ChangeLog,v
retrieving revision 1.2820
diff -c -p -r1.2820 ChangeLog
*** gdb/testsuite/ChangeLog	2 Aug 2011 20:59:44 -0000	1.2820
--- gdb/testsuite/ChangeLog	3 Aug 2011 21:07:40 -0000
***************
*** 1,3 ****
--- 1,8 ----
+ 2011-08-03  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
+ 
+ 	* gdb.base/break-always.exp: Complete the test
+ 	with duplicated breakpoints and enabling/disabling them.
+ 
  2011-08-02  Tom Tromey  <tromey@redhat.com>
  
  	PR gdb/11289:
Index: gdb/testsuite/gdb.base/break-always.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/break-always.exp,v
retrieving revision 1.7
diff -c -p -r1.7 break-always.exp
*** gdb/testsuite/gdb.base/break-always.exp	1 Jan 2011 15:33:40 -0000	1.7
--- gdb/testsuite/gdb.base/break-always.exp	3 Aug 2011 21:07:40 -0000
***************
*** 14,19 ****
--- 14,21 ----
  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
  
  # Test that 'set breakpoint always-inserted 1' is not a brick
+ # Also verifies that breakpoint enabling/disabling works properly
+ # with duplicated breakpoints.
  
  if { [prepare_for_testing break-always.exp break-always break-always.c] } {
      return -1
*************** gdb_test "show breakpoint always-inserte
*** 29,34 ****
--- 31,54 ----
  runto foo
  
  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.*" "initial check breakpoint state"
+ gdb_test_no_output "disable" "initial disable all breakpoints"
+ gdb_test_no_output "enable" "initial enable all breakpoints"
+ gdb_test_no_output "disable" "re-disable all breakpoints"
+ gdb_test_no_output "enable 3" "enable 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.*" "before re-enable check breakpoint state"
+ gdb_test_no_output "enable" "re-enable all breakpoints"
  gdb_continue_to_breakpoint "bar" ".*break-always.c:$bar_location.*"