* [PATCH] Permanent breakpoints degrade to normal breakpoints on enable
@ 2013-10-18 14:48 Andrew Burgess
2013-10-18 14:53 ` Luis Machado
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Andrew Burgess @ 2013-10-18 14:48 UTC (permalink / raw)
To: gdb-patches; +Cc: lgustavo
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"
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Permanent breakpoints degrade to normal breakpoints on enable
2013-10-18 14:48 [PATCH] Permanent breakpoints degrade to normal breakpoints on enable Andrew Burgess
@ 2013-10-18 14:53 ` Luis Machado
2013-10-18 16:15 ` Pedro Alves
2013-10-18 16:17 ` I think permanent breakpoints are fundamentally broken as is (was: Re: [PATCH] Permanent breakpoints degrade to normal breakpoints on enable) Pedro Alves
2 siblings, 0 replies; 6+ messages in thread
From: Luis Machado @ 2013-10-18 14:53 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Permanent breakpoints degrade to normal breakpoints on enable
2013-10-18 14:48 [PATCH] Permanent breakpoints degrade to normal breakpoints on enable Andrew Burgess
2013-10-18 14:53 ` Luis Machado
@ 2013-10-18 16:15 ` Pedro Alves
2013-10-18 16:17 ` I think permanent breakpoints are fundamentally broken as is (was: Re: [PATCH] Permanent breakpoints degrade to normal breakpoints on enable) Pedro Alves
2 siblings, 0 replies; 6+ messages in thread
From: Pedro Alves @ 2013-10-18 16:15 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches, lgustavo
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* I think permanent breakpoints are fundamentally broken as is (was: Re: [PATCH] Permanent breakpoints degrade to normal breakpoints on enable)
2013-10-18 14:48 [PATCH] Permanent breakpoints degrade to normal breakpoints on enable Andrew Burgess
2013-10-18 14:53 ` Luis Machado
2013-10-18 16:15 ` Pedro Alves
@ 2013-10-18 16:17 ` Pedro Alves
2013-10-29 17:52 ` I think permanent breakpoints are fundamentally broken as is Andrew Burgess
2 siblings, 1 reply; 6+ messages in thread
From: Pedro Alves @ 2013-10-18 16:17 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches, lgustavo
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: I think permanent breakpoints are fundamentally broken as is
2013-10-18 16:17 ` I think permanent breakpoints are fundamentally broken as is (was: Re: [PATCH] Permanent breakpoints degrade to normal breakpoints on enable) Pedro Alves
@ 2013-10-29 17:52 ` Andrew Burgess
2013-11-05 18:23 ` Pedro Alves
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Burgess @ 2013-10-29 17:52 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches, lgustavo
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);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: I think permanent breakpoints are fundamentally broken as is
2013-10-29 17:52 ` I think permanent breakpoints are fundamentally broken as is Andrew Burgess
@ 2013-11-05 18:23 ` Pedro Alves
0 siblings, 0 replies; 6+ messages in thread
From: Pedro Alves @ 2013-11-05 18:23 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches, lgustavo
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-11-05 18:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-10-18 14:48 [PATCH] Permanent breakpoints degrade to normal breakpoints on enable Andrew Burgess
2013-10-18 14:53 ` Luis Machado
2013-10-18 16:15 ` Pedro Alves
2013-10-18 16:17 ` I think permanent breakpoints are fundamentally broken as is (was: Re: [PATCH] Permanent breakpoints degrade to normal breakpoints on enable) Pedro Alves
2013-10-29 17:52 ` I think permanent breakpoints are fundamentally broken as is Andrew Burgess
2013-11-05 18:23 ` Pedro Alves
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).