RE: [PATCH] fix PR-15501

["Waqas, Muhammad" <Muhammad_Waqas@mentor.com>]

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

Thanks. Now carefully remove all typo error.
________________________________________
From: Pedro Alves [palves@redhat.com]
Sent: Thursday, August 22, 2013 5:02 PM
To: Waqas, Muhammad
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH] fix PR-15501

On 08/22/2013 10:36 AM, Waqas, Muhammad wrote:

> +# This proc will be able to perform test for disable/enable
> +# commads on multiple locations and breakpoints,
> +# if args will be according to below explained values.
> +# Here arg are
> +# "what" is command (disable/enable),
> +# "what_res" is for breakpoints should be enabled or not,
> +# "p1/p2" are proc(pass/fail) but must be opposite.
> +#
> +# Here arg's values
> +# If "what" = "disable" then
> +# "what_res" = "n"
> +# "p1" = "pass"
> +# "p2" = "fail".
> +#
> +# If "what" = "enable" then
> +# "what_res" = "y"
> +# "p1" = "fail"
> +# "p2" = "pass".

Thanks.  I suggest this edit then:

# Perform tests for disable/enable commands on multiple
# locations and breakpoints.
#
# WHAT     - the command to test (disable/enable),
# WHAT_RES - whether breakpoints are expected to end
#            up enabled or disabled.
# P1/P2    - proc to call (pass/fail).  Must be
#            opposites.
#
# Furthermore, arguments must follow these rules:
#
# 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
> +
> +    # Now enable/disable $b.1 $b2.1.
> +    gdb_test_no_output "$what $b1.1 $b2.1" "$what \$b1.1 \$b2.1"
> +    set test1 "${what}d \$b1.1 and \$b2.1"
> +
> +    # Now $b1.1 and $b2.1 should be enabled/disabled
> +    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"
> +       }
> +    }
> +
> +    # Now enable/disbale $b1 fooo.1, it should give error on fooo.

Typo: disbale.

> +    # Now enable/disbale $b1 fooo.1, it should give error on fooo.
> +    gdb_test "$what $b1 fooo.1" \
> +       "Bad breakpoint number 'fooo'" \
> +       "$what \$b1 fooo.1"
> +
> +    # $b1 should be enabled/disabled.
> +    gdb_test "info break" \
> +       "(${b1})(\[^\n\r]*)( $what_res.*)" \
> +       "${what}d \$b1"
> +
> +    # Here "oppos" is commmand that should be opposite of "what".
> +    set oppos "enable"
> +    set oppos_res "y"
> +
> +    if { $what == "enable" } {
> +        set oppos "disable"
> +     set oppos_res "n"

Indentation is wrong here.  If we're doing this, then
one has to wonder why does the routine expect all
these arguments:

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

... instead of accepting only "what", and computing
the rest from that itself.

This is good to only receive "what" and computing
the rest from that itself. I fixed this.

> +

Spurious newline.

> +    }
> +
> +    gdb_test_no_output "$oppos $b3" "$oppos \$b3"
> +    gdb_test_no_output "$what $b4 $b3.1" "$what \$b4 \$b3.1"
> +    set test1 "${what}d \$b4 and \$b3.1,remain ${oppos}d \$b3"
> +
> +    # Now $b4 $b3.1 should be enabled/disabled and
> +    # $b3 should remain enabled/disabled

enabled/disabled ... enabled/disabled

That's confusing, and says practically nothing...  They're
all either enabled or disabled.  I suggest adjusting all
these comments to be written in terms of "enabled",
with the disabled state written on the right, in parens.
So that'd be:

    # Now $b4 $b3.1 should be enabled(disabled) and
    # $b3 should remain disabled(enabled).

So, when reading this, one now clearly understands
that $b3 should end up in the opposite state of$b4 $b3.1
(it that's what should happen).

fixed

> +    gdb_test_multiple "info break" "$test1" {
> +       -re "(${b3})(\[^\n\r]*)( $oppos_res.*)(${b3}.1)(\[^\n\r\]*)( n.*)(${b4})(\[^\n\r\]*)( $what_res.*)$gdb_prompt $" {
> +           $p1 "$test1"
> +       }
> +       -re "(${b3})(\[^\n\r]*)( $oppos_res.*)(${b4})(\[^\n\r\]*)( $what_res.*)$gdb_prompt $" {
> +           $p2 "$test1"
> +       }
> +    }
> +
> +    # Now enable/disable $b4.1 fooobaar and
> +    # it should give warning on fooobaar.
> +    gdb_test "$what $b4.1 fooobaar" \
> +       "warning: bad breakpoint number at or near 'fooobaar'" \
> +       "$what \$b4.1 fooobar"
> +    set test1 "${what}d \$b4.1"
> +
> +    # $b4.1 should be enabled/disbaled

Typo.  Please remember to always self review for typos.  I
think I'm noticing them at all iterations.

I will be careful now on.

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

Thanks,
--
Pedro Alves

Also find the attachment for same patch.

gdb/ChangeLog

2013-08-12  Muhammad Waqas  <mwaqas@codesourcery.com>

	PR gdb/15501
	* breakpoint.c (enable_command, disable_command): Iterate over
	all specified breakpoint locations.

testsuite/ChangeLog

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.

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	22 Aug 2013 13:00:14 -0000
@@ -301,5 +301,114 @@ 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 ""]
+
+# Perform tests for disable/enable commands on multiple
+# locations and breakpoints.
+#
+# WHAT     - the command to test (disable/enable).
+#
+proc test_ena_dis_br { what } {
+    global b1
+    global b2
+    global b3
+    global b4
+    global gdb_prompt
+    
+    # OPPOS    - the command opposite to WHAT.
+    # WHAT_RES - whether breakpoints are expected to end
+    #            up enabled or disabled.
+    # OPPOS_RES- same as WHAT_RES but opposite.
+    # P1/P2    - proc to call (pass/fail).  Must be
+    #            opposites.
+    # Set variable values for disable command.
+    set oppos "enable"
+    set oppos_res "y"
+    set what_res "n"
+    set p1 "pass"
+    set p2 "fail"
+
+    if { "$what" == "enable" } {
+	# Set variable values for enable command.
+	set oppos "disable"
+	set oppos_res "n"
+	set what_res "y"
+	set p1 "fail"
+	set p2 "pass"
+
+    }
+
+    # Now enable(disable) $b.1 $b2.1.
+    gdb_test_no_output "$what $b1.1 $b2.1" "$what \$b1.1 \$b2.1"
+    set test1 "${what}d \$b1.1 and \$b2.1"
+
+    # Now $b1.1 and $b2.1 should be enabled(disabled)
+    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"
+       }
+    }
+
+    # Now enable(disable) $b1 fooo.1, it should give error on fooo.
+    gdb_test "$what $b1 fooo.1" \
+       "Bad breakpoint number 'fooo'" \
+       "$what \$b1 fooo.1"
+
+    # $b1 should be enabled(disabled).
+    gdb_test "info break" \
+       "(${b1})(\[^\n\r]*)( $what_res.*)" \
+       "${what}d \$b1"
+
+    gdb_test_no_output "$oppos $b3" "$oppos \$b3"
+    gdb_test_no_output "$what $b4 $b3.1" "$what \$b4 \$b3.1"
+    set test1 "${what}d \$b4 and \$b3.1,remain ${oppos}d \$b3"
+
+    # Now $b4 $b3.1 should be enabled(disabled) and
+    # $b3 should remain disabled(enabled)
+    gdb_test_multiple "info break" "$test1" {
+       -re "(${b3})(\[^\n\r]*)( $oppos_res.*)(${b3}.1)(\[^\n\r\]*)( n.*)(${b4})(\[^\n\r\]*)( $what_res.*)$gdb_prompt $" {
+           $p1 "$test1"
+       }
+       -re "(${b3})(\[^\n\r]*)( $oppos_res.*)(${b4})(\[^\n\r\]*)( $what_res.*)$gdb_prompt $" {
+           $p2 "$test1"
+       }
+    }
+
+    # Now enable(disable) $b4.1 fooobaar and
+    # it should give warning on fooobaar.
+    gdb_test "$what $b4.1 fooobaar" \
+       "warning: bad breakpoint number at or near 'fooobaar'" \
+       "$what \$b4.1 fooobar"
+    set test1 "${what}d \$b4.1"
+
+    # $b4.1 should be enabled(disabled)
+    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"
+test_ena_dis_br "enable"
+
 gdb_exit
 return 0

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: ena-dis-br.exp.patch --]
[-- Type: text/x-patch; name="ena-dis-br.exp.patch", Size: 3590 bytes --]

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	22 Aug 2013 13:00:14 -0000
@@ -301,5 +301,114 @@ 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 ""]
+
+# Perform tests for disable/enable commands on multiple
+# locations and breakpoints.
+#
+# WHAT     - the command to test (disable/enable).
+#
+proc test_ena_dis_br { what } {
+    global b1
+    global b2
+    global b3
+    global b4
+    global gdb_prompt
+    
+    # OPPOS    - the command opposite to WHAT.
+    # WHAT_RES - whether breakpoints are expected to end
+    #            up enabled or disabled.
+    # OPPOS_RES- same as WHAT_RES but opposite.
+    # P1/P2    - proc to call (pass/fail).  Must be
+    #            opposites.
+    # Set variable values for disable command.
+    set oppos "enable"
+    set oppos_res "y"
+    set what_res "n"
+    set p1 "pass"
+    set p2 "fail"
+
+    if { "$what" == "enable" } {
+	# Set varibale values for enable command.
+	set oppos "disable"
+	set oppos_res "n"
+	set what_res "y"
+	set p1 "fail"
+	set p2 "pass"
+
+    }
+
+    # Now enable(disable) $b.1 $b2.1.
+    gdb_test_no_output "$what $b1.1 $b2.1" "$what \$b1.1 \$b2.1"
+    set test1 "${what}d \$b1.1 and \$b2.1"
+
+    # Now $b1.1 and $b2.1 should be enabled(disabled)
+    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"
+       }
+    }
+
+    # Now enable(disable) $b1 fooo.1, it should give error on fooo.
+    gdb_test "$what $b1 fooo.1" \
+       "Bad breakpoint number 'fooo'" \
+       "$what \$b1 fooo.1"
+
+    # $b1 should be enabled(disabled).
+    gdb_test "info break" \
+       "(${b1})(\[^\n\r]*)( $what_res.*)" \
+       "${what}d \$b1"
+
+    gdb_test_no_output "$oppos $b3" "$oppos \$b3"
+    gdb_test_no_output "$what $b4 $b3.1" "$what \$b4 \$b3.1"
+    set test1 "${what}d \$b4 and \$b3.1,remain ${oppos}d \$b3"
+
+    # Now $b4 $b3.1 should be enabled(disabled) and
+    # $b3 should remain disabled(enabled)
+    gdb_test_multiple "info break" "$test1" {
+       -re "(${b3})(\[^\n\r]*)( $oppos_res.*)(${b3}.1)(\[^\n\r\]*)( n.*)(${b4})(\[^\n\r\]*)( $what_res.*)$gdb_prompt $" {
+           $p1 "$test1"
+       }
+       -re "(${b3})(\[^\n\r]*)( $oppos_res.*)(${b4})(\[^\n\r\]*)( $what_res.*)$gdb_prompt $" {
+           $p2 "$test1"
+       }
+    }
+
+    # Now enable(disable) $b4.1 fooobaar and
+    # it should give warning on fooobaar.
+    gdb_test "$what $b4.1 fooobaar" \
+       "warning: bad breakpoint number at or near 'fooobaar'" \
+       "$what \$b4.1 fooobar"
+    set test1 "${what}d \$b4.1"
+
+    # $b4.1 should be enabled(disabled)
+    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"
+test_ena_dis_br "enable"
+
 gdb_exit
 return 0