[PATCH] Permanent breakpoints degrade to normal breakpoints on enable

[PATCH] Permanent breakpoints degrade to normal breakpoints on enable

[Andrew Burgess]

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.

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

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

[Luis Machado]

Hi,

On 10/18/2013 11:47 AM, 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.
>
> 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);
>

It does look bogus. The fix makes sense to me.

Luis

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

[Pedro Alves]

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

I think permanent breakpoints are fundamentally broken as is (was: Re: [PATCH] Permanent breakpoints degrade to normal breakpoints on enable)

[Pedro Alves]

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.

I actually think "permanent breakpoints" are quite weird beasts,
both from a user interface, and implementation perspectives.

First, they're displayed as enabled=='n':

(gdb) b main
Breakpoint 3 at 0x40053c: file ../../../src/gdb/testsuite/gdb.arch/i386-permbkpt.S, line 29.
(gdb) info breakpoints
Num     Type           Disp Enb Address            What
3       breakpoint     keep n   0x000000000040053c ../../../src/gdb/testsuite/gdb.arch/i386-permbkpt.S:29

If one can't ever disable those, then that's just ... odd,
to say the least.

Then, why can't you really disable them?  If one adds
commands to such a breakpoint, they're _always_ ran:

(gdb) start
Starting program: /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.arch/i386-permbkpt
...
Temporary breakpoint 1, main () at ../../../src/gdb/testsuite/gdb.arch/i386-permbkpt.S:29
29              int3
(gdb) b main
Breakpoint 2 at 0x40053c: file ../../../src/gdb/testsuite/gdb.arch/i386-permbkpt.S, line 29.
(gdb) info breakpoints
Num     Type           Disp Enb Address            What
2       breakpoint     keep n   0x000000000040053c ../../../src/gdb/testsuite/gdb.arch/i386-permbkpt.S:29
(gdb) commands
Type commands for breakpoint(s) 2, one per line.
End with a line saying just "end".
>echo "hello!"
>end
(gdb) disable 2
(gdb) start
The program being debugged has been started already.
Start it from the beginning? (y or n) y
Temporary breakpoint 3 at 0x40053c: file ../../../src/gdb/testsuite/gdb.arch/i386-permbkpt.S, line 29.
Starting program: /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.arch/i386-permbkpt

Breakpoint 2, main () at ../../../src/gdb/testsuite/gdb.arch/i386-permbkpt.S:29
29              int3
"hello!"(gdb)

I claim that one should be able to disable such a breakpoint,
and that GDB would behave just like it the user hadn't
created it that case.

And then, "permanent"-ness is a property of the breakpoint.
But it should actually be an implementation detail of a _location_.

This bit in infrun.c:

  /* Normally, by the time we reach `resume', the breakpoints are either
     removed or inserted, as appropriate.  The exception is if we're sitting
     at a permanent breakpoint; we need to step over it, but permanent
     breakpoints can't be removed.  So we have to test for it here.  */
  if (breakpoint_here_p (aspace, pc) == permanent_breakpoint_here)
    {
      if (gdbarch_skip_permanent_breakpoint_p (gdbarch))
	gdbarch_skip_permanent_breakpoint (gdbarch, regcache);
      else
	error (_("\
The program is stopped at a permanent breakpoint, but GDB does not know\n\
how to step past a permanent breakpoint on this architecture.  Try using\n\
a command like `return' or `jump' to continue execution."));
    }

will do nasty things if we have a multiple location breakpoint
where the whole breakpoint was set to "permanent" if one of
the locations happened to be permanent, but the one GDB is
resuming from is not...


(I'm not even sure the whole "if you want to have GDB move past
a hardcoded trap for you, set a breakpoint on top" user interface
is actually sane at all.  Could make more sense to actually have
a "permanent breakpoint" command, distinct from a normal
breakpoint.  Then, continuing from a normal breakpoint on top
of a trap would actually execute the trap, and report the SIGTRAP
to the user, giving the user control over whether to pass that
signal back to the program).

-- 
Pedro Alves

Re: I think permanent breakpoints are fundamentally broken as is

[Andrew Burgess]

On 18/10/2013 5:17 PM, Pedro Alves wrote:
> 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.
> 
> I actually think "permanent breakpoints" are quite weird beasts,
> both from a user interface, and implementation perspectives.

<snip: lots of good points about permanent breakpoints>

OK, given all you've said I'd like to just commit the patch below.  This is basically removing the stray line I mention above but without adding any new tests.

I'd never even heard about "permanent breakpoints" before I spotted the odd looking extra line, so only added the tests as "good practice" to 
ensure the same bug was not added again.

Given that we're not really sure exactly how permanent breakpoints should operate I think just removing the stray line for now would be best, then if anyone re-works permanent breakpoints they'll not have to find/consider this tiny "ooops".

OK to apply?

Thanks,
Andrew

gdb/ChangeLog

2013-10-29  Andrew Burgess  <aburgess@broadcom.com>

	* breakpoint.c (enable_breakpoint_disp): Remove setting of
	enabled_state for permanent breakpoints.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 608463d..b5bb3da 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -14725,8 +14725,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);
 

Re: I think permanent breakpoints are fundamentally broken as is

[Pedro Alves]

On 10/29/2013 05:52 PM, Andrew Burgess wrote:
> On 18/10/2013 5:17 PM, Pedro Alves wrote:
>> 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.
>>
>> I actually think "permanent breakpoints" are quite weird beasts,
>> both from a user interface, and implementation perspectives.
> 
> <snip: lots of good points about permanent breakpoints>
> 
> OK, given all you've said I'd like to just commit the patch below.  This is basically removing the stray line I mention above but without adding any new tests.
> 
> I'd never even heard about "permanent breakpoints" before I spotted the odd looking extra line, so only added the tests as "good practice" to 
> ensure the same bug was not added again.
> 
> Given that we're not really sure exactly how permanent breakpoints should operate I think just removing the stray line for now would be best, then if anyone re-works permanent breakpoints they'll not have to find/consider this tiny "ooops".
> 
> OK to apply?

OK.

> 
> Thanks,
> Andrew
> 
> gdb/ChangeLog
> 
> 2013-10-29  Andrew Burgess  <aburgess@broadcom.com>
> 
> 	* breakpoint.c (enable_breakpoint_disp): Remove setting of
> 	enabled_state for permanent breakpoints.
-- 
Pedro Alves