Re: [PATCH] Permanent breakpoints degrade to normal breakpoints on enable

[Pedro Alves <alves.ped@gmail.com>]

On 10/18/2013 03:47 PM, Andrew Burgess wrote:
> This patch:
>    https://sourceware.org/ml/gdb-patches/2012-01/msg00964.html
>
> introduced what I believe is a stray line that causes permanent
> breakpoints to become normal breakpoints if the user ever tries
> to "enable" the permanent breakpoint.

Indeed.

> I've removed the extra line and written a test to cover this case.
>
> OK to apply?
>
> Andrew
>
> gdb/ChangeLog
>
> 2013-10-18  Andrew Burgess  <aburgess@broadcom.com>
>
> 	* breakpoint.c (enable_breakpoint_disp): Remove setting of
> 	enabled_state for permanent breakpoints.
>
> gdb/testsuite/ChangeLog
>
> 2013-10-18  Andrew Burgess  <aburgess@broadcom.com>
>
> 	* gdb.arch/i386-permbkpt.exp: Extend the existing tests, and add
> 	more tests.
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 911f7b5..53ece71 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -14666,8 +14666,6 @@ enable_breakpoint_disp (struct breakpoint *bpt, enum bpdisp disposition,
>    if (bpt->enable_state != bp_permanent)
>      bpt->enable_state = bp_enabled;
>
> -  bpt->enable_state = bp_enabled;
> -
>    /* Mark breakpoint locations modified.  */
>    mark_breakpoint_modified (bpt);
>
> diff --git a/gdb/testsuite/gdb.arch/i386-permbkpt.exp b/gdb/testsuite/gdb.arch/i386-permbkpt.exp
> index 88bfff3..52c2ee1 100644
> --- a/gdb/testsuite/gdb.arch/i386-permbkpt.exp
> +++ b/gdb/testsuite/gdb.arch/i386-permbkpt.exp
> @@ -18,6 +18,9 @@
>
>  # Test inserting breakpoints over permanent breakpoints on i386 and amd64.
>
> +global hex
> +global decimal
> +

I don't think you actually need to declare these.

>  if { ![istarget "i?86-*-*"] && ![istarget "x86_64-*-*"] } then {
>      verbose "Skipping i386 test for multi break at permanent breakpoint location."
>      return
> @@ -35,5 +38,28 @@ if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list
>
>  clean_restart ${binfile}
>
> -gdb_test "break main" "" "First permanent break"
> -gdb_test "break main" "" "Second permanent break"
> +gdb_test "break main" \
> +    "Breakpoint $decimal at $hex: .*" \
> +    "First permanent break"
> +gdb_test_no_output "set \$bp1 = \$bpnum"
> +
> +# Place a second breakpoint at the same place as the first, check
> +# that the first is called a "permanent" breakpoint.
> +gdb_test "break main" \
> +    "Note: breakpoint $decimal \\(permanent\\) also set at pc $hex\.\[\n\r\]+Breakpoint $decimal at $hex: .*" \
> +    "Second permanent break"
> +gdb_test_no_output "set \$bp2 = \$bpnum"
> +
> +# Now, delete the last breakpoint placed.
> +gdb_test_no_output "delete \$bp2"
> +
> +# Now 'enable' the first breakpoint, this should have no impact as the
> +# breakpoint is already enabled, however, we've had bugs in this code
> +# before now.

s/now//.

> +gdb_test_no_output "enable \$bp1"
> +
> +# Check that placing (another) second breakpoint at the same location as
> +# the first still reports the first as a permanent breakpoint.
> +gdb_test "break main" \
> +    "Note: breakpoint $decimal \\(permanent\\) also set at pc $hex\.\[\n\r\]+Breakpoint $decimal at $hex: .*" \
> +    "Third permanent break"
>

Otherwise OK.  Thanks!

-- 
Pedro Alves