* [PATCH 0/3] fix permanent breakpoints
@ 2014-11-04 19:03 Pedro Alves
2014-11-04 19:03 ` [PATCH 3/3] fix skipping " Pedro Alves
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Pedro Alves @ 2014-11-04 19:03 UTC (permalink / raw)
To: gdb-patches
This series fixes a few permanent breakpoints things.
- Adds a default method to gdbarch_skip_permanent_breakpoint, which
should be right for most archs.
- Makes permanent breakpoints be a detail of a breakpoint location
rather than of the whole breakpoint. Bad things happen today if a
multi-location breakpoint happens to have some (but not all) of its
locations on top of a breakpoint instruction.
- Makes permanent breakpoints disablable. If the user disables the
breakpoint, she gets the random SIGTRAP as if she hadn't set the
breakpoint in the first place.
- Fixes the skip-permanent-breakpoint code to actually just skip the
breakpoint. GDB currently steps one instruction too much, along
with getting other tricky details wrong.
But the reason I'm looking at this now is that a recent change of mine
regressed the gdb.arch/i386-bp_permanent.exp test. See:
https://sourceware.org/ml/gdb/2014-11/msg00001.html
Specifically, GDB is now hitting an assertion when skipping a
permanent breakpoint:
(gdb) stepi
../../src/gdb/infrun.c:2237: internal-error: resume: Assertion `sig != GDB_SIGNAL_0' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n) FAIL: gdb.arch/i386-bp_permanent.exp: Single stepping past permanent breakpoint. (GDB internal error)
However, taking a closer look, the problem is in permanent
breakpoint's code, not on the assertion itself. That is, the
assertion caught something bogus.
I've complained about permanent breakpoints being fundamentally broken
before, at:
https://www.sourceware.org/ml/gdb-patches/2013-10/msg00574.html
And it turns out that the new test added by this series trips on
issues related to what I mentioned in that email, and I so I ended up
having to do the changes I proposed in that email...
Tested on x86-64 Fedora 20.
Pedro Alves (3):
add a default method for gdbarch_skip_permanent_breakpoint
make "permanent breakpoints" per location and disableable
fix skipping permanent breakpoints
gdb/arch-utils.c | 13 +-
gdb/arch-utils.h | 8 +
gdb/breakpoint.c | 71 +++----
gdb/breakpoint.h | 13 +-
gdb/gdbarch.c | 13 +-
gdb/gdbarch.h | 2 -
gdb/gdbarch.sh | 2 +-
gdb/i386-tdep.c | 15 --
gdb/infrun.c | 153 +++++++++++++--
gdb/testsuite/gdb.arch/i386-bp_permanent.c | 57 ++++++
gdb/testsuite/gdb.arch/i386-bp_permanent.exp | 49 ++---
gdb/testsuite/gdb.base/bp-permanent.c | 128 +++++++++++++
gdb/testsuite/gdb.base/bp-permanent.exp | 276 +++++++++++++++++++++++++++
13 files changed, 672 insertions(+), 128 deletions(-)
create mode 100644 gdb/testsuite/gdb.arch/i386-bp_permanent.c
create mode 100644 gdb/testsuite/gdb.base/bp-permanent.c
create mode 100644 gdb/testsuite/gdb.base/bp-permanent.exp
--
1.9.3
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 3/3] fix skipping permanent breakpoints 2014-11-04 19:03 [PATCH 0/3] fix permanent breakpoints Pedro Alves @ 2014-11-04 19:03 ` Pedro Alves 2014-11-05 12:26 ` Yao Qi 2014-11-04 19:03 ` [PATCH 1/3] add a default method for gdbarch_skip_permanent_breakpoint Pedro Alves 2014-11-04 19:03 ` [PATCH 2/3] make "permanent breakpoints" per location and disableable Pedro Alves 2 siblings, 1 reply; 13+ messages in thread From: Pedro Alves @ 2014-11-04 19:03 UTC (permalink / raw) To: gdb-patches The gdb.arch/i386-bp_permanent.exp test is currently failing an assertion I've recently added: (gdb) stepi ../../src/gdb/infrun.c:2237: internal-error: resume: Assertion `sig != GDB_SIGNAL_0' failed. A problem internal to GDB has been detected, further debugging may prove unreliable. Quit this debugging session? (y or n) FAIL: gdb.arch/i386-bp_permanent.exp: Single stepping past permanent breakpoint. (GDB internal error) The assertion expects that the only reason we currently need to step a breakpoint instruction is when we have a signal to deliver. But when stepping a permanent breakpoint (with or without a signal) we also reach this code. IMO, the assertion is correct and the permanent breakpoints skipping code is wrong. Consider the case of the user doing "step/stepi" when stopped at a permanent breakpoint. GDB's `resume' calls the gdbarch_skip_permanent_breakpoint hook and then happily continues stepping: /* 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) { gdbarch_skip_permanent_breakpoint (gdbarch, regcache); } But since gdbarch_skip_permanent_breakpoint already advanced the PC manually, this ends up executing the instruction that is _after_ the breakpoint instruction. The user-visible result is that a single-step steps two instructions. The gdb.arch/i386-bp_permanent.exp test is actually ensuring that that's indeed how things work. It runs to an int3 instruction, does "stepi", and checks that "leave" was executed with that "stepi". Like this: (gdb) b *0x0804848c Breakpoint 2 at 0x804848c (gdb) c Continuing. Breakpoint 2, 0x0804848c in standard () (gdb) disassemble Dump of assembler code for function standard: 0x08048488 <+0>: push %ebp 0x08048489 <+1>: mov %esp,%ebp 0x0804848b <+3>: push %edi => 0x0804848c <+4>: int3 0x0804848d <+5>: leave 0x0804848e <+6>: ret 0x0804848f <+7>: nop (gdb) si 0x0804848e in standard () (gdb) disassemble Dump of assembler code for function standard: 0x08048488 <+0>: push %ebp 0x08048489 <+1>: mov %esp,%ebp 0x0804848b <+3>: push %edi 0x0804848c <+4>: int3 0x0804848d <+5>: leave => 0x0804848e <+6>: ret 0x0804848f <+7>: nop End of assembler dump. (gdb) I would instead expect that a stepi at 0x0804848c stops at 0x0804848d, _before_ the "leave" is executed. This commit changes GDB this way. Care is taken to make stepping into a signal handler when the step starts at a permanent breakpoint instruction work correctly. The patch adjusts gdb.arch/i386-bp_permanent.exp in this direction, and also makes it work on x86_64 (currently it only works on i*86). The patch also adds a new gdb.base/bp-permanent.exp test that exercises many different code paths related to stepping permanent breakpoints, including the stepping with signals cases. The test uses "hack/trick" to make it work on all (or most) platforms -- it doesn't really hard code a breakpoint instruction. Tested on x86_64 Fedora 20. gdb/ 2014-11-04 Pedro Alves <palves@redhat.com> * infrun.c (resume): Clear the thread's 'stepped_breakpoint' flag. Rewrite stepping over a permanent breakpoint. (thread_still_needs_step_over, proceed): Don't step stepping_over_breakpoint for permanent breakpoints. (handle_signal_stop): Don't clear stepped_breakpoint. (process_event_stop_test): If stepping a permanent breakpoint doesn't hit the step-resume breakpoint, delete the step-resume breakpoint. (currently_stepping): Return true if the thread stepped a breakpoint. (insert_step_resume_breakpoint_at_frame_1): New function, factored out from ... (insert_hp_step_resume_breakpoint_at_frame) ... this. (insert_step_resume_breakpoint_at_frame): New function. gdb/testsuite/ 2014-11-04 Pedro Alves <palves@redhat.com> * gdb.arch/i386-bp_permanent.c: New file. * gdb.arch/i386-bp_permanent.exp: Don't skip on x86_64. (srcfile): Set to i386-bp_permanent.c. (top level): Adjust to work in both 32-bit and 64-bit modes. Test that stepi does not execute the 'leave' instruction, instead of testing it does execute. * gdb.base/bp-permanent.c: New file. * gdb.base/bp-permanent.exp: New file. --- gdb/infrun.c | 149 +++++++++++++-- gdb/testsuite/gdb.arch/i386-bp_permanent.c | 57 ++++++ gdb/testsuite/gdb.arch/i386-bp_permanent.exp | 49 ++--- gdb/testsuite/gdb.base/bp-permanent.c | 128 +++++++++++++ gdb/testsuite/gdb.base/bp-permanent.exp | 276 +++++++++++++++++++++++++++ 5 files changed, 615 insertions(+), 44 deletions(-) create mode 100644 gdb/testsuite/gdb.arch/i386-bp_permanent.c create mode 100644 gdb/testsuite/gdb.base/bp-permanent.c create mode 100644 gdb/testsuite/gdb.base/bp-permanent.exp diff --git a/gdb/infrun.c b/gdb/infrun.c index 4c4f03b..24f2603 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -2013,6 +2013,8 @@ user_visible_resume_ptid (int step) return resume_ptid; } +static void insert_step_resume_breakpoint_at_frame (struct frame_info *return_frame); + /* Resume the inferior, but allow a QUIT. This is useful if the user wants to interrupt some lengthy single-stepping operation (for child processes, the SIGINT goes to the inferior, and so @@ -2039,6 +2041,8 @@ resume (int step, enum gdb_signal sig) applies, it's the callers intention that counts. */ const int entry_step = step; + tp->stepped_breakpoint = 0; + QUIT; if (current_inferior ()->waiting_for_vfork_done) @@ -2075,7 +2079,76 @@ resume (int step, enum gdb_signal sig) breakpoints can't be removed. So we have to test for it here. */ if (breakpoint_here_p (aspace, pc) == permanent_breakpoint_here) { - gdbarch_skip_permanent_breakpoint (gdbarch, regcache); + if (sig != GDB_SIGNAL_0) + { + /* We have a signal to pass to the inferior. The resume + may, or may not take us to the signal handler. If this + is a step, we'll need to stop in the signal handler, if + there's one, (if the target supports stepping into + handlers), or in the next mainline instruction, if + there's no handler. If this is a continue, we need to be + sure to run the handler with all breakpoints inserted. + In all cases, set a breakpoint at the current address + (where the handler returns to), and once that breakpoint + is hit, resume skipping the permanent breakpoint. If + that breakpoint isn't hit, then we've stepped into the + signal handler (or hit some other event). We'll delete + the single-step breakpoint then. */ + + if (debug_infrun) + fprintf_unfiltered (gdb_stdlog, + "infrun: resume: skipping permanent breakpoint, " + "deliver signal first\n"); + + clear_step_over_info (); + tp->control.trap_expected = 0; + + if (tp->control.step_resume_breakpoint == NULL) + { + /* Set a "high-priority" step-resume, as we don't want + user breakpoints at PC to be trigger (again) when + this hits. */ + insert_hp_step_resume_breakpoint_at_frame (get_current_frame ()); + gdb_assert (tp->control.step_resume_breakpoint->loc->permanent); + + tp->step_after_step_resume_breakpoint = step; + } + + insert_breakpoints (); + } + else + { + /* There's no signal to pass. Skip the permanent + breakpoint, and arrange for any breakpoint at the new PC + to be reported back to handle_inferior_event. */ + + if (debug_infrun) + fprintf_unfiltered (gdb_stdlog, + "infrun: resume: skipping permanent breakpoint\n"); + gdbarch_skip_permanent_breakpoint (gdbarch, regcache); + /* Update pc to reflect the new address from which we will + execute instructions. */ + pc = regcache_read_pc (get_thread_regcache (inferior_ptid)); + tp->prev_pc = pc; + + clear_step_over_info (); + tp->control.trap_expected = 0; + + /* There may or not be a breakpoint at the new address. If + we're stepping then we're done. Otherwise, make sure + there's a breakpoint at the current address, and let it + trigger. */ + if (step) + { + /* If we already have a step-resume breakpoint set, then + we can just continue until that one is hit. */ + if (tp->control.step_resume_breakpoint == NULL) + insert_step_resume_breakpoint_at_frame (get_current_frame ()); + step = 0; + } + + insert_breakpoints (); + } } /* If we have a breakpoint to step over, make sure to do a single @@ -2382,7 +2455,8 @@ thread_still_needs_step_over (struct thread_info *tp) struct regcache *regcache = get_thread_regcache (tp->ptid); if (breakpoint_here_p (get_regcache_aspace (regcache), - regcache_read_pc (regcache))) + regcache_read_pc (regcache)) + == ordinary_breakpoint_here) return 1; tp->stepping_over_breakpoint = 0; @@ -2498,7 +2572,8 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step) if (addr == (CORE_ADDR) -1) { - if (pc == stop_pc && breakpoint_here_p (aspace, pc) + if (pc == stop_pc + && breakpoint_here_p (aspace, pc) == ordinary_breakpoint_here && execution_direction != EXEC_REVERSE) /* There is a breakpoint at the address we will resume at, step one instruction before inserting breakpoints so that @@ -4280,7 +4355,6 @@ handle_signal_stop (struct execution_control_state *ecs) return; } - ecs->event_thread->stepped_breakpoint = 0; ecs->event_thread->stepping_over_breakpoint = 0; ecs->event_thread->stepping_over_watchpoint = 0; bpstat_clear (&ecs->event_thread->control.stop_bpstat); @@ -4788,6 +4862,30 @@ process_event_stop_test (struct execution_control_state *ecs) break; } + /* If we stepped a permanent breakpoint and we had a high priority + step-resume breakpoint for the address we stepped, but we didn't + hit it, then we must have stepped into the signal handler. The + step-resume was only necessary to catch the case _not_ stepping + into the handler, so delete it, and fall through to checking + whether the step finished. */ + if (ecs->event_thread->stepped_breakpoint) + { + struct breakpoint *sr_bp + = ecs->event_thread->control.step_resume_breakpoint; + + if (sr_bp->loc->permanent + && sr_bp->type == bp_hp_step_resume + && sr_bp->loc->address == ecs->event_thread->prev_pc) + { + if (debug_infrun) + fprintf_unfiltered (gdb_stdlog, + "infrun: stepped permanent breakpoint, stopped in " + "handler\n"); + delete_step_resume_breakpoint (ecs->event_thread); + ecs->event_thread->step_after_step_resume_breakpoint = 0; + } + } + /* We come here if we hit a breakpoint but should not stop for it. Possibly we also were stepping and should stop for that. So fall through and test for stepping. But, if not stepping, do not @@ -5604,6 +5702,7 @@ currently_stepping (struct thread_info *tp) return ((tp->control.step_range_end && tp->control.step_resume_breakpoint == NULL) || tp->control.trap_expected + || tp->stepped_breakpoint || bpstat_should_step ()); } @@ -5758,6 +5857,28 @@ insert_step_resume_breakpoint_at_sal (struct gdbarch *gdbarch, bp_step_resume); } +/* Insert a "step-resume breakpoint" of type SR_TYPE at FRAME.pc. */ + +static void +insert_step_resume_breakpoint_at_frame_1 (struct frame_info *frame, + enum bptype sr_type) +{ + struct symtab_and_line sr_sal; + struct gdbarch *gdbarch; + + gdb_assert (frame != NULL); + init_sal (&sr_sal); /* initialize to zeros */ + + gdbarch = get_frame_arch (frame); + sr_sal.pc = gdbarch_addr_bits_remove (gdbarch, get_frame_pc (frame)); + sr_sal.section = find_pc_overlay (sr_sal.pc); + sr_sal.pspace = get_frame_program_space (frame); + + insert_step_resume_breakpoint_at_sal_1 (gdbarch, sr_sal, + get_stack_frame_id (frame), + sr_type); +} + /* Insert a "high-priority step-resume breakpoint" at RETURN_FRAME.pc. This is used to skip a potential signal handler. @@ -5768,20 +5889,16 @@ insert_step_resume_breakpoint_at_sal (struct gdbarch *gdbarch, static void insert_hp_step_resume_breakpoint_at_frame (struct frame_info *return_frame) { - struct symtab_and_line sr_sal; - struct gdbarch *gdbarch; - - gdb_assert (return_frame != NULL); - init_sal (&sr_sal); /* initialize to zeros */ + insert_step_resume_breakpoint_at_frame_1 (return_frame, bp_hp_step_resume); +} - gdbarch = get_frame_arch (return_frame); - sr_sal.pc = gdbarch_addr_bits_remove (gdbarch, get_frame_pc (return_frame)); - sr_sal.section = find_pc_overlay (sr_sal.pc); - sr_sal.pspace = get_frame_program_space (return_frame); +/* Insert a regular "step-resume breakpoint" at FRAME.pc. + This is used to skip a permanent breakpoint. */ - insert_step_resume_breakpoint_at_sal_1 (gdbarch, sr_sal, - get_stack_frame_id (return_frame), - bp_hp_step_resume); +static void +insert_step_resume_breakpoint_at_frame (struct frame_info *frame) +{ + insert_step_resume_breakpoint_at_frame_1 (frame, bp_step_resume); } /* Insert a "step-resume breakpoint" at the previous frame's PC. This diff --git a/gdb/testsuite/gdb.arch/i386-bp_permanent.c b/gdb/testsuite/gdb.arch/i386-bp_permanent.c new file mode 100644 index 0000000..dc36d89 --- /dev/null +++ b/gdb/testsuite/gdb.arch/i386-bp_permanent.c @@ -0,0 +1,57 @@ +/* Copyright (C) 2003-2014 Free Software Foundation, Inc. + + This file is part of GDB. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +#ifdef SYMBOL_PREFIX +#define SYMBOL(str) SYMBOL_PREFIX #str +#else +#define SYMBOL(str) #str +#endif + +int +main (void) +{ + standard (); + return 0; +} + +/* A normal prologue. */ + +#ifdef __x86_64__ +asm(".text\n" + " .align 8\n" + SYMBOL (standard) ":\n" + " push %rbp\n" + " mov %rsp, %rbp\n" + " push %rdi\n" + " int3\n" + " leaveq\n" + " retq\n"); + +#else + +asm(".text\n" + " .align 8\n" + " .global " SYMBOL(standard) "\n" + SYMBOL (standard) ":\n" + " pushl %ebp\n" + " movl %esp, %ebp\n" + " pushl %edi\n" + " int3\n" + " leave\n" + " ret\n"); + +#endif diff --git a/gdb/testsuite/gdb.arch/i386-bp_permanent.exp b/gdb/testsuite/gdb.arch/i386-bp_permanent.exp index 6be1626..833b4ed 100644 --- a/gdb/testsuite/gdb.arch/i386-bp_permanent.exp +++ b/gdb/testsuite/gdb.arch/i386-bp_permanent.exp @@ -18,13 +18,13 @@ # Test stepping over permanent breakpoints on i386. -if { ![is_x86_like_target] } then { +if { ![istarget "i?86-*-*"] && ![istarget "x86_64-*-*"] } then { verbose "Skipping skip over permanent breakpoint on i386 tests." return } set testfile "i386-bp_permanent" -set srcfile i386-prologue.c +set srcfile i386-bp_permanent.c set binfile ${objdir}/${subdir}/${testfile} # some targets have leading underscores on assembly symbols. @@ -51,14 +51,12 @@ if ![runto_main] then { return -1 } -set function standard +set function "standard" set retcode [gdb_test_multiple "disassemble $function" "Disassemble function '$function'" { - -re ".*($hex) <\\+0>.*($hex) <\\+4>.*($hex) <\\+5>.*($hex) <\\+6>.*$gdb_prompt $" { - set function_start $expect_out(1,string) - set address $expect_out(2,string) - set address1 $expect_out(3,string) - set address2 $expect_out(4,string) + -re "($hex) <\\+0>.*($hex) <\\+$decimal>.*int3.*($hex) <\\+$decimal>.*leave.*$gdb_prompt $" { + set address_bp $expect_out(2,string) + set address_after_bp $expect_out(3,string) } }] @@ -67,30 +65,25 @@ if {$retcode != 0} { return -1 } -gdb_breakpoint "*$function_start" +gdb_breakpoint "*$address_bp" -gdb_breakpoint "*$address" +gdb_test "continue" "Breakpoint .*, $address_bp in $function.*" \ + "stop at permanent breakpoint" -gdb_test "continue" "Breakpoint .*, $function_start in $function.*" \ - "Stop at the '$function' start breakpoint (fetching esp)." - -# We want to fetch esp at the start of '$function' function to make sure -# skip_permanent_breakpoint implementation really skips only the perm. -# breakpoint. If, for whatever reason, 'leave' instruction doesn't get -# executed, esp will not have this value. -set start_esp 0 -gdb_test_multiple "print \$esp" "Fetch esp value." { +# We want to fetch the stack pointer at the start of '$function' +# function to make sure the skip_permanent_breakpoint implementation +# really skips only the permanent breakpoint. If, for whatever +# reason, the 'leave' instruction executes, the stack pointer will not +# have this value. +set start_sp 0 +gdb_test_multiple "print \$sp" "fetch stack pointer value" { -re "\\\$1.*($hex).*$gdb_prompt $" { - set start_esp $expect_out(1,string) + set start_sp $expect_out(1,string) } } -gdb_test "continue" "Breakpoint .*, $address in $function.*" \ - "Stop at permanent breakpoint." - -gdb_test "stepi" "$address1|$address2 in $function.*" \ - "Single stepping past permanent breakpoint." - -gdb_test "print \$esp" ".*$start_esp.*" \ - "ESP value does not match - step_permanent_breakpoint wrong." +gdb_test "stepi" "$address_after_bp in $function.*" \ + "single-step past permanent breakpoint" +gdb_test "print \$sp" ".*$start_sp.*" \ + "stack pointer value matches" diff --git a/gdb/testsuite/gdb.base/bp-permanent.c b/gdb/testsuite/gdb.base/bp-permanent.c new file mode 100644 index 0000000..62bf34f --- /dev/null +++ b/gdb/testsuite/gdb.base/bp-permanent.c @@ -0,0 +1,128 @@ +/* Copyright (C) 2014 Free Software Foundation, Inc. + + This file is part of GDB. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +#include <string.h> +#ifdef SIGNALS +#include <signal.h> +#endif + +#define NOP asm("nop") + +/* Buffer holding the breakpoint instruction. */ +unsigned char buffer[16]; + +volatile unsigned char *addr_bp; +volatile unsigned char *addr_after_bp; +int counter = 0; + +void +test (void) +{ + NOP; + NOP; + NOP; + NOP; /* write permanent bp here */ + NOP; /* after permanent bp */ + NOP; + NOP; + NOP; + NOP; + NOP; + + counter++; +} + +void +setup (void) +{ + memcpy (buffer, (void *) addr_bp, addr_after_bp - addr_bp); +} + +void +test_basics (void) +{ + test (); /* for SIGTRAP */ + test (); /* for breakpoint once */ + test (); /* for breakpoint twice */ + test (); /* for disabled bp SIGTRAP */ + test (); /* for breakpoint thrice */ +} + +void +test_next (void) +{ + test (); /* for next */ + counter = 0; /* after next */ +} + +#ifdef SIGNALS + +static void +test_signal_handler (int sig) +{ +} + +void +test_signal_with_handler (void) +{ + signal (SIGUSR1, test_signal_handler); + test (); +} + +void +test_signal_no_handler (void) +{ + signal (SIGUSR1, SIG_IGN); + test (); +} + +static void +test_signal_nested_handler () +{ + test (); +} + +void +test_signal_nested_done (void) +{ +} + +void +test_signal_nested (void) +{ + counter = 0; + signal (SIGALRM, test_signal_nested_handler); + alarm (1); + test (); + test_signal_nested_done (); +} + +#endif + +int +main (void) +{ + setup (); + test_basics (); + test_next (); +#ifdef SIGNALS + test_signal_with_handler (); + test_signal_no_handler (); + test_signal_nested (); +#endif + return 0; +} diff --git a/gdb/testsuite/gdb.base/bp-permanent.exp b/gdb/testsuite/gdb.base/bp-permanent.exp new file mode 100644 index 0000000..2bb1ef1 --- /dev/null +++ b/gdb/testsuite/gdb.base/bp-permanent.exp @@ -0,0 +1,276 @@ +# Copyright (C) 2014 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +# This file is part of the gdb testsuite. + +# Test stepping over permanent breakpoints. + +standard_testfile + +set options { debug } +if { ![target_info exists gdb,nosignals] } { + lappend options "additional_flags=-DSIGNALS" +} + +if {[build_executable "failed to prepare" $testfile $srcfile $options]} { + return -1 +} + +set line_bp [gdb_get_line_number "write permanent bp"] + +# The test proper. ALWAYS_INSERTED indicates whether testing in +# "breakpoint always-inserted" mode. If SW_WATCHPOINT is true, set a +# software watchpoint, which forces constantly single-stepping, and +# exercises stepping the permanent breakpoint while delivering a +# signal at the same time. + +proc test {always_inserted sw_watchpoint} { + global line_bp + global hex decimal + global gdb_prompt + global srcfile binfile + + clean_restart $binfile + + if ![runto_main] then { + return -1 + } + + gdb_test "set breakpoint always-inserted $always_inserted" + + if {$sw_watchpoint} { + # Watching a convenience variable forces a software + # watchpoint. + gdb_test "watch \$dummy_convenience" "Watchpoint .*" + } + + set address_bp "" + set address_after_bp "" + + with_test_prefix "setup" { + + # Set a breakpoint where we'll manually plant a permanent + # breakpoint. + set test "set probe breakpoint" + gdb_test_multiple "break $line_bp" $test { + -re "Breakpoint .* at ($hex).*$gdb_prompt $" { + set address_bp $expect_out(1,string) + pass $test + } + } + if {$address_bp == ""} { + return + } + + # Get the size of the instruction where the breakpoint will + # manually inserted. + set test "get size of instruction" + gdb_test_multiple "x/2i $address_bp" $test { + -re ".*$hex <test\\+$decimal>:\[^\r\n\]+\r\n\[ \]+($hex).*\.\r\n$gdb_prompt $" { + set address_after_bp $expect_out(1,string) + pass $test + } + } + if {$address_after_bp == ""} { + return + } + + # Write address range where the breakpoint is inserted to the + # corresponding variables in the inferior. + gdb_test "p /x addr_bp = $address_bp" " = $address_bp" \ + "write addr_bp" + gdb_test "p /x addr_after_bp = $address_after_bp" " = $address_after_bp" \ + "write addr_after_bp" + + # Run the "setup" function in the inferior. This memcpy's the + # breakpoint instruction to a buffer in the inferior. + gdb_test "next" "test.*" "next over setup" + + delete_breakpoints + + # We now have the breakpoint instruction stored in 'buffer'. Poke it + # to memory manually. + set count [expr $address_after_bp - $address_bp] + for {set i 0} {$i < $count} {incr i} { + gdb_test "p /x addr_bp\[$i\] = buffer\[$i\]" " = .*" + } + } + + with_test_prefix "basics" { + # Run to the permanent breakpoint, just to make sure we've inserted it + # correctly. + gdb_test "continue" "Program received signal SIGTRAP.*" \ + "permanent breakpoint causes random signal" + + # Now set a breakpoint on top, thus creating a permanent breakpoint. + gdb_breakpoint "$line_bp" + + # Depending on whether this is a decr_pc_after_break arch, the PC will + # be either pointing at the permanent breakpoint address, or just + # after. Set the GDB breakpoint on top, and continue, twice. At + # least once, GDB will need to step-over the permanent breakpoint. + + gdb_test "continue" "Breakpoint .*" "stop at permanent breakpoint" + + gdb_test "p \$prev_counter = counter" " = $decimal" + + gdb_test "continue" "Breakpoint .*" "stop at permanent breakpoint twice" + + # Check that indeed the continue made progress, instead of re-trapping + # without advancing. + gdb_test "p counter - \$prev_counter" " = 1" + + gdb_test "info breakpoints" \ + "breakpoint.*keep.*y.*$hex.*in test at .*$srcfile:$line_bp.*already hit 2 times.*" \ + "info breakpoints show enabled breakpoint" + + gdb_test "disable \$bpnum" + + gdb_test "commands\nset \$commands_ran = 1\nend" "" \ + "set breakpoint commands" + + gdb_test "info breakpoints" \ + "breakpoint.*keep.*n.*$hex.*in test at .*$srcfile:$line_bp.*already hit 2 times.*" \ + "info breakpoints shows disabled breakpoint" + + # Run to the permanent breakpoint again. This time, since it's + # disabled, it should act as if we hadn't created it in the first + # place. IOW, we should get a random signal, and, the breakpoint's + # command should not run. + gdb_test "continue" "Program received signal SIGTRAP.*" \ + "disabled permanent breakpoint doesn't explain stop" + + gdb_test "info breakpoints" \ + "breakpoint.*keep.*n.*$hex.*in test at .*$srcfile:$line_bp.*already hit 2 times.*" \ + "info breakpoints still shows same number of hits" + + gdb_test "print \$commands_ran" " = void" \ + "breakpoint commands didn't run" + + # Reenable the breakpoint, and check that it gets hit and accounted + # for this time. + gdb_test "enable \$bpnum" "" "reenable breakpoint" + + gdb_test "continue" "Breakpoint .*" \ + "stop at permanent breakpoint thrice" + + gdb_test "info breakpoints" \ + "breakpoint.*keep.*y.*$hex.*in test at .*$srcfile:$line_bp.*already hit 3 times.*" \ + "info breakpoints shows one more hit" + + gdb_test "print \$commands_ran" " = 1" "breakpoint commands ran" + + # Check that stepi advances only past the permanent breakpoint, and + # not a single instruction more. + gdb_test "stepi" "after permanent bp .*" \ + "single-step past permanent breakpoint" + } + + with_test_prefix "next trips on permanent bp" { + delete_breakpoints + + gdb_breakpoint "test_next" + gdb_continue_to_breakpoint "test_next" + + gdb_breakpoint "$line_bp" + gdb_test "condition \$bpnum 0" + + gdb_test "next" "after next .*" + } + + if ![target_info exists gdb,nosignals] { + + with_test_prefix "stepi signal with handler" { + delete_breakpoints + + gdb_breakpoint "test_signal_with_handler" + gdb_continue_to_breakpoint "test_signal_with_handler" + + gdb_breakpoint "$line_bp" + + gdb_test "continue" "Breakpoint .*" "stop at permanent breakpoint" + + gdb_test "queue-signal SIGUSR1" + + set test "single-step to handler" + gdb_test_multiple "stepi" $test { + -re "Program received signal SIGTRAP.*$gdb_prompt $" { + fail $test + } + -re "handler .*$gdb_prompt $" { + pass $test + } + } + + # Check that the mainline PC points at the permanent + # breakpoint. + gdb_test "up 2" "test .*" "up to mainline code" + + gdb_test "p /x \$pc" " = $address_bp" \ + "mainline pc points at permanent breakpoint" + + gdb_test "continue" "Breakpoint .*" \ + "stop at permanent breakpoint, out of handler" + } + + with_test_prefix "stepi signal with no handler" { + gdb_breakpoint "test_signal_no_handler" + gdb_continue_to_breakpoint "test_signal_no_handler" + + gdb_test "continue" "Breakpoint .*" "stop at permanent breakpoint" + + gdb_test "queue-signal SIGUSR1" + + gdb_test "stepi" "after permanent bp .*" \ + "single-step past permanent breakpoint" + } + + with_test_prefix "continue trips on nested permanent bp" { + delete_breakpoints + + gdb_breakpoint "test_signal_nested" + gdb_continue_to_breakpoint "test_signal_nested" + + gdb_breakpoint "$line_bp" + gdb_continue_to_breakpoint "permanent bp" + gdb_test "condition \$bpnum 0" + + # Let SIGALRM trigger. + sleep 2 + + # We're now stopped at a permanent breakpoint, with a + # signal pending. Continuing first skips the permanent + # breakpoint, sets a step-resume at the following + # instruction, and delivers the signal. The handler trips + # on the permanent breakpoint again, and this time, no + # step-resume should be set, as we already had one. + gdb_breakpoint "test_signal_nested_done" + gdb_continue_to_breakpoint "test_signal_nested_done" + + # Ensure that the handler did run. There's one call to + # test in the mainline code, and another in the signal + # handler. + gdb_test "p counter" " = 2" + } + } +} + +foreach always_inserted {off on} { + foreach sw_watchpoint {0 1} { + with_test_prefix "always_inserted=$always_inserted, sw_watchpoint=$sw_watchpoint" { + test $always_inserted $sw_watchpoint + } + } +} -- 1.9.3 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] fix skipping permanent breakpoints 2014-11-04 19:03 ` [PATCH 3/3] fix skipping " Pedro Alves @ 2014-11-05 12:26 ` Yao Qi 2014-11-07 19:53 ` Pedro Alves 0 siblings, 1 reply; 13+ messages in thread From: Yao Qi @ 2014-11-05 12:26 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches Pedro Alves <palves@redhat.com> writes: > The patch also adds a new gdb.base/bp-permanent.exp test that > exercises many different code paths related to stepping permanent > breakpoints, including the stepping with signals cases. The test uses > "hack/trick" to make it work on all (or most) platforms -- it doesn't > really hard code a breakpoint instruction. It is great to test permanent breakpoint in an arch-independent way. It exposes an existing issue on permanent breakpoint which is found when I test your patch series on arm. breakpoint.c:bp_location_has_shadow should return false for permanent breakpoint because permanent breakpoint doesn't have shadow, otherwise, breakpoint_xfer_memory destroys contents read from memory_xfer_partial_1 and the internal error is triggered as below, (gdb) PASS: gdb.base/bp-permanent.exp: always_inserted=off, sw_watchpoint=0: stepi signal with handler: queue-signal SIGUSR1 stepi^M ../../../git/gdb/infrun.c:2112: internal-error: resume: Assertion `tp->control.step_resume_breakpoint->loc->permanent' failed.^M A problem internal to GDB has been detected, > + else > + { > + /* There's no signal to pass. Skip the permanent > + breakpoint, and arrange for any breakpoint at the new PC > + to be reported back to handle_inferior_event. */ > + > + if (debug_infrun) > + fprintf_unfiltered (gdb_stdlog, > + "infrun: resume: skipping permanent breakpoint\n"); > + gdbarch_skip_permanent_breakpoint (gdbarch, regcache); > + /* Update pc to reflect the new address from which we will > + execute instructions. */ > + pc = regcache_read_pc (get_thread_regcache (inferior_ptid)); Is get_thread_regcache (inferior_ptid) equivalent to regcache? If it is, we can use regcache which is shorter. > + tp->prev_pc = pc; > + > + clear_step_over_info (); > + tp->control.trap_expected = 0; > + > + /* There may or not be a breakpoint at the new address. If > + we're stepping then we're done. Otherwise, make sure > + there's a breakpoint at the current address, and let it > + trigger. */ Sorry, I can't map the comments to the code. Is "Otherwise" in the comments about the else branch which doesn't exist? Can you elaborate? > + if (step) > + { > + /* If we already have a step-resume breakpoint set, then > + we can just continue until that one is hit. */ When I read the comments, the code in my mind is like: if (tp->control.step_resume_breakpoint != NULL) step = 0; which is different from your code below. > + if (tp->control.step_resume_breakpoint == NULL) > + insert_step_resume_breakpoint_at_frame (get_current_frame ()); > + step = 0; > + } > + > + insert_breakpoints (); > + } > } > > + > + with_test_prefix "next trips on permanent bp" { > + delete_breakpoints > + > + gdb_breakpoint "test_next" > + gdb_continue_to_breakpoint "test_next" > + > + gdb_breakpoint "$line_bp" > + gdb_test "condition \$bpnum 0" > + > + gdb_test "next" "after next .*" > + } > + > + if ![target_info exists gdb,nosignals] { > + We need also check can_single_step_to_signal_handler. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] fix skipping permanent breakpoints 2014-11-05 12:26 ` Yao Qi @ 2014-11-07 19:53 ` Pedro Alves 2014-11-12 0:54 ` Yao Qi 0 siblings, 1 reply; 13+ messages in thread From: Pedro Alves @ 2014-11-07 19:53 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On 11/05/2014 12:26 PM, Yao Qi wrote: > Pedro Alves <palves@redhat.com> writes: > >> The patch also adds a new gdb.base/bp-permanent.exp test that >> exercises many different code paths related to stepping permanent >> breakpoints, including the stepping with signals cases. The test uses >> "hack/trick" to make it work on all (or most) platforms -- it doesn't >> really hard code a breakpoint instruction. > > It is great to test permanent breakpoint in an arch-independent way. It > exposes an existing issue on permanent breakpoint which is found when I Nice. That is catches bugs, that is. :-) > test your patch series on arm. breakpoint.c:bp_location_has_shadow > should return false for permanent breakpoint because permanent > breakpoint doesn't have shadow, otherwise, breakpoint_xfer_memory > destroys contents read from memory_xfer_partial_1 and the internal error > is triggered as below, I wonder why we don't see that on x86. > > (gdb) PASS: gdb.base/bp-permanent.exp: always_inserted=off, sw_watchpoint=0: stepi signal with handler: queue-signal SIGUSR1 > stepi^M > ../../../git/gdb/infrun.c:2112: internal-error: resume: Assertion `tp->control.step_resume_breakpoint->loc->permanent' failed.^M > A problem internal to GDB has been detected, > >> + else >> + { >> + /* There's no signal to pass. Skip the permanent >> + breakpoint, and arrange for any breakpoint at the new PC >> + to be reported back to handle_inferior_event. */ >> + >> + if (debug_infrun) >> + fprintf_unfiltered (gdb_stdlog, >> + "infrun: resume: skipping permanent breakpoint\n"); >> + gdbarch_skip_permanent_breakpoint (gdbarch, regcache); >> + /* Update pc to reflect the new address from which we will >> + execute instructions. */ >> + pc = regcache_read_pc (get_thread_regcache (inferior_ptid)); > > Is get_thread_regcache (inferior_ptid) equivalent to regcache? If it > is, we can use regcache which is shorter. Indeed. > >> + tp->prev_pc = pc; >> + >> + clear_step_over_info (); >> + tp->control.trap_expected = 0; >> + >> + /* There may or not be a breakpoint at the new address. If >> + we're stepping then we're done. Otherwise, make sure >> + there's a breakpoint at the current address, and let it >> + trigger. */ > > Sorry, I can't map the comments to the code. Is "Otherwise" in the > comments about the else branch which doesn't exist? Can you > elaborate? I'll do better and rewrite this piece of code. :-P That comment was referring to a somewhat earlier version of the patch, and doesn't really make sense. > >> + if (step) >> + { >> + /* If we already have a step-resume breakpoint set, then >> + we can just continue until that one is hit. */ > > When I read the comments, the code in my mind is like: > > if (tp->control.step_resume_breakpoint != NULL) > step = 0; > > which is different from your code below. Yeah, sorry about that. > >> + if (tp->control.step_resume_breakpoint == NULL) >> + insert_step_resume_breakpoint_at_frame (get_current_frame ()); >> + step = 0; I've thought this over some more, and realized two things: If there was already an outermost step-resume breakpoint set (we're already skipping a function for "next", and tripped on a permanent breakpoint), we shouldn't clear step, as that'll make us miss software watchpoints. We'd avoid this issue if we could have more than one step-resume breakpoint installed for the same thread, that isn't allowed for good reason. Thinking again, we have a better breakpoint type for this -- a software single-step breakpoint. We're implementing single-step ourselves, so sounds like a perfect fit. The only issue is that a couple paths in infrun.c we need are guarded by gdbarch_software_single_step_p. Those are optimization guards. Given we need that code on hardware step targets too, we just remove them... (I'll reindent in the final version.) >> + if ![target_info exists gdb,nosignals] { >> + > > We need also check can_single_step_to_signal_handler. Indeed. Let me know what you think of this version. ------ From 77e408373ac276bbdf193076b4a04a86400aecf4 Mon Sep 17 00:00:00 2001 From: Pedro Alves <palves@redhat.com> Date: Fri, 7 Nov 2014 19:09:58 +0000 Subject: [PATCH] fix skipping permanent breakpoints The gdb.arch/i386-bp_permanent.exp test is currently failing an assertion I've recently added: (gdb) stepi ../../src/gdb/infrun.c:2237: internal-error: resume: Assertion `sig != GDB_SIGNAL_0' failed. A problem internal to GDB has been detected, further debugging may prove unreliable. Quit this debugging session? (y or n) FAIL: gdb.arch/i386-bp_permanent.exp: Single stepping past permanent breakpoint. (GDB internal error) The assertion expects that the only reason we currently need to step a breakpoint instruction is when we have a signal to deliver. But when stepping a permanent breakpoint (with or without a signal) we also reach this code. IMO, the assertion is correct and the permanent breakpoints skipping code is wrong. Consider the case of the user doing "step/stepi" when stopped at a permanent breakpoint. GDB's `resume' calls the gdbarch_skip_permanent_breakpoint hook and then happily continues stepping: /* 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) { gdbarch_skip_permanent_breakpoint (gdbarch, regcache); } But since gdbarch_skip_permanent_breakpoint already advanced the PC manually, this ends up executing the instruction that is _after_ the breakpoint instruction. The user-visible result is that a single-step steps two instructions. The gdb.arch/i386-bp_permanent.exp test is actually ensuring that that's indeed how things work. It runs to an int3 instruction, does "stepi", and checks that "leave" was executed with that "stepi". Like this: (gdb) b *0x0804848c Breakpoint 2 at 0x804848c (gdb) c Continuing. Breakpoint 2, 0x0804848c in standard () (gdb) disassemble Dump of assembler code for function standard: 0x08048488 <+0>: push %ebp 0x08048489 <+1>: mov %esp,%ebp 0x0804848b <+3>: push %edi => 0x0804848c <+4>: int3 0x0804848d <+5>: leave 0x0804848e <+6>: ret 0x0804848f <+7>: nop (gdb) si 0x0804848e in standard () (gdb) disassemble Dump of assembler code for function standard: 0x08048488 <+0>: push %ebp 0x08048489 <+1>: mov %esp,%ebp 0x0804848b <+3>: push %edi 0x0804848c <+4>: int3 0x0804848d <+5>: leave => 0x0804848e <+6>: ret 0x0804848f <+7>: nop End of assembler dump. (gdb) I would instead expect that a stepi at 0x0804848c stops at 0x0804848d, _before_ the "leave" is executed. This commit changes GDB this way. Care is taken to make stepping into a signal handler when the step starts at a permanent breakpoint instruction work correctly. The patch adjusts gdb.arch/i386-bp_permanent.exp in this direction, and also makes it work on x86_64 (currently it only works on i*86). The patch also adds a new gdb.base/bp-permanent.exp test that exercises many different code paths related to stepping permanent breakpoints, including the stepping with signals cases. The test uses "hack/trick" to make it work on all (or most) platforms -- it doesn't really hard code a breakpoint instruction. Tested on x86_64 Fedora 20. gdb/ 2014-11-07 Pedro Alves <palves@redhat.com> * infrun.c (resume): Clear the thread's 'stepped_breakpoint' flag. Rewrite stepping over a permanent breakpoint. (thread_still_needs_step_over, proceed): Don't set stepping_over_breakpoint for permanent breakpoints. (handle_signal_stop): Don't clear stepped_breakpoint. Also pull single-step breakpoints out of the target on hardware step targets. (process_event_stop_test): If stepping a permanent breakpoint doesn't hit the step-resume breakpoint, delete the step-resume breakpoint. (switch_back_to_stepped_thread): Also check if the stepped thread has advanced already on hardware step targets. (currently_stepping): Return true if the thread stepped a breakpoint. gdb/testsuite/ 2014-11-07 Pedro Alves <palves@redhat.com> * gdb.arch/i386-bp_permanent.c: New file. * gdb.arch/i386-bp_permanent.exp: Don't skip on x86_64. (srcfile): Set to i386-bp_permanent.c. (top level): Adjust to work in both 32-bit and 64-bit modes. Test that stepi does not execute the 'leave' instruction, instead of testing it does execute. * gdb.base/bp-permanent.c: New file. * gdb.base/bp-permanent.exp: New file. --- gdb/infrun.c | 114 ++++++++++- gdb/testsuite/gdb.arch/i386-bp_permanent.c | 57 ++++++ gdb/testsuite/gdb.arch/i386-bp_permanent.exp | 49 ++--- gdb/testsuite/gdb.base/bp-permanent.c | 128 +++++++++++++ gdb/testsuite/gdb.base/bp-permanent.exp | 275 +++++++++++++++++++++++++++ 5 files changed, 588 insertions(+), 35 deletions(-) create mode 100644 gdb/testsuite/gdb.arch/i386-bp_permanent.c create mode 100644 gdb/testsuite/gdb.base/bp-permanent.c create mode 100644 gdb/testsuite/gdb.base/bp-permanent.exp diff --git a/gdb/infrun.c b/gdb/infrun.c index f57920f..ff338f7 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -2039,6 +2039,8 @@ resume (int step, enum gdb_signal sig) applies, it's the callers intention that counts. */ const int entry_step = step; + tp->stepped_breakpoint = 0; + QUIT; if (current_inferior ()->waiting_for_vfork_done) @@ -2075,7 +2077,81 @@ resume (int step, enum gdb_signal sig) breakpoints can't be removed. So we have to test for it here. */ if (breakpoint_here_p (aspace, pc) == permanent_breakpoint_here) { - gdbarch_skip_permanent_breakpoint (gdbarch, regcache); + if (sig != GDB_SIGNAL_0) + { + /* We have a signal to pass to the inferior. The resume + may, or may not take us to the signal handler. If this + is a step, we'll need to stop in the signal handler, if + there's one, (if the target supports stepping into + handlers), or in the next mainline instruction, if + there's no handler. If this is a continue, we need to be + sure to run the handler with all breakpoints inserted. + In all cases, set a breakpoint at the current address + (where the handler returns to), and once that breakpoint + is hit, resume skipping the permanent breakpoint. If + that breakpoint isn't hit, then we've stepped into the + signal handler (or hit some other event). We'll delete + the step-resume breakpoint then. */ + + if (debug_infrun) + fprintf_unfiltered (gdb_stdlog, + "infrun: resume: skipping permanent breakpoint, " + "deliver signal first\n"); + + clear_step_over_info (); + tp->control.trap_expected = 0; + + if (tp->control.step_resume_breakpoint == NULL) + { + /* Set a "high-priority" step-resume, as we don't want + user breakpoints at PC to trigger (again) when this + hits. */ + insert_hp_step_resume_breakpoint_at_frame (get_current_frame ()); + gdb_assert (tp->control.step_resume_breakpoint->loc->permanent); + + tp->step_after_step_resume_breakpoint = step; + } + + insert_breakpoints (); + } + else + { + /* There's no signal to pass, we can go ahead and skip the + permanent breakpoint manually. */ + if (debug_infrun) + fprintf_unfiltered (gdb_stdlog, + "infrun: resume: skipping permanent breakpoint\n"); + gdbarch_skip_permanent_breakpoint (gdbarch, regcache); + /* Update pc to reflect the new address from which we will + execute instructions. */ + pc = regcache_read_pc (regcache); + + if (step) + { + /* We've already advanced the PC, so the stepping part + is done. Now we need to arrange for a trap to be + reported to handle_inferior_event. Set a breakpoint + at the current PC, and run to it. Don't update + prev_pc, because if we end in + switch_back_to_stepping, we want the "expected thread + advanced also" branch to be taken. IOW, we don't + want this thread to step further from PC + (overstep). */ + insert_single_step_breakpoint (gdbarch, aspace, pc); + insert_breakpoints (); + + tp->suspend.stop_signal = GDB_SIGNAL_0; + /* We're continuing with all breakpoints inserted. It's + safe to let the target bypass signals. */ + target_pass_signals ((int) GDB_SIGNAL_LAST, signal_pass); + /* ... and safe to let other threads run, according to + schedlock. */ + resume_ptid = user_visible_resume_ptid (entry_step); + target_resume (resume_ptid, 0, GDB_SIGNAL_0); + discard_cleanups (old_cleanups); + return; + } + } } /* If we have a breakpoint to step over, make sure to do a single @@ -2382,7 +2458,8 @@ thread_still_needs_step_over (struct thread_info *tp) struct regcache *regcache = get_thread_regcache (tp->ptid); if (breakpoint_here_p (get_regcache_aspace (regcache), - regcache_read_pc (regcache))) + regcache_read_pc (regcache)) + == ordinary_breakpoint_here) return 1; tp->stepping_over_breakpoint = 0; @@ -2498,7 +2575,8 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step) if (addr == (CORE_ADDR) -1) { - if (pc == stop_pc && breakpoint_here_p (aspace, pc) + if (pc == stop_pc + && breakpoint_here_p (aspace, pc) == ordinary_breakpoint_here && execution_direction != EXEC_REVERSE) /* There is a breakpoint at the address we will resume at, step one instruction before inserting breakpoints so that @@ -4190,7 +4268,6 @@ handle_signal_stop (struct execution_control_state *ecs) gdbarch = get_frame_arch (frame); /* Pull the single step breakpoints out of the target. */ - if (gdbarch_software_single_step_p (gdbarch)) { if (ecs->event_thread->suspend.stop_signal == GDB_SIGNAL_TRAP) { @@ -4280,7 +4357,6 @@ handle_signal_stop (struct execution_control_state *ecs) return; } - ecs->event_thread->stepped_breakpoint = 0; ecs->event_thread->stepping_over_breakpoint = 0; ecs->event_thread->stepping_over_watchpoint = 0; bpstat_clear (&ecs->event_thread->control.stop_bpstat); @@ -4788,6 +4864,30 @@ process_event_stop_test (struct execution_control_state *ecs) break; } + /* If we stepped a permanent breakpoint and we had a high priority + step-resume breakpoint for the address we stepped, but we didn't + hit it, then we must have stepped into the signal handler. The + step-resume was only necessary to catch the case of _not_ + stepping into the handler, so delete it, and fall through to + checking whether the step finished. */ + if (ecs->event_thread->stepped_breakpoint) + { + struct breakpoint *sr_bp + = ecs->event_thread->control.step_resume_breakpoint; + + if (sr_bp->loc->permanent + && sr_bp->type == bp_hp_step_resume + && sr_bp->loc->address == ecs->event_thread->prev_pc) + { + if (debug_infrun) + fprintf_unfiltered (gdb_stdlog, + "infrun: stepped permanent breakpoint, stopped in " + "handler\n"); + delete_step_resume_breakpoint (ecs->event_thread); + ecs->event_thread->step_after_step_resume_breakpoint = 0; + } + } + /* We come here if we hit a breakpoint but should not stop for it. Possibly we also were stepping and should stop for that. So fall through and test for stepping. But, if not stepping, do not @@ -5552,8 +5652,7 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs) breakpoint forward, one instruction at a time, overstepping. */ - if (gdbarch_software_single_step_p (gdbarch) - && stop_pc != tp->prev_pc) + if (stop_pc != tp->prev_pc) { if (debug_infrun) fprintf_unfiltered (gdb_stdlog, @@ -5598,6 +5697,7 @@ currently_stepping (struct thread_info *tp) return ((tp->control.step_range_end && tp->control.step_resume_breakpoint == NULL) || tp->control.trap_expected + || tp->stepped_breakpoint || bpstat_should_step ()); } diff --git a/gdb/testsuite/gdb.arch/i386-bp_permanent.c b/gdb/testsuite/gdb.arch/i386-bp_permanent.c new file mode 100644 index 0000000..dc36d89 --- /dev/null +++ b/gdb/testsuite/gdb.arch/i386-bp_permanent.c @@ -0,0 +1,57 @@ +/* Copyright (C) 2003-2014 Free Software Foundation, Inc. + + This file is part of GDB. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +#ifdef SYMBOL_PREFIX +#define SYMBOL(str) SYMBOL_PREFIX #str +#else +#define SYMBOL(str) #str +#endif + +int +main (void) +{ + standard (); + return 0; +} + +/* A normal prologue. */ + +#ifdef __x86_64__ +asm(".text\n" + " .align 8\n" + SYMBOL (standard) ":\n" + " push %rbp\n" + " mov %rsp, %rbp\n" + " push %rdi\n" + " int3\n" + " leaveq\n" + " retq\n"); + +#else + +asm(".text\n" + " .align 8\n" + " .global " SYMBOL(standard) "\n" + SYMBOL (standard) ":\n" + " pushl %ebp\n" + " movl %esp, %ebp\n" + " pushl %edi\n" + " int3\n" + " leave\n" + " ret\n"); + +#endif diff --git a/gdb/testsuite/gdb.arch/i386-bp_permanent.exp b/gdb/testsuite/gdb.arch/i386-bp_permanent.exp index 6be1626..833b4ed 100644 --- a/gdb/testsuite/gdb.arch/i386-bp_permanent.exp +++ b/gdb/testsuite/gdb.arch/i386-bp_permanent.exp @@ -18,13 +18,13 @@ # Test stepping over permanent breakpoints on i386. -if { ![is_x86_like_target] } then { +if { ![istarget "i?86-*-*"] && ![istarget "x86_64-*-*"] } then { verbose "Skipping skip over permanent breakpoint on i386 tests." return } set testfile "i386-bp_permanent" -set srcfile i386-prologue.c +set srcfile i386-bp_permanent.c set binfile ${objdir}/${subdir}/${testfile} # some targets have leading underscores on assembly symbols. @@ -51,14 +51,12 @@ if ![runto_main] then { return -1 } -set function standard +set function "standard" set retcode [gdb_test_multiple "disassemble $function" "Disassemble function '$function'" { - -re ".*($hex) <\\+0>.*($hex) <\\+4>.*($hex) <\\+5>.*($hex) <\\+6>.*$gdb_prompt $" { - set function_start $expect_out(1,string) - set address $expect_out(2,string) - set address1 $expect_out(3,string) - set address2 $expect_out(4,string) + -re "($hex) <\\+0>.*($hex) <\\+$decimal>.*int3.*($hex) <\\+$decimal>.*leave.*$gdb_prompt $" { + set address_bp $expect_out(2,string) + set address_after_bp $expect_out(3,string) } }] @@ -67,30 +65,25 @@ if {$retcode != 0} { return -1 } -gdb_breakpoint "*$function_start" +gdb_breakpoint "*$address_bp" -gdb_breakpoint "*$address" +gdb_test "continue" "Breakpoint .*, $address_bp in $function.*" \ + "stop at permanent breakpoint" -gdb_test "continue" "Breakpoint .*, $function_start in $function.*" \ - "Stop at the '$function' start breakpoint (fetching esp)." - -# We want to fetch esp at the start of '$function' function to make sure -# skip_permanent_breakpoint implementation really skips only the perm. -# breakpoint. If, for whatever reason, 'leave' instruction doesn't get -# executed, esp will not have this value. -set start_esp 0 -gdb_test_multiple "print \$esp" "Fetch esp value." { +# We want to fetch the stack pointer at the start of '$function' +# function to make sure the skip_permanent_breakpoint implementation +# really skips only the permanent breakpoint. If, for whatever +# reason, the 'leave' instruction executes, the stack pointer will not +# have this value. +set start_sp 0 +gdb_test_multiple "print \$sp" "fetch stack pointer value" { -re "\\\$1.*($hex).*$gdb_prompt $" { - set start_esp $expect_out(1,string) + set start_sp $expect_out(1,string) } } -gdb_test "continue" "Breakpoint .*, $address in $function.*" \ - "Stop at permanent breakpoint." - -gdb_test "stepi" "$address1|$address2 in $function.*" \ - "Single stepping past permanent breakpoint." - -gdb_test "print \$esp" ".*$start_esp.*" \ - "ESP value does not match - step_permanent_breakpoint wrong." +gdb_test "stepi" "$address_after_bp in $function.*" \ + "single-step past permanent breakpoint" +gdb_test "print \$sp" ".*$start_sp.*" \ + "stack pointer value matches" diff --git a/gdb/testsuite/gdb.base/bp-permanent.c b/gdb/testsuite/gdb.base/bp-permanent.c new file mode 100644 index 0000000..53b3777 --- /dev/null +++ b/gdb/testsuite/gdb.base/bp-permanent.c @@ -0,0 +1,128 @@ +/* Copyright (C) 2014 Free Software Foundation, Inc. + + This file is part of GDB. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +#include <string.h> +#ifdef SIGNALS +#include <signal.h> +#endif + +#define NOP asm("nop") + +/* Buffer holding the breakpoint instruction. */ +unsigned char buffer[16]; + +volatile unsigned char *addr_bp; +volatile unsigned char *addr_after_bp; +int counter = 0; + +void +test (void) +{ + NOP; + NOP; + NOP; + NOP; /* write permanent bp here */ + NOP; /* after permanent bp */ + NOP; + NOP; + NOP; + NOP; + NOP; + + counter++; +} + +void +setup (void) +{ + memcpy (buffer, (void *) addr_bp, addr_after_bp - addr_bp); +} + +void +test_basics (void) +{ + test (); /* for SIGTRAP */ + test (); /* for breakpoint once */ + test (); /* for breakpoint twice */ + test (); /* for disabled bp SIGTRAP */ + test (); /* for breakpoint thrice */ +} + +void +test_next (void) +{ + test (); /* for next */ + counter = 0; /* after next */ +} + +#ifdef SIGNALS + +static void +test_signal_handler (int sig) +{ +} + +void +test_signal_with_handler (void) +{ + signal (SIGUSR1, test_signal_handler); + test (); +} + +void +test_signal_no_handler (void) +{ + signal (SIGUSR1, SIG_IGN); + test (); +} + +static void +test_signal_nested_handler () +{ + test (); +} + +void +test_signal_nested_done (void) +{ +} + +void +test_signal_nested (void) +{ + counter = 0; + signal (SIGALRM, test_signal_nested_handler); + alarm (1); + test (); + test_signal_nested_done (); +} + +#endif + +int +main (void) +{ + setup (); + test_basics (); + test_next (); +#ifdef SIGNALS + test_signal_nested (); + test_signal_with_handler (); + test_signal_no_handler (); +#endif + return 0; +} diff --git a/gdb/testsuite/gdb.base/bp-permanent.exp b/gdb/testsuite/gdb.base/bp-permanent.exp new file mode 100644 index 0000000..95db700 --- /dev/null +++ b/gdb/testsuite/gdb.base/bp-permanent.exp @@ -0,0 +1,275 @@ +# Copyright (C) 2014 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +# This file is part of the gdb testsuite. + +# Test stepping over permanent breakpoints. + +standard_testfile + +set options { debug } +if { ![target_info exists gdb,nosignals] } { + lappend options "additional_flags=-DSIGNALS" +} + +if {[build_executable "failed to prepare" $testfile $srcfile $options]} { + return -1 +} + +set line_bp [gdb_get_line_number "write permanent bp"] + +# The test proper. ALWAYS_INSERTED indicates whether testing in +# "breakpoint always-inserted" mode. If SW_WATCHPOINT is true, set a +# software watchpoint, which forces constantly single-stepping, and +# exercises stepping the permanent breakpoint while delivering a +# signal at the same time. + +proc test {always_inserted sw_watchpoint} { + global line_bp + global hex decimal + global gdb_prompt + global srcfile binfile + + clean_restart $binfile + + if ![runto_main] then { + return -1 + } + + gdb_test "set breakpoint always-inserted $always_inserted" + + if {$sw_watchpoint} { + # Watching a convenience variable forces a software + # watchpoint. + gdb_test "watch \$dummy_convenience" "Watchpoint .*" + } + + set address_bp "" + set address_after_bp "" + + with_test_prefix "setup" { + + # Set a breakpoint where we'll manually plant a permanent + # breakpoint. + set test "set probe breakpoint" + gdb_test_multiple "break $line_bp" $test { + -re "Breakpoint .* at ($hex).*$gdb_prompt $" { + set address_bp $expect_out(1,string) + pass $test + } + } + if {$address_bp == ""} { + return + } + + # Get the size of the instruction where the breakpoint will + # manually inserted. + set test "get size of instruction" + gdb_test_multiple "x/2i $address_bp" $test { + -re ".*$hex <test\\+$decimal>:\[^\r\n\]+\r\n\[ \]+($hex).*\.\r\n$gdb_prompt $" { + set address_after_bp $expect_out(1,string) + pass $test + } + } + if {$address_after_bp == ""} { + return + } + + # Write address range where the breakpoint is inserted to the + # corresponding variables in the inferior. + gdb_test "p /x addr_bp = $address_bp" " = $address_bp" \ + "write addr_bp" + gdb_test "p /x addr_after_bp = $address_after_bp" " = $address_after_bp" \ + "write addr_after_bp" + + # Run the "setup" function in the inferior. This memcpy's the + # breakpoint instruction to a buffer in the inferior. + gdb_test "next" "test.*" "next over setup" + + delete_breakpoints + + # We now have the breakpoint instruction stored in 'buffer'. Poke it + # to memory manually. + set count [expr $address_after_bp - $address_bp] + for {set i 0} {$i < $count} {incr i} { + gdb_test "p /x addr_bp\[$i\] = buffer\[$i\]" " = .*" + } + } + + with_test_prefix "basics" { + # Run to the permanent breakpoint, just to make sure we've inserted it + # correctly. + gdb_test "continue" "Program received signal SIGTRAP.*" \ + "permanent breakpoint causes random signal" + + # Now set a breakpoint on top, thus creating a permanent breakpoint. + gdb_breakpoint "$line_bp" + + # Depending on whether this is a decr_pc_after_break arch, the PC will + # be either pointing at the permanent breakpoint address, or just + # after. Set the GDB breakpoint on top, and continue, twice. At + # least once, GDB will need to step-over the permanent breakpoint. + + gdb_test "continue" "Breakpoint .*" "stop at permanent breakpoint" + + gdb_test "p \$prev_counter = counter" " = $decimal" + + gdb_test "continue" "Breakpoint .*" "stop at permanent breakpoint twice" + + # Check that indeed the continue made progress, instead of re-trapping + # without advancing. + gdb_test "p counter - \$prev_counter" " = 1" + + gdb_test "info breakpoints" \ + "breakpoint.*keep.*y.*$hex.*in test at .*$srcfile:$line_bp.*already hit 2 times.*" \ + "info breakpoints show enabled breakpoint" + + gdb_test "disable \$bpnum" + + gdb_test "commands\nset \$commands_ran = 1\nend" "" \ + "set breakpoint commands" + + gdb_test "info breakpoints" \ + "breakpoint.*keep.*n.*$hex.*in test at .*$srcfile:$line_bp.*already hit 2 times.*" \ + "info breakpoints shows disabled breakpoint" + + # Run to the permanent breakpoint again. This time, since it's + # disabled, it should act as if we hadn't created it in the first + # place. IOW, we should get a random signal, and, the breakpoint's + # command should not run. + gdb_test "continue" "Program received signal SIGTRAP.*" \ + "disabled permanent breakpoint doesn't explain stop" + + gdb_test "info breakpoints" \ + "breakpoint.*keep.*n.*$hex.*in test at .*$srcfile:$line_bp.*already hit 2 times.*" \ + "info breakpoints still shows same number of hits" + + gdb_test "print \$commands_ran" " = void" \ + "breakpoint commands didn't run" + + # Reenable the breakpoint, and check that it gets hit and accounted + # for this time. + gdb_test "enable \$bpnum" "" "reenable breakpoint" + + gdb_test "continue" "Breakpoint .*" \ + "stop at permanent breakpoint thrice" + + gdb_test "info breakpoints" \ + "breakpoint.*keep.*y.*$hex.*in test at .*$srcfile:$line_bp.*already hit 3 times.*" \ + "info breakpoints shows one more hit" + + gdb_test "print \$commands_ran" " = 1" "breakpoint commands ran" + + # Check that stepi advances only past the permanent breakpoint, and + # not a single instruction more. + gdb_test "stepi" "after permanent bp .*" \ + "single-step past permanent breakpoint" + } + + with_test_prefix "next trips on permanent bp" { + delete_breakpoints + + gdb_breakpoint "test_next" + gdb_continue_to_breakpoint "test_next" + + gdb_breakpoint "$line_bp" + gdb_test "condition \$bpnum 0" + + gdb_test "next" "after next .*" + } + + if ![target_info exists gdb,nosignals] { + + with_test_prefix "continue trips on nested permanent bp" { + delete_breakpoints + + gdb_breakpoint "test_signal_nested" + gdb_continue_to_breakpoint "test_signal_nested" + + gdb_breakpoint "$line_bp" + gdb_continue_to_breakpoint "permanent bp" + gdb_test "condition \$bpnum 0" + + # Let SIGALRM trigger. + sleep 2 + + # We're now stopped at a permanent breakpoint, with a + # signal pending. + gdb_breakpoint "test_signal_nested_done" + gdb_continue_to_breakpoint "test_signal_nested_done" + + # Ensure that the handler did run. There's one call to + # test in the mainline code, and another in the signal + # handler. + gdb_test "p counter" " = 2" + } + + if [can_single_step_to_signal_handler] { + + with_test_prefix "stepi signal with handler" { + delete_breakpoints + + gdb_breakpoint "test_signal_with_handler" + gdb_continue_to_breakpoint "test_signal_with_handler" + + gdb_breakpoint "$line_bp" + + gdb_test "continue" "Breakpoint .*" "stop at permanent breakpoint" + + gdb_test "queue-signal SIGUSR1" + + set test "single-step to handler" + gdb_test_multiple "stepi" $test { + -re "Program received signal SIGTRAP.*$gdb_prompt $" { + fail $test + } + -re "handler .*$gdb_prompt $" { + pass $test + } + } + + # Check that the mainline PC points at the permanent + # breakpoint. + gdb_test "up 2" "test .*" "up to mainline code" + + gdb_test "p /x \$pc" " = $address_bp" \ + "mainline pc points at permanent breakpoint" + + gdb_test "continue" "Breakpoint .*" \ + "stop at permanent breakpoint, out of handler" + } + + with_test_prefix "stepi signal with no handler" { + gdb_breakpoint "test_signal_no_handler" + gdb_continue_to_breakpoint "test_signal_no_handler" + + gdb_test "continue" "Breakpoint .*" "stop at permanent breakpoint" + + gdb_test "queue-signal SIGUSR1" + + gdb_test "stepi" "after permanent bp .*" \ + "single-step past permanent breakpoint" + } + } + } +} + +foreach always_inserted {off on} { + foreach sw_watchpoint {0 1} { + with_test_prefix "always_inserted=$always_inserted, sw_watchpoint=$sw_watchpoint" { + test $always_inserted $sw_watchpoint + } + } +} -- 1.9.3 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] fix skipping permanent breakpoints 2014-11-07 19:53 ` Pedro Alves @ 2014-11-12 0:54 ` Yao Qi 2014-11-12 10:53 ` Pedro Alves 0 siblings, 1 reply; 13+ messages in thread From: Yao Qi @ 2014-11-12 0:54 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches Pedro Alves <palves@redhat.com> writes: > If there was already an outermost step-resume breakpoint set (we're > already skipping a function for "next", and tripped on a permanent > breakpoint), we shouldn't clear step, as that'll make us miss > software watchpoints. > > We'd avoid this issue if we could have more than one step-resume > breakpoint installed for the same thread, that isn't allowed for > good reason. Thinking again, we have a better breakpoint type > for this -- a software single-step breakpoint. We're implementing > single-step ourselves, so sounds like a perfect fit. The only > issue is that a couple paths in infrun.c we need are guarded by > gdbarch_software_single_step_p. Those are optimization guards. > Given we need that code on hardware step targets too, we just > remove them... (I'll reindent in the final version.) Yeah, it is clever to use single-step breakpoint here. This version looks good enough to me, although I've seen some fails on arm. I don't triage them at this moment, and we can revisit them later. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] fix skipping permanent breakpoints 2014-11-12 0:54 ` Yao Qi @ 2014-11-12 10:53 ` Pedro Alves 0 siblings, 0 replies; 13+ messages in thread From: Pedro Alves @ 2014-11-12 10:53 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On 11/12/2014 12:54 AM, Yao Qi wrote: > Pedro Alves <palves@redhat.com> writes: > >> If there was already an outermost step-resume breakpoint set (we're >> already skipping a function for "next", and tripped on a permanent >> breakpoint), we shouldn't clear step, as that'll make us miss >> software watchpoints. >> >> We'd avoid this issue if we could have more than one step-resume >> breakpoint installed for the same thread, that isn't allowed for >> good reason. Thinking again, we have a better breakpoint type >> for this -- a software single-step breakpoint. We're implementing >> single-step ourselves, so sounds like a perfect fit. The only >> issue is that a couple paths in infrun.c we need are guarded by >> gdbarch_software_single_step_p. Those are optimization guards. >> Given we need that code on hardware step targets too, we just >> remove them... (I'll reindent in the final version.) > > Yeah, it is clever to use single-step breakpoint here. This version > looks good enough to me, although I've seen some fails on arm. I don't > triage them at this moment, and we can revisit them later. > Thanks. Hopefully those fails will be related to the latent bug in the breakpoint shadows you saw before. I wondered whether those were related to software single-step, so I retested the series on top of my palves/x86_software_single_step branch as well, both native and gdbserver, but I see no regressions even then, and the new tests pass too. Here's the version that I pushed, now with the reindent. ------ From af48d08f97fa678571a42be35a8a77b61e36d7d7 Mon Sep 17 00:00:00 2001 From: Pedro Alves <palves@redhat.com> Date: Wed, 12 Nov 2014 10:10:49 +0000 Subject: [PATCH 3/3] fix skipping permanent breakpoints The gdb.arch/i386-bp_permanent.exp test is currently failing an assertion recently added: (gdb) stepi ../../src/gdb/infrun.c:2237: internal-error: resume: Assertion `sig != GDB_SIGNAL_0' failed. A problem internal to GDB has been detected, further debugging may prove unreliable. Quit this debugging session? (y or n) FAIL: gdb.arch/i386-bp_permanent.exp: Single stepping past permanent breakpoint. (GDB internal error) The assertion expects that the only reason we currently need to step a breakpoint instruction is when we have a signal to deliver. But when stepping a permanent breakpoint (with or without a signal) we also reach this code. The assertion is correct and the permanent breakpoints skipping code is wrong. Consider the case of the user doing "step/stepi" when stopped at a permanent breakpoint. GDB's `resume' calls the gdbarch_skip_permanent_breakpoint hook and then happily continues stepping: /* 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) { gdbarch_skip_permanent_breakpoint (gdbarch, regcache); } But since gdbarch_skip_permanent_breakpoint already advanced the PC manually, this ends up executing the instruction that is _after_ the breakpoint instruction. The user-visible result is that a single-step steps two instructions. The gdb.arch/i386-bp_permanent.exp test is actually ensuring that that's indeed how things work. It runs to an int3 instruction, does "stepi", and checks that "leave" was executed with that "stepi". Like this: (gdb) b *0x0804848c Breakpoint 2 at 0x804848c (gdb) c Continuing. Breakpoint 2, 0x0804848c in standard () (gdb) disassemble Dump of assembler code for function standard: 0x08048488 <+0>: push %ebp 0x08048489 <+1>: mov %esp,%ebp 0x0804848b <+3>: push %edi => 0x0804848c <+4>: int3 0x0804848d <+5>: leave 0x0804848e <+6>: ret 0x0804848f <+7>: nop (gdb) si 0x0804848e in standard () (gdb) disassemble Dump of assembler code for function standard: 0x08048488 <+0>: push %ebp 0x08048489 <+1>: mov %esp,%ebp 0x0804848b <+3>: push %edi 0x0804848c <+4>: int3 0x0804848d <+5>: leave => 0x0804848e <+6>: ret 0x0804848f <+7>: nop End of assembler dump. (gdb) One would instead expect that a stepi at 0x0804848c stops at 0x0804848d, _before_ the "leave" is executed. This commit changes GDB this way. Care is taken to make stepping into a signal handler when the step starts at a permanent breakpoint instruction work correctly. The patch adjusts gdb.arch/i386-bp_permanent.exp in this direction, and also makes it work on x86_64 (currently it only works on i*86). The patch also adds a new gdb.base/bp-permanent.exp test that exercises many different code paths related to stepping permanent breakpoints, including the stepping with signals cases. The test uses "hack/trick" to make it work on all (or most) platforms -- it doesn't really hard code a breakpoint instruction. Tested on x86_64 Fedora 20, native and gdbserver. gdb/ 2014-11-12 Pedro Alves <palves@redhat.com> * infrun.c (resume): Clear the thread's 'stepped_breakpoint' flag. Rewrite stepping over a permanent breakpoint. (thread_still_needs_step_over, proceed): Don't set stepping_over_breakpoint for permanent breakpoints. (handle_signal_stop): Don't clear stepped_breakpoint. Also pull single-step breakpoints out of the target on hardware step targets. (process_event_stop_test): If stepping a permanent breakpoint doesn't hit the step-resume breakpoint, delete the step-resume breakpoint. (switch_back_to_stepped_thread): Also check if the stepped thread has advanced already on hardware step targets. (currently_stepping): Return true if the thread stepped a breakpoint. gdb/testsuite/ 2014-11-12 Pedro Alves <palves@redhat.com> * gdb.arch/i386-bp_permanent.c: New file. * gdb.arch/i386-bp_permanent.exp: Don't skip on x86_64. (srcfile): Set to i386-bp_permanent.c. (top level): Adjust to work in both 32-bit and 64-bit modes. Test that stepi does not execute the 'leave' instruction, instead of testing it does execute. * gdb.base/bp-permanent.c: New file. * gdb.base/bp-permanent.exp: New file. --- gdb/ChangeLog | 17 ++ gdb/testsuite/ChangeLog | 11 ++ gdb/infrun.c | 171 +++++++++++++---- gdb/testsuite/gdb.arch/i386-bp_permanent.c | 57 ++++++ gdb/testsuite/gdb.arch/i386-bp_permanent.exp | 49 ++--- gdb/testsuite/gdb.base/bp-permanent.c | 128 +++++++++++++ gdb/testsuite/gdb.base/bp-permanent.exp | 275 +++++++++++++++++++++++++++ 7 files changed, 643 insertions(+), 65 deletions(-) create mode 100644 gdb/testsuite/gdb.arch/i386-bp_permanent.c create mode 100644 gdb/testsuite/gdb.base/bp-permanent.c create mode 100644 gdb/testsuite/gdb.base/bp-permanent.exp diff --git a/gdb/ChangeLog b/gdb/ChangeLog index a50f67e..43bd9b7 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,5 +1,22 @@ 2014-11-12 Pedro Alves <palves@redhat.com> + * infrun.c (resume): Clear the thread's 'stepped_breakpoint' flag. + Rewrite stepping over a permanent breakpoint. + (thread_still_needs_step_over, proceed): Don't set + stepping_over_breakpoint for permanent breakpoints. + (handle_signal_stop): Don't clear stepped_breakpoint. Also pull + single-step breakpoints out of the target on hardware step + targets. + (process_event_stop_test): If stepping a permanent breakpoint + doesn't hit the step-resume breakpoint, delete the step-resume + breakpoint. + (switch_back_to_stepped_thread): Also check if the stepped thread + has advanced already on hardware step targets. + (currently_stepping): Return true if the thread stepped a + breakpoint. + +2014-11-12 Pedro Alves <palves@redhat.com> + Mark locations as permanent, not the whole breakpoint. * breakpoint.c (remove_breakpoint_1, remove_breakpoint): Adjust. (mark_breakpoints_out): Don't mark permanent breakpoints as diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog index 898a910..37d8290 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,14 @@ +2014-11-12 Pedro Alves <palves@redhat.com> + + * gdb.arch/i386-bp_permanent.c: New file. + * gdb.arch/i386-bp_permanent.exp: Don't skip on x86_64. + (srcfile): Set to i386-bp_permanent.c. + (top level): Adjust to work in both 32-bit and 64-bit modes. Test + that stepi does not execute the 'leave' instruction, instead of + testing it does execute. + * gdb.base/bp-permanent.c: New file. + * gdb.base/bp-permanent.exp: New file. + 2014-11-10 Doug Evans <xdje42@gmail.com> PR symtab/17564 diff --git a/gdb/infrun.c b/gdb/infrun.c index f57920f..0dcc141 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -2039,6 +2039,8 @@ resume (int step, enum gdb_signal sig) applies, it's the callers intention that counts. */ const int entry_step = step; + tp->stepped_breakpoint = 0; + QUIT; if (current_inferior ()->waiting_for_vfork_done) @@ -2075,7 +2077,81 @@ resume (int step, enum gdb_signal sig) breakpoints can't be removed. So we have to test for it here. */ if (breakpoint_here_p (aspace, pc) == permanent_breakpoint_here) { - gdbarch_skip_permanent_breakpoint (gdbarch, regcache); + if (sig != GDB_SIGNAL_0) + { + /* We have a signal to pass to the inferior. The resume + may, or may not take us to the signal handler. If this + is a step, we'll need to stop in the signal handler, if + there's one, (if the target supports stepping into + handlers), or in the next mainline instruction, if + there's no handler. If this is a continue, we need to be + sure to run the handler with all breakpoints inserted. + In all cases, set a breakpoint at the current address + (where the handler returns to), and once that breakpoint + is hit, resume skipping the permanent breakpoint. If + that breakpoint isn't hit, then we've stepped into the + signal handler (or hit some other event). We'll delete + the step-resume breakpoint then. */ + + if (debug_infrun) + fprintf_unfiltered (gdb_stdlog, + "infrun: resume: skipping permanent breakpoint, " + "deliver signal first\n"); + + clear_step_over_info (); + tp->control.trap_expected = 0; + + if (tp->control.step_resume_breakpoint == NULL) + { + /* Set a "high-priority" step-resume, as we don't want + user breakpoints at PC to trigger (again) when this + hits. */ + insert_hp_step_resume_breakpoint_at_frame (get_current_frame ()); + gdb_assert (tp->control.step_resume_breakpoint->loc->permanent); + + tp->step_after_step_resume_breakpoint = step; + } + + insert_breakpoints (); + } + else + { + /* There's no signal to pass, we can go ahead and skip the + permanent breakpoint manually. */ + if (debug_infrun) + fprintf_unfiltered (gdb_stdlog, + "infrun: resume: skipping permanent breakpoint\n"); + gdbarch_skip_permanent_breakpoint (gdbarch, regcache); + /* Update pc to reflect the new address from which we will + execute instructions. */ + pc = regcache_read_pc (regcache); + + if (step) + { + /* We've already advanced the PC, so the stepping part + is done. Now we need to arrange for a trap to be + reported to handle_inferior_event. Set a breakpoint + at the current PC, and run to it. Don't update + prev_pc, because if we end in + switch_back_to_stepping, we want the "expected thread + advanced also" branch to be taken. IOW, we don't + want this thread to step further from PC + (overstep). */ + insert_single_step_breakpoint (gdbarch, aspace, pc); + insert_breakpoints (); + + tp->suspend.stop_signal = GDB_SIGNAL_0; + /* We're continuing with all breakpoints inserted. It's + safe to let the target bypass signals. */ + target_pass_signals ((int) GDB_SIGNAL_LAST, signal_pass); + /* ... and safe to let other threads run, according to + schedlock. */ + resume_ptid = user_visible_resume_ptid (entry_step); + target_resume (resume_ptid, 0, GDB_SIGNAL_0); + discard_cleanups (old_cleanups); + return; + } + } } /* If we have a breakpoint to step over, make sure to do a single @@ -2382,7 +2458,8 @@ thread_still_needs_step_over (struct thread_info *tp) struct regcache *regcache = get_thread_regcache (tp->ptid); if (breakpoint_here_p (get_regcache_aspace (regcache), - regcache_read_pc (regcache))) + regcache_read_pc (regcache)) + == ordinary_breakpoint_here) return 1; tp->stepping_over_breakpoint = 0; @@ -2498,7 +2575,8 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step) if (addr == (CORE_ADDR) -1) { - if (pc == stop_pc && breakpoint_here_p (aspace, pc) + if (pc == stop_pc + && breakpoint_here_p (aspace, pc) == ordinary_breakpoint_here && execution_direction != EXEC_REVERSE) /* There is a breakpoint at the address we will resume at, step one instruction before inserting breakpoints so that @@ -4190,50 +4268,46 @@ handle_signal_stop (struct execution_control_state *ecs) gdbarch = get_frame_arch (frame); /* Pull the single step breakpoints out of the target. */ - if (gdbarch_software_single_step_p (gdbarch)) + if (ecs->event_thread->suspend.stop_signal == GDB_SIGNAL_TRAP) { - if (ecs->event_thread->suspend.stop_signal == GDB_SIGNAL_TRAP) - { - struct regcache *regcache; - struct address_space *aspace; - CORE_ADDR pc; + struct regcache *regcache; + struct address_space *aspace; + CORE_ADDR pc; - regcache = get_thread_regcache (ecs->ptid); - aspace = get_regcache_aspace (regcache); - pc = regcache_read_pc (regcache); + regcache = get_thread_regcache (ecs->ptid); + aspace = get_regcache_aspace (regcache); + pc = regcache_read_pc (regcache); - /* However, before doing so, if this single-step breakpoint was - actually for another thread, set this thread up for moving - past it. */ - if (!thread_has_single_step_breakpoint_here (ecs->event_thread, - aspace, pc)) - { - if (single_step_breakpoint_inserted_here_p (aspace, pc)) - { - if (debug_infrun) - { - fprintf_unfiltered (gdb_stdlog, - "infrun: [%s] hit another thread's " - "single-step breakpoint\n", - target_pid_to_str (ecs->ptid)); - } - ecs->hit_singlestep_breakpoint = 1; - } - } - else + /* However, before doing so, if this single-step breakpoint was + actually for another thread, set this thread up for moving + past it. */ + if (!thread_has_single_step_breakpoint_here (ecs->event_thread, + aspace, pc)) + { + if (single_step_breakpoint_inserted_here_p (aspace, pc)) { if (debug_infrun) { fprintf_unfiltered (gdb_stdlog, - "infrun: [%s] hit its " + "infrun: [%s] hit another thread's " "single-step breakpoint\n", target_pid_to_str (ecs->ptid)); } + ecs->hit_singlestep_breakpoint = 1; + } + } + else + { + if (debug_infrun) + { + fprintf_unfiltered (gdb_stdlog, + "infrun: [%s] hit its " + "single-step breakpoint\n", + target_pid_to_str (ecs->ptid)); } } - - delete_just_stopped_threads_single_step_breakpoints (); } + delete_just_stopped_threads_single_step_breakpoints (); if (ecs->event_thread->suspend.stop_signal == GDB_SIGNAL_TRAP && ecs->event_thread->control.trap_expected @@ -4280,7 +4354,6 @@ handle_signal_stop (struct execution_control_state *ecs) return; } - ecs->event_thread->stepped_breakpoint = 0; ecs->event_thread->stepping_over_breakpoint = 0; ecs->event_thread->stepping_over_watchpoint = 0; bpstat_clear (&ecs->event_thread->control.stop_bpstat); @@ -4788,6 +4861,30 @@ process_event_stop_test (struct execution_control_state *ecs) break; } + /* If we stepped a permanent breakpoint and we had a high priority + step-resume breakpoint for the address we stepped, but we didn't + hit it, then we must have stepped into the signal handler. The + step-resume was only necessary to catch the case of _not_ + stepping into the handler, so delete it, and fall through to + checking whether the step finished. */ + if (ecs->event_thread->stepped_breakpoint) + { + struct breakpoint *sr_bp + = ecs->event_thread->control.step_resume_breakpoint; + + if (sr_bp->loc->permanent + && sr_bp->type == bp_hp_step_resume + && sr_bp->loc->address == ecs->event_thread->prev_pc) + { + if (debug_infrun) + fprintf_unfiltered (gdb_stdlog, + "infrun: stepped permanent breakpoint, stopped in " + "handler\n"); + delete_step_resume_breakpoint (ecs->event_thread); + ecs->event_thread->step_after_step_resume_breakpoint = 0; + } + } + /* We come here if we hit a breakpoint but should not stop for it. Possibly we also were stepping and should stop for that. So fall through and test for stepping. But, if not stepping, do not @@ -5552,8 +5649,7 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs) breakpoint forward, one instruction at a time, overstepping. */ - if (gdbarch_software_single_step_p (gdbarch) - && stop_pc != tp->prev_pc) + if (stop_pc != tp->prev_pc) { if (debug_infrun) fprintf_unfiltered (gdb_stdlog, @@ -5598,6 +5694,7 @@ currently_stepping (struct thread_info *tp) return ((tp->control.step_range_end && tp->control.step_resume_breakpoint == NULL) || tp->control.trap_expected + || tp->stepped_breakpoint || bpstat_should_step ()); } diff --git a/gdb/testsuite/gdb.arch/i386-bp_permanent.c b/gdb/testsuite/gdb.arch/i386-bp_permanent.c new file mode 100644 index 0000000..dc36d89 --- /dev/null +++ b/gdb/testsuite/gdb.arch/i386-bp_permanent.c @@ -0,0 +1,57 @@ +/* Copyright (C) 2003-2014 Free Software Foundation, Inc. + + This file is part of GDB. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +#ifdef SYMBOL_PREFIX +#define SYMBOL(str) SYMBOL_PREFIX #str +#else +#define SYMBOL(str) #str +#endif + +int +main (void) +{ + standard (); + return 0; +} + +/* A normal prologue. */ + +#ifdef __x86_64__ +asm(".text\n" + " .align 8\n" + SYMBOL (standard) ":\n" + " push %rbp\n" + " mov %rsp, %rbp\n" + " push %rdi\n" + " int3\n" + " leaveq\n" + " retq\n"); + +#else + +asm(".text\n" + " .align 8\n" + " .global " SYMBOL(standard) "\n" + SYMBOL (standard) ":\n" + " pushl %ebp\n" + " movl %esp, %ebp\n" + " pushl %edi\n" + " int3\n" + " leave\n" + " ret\n"); + +#endif diff --git a/gdb/testsuite/gdb.arch/i386-bp_permanent.exp b/gdb/testsuite/gdb.arch/i386-bp_permanent.exp index 6be1626..833b4ed 100644 --- a/gdb/testsuite/gdb.arch/i386-bp_permanent.exp +++ b/gdb/testsuite/gdb.arch/i386-bp_permanent.exp @@ -18,13 +18,13 @@ # Test stepping over permanent breakpoints on i386. -if { ![is_x86_like_target] } then { +if { ![istarget "i?86-*-*"] && ![istarget "x86_64-*-*"] } then { verbose "Skipping skip over permanent breakpoint on i386 tests." return } set testfile "i386-bp_permanent" -set srcfile i386-prologue.c +set srcfile i386-bp_permanent.c set binfile ${objdir}/${subdir}/${testfile} # some targets have leading underscores on assembly symbols. @@ -51,14 +51,12 @@ if ![runto_main] then { return -1 } -set function standard +set function "standard" set retcode [gdb_test_multiple "disassemble $function" "Disassemble function '$function'" { - -re ".*($hex) <\\+0>.*($hex) <\\+4>.*($hex) <\\+5>.*($hex) <\\+6>.*$gdb_prompt $" { - set function_start $expect_out(1,string) - set address $expect_out(2,string) - set address1 $expect_out(3,string) - set address2 $expect_out(4,string) + -re "($hex) <\\+0>.*($hex) <\\+$decimal>.*int3.*($hex) <\\+$decimal>.*leave.*$gdb_prompt $" { + set address_bp $expect_out(2,string) + set address_after_bp $expect_out(3,string) } }] @@ -67,30 +65,25 @@ if {$retcode != 0} { return -1 } -gdb_breakpoint "*$function_start" +gdb_breakpoint "*$address_bp" -gdb_breakpoint "*$address" +gdb_test "continue" "Breakpoint .*, $address_bp in $function.*" \ + "stop at permanent breakpoint" -gdb_test "continue" "Breakpoint .*, $function_start in $function.*" \ - "Stop at the '$function' start breakpoint (fetching esp)." - -# We want to fetch esp at the start of '$function' function to make sure -# skip_permanent_breakpoint implementation really skips only the perm. -# breakpoint. If, for whatever reason, 'leave' instruction doesn't get -# executed, esp will not have this value. -set start_esp 0 -gdb_test_multiple "print \$esp" "Fetch esp value." { +# We want to fetch the stack pointer at the start of '$function' +# function to make sure the skip_permanent_breakpoint implementation +# really skips only the permanent breakpoint. If, for whatever +# reason, the 'leave' instruction executes, the stack pointer will not +# have this value. +set start_sp 0 +gdb_test_multiple "print \$sp" "fetch stack pointer value" { -re "\\\$1.*($hex).*$gdb_prompt $" { - set start_esp $expect_out(1,string) + set start_sp $expect_out(1,string) } } -gdb_test "continue" "Breakpoint .*, $address in $function.*" \ - "Stop at permanent breakpoint." - -gdb_test "stepi" "$address1|$address2 in $function.*" \ - "Single stepping past permanent breakpoint." - -gdb_test "print \$esp" ".*$start_esp.*" \ - "ESP value does not match - step_permanent_breakpoint wrong." +gdb_test "stepi" "$address_after_bp in $function.*" \ + "single-step past permanent breakpoint" +gdb_test "print \$sp" ".*$start_sp.*" \ + "stack pointer value matches" diff --git a/gdb/testsuite/gdb.base/bp-permanent.c b/gdb/testsuite/gdb.base/bp-permanent.c new file mode 100644 index 0000000..53b3777 --- /dev/null +++ b/gdb/testsuite/gdb.base/bp-permanent.c @@ -0,0 +1,128 @@ +/* Copyright (C) 2014 Free Software Foundation, Inc. + + This file is part of GDB. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +#include <string.h> +#ifdef SIGNALS +#include <signal.h> +#endif + +#define NOP asm("nop") + +/* Buffer holding the breakpoint instruction. */ +unsigned char buffer[16]; + +volatile unsigned char *addr_bp; +volatile unsigned char *addr_after_bp; +int counter = 0; + +void +test (void) +{ + NOP; + NOP; + NOP; + NOP; /* write permanent bp here */ + NOP; /* after permanent bp */ + NOP; + NOP; + NOP; + NOP; + NOP; + + counter++; +} + +void +setup (void) +{ + memcpy (buffer, (void *) addr_bp, addr_after_bp - addr_bp); +} + +void +test_basics (void) +{ + test (); /* for SIGTRAP */ + test (); /* for breakpoint once */ + test (); /* for breakpoint twice */ + test (); /* for disabled bp SIGTRAP */ + test (); /* for breakpoint thrice */ +} + +void +test_next (void) +{ + test (); /* for next */ + counter = 0; /* after next */ +} + +#ifdef SIGNALS + +static void +test_signal_handler (int sig) +{ +} + +void +test_signal_with_handler (void) +{ + signal (SIGUSR1, test_signal_handler); + test (); +} + +void +test_signal_no_handler (void) +{ + signal (SIGUSR1, SIG_IGN); + test (); +} + +static void +test_signal_nested_handler () +{ + test (); +} + +void +test_signal_nested_done (void) +{ +} + +void +test_signal_nested (void) +{ + counter = 0; + signal (SIGALRM, test_signal_nested_handler); + alarm (1); + test (); + test_signal_nested_done (); +} + +#endif + +int +main (void) +{ + setup (); + test_basics (); + test_next (); +#ifdef SIGNALS + test_signal_nested (); + test_signal_with_handler (); + test_signal_no_handler (); +#endif + return 0; +} diff --git a/gdb/testsuite/gdb.base/bp-permanent.exp b/gdb/testsuite/gdb.base/bp-permanent.exp new file mode 100644 index 0000000..95db700 --- /dev/null +++ b/gdb/testsuite/gdb.base/bp-permanent.exp @@ -0,0 +1,275 @@ +# Copyright (C) 2014 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +# This file is part of the gdb testsuite. + +# Test stepping over permanent breakpoints. + +standard_testfile + +set options { debug } +if { ![target_info exists gdb,nosignals] } { + lappend options "additional_flags=-DSIGNALS" +} + +if {[build_executable "failed to prepare" $testfile $srcfile $options]} { + return -1 +} + +set line_bp [gdb_get_line_number "write permanent bp"] + +# The test proper. ALWAYS_INSERTED indicates whether testing in +# "breakpoint always-inserted" mode. If SW_WATCHPOINT is true, set a +# software watchpoint, which forces constantly single-stepping, and +# exercises stepping the permanent breakpoint while delivering a +# signal at the same time. + +proc test {always_inserted sw_watchpoint} { + global line_bp + global hex decimal + global gdb_prompt + global srcfile binfile + + clean_restart $binfile + + if ![runto_main] then { + return -1 + } + + gdb_test "set breakpoint always-inserted $always_inserted" + + if {$sw_watchpoint} { + # Watching a convenience variable forces a software + # watchpoint. + gdb_test "watch \$dummy_convenience" "Watchpoint .*" + } + + set address_bp "" + set address_after_bp "" + + with_test_prefix "setup" { + + # Set a breakpoint where we'll manually plant a permanent + # breakpoint. + set test "set probe breakpoint" + gdb_test_multiple "break $line_bp" $test { + -re "Breakpoint .* at ($hex).*$gdb_prompt $" { + set address_bp $expect_out(1,string) + pass $test + } + } + if {$address_bp == ""} { + return + } + + # Get the size of the instruction where the breakpoint will + # manually inserted. + set test "get size of instruction" + gdb_test_multiple "x/2i $address_bp" $test { + -re ".*$hex <test\\+$decimal>:\[^\r\n\]+\r\n\[ \]+($hex).*\.\r\n$gdb_prompt $" { + set address_after_bp $expect_out(1,string) + pass $test + } + } + if {$address_after_bp == ""} { + return + } + + # Write address range where the breakpoint is inserted to the + # corresponding variables in the inferior. + gdb_test "p /x addr_bp = $address_bp" " = $address_bp" \ + "write addr_bp" + gdb_test "p /x addr_after_bp = $address_after_bp" " = $address_after_bp" \ + "write addr_after_bp" + + # Run the "setup" function in the inferior. This memcpy's the + # breakpoint instruction to a buffer in the inferior. + gdb_test "next" "test.*" "next over setup" + + delete_breakpoints + + # We now have the breakpoint instruction stored in 'buffer'. Poke it + # to memory manually. + set count [expr $address_after_bp - $address_bp] + for {set i 0} {$i < $count} {incr i} { + gdb_test "p /x addr_bp\[$i\] = buffer\[$i\]" " = .*" + } + } + + with_test_prefix "basics" { + # Run to the permanent breakpoint, just to make sure we've inserted it + # correctly. + gdb_test "continue" "Program received signal SIGTRAP.*" \ + "permanent breakpoint causes random signal" + + # Now set a breakpoint on top, thus creating a permanent breakpoint. + gdb_breakpoint "$line_bp" + + # Depending on whether this is a decr_pc_after_break arch, the PC will + # be either pointing at the permanent breakpoint address, or just + # after. Set the GDB breakpoint on top, and continue, twice. At + # least once, GDB will need to step-over the permanent breakpoint. + + gdb_test "continue" "Breakpoint .*" "stop at permanent breakpoint" + + gdb_test "p \$prev_counter = counter" " = $decimal" + + gdb_test "continue" "Breakpoint .*" "stop at permanent breakpoint twice" + + # Check that indeed the continue made progress, instead of re-trapping + # without advancing. + gdb_test "p counter - \$prev_counter" " = 1" + + gdb_test "info breakpoints" \ + "breakpoint.*keep.*y.*$hex.*in test at .*$srcfile:$line_bp.*already hit 2 times.*" \ + "info breakpoints show enabled breakpoint" + + gdb_test "disable \$bpnum" + + gdb_test "commands\nset \$commands_ran = 1\nend" "" \ + "set breakpoint commands" + + gdb_test "info breakpoints" \ + "breakpoint.*keep.*n.*$hex.*in test at .*$srcfile:$line_bp.*already hit 2 times.*" \ + "info breakpoints shows disabled breakpoint" + + # Run to the permanent breakpoint again. This time, since it's + # disabled, it should act as if we hadn't created it in the first + # place. IOW, we should get a random signal, and, the breakpoint's + # command should not run. + gdb_test "continue" "Program received signal SIGTRAP.*" \ + "disabled permanent breakpoint doesn't explain stop" + + gdb_test "info breakpoints" \ + "breakpoint.*keep.*n.*$hex.*in test at .*$srcfile:$line_bp.*already hit 2 times.*" \ + "info breakpoints still shows same number of hits" + + gdb_test "print \$commands_ran" " = void" \ + "breakpoint commands didn't run" + + # Reenable the breakpoint, and check that it gets hit and accounted + # for this time. + gdb_test "enable \$bpnum" "" "reenable breakpoint" + + gdb_test "continue" "Breakpoint .*" \ + "stop at permanent breakpoint thrice" + + gdb_test "info breakpoints" \ + "breakpoint.*keep.*y.*$hex.*in test at .*$srcfile:$line_bp.*already hit 3 times.*" \ + "info breakpoints shows one more hit" + + gdb_test "print \$commands_ran" " = 1" "breakpoint commands ran" + + # Check that stepi advances only past the permanent breakpoint, and + # not a single instruction more. + gdb_test "stepi" "after permanent bp .*" \ + "single-step past permanent breakpoint" + } + + with_test_prefix "next trips on permanent bp" { + delete_breakpoints + + gdb_breakpoint "test_next" + gdb_continue_to_breakpoint "test_next" + + gdb_breakpoint "$line_bp" + gdb_test "condition \$bpnum 0" + + gdb_test "next" "after next .*" + } + + if ![target_info exists gdb,nosignals] { + + with_test_prefix "continue trips on nested permanent bp" { + delete_breakpoints + + gdb_breakpoint "test_signal_nested" + gdb_continue_to_breakpoint "test_signal_nested" + + gdb_breakpoint "$line_bp" + gdb_continue_to_breakpoint "permanent bp" + gdb_test "condition \$bpnum 0" + + # Let SIGALRM trigger. + sleep 2 + + # We're now stopped at a permanent breakpoint, with a + # signal pending. + gdb_breakpoint "test_signal_nested_done" + gdb_continue_to_breakpoint "test_signal_nested_done" + + # Ensure that the handler did run. There's one call to + # test in the mainline code, and another in the signal + # handler. + gdb_test "p counter" " = 2" + } + + if [can_single_step_to_signal_handler] { + + with_test_prefix "stepi signal with handler" { + delete_breakpoints + + gdb_breakpoint "test_signal_with_handler" + gdb_continue_to_breakpoint "test_signal_with_handler" + + gdb_breakpoint "$line_bp" + + gdb_test "continue" "Breakpoint .*" "stop at permanent breakpoint" + + gdb_test "queue-signal SIGUSR1" + + set test "single-step to handler" + gdb_test_multiple "stepi" $test { + -re "Program received signal SIGTRAP.*$gdb_prompt $" { + fail $test + } + -re "handler .*$gdb_prompt $" { + pass $test + } + } + + # Check that the mainline PC points at the permanent + # breakpoint. + gdb_test "up 2" "test .*" "up to mainline code" + + gdb_test "p /x \$pc" " = $address_bp" \ + "mainline pc points at permanent breakpoint" + + gdb_test "continue" "Breakpoint .*" \ + "stop at permanent breakpoint, out of handler" + } + + with_test_prefix "stepi signal with no handler" { + gdb_breakpoint "test_signal_no_handler" + gdb_continue_to_breakpoint "test_signal_no_handler" + + gdb_test "continue" "Breakpoint .*" "stop at permanent breakpoint" + + gdb_test "queue-signal SIGUSR1" + + gdb_test "stepi" "after permanent bp .*" \ + "single-step past permanent breakpoint" + } + } + } +} + +foreach always_inserted {off on} { + foreach sw_watchpoint {0 1} { + with_test_prefix "always_inserted=$always_inserted, sw_watchpoint=$sw_watchpoint" { + test $always_inserted $sw_watchpoint + } + } +} -- 1.9.3 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] add a default method for gdbarch_skip_permanent_breakpoint 2014-11-04 19:03 [PATCH 0/3] fix permanent breakpoints Pedro Alves 2014-11-04 19:03 ` [PATCH 3/3] fix skipping " Pedro Alves @ 2014-11-04 19:03 ` Pedro Alves 2014-11-04 19:03 ` [PATCH 2/3] make "permanent breakpoints" per location and disableable Pedro Alves 2 siblings, 0 replies; 13+ messages in thread From: Pedro Alves @ 2014-11-04 19:03 UTC (permalink / raw) To: gdb-patches breakpoint.c uses gdbarch_breakpoint_from_pc to determine whether a breakpoint location points at a permanent breakpoint: static int bp_loc_is_permanent (struct bp_location *loc) { ... addr = loc->address; bpoint = gdbarch_breakpoint_from_pc (loc->gdbarch, &addr, &len); ... if (target_read_memory (loc->address, target_mem, len) == 0 && memcmp (target_mem, bpoint, len) == 0) retval = 1; ... So I think we should default the gdbarch_skip_permanent_breakpoint hook to advancing the PC by the length of the breakpoint instruction, as determined by gdbarch_breakpoint_from_pc. I believe that simple implementation does the right thing for most architectures. If there's an oddball architecture where that doesn't work, then it should override the hook, just like it should be overriding the hook if there was no default anyway. The only two implementation of skip_permanent_breakpoint are i386_skip_permanent_breakpoint, for x86, and hppa_skip_permanent_breakpoint, for PA-RISC/HP-UX The x86 implementation is trivial, and can clearly be replaced by the new default. I don't know about the HP-UX one though, I know almost nothing about PA. It may well be advancing the PC ends up being equivalent. Otherwise, it must be that "jump $pc_after_bp" doesn't work either... Tested on x86_64 Fedora 20. gdb/ 2014-11-04 Pedro Alves <palves@redhat.com> * arch-utils.c (default_skip_permanent_breakpoint): New function. * arch-utils.h (default_skip_permanent_breakpoint): New declaration. * gdbarch.sh (skip_permanent_breakpoint): Now an 'f' function. Install default_skip_permanent_breakpoint as default method. * i386-tdep.c (i386_skip_permanent_breakpoint): Delete function. (i386_gdbarch_init): Don't install it. * infrun.c (resume): Assume there's always a gdbarch_skip_permanent_breakpoint implementation. --- gdb/arch-utils.c | 13 ++++++++++++- gdb/arch-utils.h | 8 ++++++++ gdb/gdbarch.c | 13 ++----------- gdb/gdbarch.h | 2 -- gdb/gdbarch.sh | 2 +- gdb/i386-tdep.c | 15 --------------- gdb/infrun.c | 8 +------- 7 files changed, 24 insertions(+), 37 deletions(-) diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c index 41cf828..a2e76de 100644 --- a/gdb/arch-utils.c +++ b/gdb/arch-utils.c @@ -826,7 +826,18 @@ int default_insn_is_jump (struct gdbarch *gdbarch, CORE_ADDR addr) return 0; } -/* */ +void +default_skip_permanent_breakpoint (struct regcache *regcache) +{ + struct gdbarch *gdbarch = get_regcache_arch (regcache); + CORE_ADDR current_pc = regcache_read_pc (regcache); + const gdb_byte *bp_insn; + int bp_len; + + bp_insn = gdbarch_breakpoint_from_pc (gdbarch, ¤t_pc, &bp_len); + current_pc += bp_len; + regcache_write_pc (regcache, current_pc); +} /* -Wmissing-prototypes */ extern initialize_file_ftype _initialize_gdbarch_utils; diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h index a609662..1f5dd55 100644 --- a/gdb/arch-utils.h +++ b/gdb/arch-utils.h @@ -178,4 +178,12 @@ extern int default_insn_is_jump (struct gdbarch *, CORE_ADDR); /* Do-nothing version of vsyscall_range. Returns false. */ extern int default_vsyscall_range (struct gdbarch *gdbarch, struct mem_range *range); + +/* Default way to advance the PC to the next instruction in order to + skip a permanent breakpoint. Increments the PC by the size of a + software breakpoint instruction, as determined with + gdbarch_breakpoint_from_pc. This matches how the breakpoints + module determines whether a breakpoint is permanent. */ +extern void default_skip_permanent_breakpoint (struct regcache *regcache); + #endif diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c index 2984358..bd53acc 100644 --- a/gdb/gdbarch.c +++ b/gdb/gdbarch.c @@ -393,6 +393,7 @@ gdbarch_alloc (const struct gdbarch_info *info, gdbarch->elf_make_msymbol_special = default_elf_make_msymbol_special; gdbarch->coff_make_msymbol_special = default_coff_make_msymbol_special; gdbarch->register_reggroup_p = default_register_reggroup_p; + gdbarch->skip_permanent_breakpoint = default_skip_permanent_breakpoint; gdbarch->displaced_step_hw_singlestep = default_displaced_step_hw_singlestep; gdbarch->displaced_step_fixup = NULL; gdbarch->displaced_step_free_closure = NULL; @@ -581,7 +582,7 @@ verify_gdbarch (struct gdbarch *gdbarch) /* Skip verify of gcore_bfd_target, has predicate. */ /* Skip verify of vtable_function_descriptors, invalid_p == 0 */ /* Skip verify of vbit_in_delta, invalid_p == 0 */ - /* Skip verify of skip_permanent_breakpoint, has predicate. */ + /* Skip verify of skip_permanent_breakpoint, invalid_p == 0 */ /* Skip verify of max_insn_length, has predicate. */ /* Skip verify of displaced_step_copy_insn, has predicate. */ /* Skip verify of displaced_step_hw_singlestep, invalid_p == 0 */ @@ -1190,9 +1191,6 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file) "gdbarch_dump: skip_main_prologue = <%s>\n", host_address_to_string (gdbarch->skip_main_prologue)); fprintf_unfiltered (file, - "gdbarch_dump: gdbarch_skip_permanent_breakpoint_p() = %d\n", - gdbarch_skip_permanent_breakpoint_p (gdbarch)); - fprintf_unfiltered (file, "gdbarch_dump: skip_permanent_breakpoint = <%s>\n", host_address_to_string (gdbarch->skip_permanent_breakpoint)); fprintf_unfiltered (file, @@ -3465,13 +3463,6 @@ set_gdbarch_vbit_in_delta (struct gdbarch *gdbarch, gdbarch->vbit_in_delta = vbit_in_delta; } -int -gdbarch_skip_permanent_breakpoint_p (struct gdbarch *gdbarch) -{ - gdb_assert (gdbarch != NULL); - return gdbarch->skip_permanent_breakpoint != NULL; -} - void gdbarch_skip_permanent_breakpoint (struct gdbarch *gdbarch, struct regcache *regcache) { diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h index f5330c2..5cd4197 100644 --- a/gdb/gdbarch.h +++ b/gdb/gdbarch.h @@ -829,8 +829,6 @@ extern void set_gdbarch_vbit_in_delta (struct gdbarch *gdbarch, int vbit_in_delt /* Advance PC to next instruction in order to skip a permanent breakpoint. */ -extern int gdbarch_skip_permanent_breakpoint_p (struct gdbarch *gdbarch); - typedef void (gdbarch_skip_permanent_breakpoint_ftype) (struct regcache *regcache); extern void gdbarch_skip_permanent_breakpoint (struct gdbarch *gdbarch, struct regcache *regcache); extern void set_gdbarch_skip_permanent_breakpoint (struct gdbarch *gdbarch, gdbarch_skip_permanent_breakpoint_ftype *skip_permanent_breakpoint); diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh index 5442799..d9b55c2 100755 --- a/gdb/gdbarch.sh +++ b/gdb/gdbarch.sh @@ -699,7 +699,7 @@ v:int:vtable_function_descriptors:::0:0::0 v:int:vbit_in_delta:::0:0::0 # Advance PC to next instruction in order to skip a permanent breakpoint. -F:void:skip_permanent_breakpoint:struct regcache *regcache:regcache +f:void:skip_permanent_breakpoint:struct regcache *regcache:regcache:default_skip_permanent_breakpoint:default_skip_permanent_breakpoint::0 # The maximum length of an instruction on this architecture in bytes. V:ULONGEST:max_insn_length:::0:0 diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c index 8387c72..83956bf 100644 --- a/gdb/i386-tdep.c +++ b/gdb/i386-tdep.c @@ -4523,18 +4523,6 @@ i386_fetch_pointer_argument (struct frame_info *frame, int argi, return read_memory_unsigned_integer (sp + (4 * (argi + 1)), 4, byte_order); } -static void -i386_skip_permanent_breakpoint (struct regcache *regcache) -{ - CORE_ADDR current_pc = regcache_read_pc (regcache); - - /* On i386, breakpoint is exactly 1 byte long, so we just - adjust the PC in the regcache. */ - current_pc += 1; - regcache_write_pc (regcache, current_pc); -} - - #define PREFIX_REPZ 0x01 #define PREFIX_REPNZ 0x02 #define PREFIX_LOCK 0x04 @@ -8566,9 +8554,6 @@ i386_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) set_gdbarch_iterate_over_regset_sections (gdbarch, i386_iterate_over_regset_sections); - set_gdbarch_skip_permanent_breakpoint (gdbarch, - i386_skip_permanent_breakpoint); - set_gdbarch_fast_tracepoint_valid_at (gdbarch, i386_fast_tracepoint_valid_at); diff --git a/gdb/infrun.c b/gdb/infrun.c index b950b74..4c4f03b 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -2075,13 +2075,7 @@ resume (int step, enum gdb_signal sig) 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.")); + gdbarch_skip_permanent_breakpoint (gdbarch, regcache); } /* If we have a breakpoint to step over, make sure to do a single -- 1.9.3 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] make "permanent breakpoints" per location and disableable 2014-11-04 19:03 [PATCH 0/3] fix permanent breakpoints Pedro Alves 2014-11-04 19:03 ` [PATCH 3/3] fix skipping " Pedro Alves 2014-11-04 19:03 ` [PATCH 1/3] add a default method for gdbarch_skip_permanent_breakpoint Pedro Alves @ 2014-11-04 19:03 ` Pedro Alves 2014-11-05 6:37 ` Yao Qi 2 siblings, 1 reply; 13+ messages in thread From: Pedro Alves @ 2014-11-04 19:03 UTC (permalink / raw) To: gdb-patches "permanent"-ness is currently a property of the breakpoint. But, it should actually be an implementation detail of a _location_. Consider 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.")); } This will wrongly skip a non-breakpoint instruction if we have a multiple location breakpoint where the whole breakpoint was set to "permanent" because one of the locations happened to be permanent, even if the one GDB is resuming from is not. Related, because the permanent breakpoints are only marked as such in init_breakpoint_sal, we currently miss marking momentary breakpoints as permanent. A test added by a following patch trips on that. Making permanent-ness be per-location, and marking locations as such in add_location_to_breakpoint, the natural place to do this, fixes this issue... ... and then exposes a latent issue with mark_breakpoints_out. It's clearing the inserted flag of permanent breakpoints. This results in assertions failing like this: Breakpoint 1, main () at testsuite/gdb.base/callexit.c:32 32 return 0; (gdb) call callexit() [Inferior 1 (process 15849) exited normally] gdb/breakpoint.c:12854: internal-error: allegedly permanent breakpoint is not actually inserted A problem internal to GDB has been detected, further debugging may prove unreliable. The call dummy breakpoint, which is a momentary breakpoint, is set on top of a manually inserted breakpoint instruction, and so is now rightfully marked as a permanent breakpoint. See "Write a legitimate instruction at the point where the infcall breakpoint is going to be inserted." comment in infcall.c. Re. make_breakpoint_permanent. That's only called by solib-pa64.c. Permanent breakpoints were actually originally invented for HP-UX [1]. I believe that that call (the only one in the tree) is unnecessary nowadays, given that nowadays the core breakpoints code analyzes the instruction under the breakpoint to automatically detect whether it's setting a breakpoint on top of a breakpoint instruction in the program. I know close to nothing about HP-PA/HP-UX, though. [1] https://sourceware.org/ml/gdb-patches/1999-q3/msg00245.html, and https://sourceware.org/ml/gdb-patches/1999-q3/msg00242.html In addition to the per-location issue, "permanent breakpoints" are currently always 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 But OTOH they're always enabled; there's no way to disable them... In turn, this means that if one adds commands to such a breakpoint, they're _always_ run: (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) IMO, one should be able to disable such a breakpoint, and GDB should then behave just like if the user hadn't created the breakpoint in the first place (that is, report a SIGTRAP). By making permanent-ness a property of the location, and eliminating the bp_permanent enum enable_state state ends up fixing that as well. No tests are added for these changes yet; they'll be added in a follow up patch, as skipping permanent breakpoints is currently broken and trips on an assertion in infrun. Tested on x86_64 Fedora 20. gdb/ChangeLog: 2014-11-04 Pedro Alves <palves@redhat.com> Mark locations as permanent, not the whole breakpoint. * breakpoint.c (remove_breakpoint_1, remove_breakpoint): Adjust. (mark_breakpoints_out): Don't mark permanent breakpoints as uninserted. (breakpoint_init_inferior): Use mark_breakpoints_out. (breakpoint_here_p): Adjust. (bpstat_stop_status, describe_other_breakpoints): Remove handling of permanent breakpoints. (make_breakpoint_permanent): Mark each location as permanent, instead of marking the breakpoint. (add_location_to_breakpoint): If the location is permanent, mark it as such, and as inserted. (init_breakpoint_sal): Don't make the breakpoint permanent here. (bp_location_compare, update_global_location_list): Adjust. (update_breakpoint_locations): Don't make the breakpoint permanent here. (disable_breakpoint, enable_breakpoint_disp): Don't skip permanent breakpoints. * breakpoint.h (enum enable_state) <bp_permanent>: Delete field. (struct bp_location) <permanent>: New field. --- gdb/breakpoint.c | 71 ++++++++++++++++++++++---------------------------------- gdb/breakpoint.h | 13 ++++++----- 2 files changed, 35 insertions(+), 49 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index bd51f5d..3ebe9c9 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -3883,7 +3883,7 @@ remove_breakpoint_1 (struct bp_location *bl, insertion_state_t is) /* BL is never in moribund_locations by our callers. */ gdb_assert (bl->owner != NULL); - if (bl->owner->enable_state == bp_permanent) + if (bl->permanent) /* Permanent breakpoints cannot be inserted or removed. */ return 0; @@ -4033,7 +4033,7 @@ remove_breakpoint (struct bp_location *bl, insertion_state_t is) /* BL is never in moribund_locations by our callers. */ gdb_assert (bl->owner != NULL); - if (bl->owner->enable_state == bp_permanent) + if (bl->permanent) /* Permanent breakpoints cannot be inserted or removed. */ return 0; @@ -4059,7 +4059,8 @@ mark_breakpoints_out (void) struct bp_location *bl, **blp_tmp; ALL_BP_LOCATIONS (bl, blp_tmp) - if (bl->pspace == current_program_space) + if (bl->pspace == current_program_space + && !bl->permanent) bl->inserted = 0; } @@ -4088,13 +4089,7 @@ breakpoint_init_inferior (enum inf_context context) if (gdbarch_has_global_breakpoints (target_gdbarch ())) return; - ALL_BP_LOCATIONS (bl, blp_tmp) - { - /* ALL_BP_LOCATIONS bp_location has BL->OWNER always non-NULL. */ - if (bl->pspace == pspace - && bl->owner->enable_state != bp_permanent) - bl->inserted = 0; - } + mark_breakpoints_out (); ALL_BREAKPOINTS_SAFE (b, b_tmp) { @@ -4202,14 +4197,14 @@ breakpoint_here_p (struct address_space *aspace, CORE_ADDR pc) /* ALL_BP_LOCATIONS bp_location has BL->OWNER always non-NULL. */ if ((breakpoint_enabled (bl->owner) - || bl->owner->enable_state == bp_permanent) + || bl->permanent) && breakpoint_location_address_match (bl, aspace, pc)) { if (overlay_debugging && section_is_overlay (bl->section) && !section_is_mapped (bl->section)) continue; /* unmapped overlay -- can't be a match */ - else if (bl->owner->enable_state == bp_permanent) + else if (bl->permanent) return permanent_breakpoint_here; else any_breakpoint_here = 1; @@ -5475,7 +5470,7 @@ bpstat_stop_status (struct address_space *aspace, ALL_BREAKPOINTS (b) { - if (!breakpoint_enabled (b) && b->enable_state != bp_permanent) + if (!breakpoint_enabled (b)) continue; for (bl = b->loc; bl != NULL; bl = bl->next) @@ -5571,8 +5566,7 @@ bpstat_stop_status (struct address_space *aspace, if (b->disposition == disp_disable) { --(b->enable_count); - if (b->enable_count <= 0 - && b->enable_state != bp_permanent) + if (b->enable_count <= 0) b->enable_state = bp_disabled; removed_any = 1; } @@ -6856,8 +6850,6 @@ describe_other_breakpoints (struct gdbarch *gdbarch, ((b->enable_state == bp_disabled || b->enable_state == bp_call_disabled) ? " (disabled)" - : b->enable_state == bp_permanent - ? " (permanent)" : ""), (others > 1) ? "," : ((others == 1) ? " and" : "")); @@ -7389,15 +7381,16 @@ make_breakpoint_permanent (struct breakpoint *b) { struct bp_location *bl; - b->enable_state = bp_permanent; - /* By definition, permanent breakpoints are already present in the code. Mark all locations as inserted. For now, make_breakpoint_permanent is called in just one place, so it's hard to say if it's reasonable to have permanent breakpoint with multiple locations or not, but it's easy to implement. */ for (bl = b->loc; bl; bl = bl->next) - bl->inserted = 1; + { + bl->permanent = 1; + bl->inserted = 1; + } } /* Call this routine when stepping and nexting to enable a breakpoint @@ -9227,6 +9220,8 @@ mention (struct breakpoint *b) } \f +static int bp_loc_is_permanent (struct bp_location *loc); + static struct bp_location * add_location_to_breakpoint (struct breakpoint *b, const struct symtab_and_line *sal) @@ -9268,6 +9263,13 @@ add_location_to_breakpoint (struct breakpoint *b, set_breakpoint_location_function (loc, sal->explicit_pc || sal->explicit_line); + + if (bp_loc_is_permanent (loc)) + { + loc->inserted = 1; + loc->permanent = 1; + } + return loc; } \f @@ -9510,9 +9512,6 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch, loc->inserted = 1; } - if (bp_loc_is_permanent (loc)) - make_breakpoint_permanent (b); - if (b->cond_string) { const char *arg = b->cond_string; @@ -12352,7 +12351,7 @@ breakpoint_auto_delete (bpstat bs) /* A comparison function for bp_location AP and BP being interfaced to qsort. Sort elements primarily by their ADDRESS (no matter what does breakpoint_address_is_meaningful say for its OWNER), - secondarily by ordering first bp_permanent OWNERed elements and + secondarily by ordering first permanent elements and terciarily just ensuring the array is sorted stable way despite qsort being an unstable algorithm. */ @@ -12361,9 +12360,6 @@ bp_location_compare (const void *ap, const void *bp) { struct bp_location *a = *(void **) ap; struct bp_location *b = *(void **) bp; - /* A and B come from existing breakpoints having non-NULL OWNER. */ - int a_perm = a->owner->enable_state == bp_permanent; - int b_perm = b->owner->enable_state == bp_permanent; if (a->address != b->address) return (a->address > b->address) - (a->address < b->address); @@ -12377,8 +12373,8 @@ bp_location_compare (const void *ap, const void *bp) - (a->pspace->num < b->pspace->num)); /* Sort permanent breakpoints first. */ - if (a_perm != b_perm) - return (a_perm < b_perm) - (a_perm > b_perm); + if (a->permanent != b->permanent) + return (a->permanent < b->permanent) - (a->permanent > b->permanent); /* Make the internal GDB representation stable across GDB runs where A and B memory inside GDB can differ. Breakpoint locations of @@ -12849,7 +12845,7 @@ update_global_location_list (enum ugll_insert_mode insert_mode) } /* Permanent breakpoint should always be inserted. */ - if (b->enable_state == bp_permanent && ! loc->inserted) + if (loc->permanent && ! loc->inserted) internal_error (__FILE__, __LINE__, _("allegedly permanent breakpoint is not " "actually inserted")); @@ -12890,8 +12886,8 @@ update_global_location_list (enum ugll_insert_mode insert_mode) /* Clear the condition modification flag. */ loc->condition_changed = condition_unchanged; - if ((*loc_first_p)->owner->enable_state == bp_permanent && loc->inserted - && b->enable_state != bp_permanent) + if (loc->inserted && !loc->permanent + && (*loc_first_p)->permanent) internal_error (__FILE__, __LINE__, _("another breakpoint was inserted on top of " "a permanent breakpoint")); @@ -14437,10 +14433,6 @@ update_breakpoint_locations (struct breakpoint *b, } } - /* Update locations of permanent breakpoints. */ - if (b->enable_state == bp_permanent) - make_breakpoint_permanent (b); - /* If possible, carry over 'disable' status from existing breakpoints. */ { @@ -14923,10 +14915,6 @@ disable_breakpoint (struct breakpoint *bpt) if (bpt->type == bp_watchpoint_scope) return; - /* You can't disable permanent breakpoints. */ - if (bpt->enable_state == bp_permanent) - return; - bpt->enable_state = bp_disabled; /* Mark breakpoint locations modified. */ @@ -15047,9 +15035,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. */ diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h index 118a37f..c383cd4 100644 --- a/gdb/breakpoint.h +++ b/gdb/breakpoint.h @@ -193,12 +193,6 @@ enum enable_state automatically enabled and reset when the call "lands" (either completes, or stops at another eventpoint). */ - bp_permanent /* There is a breakpoint instruction - hard-wired into the target's code. Don't - try to write another breakpoint - instruction on top of it, or restore its - value. Step over it using the - architecture's SKIP_INSN macro. */ }; @@ -376,6 +370,13 @@ struct bp_location /* Nonzero if this breakpoint is now inserted. */ char inserted; + /* Nonzero if this is a permanent breakpoint. There is a breakpoint + instruction hard-wired into the target's code. Don't try to + write another breakpoint instruction on top of it, or restore its + value. Step over it using the architecture's + gdbarch_skip_permanent_breakpoint method. */ + char permanent; + /* Nonzero if this is not the first breakpoint in the list for the given address. location of tracepoint can _never_ be duplicated with other locations of tracepoints and other -- 1.9.3 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] make "permanent breakpoints" per location and disableable 2014-11-04 19:03 ` [PATCH 2/3] make "permanent breakpoints" per location and disableable Pedro Alves @ 2014-11-05 6:37 ` Yao Qi 2014-11-12 10:55 ` Pedro Alves 0 siblings, 1 reply; 13+ messages in thread From: Yao Qi @ 2014-11-05 6:37 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches Pedro Alves <palves@redhat.com> writes: Hi Pedro, I go through this patch, and don't see anything wrong except this below, > diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h > index 118a37f..c383cd4 100644 > --- a/gdb/breakpoint.h > +++ b/gdb/breakpoint.h > @@ -193,12 +193,6 @@ enum enable_state > automatically enabled and reset when the > call "lands" (either completes, or stops > at another eventpoint). */ > - bp_permanent /* There is a breakpoint instruction > - hard-wired into the target's code. Don't > - try to write another breakpoint > - instruction on top of it, or restore its > - value. Step over it using the > - architecture's SKIP_INSN macro. */ > }; This causes a compilation error in guile/scm-breakpoint.c where bp_permanent is in use. ../../../git/gdb/guile/scm-breakpoint.c: In function ‘bpscm_enable_state_to_string’: ../../../git/gdb/guile/scm-breakpoint.c:154:10: error: ‘bp_permanent’ undeclared (first use in this function) case bp_permanent: return "permanent"; static const char * bpscm_enable_state_to_string (enum enable_state enable_state) { switch (enable_state) { case bp_disabled: return "disabled"; case bp_enabled: return "enabled"; case bp_call_disabled: return "call_disabled"; case bp_permanent: return "permanent"; ^^^^^^^^^^^^ default: return "unknown"; } } -- Yao (齐尧) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] make "permanent breakpoints" per location and disableable 2014-11-05 6:37 ` Yao Qi @ 2014-11-12 10:55 ` Pedro Alves 2014-11-20 16:41 ` [RFA] Always consider infcall breakpoints as non-permanent Joel Brobecker 0 siblings, 1 reply; 13+ messages in thread From: Pedro Alves @ 2014-11-12 10:55 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On 11/05/2014 06:37 AM, Yao Qi wrote: > Pedro Alves <palves@redhat.com> writes: > > Hi Pedro, > I go through this patch, and don't see anything wrong except this below, > >> diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h >> index 118a37f..c383cd4 100644 >> --- a/gdb/breakpoint.h >> +++ b/gdb/breakpoint.h >> @@ -193,12 +193,6 @@ enum enable_state >> automatically enabled and reset when the >> call "lands" (either completes, or stops >> at another eventpoint). */ >> - bp_permanent /* There is a breakpoint instruction >> - hard-wired into the target's code. Don't >> - try to write another breakpoint >> - instruction on top of it, or restore its >> - value. Step over it using the >> - architecture's SKIP_INSN macro. */ >> }; > > This causes a compilation error in guile/scm-breakpoint.c where > bp_permanent is in use. > > ../../../git/gdb/guile/scm-breakpoint.c: In function ‘bpscm_enable_state_to_string’: > ../../../git/gdb/guile/scm-breakpoint.c:154:10: error: ‘bp_permanent’ undeclared (first use in this function) > case bp_permanent: return "permanent"; > > static const char * > bpscm_enable_state_to_string (enum enable_state enable_state) > { > switch (enable_state) > { > case bp_disabled: return "disabled"; > case bp_enabled: return "enabled"; > case bp_call_disabled: return "call_disabled"; > case bp_permanent: return "permanent"; > ^^^^^^^^^^^^ > default: return "unknown"; > } > } Thanks, I hadn't noticed I didn't have guile-devel installed on this machine. Now fixed and pushed, as below. --- From 1a853c5224e2b8fedfac6d029365522b83080b40 Mon Sep 17 00:00:00 2001 From: Pedro Alves <palves@redhat.com> Date: Wed, 12 Nov 2014 10:10:49 +0000 Subject: [PATCH 2/3] make "permanent breakpoints" per location and disableable "permanent"-ness is currently a property of the breakpoint. But, it should actually be an implementation detail of a _location_. Consider 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.")); } This will wrongly skip a non-breakpoint instruction if we have a multiple location breakpoint where the whole breakpoint was set to "permanent" because one of the locations happened to be permanent, even if the one GDB is resuming from is not. Related, because the permanent breakpoints are only marked as such in init_breakpoint_sal, we currently miss marking momentary breakpoints as permanent. A test added by a following patch trips on that. Making permanent-ness be per-location, and marking locations as such in add_location_to_breakpoint, the natural place to do this, fixes this issue... ... and then exposes a latent issue with mark_breakpoints_out. It's clearing the inserted flag of permanent breakpoints. This results in assertions failing like this: Breakpoint 1, main () at testsuite/gdb.base/callexit.c:32 32 return 0; (gdb) call callexit() [Inferior 1 (process 15849) exited normally] gdb/breakpoint.c:12854: internal-error: allegedly permanent breakpoint is not actually inserted A problem internal to GDB has been detected, further debugging may prove unreliable. The call dummy breakpoint, which is a momentary breakpoint, is set on top of a manually inserted breakpoint instruction, and so is now rightfully marked as a permanent breakpoint. See "Write a legitimate instruction at the point where the infcall breakpoint is going to be inserted." comment in infcall.c. Re. make_breakpoint_permanent. That's only called by solib-pa64.c. Permanent breakpoints were actually originally invented for HP-UX [1]. I believe that that call (the only one in the tree) is unnecessary nowadays, given that nowadays the core breakpoints code analyzes the instruction under the breakpoint to automatically detect whether it's setting a breakpoint on top of a breakpoint instruction in the program. I know close to nothing about HP-PA/HP-UX, though. [1] https://sourceware.org/ml/gdb-patches/1999-q3/msg00245.html, and https://sourceware.org/ml/gdb-patches/1999-q3/msg00242.html In addition to the per-location issue, "permanent breakpoints" are currently always 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 But OTOH they're always enabled; there's no way to disable them... In turn, this means that if one adds commands to such a breakpoint, they're _always_ run: (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) IMO, one should be able to disable such a breakpoint, and GDB should then behave just like if the user hadn't created the breakpoint in the first place (that is, report a SIGTRAP). By making permanent-ness a property of the location, and eliminating the bp_permanent enum enable_state state ends up fixing that as well. No tests are added for these changes yet; they'll be added in a follow up patch, as skipping permanent breakpoints is currently broken and trips on an assertion in infrun. Tested on x86_64 Fedora 20, native and gdbserver. gdb/ChangeLog: 2014-11-12 Pedro Alves <palves@redhat.com> Mark locations as permanent, not the whole breakpoint. * breakpoint.c (remove_breakpoint_1, remove_breakpoint): Adjust. (mark_breakpoints_out): Don't mark permanent breakpoints as uninserted. (breakpoint_init_inferior): Use mark_breakpoints_out. (breakpoint_here_p): Adjust. (bpstat_stop_status, describe_other_breakpoints): Remove handling of permanent breakpoints. (make_breakpoint_permanent): Mark each location as permanent, instead of marking the breakpoint. (add_location_to_breakpoint): If the location is permanent, mark it as such, and as inserted. (init_breakpoint_sal): Don't make the breakpoint permanent here. (bp_location_compare, update_global_location_list): Adjust. (update_breakpoint_locations): Don't make the breakpoint permanent here. (disable_breakpoint, enable_breakpoint_disp): Don't skip permanent breakpoints. * breakpoint.h (enum enable_state) <bp_permanent>: Delete field. (struct bp_location) <permanent>: New field. * guile/scm-breakpoint.c (bpscm_enable_state_to_string): Remove reference to bp_permanent. --- gdb/ChangeLog | 25 ++++++++++++++++ gdb/breakpoint.c | 71 ++++++++++++++++++---------------------------- gdb/breakpoint.h | 13 +++++---- gdb/guile/scm-breakpoint.c | 1 - 4 files changed, 60 insertions(+), 50 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 74db317..a50f67e 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,5 +1,30 @@ 2014-11-12 Pedro Alves <palves@redhat.com> + Mark locations as permanent, not the whole breakpoint. + * breakpoint.c (remove_breakpoint_1, remove_breakpoint): Adjust. + (mark_breakpoints_out): Don't mark permanent breakpoints as + uninserted. + (breakpoint_init_inferior): Use mark_breakpoints_out. + (breakpoint_here_p): Adjust. + (bpstat_stop_status, describe_other_breakpoints): Remove handling + of permanent breakpoints. + (make_breakpoint_permanent): Mark each location as permanent, + instead of marking the breakpoint. + (add_location_to_breakpoint): If the location is permanent, mark + it as such, and as inserted. + (init_breakpoint_sal): Don't make the breakpoint permanent here. + (bp_location_compare, update_global_location_list): Adjust. + (update_breakpoint_locations): Don't make the breakpoint permanent + here. + (disable_breakpoint, enable_breakpoint_disp): Don't skip permanent + breakpoints. + * breakpoint.h (enum enable_state) <bp_permanent>: Delete field. + (struct bp_location) <permanent>: New field. + * guile/scm-breakpoint.c (bpscm_enable_state_to_string): Remove + reference to bp_permanent. + +2014-11-12 Pedro Alves <palves@redhat.com> + * arch-utils.c (default_skip_permanent_breakpoint): New function. * arch-utils.h (default_skip_permanent_breakpoint): New declaration. diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index bd51f5d..3ebe9c9 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -3883,7 +3883,7 @@ remove_breakpoint_1 (struct bp_location *bl, insertion_state_t is) /* BL is never in moribund_locations by our callers. */ gdb_assert (bl->owner != NULL); - if (bl->owner->enable_state == bp_permanent) + if (bl->permanent) /* Permanent breakpoints cannot be inserted or removed. */ return 0; @@ -4033,7 +4033,7 @@ remove_breakpoint (struct bp_location *bl, insertion_state_t is) /* BL is never in moribund_locations by our callers. */ gdb_assert (bl->owner != NULL); - if (bl->owner->enable_state == bp_permanent) + if (bl->permanent) /* Permanent breakpoints cannot be inserted or removed. */ return 0; @@ -4059,7 +4059,8 @@ mark_breakpoints_out (void) struct bp_location *bl, **blp_tmp; ALL_BP_LOCATIONS (bl, blp_tmp) - if (bl->pspace == current_program_space) + if (bl->pspace == current_program_space + && !bl->permanent) bl->inserted = 0; } @@ -4088,13 +4089,7 @@ breakpoint_init_inferior (enum inf_context context) if (gdbarch_has_global_breakpoints (target_gdbarch ())) return; - ALL_BP_LOCATIONS (bl, blp_tmp) - { - /* ALL_BP_LOCATIONS bp_location has BL->OWNER always non-NULL. */ - if (bl->pspace == pspace - && bl->owner->enable_state != bp_permanent) - bl->inserted = 0; - } + mark_breakpoints_out (); ALL_BREAKPOINTS_SAFE (b, b_tmp) { @@ -4202,14 +4197,14 @@ breakpoint_here_p (struct address_space *aspace, CORE_ADDR pc) /* ALL_BP_LOCATIONS bp_location has BL->OWNER always non-NULL. */ if ((breakpoint_enabled (bl->owner) - || bl->owner->enable_state == bp_permanent) + || bl->permanent) && breakpoint_location_address_match (bl, aspace, pc)) { if (overlay_debugging && section_is_overlay (bl->section) && !section_is_mapped (bl->section)) continue; /* unmapped overlay -- can't be a match */ - else if (bl->owner->enable_state == bp_permanent) + else if (bl->permanent) return permanent_breakpoint_here; else any_breakpoint_here = 1; @@ -5475,7 +5470,7 @@ bpstat_stop_status (struct address_space *aspace, ALL_BREAKPOINTS (b) { - if (!breakpoint_enabled (b) && b->enable_state != bp_permanent) + if (!breakpoint_enabled (b)) continue; for (bl = b->loc; bl != NULL; bl = bl->next) @@ -5571,8 +5566,7 @@ bpstat_stop_status (struct address_space *aspace, if (b->disposition == disp_disable) { --(b->enable_count); - if (b->enable_count <= 0 - && b->enable_state != bp_permanent) + if (b->enable_count <= 0) b->enable_state = bp_disabled; removed_any = 1; } @@ -6856,8 +6850,6 @@ describe_other_breakpoints (struct gdbarch *gdbarch, ((b->enable_state == bp_disabled || b->enable_state == bp_call_disabled) ? " (disabled)" - : b->enable_state == bp_permanent - ? " (permanent)" : ""), (others > 1) ? "," : ((others == 1) ? " and" : "")); @@ -7389,15 +7381,16 @@ make_breakpoint_permanent (struct breakpoint *b) { struct bp_location *bl; - b->enable_state = bp_permanent; - /* By definition, permanent breakpoints are already present in the code. Mark all locations as inserted. For now, make_breakpoint_permanent is called in just one place, so it's hard to say if it's reasonable to have permanent breakpoint with multiple locations or not, but it's easy to implement. */ for (bl = b->loc; bl; bl = bl->next) - bl->inserted = 1; + { + bl->permanent = 1; + bl->inserted = 1; + } } /* Call this routine when stepping and nexting to enable a breakpoint @@ -9227,6 +9220,8 @@ mention (struct breakpoint *b) } \f +static int bp_loc_is_permanent (struct bp_location *loc); + static struct bp_location * add_location_to_breakpoint (struct breakpoint *b, const struct symtab_and_line *sal) @@ -9268,6 +9263,13 @@ add_location_to_breakpoint (struct breakpoint *b, set_breakpoint_location_function (loc, sal->explicit_pc || sal->explicit_line); + + if (bp_loc_is_permanent (loc)) + { + loc->inserted = 1; + loc->permanent = 1; + } + return loc; } \f @@ -9510,9 +9512,6 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch, loc->inserted = 1; } - if (bp_loc_is_permanent (loc)) - make_breakpoint_permanent (b); - if (b->cond_string) { const char *arg = b->cond_string; @@ -12352,7 +12351,7 @@ breakpoint_auto_delete (bpstat bs) /* A comparison function for bp_location AP and BP being interfaced to qsort. Sort elements primarily by their ADDRESS (no matter what does breakpoint_address_is_meaningful say for its OWNER), - secondarily by ordering first bp_permanent OWNERed elements and + secondarily by ordering first permanent elements and terciarily just ensuring the array is sorted stable way despite qsort being an unstable algorithm. */ @@ -12361,9 +12360,6 @@ bp_location_compare (const void *ap, const void *bp) { struct bp_location *a = *(void **) ap; struct bp_location *b = *(void **) bp; - /* A and B come from existing breakpoints having non-NULL OWNER. */ - int a_perm = a->owner->enable_state == bp_permanent; - int b_perm = b->owner->enable_state == bp_permanent; if (a->address != b->address) return (a->address > b->address) - (a->address < b->address); @@ -12377,8 +12373,8 @@ bp_location_compare (const void *ap, const void *bp) - (a->pspace->num < b->pspace->num)); /* Sort permanent breakpoints first. */ - if (a_perm != b_perm) - return (a_perm < b_perm) - (a_perm > b_perm); + if (a->permanent != b->permanent) + return (a->permanent < b->permanent) - (a->permanent > b->permanent); /* Make the internal GDB representation stable across GDB runs where A and B memory inside GDB can differ. Breakpoint locations of @@ -12849,7 +12845,7 @@ update_global_location_list (enum ugll_insert_mode insert_mode) } /* Permanent breakpoint should always be inserted. */ - if (b->enable_state == bp_permanent && ! loc->inserted) + if (loc->permanent && ! loc->inserted) internal_error (__FILE__, __LINE__, _("allegedly permanent breakpoint is not " "actually inserted")); @@ -12890,8 +12886,8 @@ update_global_location_list (enum ugll_insert_mode insert_mode) /* Clear the condition modification flag. */ loc->condition_changed = condition_unchanged; - if ((*loc_first_p)->owner->enable_state == bp_permanent && loc->inserted - && b->enable_state != bp_permanent) + if (loc->inserted && !loc->permanent + && (*loc_first_p)->permanent) internal_error (__FILE__, __LINE__, _("another breakpoint was inserted on top of " "a permanent breakpoint")); @@ -14437,10 +14433,6 @@ update_breakpoint_locations (struct breakpoint *b, } } - /* Update locations of permanent breakpoints. */ - if (b->enable_state == bp_permanent) - make_breakpoint_permanent (b); - /* If possible, carry over 'disable' status from existing breakpoints. */ { @@ -14923,10 +14915,6 @@ disable_breakpoint (struct breakpoint *bpt) if (bpt->type == bp_watchpoint_scope) return; - /* You can't disable permanent breakpoints. */ - if (bpt->enable_state == bp_permanent) - return; - bpt->enable_state = bp_disabled; /* Mark breakpoint locations modified. */ @@ -15047,9 +15035,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. */ diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h index 118a37f..c383cd4 100644 --- a/gdb/breakpoint.h +++ b/gdb/breakpoint.h @@ -193,12 +193,6 @@ enum enable_state automatically enabled and reset when the call "lands" (either completes, or stops at another eventpoint). */ - bp_permanent /* There is a breakpoint instruction - hard-wired into the target's code. Don't - try to write another breakpoint - instruction on top of it, or restore its - value. Step over it using the - architecture's SKIP_INSN macro. */ }; @@ -376,6 +370,13 @@ struct bp_location /* Nonzero if this breakpoint is now inserted. */ char inserted; + /* Nonzero if this is a permanent breakpoint. There is a breakpoint + instruction hard-wired into the target's code. Don't try to + write another breakpoint instruction on top of it, or restore its + value. Step over it using the architecture's + gdbarch_skip_permanent_breakpoint method. */ + char permanent; + /* Nonzero if this is not the first breakpoint in the list for the given address. location of tracepoint can _never_ be duplicated with other locations of tracepoints and other diff --git a/gdb/guile/scm-breakpoint.c b/gdb/guile/scm-breakpoint.c index aed9f9a..92b3242 100644 --- a/gdb/guile/scm-breakpoint.c +++ b/gdb/guile/scm-breakpoint.c @@ -151,7 +151,6 @@ bpscm_enable_state_to_string (enum enable_state enable_state) case bp_disabled: return "disabled"; case bp_enabled: return "enabled"; case bp_call_disabled: return "call_disabled"; - case bp_permanent: return "permanent"; default: return "unknown"; } } -- 1.9.3 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFA] Always consider infcall breakpoints as non-permanent. 2014-11-12 10:55 ` Pedro Alves @ 2014-11-20 16:41 ` Joel Brobecker 2014-11-21 11:38 ` Pedro Alves 0 siblings, 1 reply; 13+ messages in thread From: Joel Brobecker @ 2014-11-20 16:41 UTC (permalink / raw) To: gdb-patches; +Cc: Pedro Alves Hello! A recent change... commit 1a853c5224e2b8fedfac6d029365522b83080b40 Date: Wed Nov 12 10:10:49 2014 +0000 Subject: make "permanent breakpoints" per location and disableable ... broke function calls on sparc-elf when running over QEMU. Any function call should demonstrate the problem. For instance, seen from the debugger: (gdb) call pn(1234) [Inferior 1 (Remote target) exited normally] The program being debugged exited while in a function called from GDB. Evaluation of the expression containing the function And seen from QEMU: qemu: fatal: Trap 0x02 while interrupts disabled, Error state [register dump removed] What happens in this case is that GDB sets the inferior function call by not only creating the dummy frame, but also writing a breakpoint instruction at the return address for our funcation call. See infcall.c: /* Write a legitimate instruction at the point where the infcall breakpoint is going to be inserted. While this instruction is never going to be executed, a user investigating the memory from GDB would see this instruction instead of random uninitialized bytes. We chose the breakpoint instruction as it may look as the most logical one to the user and also valgrind 3.7.0 needs it for proper vgdb inferior calls. If software breakpoints are unsupported for this target we leave the user visible memory content uninitialized. */ bp_addr_as_address = bp_addr; bp_bytes = gdbarch_breakpoint_from_pc (gdbarch, &bp_addr_as_address, &bp_size); if (bp_bytes != NULL) write_memory (bp_addr_as_address, bp_bytes, bp_size); This instruction triggers a change introduced by the commit above, where we consider bp locations as being permanent breakpoints if there is already a breakpoint instruction at that address: + if (bp_loc_is_permanent (loc)) + { + loc->inserted = 1; + loc->permanent = 1; + } As a result, when resuming the program's execution for the inferior function call, GDB decides that it does not need to insert a breakpoint at this address, expecting the target to just report a SIGTRAP when trying to execute that instruction. But unfortunately for us, at least some versions of QEMU for SPARC just terminate the execution entirely instead of reporting a breakpoint, thus producing the behavior reported here. Although it appears like QEMU might be misbehaving and should therefore be fixed (to be verified) from the user's point of view, the recent change does introduce a regression. So this patch tries to mitigate a bit the damage by handling such infcall breakpoints as special and making sure that they are never considered permanent, thus restoring the previous behavior specifically for those breakpoints. The option of not writing the breakpoint instructions ini the first place was considered, and would probably work also. But the comment associated to it seems to indicate that there is still reason to keep it. gdb/ChangeLog: * breakpoint.c (bp_loc_is_permanent): Return 0 if LOC corresponds to a bp_call_dummy breakpoint type. Tested on x86_64-linux. Also testing on sparc-elf/QEMU using AdaCore's testsuite. --- gdb/breakpoint.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 1c0a417..e22ac92 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -9289,6 +9289,19 @@ bp_loc_is_permanent (struct bp_location *loc) gdb_assert (loc != NULL); + /* bp_call_dummy breakpoint locations are usually memory locations where + GDB just wrote a breakpoint instruction, making it look as if there is + a permanent breakpoint at that location. Considering it permanent makes + GDB rely on that breakpoint instruction to stop the program, thus removing + the need to insert its own breakpoint there. This is normally expected to + work, except that some versions of QEMU (Eg: QEMU 2.0.0 for SPARC) just + report a fatal problem (Trap 0x02 while interrupts disabled, Error state) + intead of reporting a SIGTRAP. QEMU should probably be fixed, but in + the interest of compatibility with versions that behave this way, we always + consider bp_call_dummy breakpoint locations as non-permanent. */ + if (loc->owner->type == bp_call_dummy) + return 0; + addr = loc->address; bpoint = gdbarch_breakpoint_from_pc (loc->gdbarch, &addr, &len); -- 1.9.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFA] Always consider infcall breakpoints as non-permanent. 2014-11-20 16:41 ` [RFA] Always consider infcall breakpoints as non-permanent Joel Brobecker @ 2014-11-21 11:38 ` Pedro Alves 2014-11-23 10:44 ` Joel Brobecker 0 siblings, 1 reply; 13+ messages in thread From: Pedro Alves @ 2014-11-21 11:38 UTC (permalink / raw) To: Joel Brobecker, gdb-patches, qemu-devel [Adding qemu-devel@, so folks over there are aware of this. Original thread at https://sourceware.org/ml/gdb-patches/2014-11/msg00478.html] On 11/20/2014 04:41 PM, Joel Brobecker wrote: > A recent change... > > commit 1a853c5224e2b8fedfac6d029365522b83080b40 > Date: Wed Nov 12 10:10:49 2014 +0000 > Subject: make "permanent breakpoints" per location and disableable > > ... broke function calls on sparc-elf when running over QEMU. Any > function call should demonstrate the problem. > > For instance, seen from the debugger: > > (gdb) call pn(1234) > [Inferior 1 (Remote target) exited normally] > The program being debugged exited while in a function called from GDB. > Evaluation of the expression containing the function > > And seen from QEMU: > > qemu: fatal: Trap 0x02 while interrupts disabled, Error state > [register dump removed] Bah. Do you know whether this happens only on SPARC QEMU, or is this an issue for all QEMU ports? Basically, it sounds like with QEMU, GDB can't ever poke breakpoint instructions to memory. It must always set breakpoints with the Z0 packet. > > What happens in this case is that GDB sets the inferior function call > by not only creating the dummy frame, but also writing a breakpoint > instruction at the return address for our funcation call. See infcall.c: "funcation" > > /* Write a legitimate instruction at the point where the infcall > breakpoint is going to be inserted. While this instruction > is never going to be executed, a user investigating the > memory from GDB would see this instruction instead of random > uninitialized bytes. We chose the breakpoint instruction > as it may look as the most logical one to the user and also > valgrind 3.7.0 needs it for proper vgdb inferior calls. > > If software breakpoints are unsupported for this target we > leave the user visible memory content uninitialized. */ > > bp_addr_as_address = bp_addr; > bp_bytes = gdbarch_breakpoint_from_pc (gdbarch, &bp_addr_as_address, > &bp_size); > if (bp_bytes != NULL) > write_memory (bp_addr_as_address, bp_bytes, bp_size); > > This instruction triggers a change introduced by the commit above, > where we consider bp locations as being permanent breakpoints > if there is already a breakpoint instruction at that address: > > + if (bp_loc_is_permanent (loc)) > + { > + loc->inserted = 1; > + loc->permanent = 1; > + } > > As a result, when resuming the program's execution for the inferior > function call, GDB decides that it does not need to insert a breakpoint > at this address, expecting the target to just report a SIGTRAP when > trying to execute that instruction. > > But unfortunately for us, at least some versions of QEMU for SPARC > just terminate the execution entirely instead of reporting a breakpoint, > thus producing the behavior reported here. > > Although it appears like QEMU might be misbehaving and should therefore > be fixed (to be verified) from the user's point of view, the recent > change does introduce a regression. So this patch tries to mitigate > a bit the damage by handling such infcall breakpoints as special and > making sure that they are never considered permanent, thus restoring > the previous behavior specifically for those breakpoints. > > The option of not writing the breakpoint instructions ini the first "ini" > place was considered, and would probably work also. But the comment > associated to it seems to indicate that there is still reason to > keep it. > > gdb/ChangeLog: > > * breakpoint.c (bp_loc_is_permanent): Return 0 if LOC corresponds > to a bp_call_dummy breakpoint type. > > Tested on x86_64-linux. Also testing on sparc-elf/QEMU using > AdaCore's testsuite. > --- > gdb/breakpoint.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index 1c0a417..e22ac92 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -9289,6 +9289,19 @@ bp_loc_is_permanent (struct bp_location *loc) > > gdb_assert (loc != NULL); > > + /* bp_call_dummy breakpoint locations are usually memory locations where > + GDB just wrote a breakpoint instruction, making it look as if there is > + a permanent breakpoint at that location. Considering it permanent makes > + GDB rely on that breakpoint instruction to stop the program, thus removing > + the need to insert its own breakpoint there. This is normally expected to > + work, except that some versions of QEMU (Eg: QEMU 2.0.0 for SPARC) just > + report a fatal problem (Trap 0x02 while interrupts disabled, Error state) > + intead of reporting a SIGTRAP. QEMU should probably be fixed, but in "intead" > + the interest of compatibility with versions that behave this way, we always > + consider bp_call_dummy breakpoint locations as non-permanent. */ > + if (loc->owner->type == bp_call_dummy) > + return 0; > + > addr = loc->address; > bpoint = gdbarch_breakpoint_from_pc (loc->gdbarch, &addr, &len); Patch is OK. Thanks Joel. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFA] Always consider infcall breakpoints as non-permanent. 2014-11-21 11:38 ` Pedro Alves @ 2014-11-23 10:44 ` Joel Brobecker 0 siblings, 0 replies; 13+ messages in thread From: Joel Brobecker @ 2014-11-23 10:44 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, qemu-devel [-- Attachment #1: Type: text/plain, Size: 849 bytes --] > > And seen from QEMU: > > > > qemu: fatal: Trap 0x02 while interrupts disabled, Error state > > [register dump removed] > > Bah. Do you know whether this happens only on SPARC QEMU, or > is this an issue for all QEMU ports? According to Fabien, one of our QEMU experts, it's related to the handling of that specific instruction: it does what the manual says it should do - when interrupts are disables, the CPU resets. So while it does not preclude from other ports from having the same sort of issue, it looks like it's mostly target-specific. > > gdb/ChangeLog: > > > > * breakpoint.c (bp_loc_is_permanent): Return 0 if LOC corresponds > > to a bp_call_dummy breakpoint type. > Patch is OK. Thanks Joel. Thank you. Attached is the patch I checked in with the corrections you pointed out. Thanks, Pedro. -- Joel [-- Attachment #2: 0001-Always-consider-infcall-breakpoints-as-non-permanent.patch --] [-- Type: text/x-diff, Size: 5287 bytes --] From e8af5d7a5cd4c58136a4733e87612f49061bf28b Mon Sep 17 00:00:00 2001 From: Joel Brobecker <brobecker@adacore.com> Date: Thu, 20 Nov 2014 20:41:25 +0400 Subject: [PATCH] Always consider infcall breakpoints as non-permanent. A recent change... commit 1a853c5224e2b8fedfac6d029365522b83080b40 Date: Wed Nov 12 10:10:49 2014 +0000 Subject: make "permanent breakpoints" per location and disableable ... broke function calls on sparc-elf when running over QEMU. Any function call should demonstrate the problem. For instance, seen from the debugger: (gdb) call pn(1234) [Inferior 1 (Remote target) exited normally] The program being debugged exited while in a function called from GDB. Evaluation of the expression containing the function And seen from QEMU: qemu: fatal: Trap 0x02 while interrupts disabled, Error state [register dump removed] What happens in this case is that GDB sets the inferior function call by not only creating the dummy frame, but also writing a breakpoint instruction at the return address for our function call. See infcall.c: /* Write a legitimate instruction at the point where the infcall breakpoint is going to be inserted. While this instruction is never going to be executed, a user investigating the memory from GDB would see this instruction instead of random uninitialized bytes. We chose the breakpoint instruction as it may look as the most logical one to the user and also valgrind 3.7.0 needs it for proper vgdb inferior calls. If software breakpoints are unsupported for this target we leave the user visible memory content uninitialized. */ bp_addr_as_address = bp_addr; bp_bytes = gdbarch_breakpoint_from_pc (gdbarch, &bp_addr_as_address, &bp_size); if (bp_bytes != NULL) write_memory (bp_addr_as_address, bp_bytes, bp_size); This instruction triggers a change introduced by the commit above, where we consider bp locations as being permanent breakpoints if there is already a breakpoint instruction at that address: + if (bp_loc_is_permanent (loc)) + { + loc->inserted = 1; + loc->permanent = 1; + } As a result, when resuming the program's execution for the inferior function call, GDB decides that it does not need to insert a breakpoint at this address, expecting the target to just report a SIGTRAP when trying to execute that instruction. But unfortunately for us, at least some versions of QEMU for SPARC just terminate the execution entirely instead of reporting a breakpoint, thus producing the behavior reported here. Although it appears like QEMU might be misbehaving and should therefore be fixed (to be verified) from the user's point of view, the recent change does introduce a regression. So this patch tries to mitigate a bit the damage by handling such infcall breakpoints as special and making sure that they are never considered permanent, thus restoring the previous behavior specifically for those breakpoints. The option of not writing the breakpoint instructions in the first place was considered, and would probably work also. But the comment associated to it seems to indicate that there is still reason to keep it. gdb/ChangeLog: * breakpoint.c (bp_loc_is_permanent): Return 0 if LOC corresponds to a bp_call_dummy breakpoint type. Tested on x86_64-linux. Also testing on sparc-elf/QEMU using AdaCore's testsuite. --- gdb/ChangeLog | 5 +++++ gdb/breakpoint.c | 14 ++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 45435fc..4e8284e 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,8 @@ +2014-11-23 Joel Brobecker <brobecker@adacore.com> + + * breakpoint.c (bp_loc_is_permanent): Return 0 if LOC corresponds + to a bp_call_dummy breakpoint type. + 2014-11-23 Patrick Palka <patrick@parcs.ath.cx> * tui/tui-win.c (tui_initialize_win): Specify SA_RESTART when diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 7b56260..574d06c 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -9293,6 +9293,20 @@ bp_loc_is_permanent (struct bp_location *loc) gdb_assert (loc != NULL); + /* bp_call_dummy breakpoint locations are usually memory locations + where GDB just wrote a breakpoint instruction, making it look + as if there is a permanent breakpoint at that location. Considering + it permanent makes GDB rely on that breakpoint instruction to stop + the program, thus removing the need to insert its own breakpoint + there. This is normally expected to work, except that some versions + of QEMU (Eg: QEMU 2.0.0 for SPARC) just report a fatal problem (Trap + 0x02 while interrupts disabled, Error state) instead of reporting + a SIGTRAP. QEMU should probably be fixed, but in the interest of + compatibility with versions that behave this way, we always consider + bp_call_dummy breakpoint locations as non-permanent. */ + if (loc->owner->type == bp_call_dummy) + return 0; + addr = loc->address; bpoint = gdbarch_breakpoint_from_pc (loc->gdbarch, &addr, &len); -- 1.9.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-11-23 10:44 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-11-04 19:03 [PATCH 0/3] fix permanent breakpoints Pedro Alves 2014-11-04 19:03 ` [PATCH 3/3] fix skipping " Pedro Alves 2014-11-05 12:26 ` Yao Qi 2014-11-07 19:53 ` Pedro Alves 2014-11-12 0:54 ` Yao Qi 2014-11-12 10:53 ` Pedro Alves 2014-11-04 19:03 ` [PATCH 1/3] add a default method for gdbarch_skip_permanent_breakpoint Pedro Alves 2014-11-04 19:03 ` [PATCH 2/3] make "permanent breakpoints" per location and disableable Pedro Alves 2014-11-05 6:37 ` Yao Qi 2014-11-12 10:55 ` Pedro Alves 2014-11-20 16:41 ` [RFA] Always consider infcall breakpoints as non-permanent Joel Brobecker 2014-11-21 11:38 ` Pedro Alves 2014-11-23 10:44 ` Joel Brobecker
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).