From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23468 invoked by alias); 3 Aug 2011 15:09:41 -0000 Received: (qmail 23451 invoked by uid 22791); 3 Aug 2011 15:09:40 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 03 Aug 2011 15:09:24 +0000 Received: (qmail 23676 invoked from network); 3 Aug 2011 15:09:23 -0000 Received: from unknown (HELO scottsdale.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 3 Aug 2011 15:09:23 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: patch: fix bug with enabling/disabling breakpoints for duplicated locations Date: Wed, 03 Aug 2011 15:09:00 -0000 User-Agent: KMail/1.13.6 (Linux/2.6.38-8-generic; KDE/4.6.2; x86_64; ; ) Cc: Philippe Waroquiers References: <1312074212.8735.4.camel@soleil> In-Reply-To: <1312074212.8735.4.camel@soleil> MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201108031609.19978.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-08/txt/msg00047.txt.bz2 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