Re: [PATCH] fix PR-15501

[Pedro Alves <palves@redhat.com>]

On 08/15/2013 11:32 AM, Muhammad Waqas wrote:
> 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   <MULTIPLE>
>>> >> 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  <mwaqas@codesourcery.com>
>>> >>
>>> >> 	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  <mwaqas@codesourccery.com>
>>> >>
>>> >> 	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   <MULTIPLE>
>>> >> 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  <mwaqas@codesourcery.com>
> 
> 	PR gdb/15501
> 	* breakpoint.c (enable_command, disable_command): Iterate over
> 	all specified breakpoint locations.
> 
> testsuite/Changlog
> 
> 2013-07-12  Muhammad Waqas  <mwaqas@codesourccery.com>
> 
> 	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.

Line too long.

> +#
> +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

# This proc will work correctly If args will be according to below explaned values

- "will work correctly" is unfortunately not a useful
indication of what the function actually does.

- "If" is uppercased for no reason.
- Typo "explaned".
- Missing period.

> +#
> +# If "what" = "disable" then
> +# "what_res" = "n"
> +# "p1" = "pass"
> +# "p2" = "fail".
> +#
> +# If "what" = "enable" then
> +# "what_res" = "y"
> +# "p1" = "fail"
> +# "p2" = "pass".
> +

Should e.g., say that WHAT is a command, etc.

> +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"

Should be "${what}d" here too, right?

Please add some comments in the body of the function,
with a minimal explanation of what the multiple testing steps
are doing.  It'll make it much easier to follow.  As is,
it's hard to grok.  Things like, "Now disable foo.  bar
should remain enabled.", etc.

> +
> +    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"

Suggest:

   set test1 "${what}d \$b4 and \$b3.1, \$b3 remains enabled"

Actually, shouldn't "enabled" here be the opposite of "${what}d" ?

Here's what we now get:

 PASS: gdb.base/ena-dis-br.exp: disabled $b4 and $b3.1,remain enabled $b3
 ...
 PASS: gdb.base/ena-dis-br.exp: enabled $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

This patch adds a series of trailing whitespace.  Please fix
that up.

-- 
Pedro Alves