From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2944 invoked by alias); 27 Nov 2007 18:55:33 -0000 Received: (qmail 2934 invoked by uid 22791); 27 Nov 2007 18:55:31 -0000 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.31) with ESMTP; Tue, 27 Nov 2007 18:55:27 +0000 Received: (qmail 22165 invoked from network); 27 Nov 2007 18:55:24 -0000 Received: from unknown (HELO wind.local) (vladimir@127.0.0.2) by mail.codesourcery.com with ESMTPA; 27 Nov 2007 18:55:24 -0000 From: Vladimir Prus To: "Ulrich Weigand" Subject: Re: [RFA] Stop infrun from tracking breakpoint insertion status. Date: Tue, 27 Nov 2007 18:55:00 -0000 User-Agent: KMail/1.9.6 Cc: gdb-patches@sources.redhat.com References: <200711261524.lAQFOu2E025275@d12av02.megacenter.de.ibm.com> In-Reply-To: <200711261524.lAQFOu2E025275@d12av02.megacenter.de.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200711272155.17999.vladimir@codesourcery.com> Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2007-11/txt/msg00513.txt.bz2 On Monday 26 November 2007 18:24:56 Ulrich Weigand wrote: > Vladimir Prus wrote: > > > On Thursday 22 November 2007 03:49:22 you wrote: > > > Hmmm, if it helps, I could run a test on AIX, which does use > > > TARGET_WAITKIND_LOADED. > > > > That would surely help in convincing ourself the patch don't > > break anything. > > I did a test run on AIX now, and unfortunately it did break. > > I didn't look into the failure in detail, but apparently it > is unrelated to TARGET_WAITKIND_LOADED, but rather to software > single-step support: The second problem was that breakpoint_inserted_here_p checks for both ordinary breakpoints and single step breakpoints (which don't go on bp_location_chain). In most cases, it does not matter, but in one place the code tries to figure if we're stopped on a single step breakpoint or an ordinary one, and surely, breakpoint_inserted_here_p returning 1 for both cases was not good. The attached revision of the patch introduces a separate regular_breakpoint_inserted_here_p function. No regressions on either x86 or arm-linux. OK? - Volodya The checks of breakpoints_inserted before calling remove_breakpoints are removed, as remove_breakpoint won't touch uninserted breakpoints. In a number of places, we're interested if a breakpoint is inserted at particular PC, and we now use breakpoint_inserted_here_p. In a few places, insert_breakpoints can be called unconditionally, since it won't try to insert already inserted breakpoint. * breakpoint.h (regular_breakpoint_inserted_here_p): New declaration. * breakpoint.c (regular_breakpoint_inserted_here_p): New. (breakpoint_inserted_here_p): Use regular_breakpoint_inserted_here_p. * infrun.c (breakpoints_inserted): Remove. (resume): Don't check for breakpoints_inserted before remove_hw_watchpoints. Use breakpoint_inserted_here_p. (proceed, init_wait_for_inferior): Don't set breakpoints_inserted. (handle_inferior_event): Don't use breakpoints_inserted. Use breakpoints_meant_to_be_inserted and breakpoints_inserted_here_p. (insert_step_resume_breakpoint_at_sal, keep_going): Use breakpoints_meant_to_be_inserted. Don't set breakpoints_inserted. (normal_stop): Don't check for breakpoints_inserted. Don't set breakpoints_inserted. (keep_going): Don't check for breakpoints_inserted. (insert_step_resume_breakpoint_at_sal): Don't insert breakpoints --- gdb/breakpoint.c | 21 ++++++++++++++---- gdb/breakpoint.h | 2 + gdb/infrun.c | 59 +++++++++++++++-------------------------------------- 3 files changed, 35 insertions(+), 47 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 2203f6e..2717302 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -1757,12 +1757,13 @@ breakpoint_here_p (CORE_ADDR pc) } -/* breakpoint_inserted_here_p (PC) is just like breakpoint_here_p(), - but it only returns true if there is actually a breakpoint inserted - at PC. */ +/* Returns non-zero if there's a breakpoint inserted at PC, which is + inserted using regular breakpoint_chain/bp_location_chain mechanism. + This does not check for single-step breakpoints, which are + inserted and removed using direct target manipulation. */ int -breakpoint_inserted_here_p (CORE_ADDR pc) +regular_breakpoint_inserted_here_p (CORE_ADDR pc) { const struct bp_location *bpt; @@ -1783,8 +1784,18 @@ breakpoint_inserted_here_p (CORE_ADDR pc) return 1; } } + return 0; +} + +/* Returns non-zero iff there's either regular breakpoint + or a single step breakpoint inserted at PC. */ + +int +breakpoint_inserted_here_p (CORE_ADDR pc) +{ + if (regular_breakpoint_inserted_here_p (pc)) + return 1; - /* Also check for software single-step breakpoints. */ if (single_step_breakpoint_inserted_here_p (pc)) return 1; diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h index e4aa72a..4b84502 100644 --- a/gdb/breakpoint.h +++ b/gdb/breakpoint.h @@ -674,6 +674,8 @@ extern enum breakpoint_here breakpoint_here_p (CORE_ADDR); extern int breakpoint_inserted_here_p (CORE_ADDR); +extern int regular_breakpoint_inserted_here_p (CORE_ADDR); + extern int software_breakpoint_inserted_here_p (CORE_ADDR); extern int breakpoint_thread_match (CORE_ADDR, ptid_t); diff --git a/gdb/infrun.c b/gdb/infrun.c index 00cd2a5..c357f27 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -214,10 +214,6 @@ static unsigned char *signal_program; static struct cmd_list_element *stop_command; -/* Nonzero if breakpoints are now inserted in the inferior. */ - -static int breakpoints_inserted; - /* Function inferior was in as of last step command. */ static struct symbol *step_start_function; @@ -519,7 +515,7 @@ resume (int step, enum target_signal sig) Work around the problem by removing hardware watchpoints if a step is requested, GDB will check for a hardware watchpoint trigger after the step anyway. */ - if (CANNOT_STEP_HW_WATCHPOINTS && step && breakpoints_inserted) + if (CANNOT_STEP_HW_WATCHPOINTS && step) remove_hw_watchpoints (); @@ -635,7 +631,7 @@ a command like `return' or `jump' to continue execution.")); /* Most targets can step a breakpoint instruction, thus executing it normally. But if this one cannot, just continue and we will hit it anyway. */ - if (step && breakpoints_inserted && breakpoint_here_p (read_pc ())) + if (step && breakpoint_inserted_here_p (read_pc ())) step = 0; } target_resume (resume_ptid, step, sig); @@ -784,12 +780,7 @@ proceed (CORE_ADDR addr, enum target_signal siggnal, int step) Continue it automatically and insert breakpoints then. */ trap_expected = 1; else - { - insert_breakpoints (); - /* If we get here there was no call to error() in - insert breakpoints -- so they were inserted. */ - breakpoints_inserted = 1; - } + insert_breakpoints (); if (siggnal != TARGET_SIGNAL_DEFAULT) stop_signal = siggnal; @@ -885,7 +876,6 @@ init_wait_for_inferior (void) /* These are meaningless until the first time through wait_for_inferior. */ prev_pc = 0; - breakpoints_inserted = 0; breakpoint_init_inferior (inf_starting); /* Don't confuse first call to proceed(). */ @@ -1335,10 +1325,7 @@ handle_inferior_event (struct execution_control_state *ecs) /* Remove breakpoints, SOLIB_ADD might adjust breakpoint addresses via breakpoint_re_set. */ - breakpoints_were_inserted = breakpoints_inserted; - if (breakpoints_inserted) - remove_breakpoints (); - breakpoints_inserted = 0; + remove_breakpoints (); /* Check for any newly added shared libraries if we're supposed to be adding them automatically. Switch @@ -1381,11 +1368,7 @@ handle_inferior_event (struct execution_control_state *ecs) for "catch load". */ /* Reinsert breakpoints and continue. */ - if (breakpoints_were_inserted) - { - insert_breakpoints (); - breakpoints_inserted = 1; - } + insert_breakpoints (); } /* If we are skipping through a shell, or through shared library @@ -1671,7 +1654,7 @@ handle_inferior_event (struct execution_control_state *ecs) /* Check if a regular breakpoint has been hit before checking for a potential single step breakpoint. Otherwise, GDB will not see this breakpoint hit when stepping onto breakpoints. */ - if (breakpoints_inserted && breakpoint_here_p (stop_pc)) + if (regular_breakpoint_inserted_here_p (stop_pc)) { ecs->random_signal = 0; if (!breakpoint_thread_match (stop_pc, ecs->ptid)) @@ -1784,7 +1767,6 @@ handle_inferior_event (struct execution_control_state *ecs) } else { /* Single step */ - breakpoints_inserted = 0; if (!ptid_equal (inferior_ptid, ecs->ptid)) context_switch (ecs); ecs->waiton_ptid = ecs->ptid; @@ -1946,7 +1928,7 @@ handle_inferior_event (struct execution_control_state *ecs) stack. */ if (stop_signal == TARGET_SIGNAL_TRAP - || (breakpoints_inserted + || (breakpoint_inserted_here_p (stop_pc) && (stop_signal == TARGET_SIGNAL_ILL || stop_signal == TARGET_SIGNAL_SEGV || stop_signal == TARGET_SIGNAL_EMT)) @@ -2077,8 +2059,8 @@ process_event_stop_test: stop_signal = TARGET_SIGNAL_0; if (prev_pc == read_pc () - && !breakpoints_inserted && breakpoint_here_p (read_pc ()) + && !breakpoint_inserted_here_p (read_pc ()) && step_resume_breakpoint == NULL) { /* We were just starting a new sequence, attempting to @@ -2151,7 +2133,6 @@ process_event_stop_test: fprintf_unfiltered (gdb_stdlog, "infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME\n"); disable_longjmp_breakpoint (); remove_breakpoints (); - breakpoints_inserted = 0; if (!gdbarch_get_longjmp_target_p (current_gdbarch) || !gdbarch_get_longjmp_target (current_gdbarch, get_current_frame (), &jmp_buf_pc)) @@ -2177,7 +2158,6 @@ process_event_stop_test: if (debug_infrun) fprintf_unfiltered (gdb_stdlog, "infrun: BPSTAT_WHAT_CLEAR_LONGJMP_RESUME\n"); remove_breakpoints (); - breakpoints_inserted = 0; disable_longjmp_breakpoint (); ecs->handling_longjmp = 0; /* FIXME */ if (what.main_action == BPSTAT_WHAT_CLEAR_LONGJMP_RESUME) @@ -2187,9 +2167,7 @@ process_event_stop_test: case BPSTAT_WHAT_SINGLE: if (debug_infrun) fprintf_unfiltered (gdb_stdlog, "infrun: BPSTAT_WHAT_SINGLE\n"); - if (breakpoints_inserted) - remove_breakpoints (); - breakpoints_inserted = 0; + remove_breakpoints (); ecs->another_trap = 1; /* Still need to check other stuff, at least the case where we are stepping and step out of the right range. */ @@ -2251,7 +2229,6 @@ process_event_stop_test: to doing that. */ ecs->step_after_step_resume_breakpoint = 0; remove_breakpoints (); - breakpoints_inserted = 0; ecs->another_trap = 1; keep_going (ecs); return; @@ -2266,9 +2243,7 @@ process_event_stop_test: /* Remove breakpoints, we eventually want to step over the shlib event breakpoint, and SOLIB_ADD might adjust breakpoint addresses via breakpoint_re_set. */ - if (breakpoints_inserted) - remove_breakpoints (); - breakpoints_inserted = 0; + remove_breakpoints (); /* Check for any newly added shared libraries if we're supposed to be adding them automatically. Switch @@ -2871,8 +2846,6 @@ insert_step_resume_breakpoint_at_sal (struct symtab_and_line sr_sal, step_resume_breakpoint = set_momentary_breakpoint (sr_sal, sr_id, bp_step_resume); - if (breakpoints_inserted) - insert_breakpoints (); } /* Insert a "step-resume breakpoint" at RETURN_FRAME.pc. This is used @@ -2969,9 +2942,13 @@ keep_going (struct execution_control_state *ecs) The signal was SIGTRAP, e.g. it was our signal, but we decided we should resume from it. - We're going to run this baby now! */ + We're going to run this baby now! - if (!breakpoints_inserted && !ecs->another_trap) + Note that insert_breakpoints won't try to re-insert + already inserted breakpoints. Therefore, we don't + care if breakpoints were already inserted, or not. */ + + if (!ecs->another_trap) { /* Stop stepping when inserting breakpoints has failed. */ @@ -2980,7 +2957,6 @@ keep_going (struct execution_control_state *ecs) stop_stepping (ecs); return; } - breakpoints_inserted = 1; } trap_expected = ecs->another_trap; @@ -3173,7 +3149,7 @@ normal_stop (void) gdbarch_decr_pc_after_break needs to just go away. */ deprecated_update_frame_pc_hack (get_current_frame (), read_pc ()); - if (target_has_execution && breakpoints_inserted) + if (target_has_execution) { if (remove_breakpoints ()) { @@ -3184,7 +3160,6 @@ It might be running in another process.\n\ Further execution is probably impossible.\n")); } } - breakpoints_inserted = 0; /* Delete the breakpoint we stopped at, if it wants to be deleted. Delete any breakpoint that is to be deleted at the next stop. */ -- 1.5.3.5