From db82e815be270306f875a1971408ad933c926cca Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Fri, 20 Nov 2009 13:08:16 +0000 Subject: [PATCH] * infrun.c (handle_inferior_event): Hardware hatchpoint traps are never random signals. * breakpoint.c (update_global_location_list): Always delete immediately delete hardware watchpoint locations and other locations whose target address isn't meaningful. Update comment explaining the hazard of moribund locations. --- gdb/ChangeLog | 9 +++++++ gdb/breakpoint.c | 61 +++++++++++++++++++++++++++++++++++++----------- gdb/infrun.c | 16 +++++++++++++ 3 files changed, 73 insertions(+), 13 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index ea55fb1842..e2e2e1e274 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,12 @@ +2009-11-20 Pedro Alves + + * infrun.c (handle_inferior_event): Hardware hatchpoint traps are + never random signals. + * breakpoint.c (update_global_location_list): Always delete + immediately delete hardware watchpoint locations and other + locations whose target address isn't meaningful. Update comment + explaining the hazard of moribund locations. + 2009-11-19 Joel Brobecker * ada-lang.c (discrete_type_p): TYPE_CODE_BOOL is also a discrete type. diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 7968068a4a..493b1f4843 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -8300,20 +8300,55 @@ update_global_location_list (int should_insert) if (!found_object) { - if (removed && non_stop) + if (removed && non_stop + && breakpoint_address_is_meaningful (old_loc->owner) + && !is_hardware_watchpoint (old_loc->owner)) { - /* This location was removed from the targets. In non-stop mode, - a race condition is possible where we've removed a breakpoint, - but stop events for that breakpoint are already queued and will - arrive later. To suppress spurious SIGTRAPs reported to user, - we keep this breakpoint location for a bit, and will retire it - after we see 3 * thread_count events. - The theory here is that reporting of events should, - "on the average", be fair, so after that many event we'll see - events from all threads that have anything of interest, and no - longer need to keep this breakpoint. This is just a - heuristic, but if it's wrong, we'll report unexpected SIGTRAP, - which is usability issue, but not a correctness problem. */ + /* This location was removed from the target. In + non-stop mode, a race condition is possible where + we've removed a breakpoint, but stop events for that + breakpoint are already queued and will arrive later. + We apply an heuristic to be able to distinguish such + SIGTRAPs from other random SIGTRAPs: we keep this + breakpoint location for a bit, and will retire it + after we see some number of events. The theory here + is that reporting of events should, "on the average", + be fair, so after a while we'll see events from all + threads that have anything of interest, and no longer + need to keep this breakpoint location around. We + don't hold locations forever so to reduce chances of + mistaking a non-breakpoint SIGTRAP for a breakpoint + SIGTRAP. + + The heuristic failing can be disastrous on + decr_pc_after_break targets. + + On decr_pc_after_break targets, like e.g., x86-linux, + if we fail to recognize a late breakpoint SIGTRAP, + because events_till_retirement has reached 0 too + soon, we'll fail to do the PC adjustment, and report + a random SIGTRAP to the user. When the user resumes + the inferior, it will most likely immediately crash + with SIGILL/SIGBUS/SEGSEGV, or worse, get silently + corrupted, because of being resumed e.g., in the + middle of a multi-byte instruction, or skipped a + one-byte instruction. This was actually seen happen + on native x86-linux, and should be less rare on + targets that do not support new thread events, like + remote, due to the heuristic depending on + thread_count. + + Mistaking a random SIGTRAP for a breakpoint trap + causes similar symptoms (PC adjustment applied when + it shouldn't), but then again, playing with SIGTRAPs + behind the debugger's back is asking for trouble. + + Since hardware watchpoint traps are always + distinguishable from other traps, so we don't need to + apply keep hardware watchpoint moribund locations + around. We simply always ignore hardware watchpoint + traps we can no longer explain. */ + old_loc->events_till_retirement = 3 * (thread_count () + 1); old_loc->owner = NULL; diff --git a/gdb/infrun.c b/gdb/infrun.c index 3c17167335..cb5278c107 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -3593,6 +3593,21 @@ targets should add new threads to the thread list themselves in non-stop mode.") function. */ stop_print_frame = 1; + /* This is where we handle "moribund" watchpoints. Unlike + software breakpoints traps, hardware watchpoint traps are + always distinguishable from random traps. If no high-level + watchpoint is associated with the reported stop data address + anymore, then the bpstat does not explain the signal --- + simply make sure to ignore it if `stopped_by_watchpoint' is + set. */ + + if (debug_infrun + && ecs->event_thread->stop_signal == TARGET_SIGNAL_TRAP + && !bpstat_explains_signal (ecs->event_thread->stop_bpstat) + && stopped_by_watchpoint) + fprintf_unfiltered (gdb_stdlog, "\ +infrun: no user watchpoint explains watchpoint SIGTRAP, ignoring\n"); + /* NOTE: cagney/2003-03-29: These two checks for a random signal at one stage in the past included checks for an inferior function call's call dummy's return breakpoint. The original @@ -3616,6 +3631,7 @@ targets should add new threads to the thread list themselves in non-stop mode.") if (ecs->event_thread->stop_signal == TARGET_SIGNAL_TRAP) ecs->random_signal = !(bpstat_explains_signal (ecs->event_thread->stop_bpstat) + || stopped_by_watchpoint || ecs->event_thread->trap_expected || (ecs->event_thread->step_range_end && ecs->event_thread->step_resume_breakpoint == NULL)); -- 2.34.1