From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13469 invoked by alias); 15 Aug 2013 10:32:20 -0000 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 Received: (qmail 13449 invoked by uid 89); 15 Aug 2013 10:32:18 -0000 X-Spam-SWARE-Status: No, score=-4.1 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL autolearn=ham version=3.3.2 Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Thu, 15 Aug 2013 10:32:17 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1V9uqk-0004Hv-Ut from Muhammad_Waqas@mentor.com ; Thu, 15 Aug 2013 03:32:15 -0700 Received: from SVR-ORW-FEM-02.mgc.mentorg.com ([147.34.96.206]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Thu, 15 Aug 2013 03:32:15 -0700 Received: from [137.202.157.111] (147.34.91.1) by SVR-ORW-FEM-02.mgc.mentorg.com (147.34.96.168) with Microsoft SMTP Server (TLS) id 14.2.247.3; Thu, 15 Aug 2013 03:32:13 -0700 Message-ID: <520CAE24.7050301@codesourcery.com> Date: Thu, 15 Aug 2013 10:32:00 -0000 From: Muhammad Waqas User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130804 Thunderbird/17.0.8 MIME-Version: 1.0 To: Pedro Alves CC: Subject: Re: [PATCH] fix PR-15501 References: <520A0453.4070309@codesourcery.com> <520A6EEB.8010808@redhat.com> In-Reply-To: <520A6EEB.8010808@redhat.com> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2013-08/txt/msg00396.txt.bz2 On 08/13/2013 10:37 PM, Pedro Alves wrote: > On 08/13/2013 11:02 AM, Muhammad Waqas wrote: >> GDB enable/disable command does not work correctly as it should be. >> http://sourceware.org/bugzilla/show_bug.cgi?id=15501 > Thanks! > > Note it'd be good to assign yourself the PR once > you start working on it, to avoid effort duplication. I > had just suggested this bug to someone else last week; > luckily he hadn't started working on it. :-) > ok next time I will be careful. >> Addition to Pedro examples. >> if we execute following commands these will be executed >> without an error. >> (gdb) info b >> Num Type Disp Enb Address What >> 1 breakpoint keep y 0x00000000004004b8 in main at 13929.c:13 >> 2 breakpoint keep y 0x00000000004004b8 in main at 13929.c:13 >> (gdb) disable 1 fooo.1 >> (gdb) info break >> Num Type Disp Enb Address What >> 1 breakpoint keep y >> 1.1 n 0x00000000004004b8 in main at 13929.c:13 >> 2 breakpoint keep y 0x00000000004004b8 in main at 13929.c:13 >> >> It should disable breakpoint 1 and error on fooo but what gdb did, it disable 1.1 >> surprisingly. >> >> I am prposing patch for this bug. >> >> Workaround: >> Pars args and handle them one by one if it contain period or not and do what it >> requires(disable/enable breakpoint or location). >> >> gdb\Changlog >> >> 2013-08-13 Muhammad Waqas >> >> PR gdb/15501 >> * breakpoint.c (enable_command): Handle multiple arguments properly. >> (disable_command): Handle multiple arguments properly. > "Properly" is subjective, and may change over time. ;-) Say what changed, > like so: > > * breakpoint.c (enable_command, disable_command): Iterate over > all specified breakpoint locations. > >> testsuite\Changlog > I can't resist saying that backslashes for dir > separators look very alien to me. :-) > >> 2013-07-13 Muhammad Waqas >> >> PR gdb/15501 >> * gdb.base/ena-dis-br.exp: Add test to verify >> enable\disable commands work correctly with arguments. > Here too. Please use forward slashes. Say: > > * gdb.base/ena-dis-br.exp: Add test to verify > enable/disable commands work correctly with > multiple arguments that include multiple locations. > > >> +set b1 0 >> +set b2 0 >> + >> +gdb_test_multiple "break main" "bp 1" { >> + -re "(Breakpoint )(\[0-9\]+)( at.* file .*$srcfile, line.*)($gdb_prompt $)" { >> + set b1 $expect_out(2,string) >> + pass "breakpoint main 1" >> + } >> +} >> + >> +gdb_test_multiple "break main" "bp 2" { >> + -re "(Breakpoint )(\[0-9\]+)( at.* file .*$srcfile, line.*)($gdb_prompt $)" { >> + set b2 $expect_out(2,string) >> + pass "breakpoint main 2" >> + } >> +} > Doesn't break_at work for this? It's defined at the top of the file. > >> + >> +gdb_test_no_output "disable $b1.1 $b2.1" "disable command" > Write: > > gdb_test_no_output "disable $b1.1 $b2.1" "disable \$b1.1 \$b2.1" > >> +gdb_test "info break" \ >> + "(${b1}.1)(\[^\n\r\]*)( n.*)(${b2}.1)(\[^\n\r\]*)( n.*)" \ >> + "disable ${b1}.1 and ${b2}.1" > I think you meant "disabled". Also, this puts the real breakpoint > number in gdb.sum. It's usually better to avoid that, as something > may cause the breakpoint numbers to change, and we'd rather > gdb.sum output was stable(-ish). So, write: > > gdb_test "info break" \ > "(${b1}.1)(\[^\n\r\]*)( n.*)(${b2}.1)(\[^\n\r\]*)( n.*)" \ > "disabled \$b1.1 and \$b2.1" > >> + >> +gdb_test "disable $b1 fooo.1" \ >> + "Bad breakpoint number 'fooo'" \ >> + "handle multiple args" > "handle multiple args" looks like a stale string from some > earlier revision... The other test above was also > about multiple args. Just do: > > gdb_test "disable $b1 fooo.1" \ > "Bad breakpoint number 'fooo'" \ > "disable \$b1 fooo.1" > > > IMO, the test is incomplete. > > - The "enable" command should be tested as well. > - It'd be good to test a mix of breakpoints > and breakpoint locations. E.g., "disable $b3.1 $b4" > - The "info break" tests should ensure that the breakpoints > that were _not_ supposed to be disabled remain enabled (and > vice versa for counterpart "enable" tests. (this suggests > moving the testing code to a procedure that repeats the > same set of tests for either enable or disable). > - This part in the PR: > ok >> In fact, everything after the first location is ignored: >> >> (gdb) disable 2.1 foofoobar >> (gdb) info breakpoints >> Num Type Disp Enb Address What >> 2 breakpoint keep y >> 2.1 n 0x00000000004004cf in main at main.c:5 >> 3 breakpoint keep y 0x00000000004004cf in main at main.c:5 >> (gdb) >> >> That should warn, just like: >> >> (gdb) disable 2 foofoobar >> warning: bad breakpoint number at or near 'foofoobar' > ... is not being tested. I think it should. > > Would you like to extend the test a bit and resubmit? > > Thanks, Thanks for reviewing my patch. Here are the things that you mention to correct. gdb/Changlog 2013-08-12 Muhammad Waqas PR gdb/15501 * breakpoint.c (enable_command, disable_command): Iterate over all specified breakpoint locations. testsuite/Changlog 2013-07-12 Muhammad Waqas PR gdb/15501 * gdb.base/ena-dis-br.exp: Add test to verify enable/disable commands work correctly with multiple arguments that include multiple locations. extended testcase Index: ena-dis-br.exp =================================================================== RCS file: /cvs/src/src/gdb/testsuite/gdb.base/ena-dis-br.exp,v retrieving revision 1.22 diff -u -p -r1.22 ena-dis-br.exp --- ena-dis-br.exp 27 Jun 2013 18:50:30 -0000 1.22 +++ ena-dis-br.exp 15 Aug 2013 09:12:04 -0000 @@ -301,5 +301,88 @@ gdb_test_multiple "continue 2" "$test" { } } +# Verify that GDB correctly handles the "enable/disable" command with arguments that +# include multiple locations. +# +if ![runto_main] then { fail "enable/disable break tests suppressed" } + +set b1 0 +set b2 0 +set b3 0 +set b4 0 + +set b1 [break_at main ""] +set b2 [break_at main ""] +set b3 [break_at main ""] +set b4 [break_at main ""] + +# This proc will work correctly If args will be according to below explaned values +# +# If "what" = "disable" then +# "what_res" = "n" +# "p1" = "pass" +# "p2" = "fail". +# +# If "what" = "enable" then +# "what_res" = "y" +# "p1" = "fail" +# "p2" = "pass". + +proc test_ena_dis_br { what what_res p1 p2 } { + global b1 + global b2 + global b3 + global b4 + global gdb_prompt + + gdb_test_no_output "$what $b1.1 $b2.1" "$what \$b1.1 \$b2.1" + set test1 "$what \$b1.1 and \$b2.1" + + gdb_test_multiple "info break" "$test1" { + -re "(${b1}.1)(\[^\n\r\]*)( n.*)(${b2}.1)(\[^\n\r\]*)( n.*)$gdb_prompt $" { + $p1 "$test1" + } + -re ".*$gdb_prompt $" { + $p2 "$test1" + } + } + + gdb_test "$what $b1 fooo.1" \ + "Bad breakpoint number 'fooo'" \ + "$what \$b1 fooo.1" + + gdb_test "info break" \ + "(${b1})(\[^\n\r]*)( $what_res.*)" \ + "${what}d \$b1" + + gdb_test_no_output "$what $b4 $b3.1" "$what \$b4 \$b3.1" + set test1 "${what}d \$b4 and \$b3.1,remain enabled \$b3" + + gdb_test_multiple "info break" "$test1" { + -re "(${b3})(\[^\n\r]*)( y.*)(${b3}.1)(\[^\n\r\]*)( n.*)(${b4})(\[^\n\r\]*)( $what_res.*)$gdb_prompt $" { + $p1 "$test1" + } + -re "(${b3})(\[^\n\r]*)( y.*)(${b4})(\[^\n\r\]*)( $what_res.*)$gdb_prompt $" { + $p2 "$test1" + } + } + + gdb_test "$what $b4.1 fooobaar" \ + "warning: bad breakpoint number at or near 'fooobaar'" \ + "$what \$b4.1 fooobar" + set test1 "${what}d \$b4.1" + + gdb_test_multiple "info break" "$test1" { + -re "(${b4}.1)(\[^\n\r\]*)( n.*)$gdb_prompt $" { + $p1 "$test1" + } + -re ".*$gdb_prompt $" { + $p2 "$test1" + } + } +} + +test_ena_dis_br "disable" "n" "pass" "fail" +test_ena_dis_br "enable" "y" "fail" "pass" + gdb_exit return 0