Re: [PATCH] fix PR-15501

[Muhammad Waqas <mwaqas@codesourcery.com>]

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