From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32210 invoked by alias); 22 Nov 2007 15:21:21 -0000 Received: (qmail 32194 invoked by uid 22791); 22 Nov 2007 15:21:19 -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; Thu, 22 Nov 2007 15:21:14 +0000 Received: (qmail 23881 invoked from network); 22 Nov 2007 15:21:11 -0000 Received: from unknown (HELO 172.16.unknown.plus.ru) (vladimir@127.0.0.2) by mail.codesourcery.com with ESMTPA; 22 Nov 2007 15:21:11 -0000 From: Vladimir Prus To: "Ulrich Weigand" Subject: Re: [RFA] Stop infrun from tracking breakpoint insertion status. Date: Thu, 22 Nov 2007 15:21:00 -0000 User-Agent: KMail/1.9.6 References: <200711220049.lAM0nMuF005074@d12av02.megacenter.de.ibm.com> In-Reply-To: <200711220049.lAM0nMuF005074@d12av02.megacenter.de.ibm.com> Cc: gdb-patches@sources.redhat.com MIME-Version: 1.0 Content-Type: Multipart/Mixed; boundary="Boundary-00=_e5ZRHBx9Uev8Cxb" Message-Id: <200711221821.02413.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/msg00411.txt.bz2 --Boundary-00=_e5ZRHBx9Uev8Cxb Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Content-length: 12408 On Thursday 22 November 2007 03:49:22 you wrote: > Vladimir Prus wrote: > > > The use of breakpoints_meant_to_be_inserted in handle_inferiour_event, > > for the TARGET_WAITKIND_LOADED, did not matter because > > TARGET_WAITKIND_LOADED is used by just a few targets. > > And even if remote.c can use it, it does not do when > > using gdbserver, which makes it hard to test. > > 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. > In any case, at this point breakpoints *must* be inserted -- the > very next thing the code does is to call > resume (0, TARGET_SIGNAL_0); > so if breakpoints were not inserted, we'd just run the inferior > to completion now. Yes, I think you're right. > So I think the check is not necessary, and we should simply > unconditionally insert breakpoints at this point. > > > The reason that use in keep_going does not break anything is that > > the intention of the code is to insert breakpoints, unless either > > they are already inserted, or ecs->another_trap is non-zero. However, > > insert_bp_location will immediately return if breakpoint location > > is already inserted. Therefore, the "already inserted" check is > > not necessary at all, and I removed it. > > OK, agreed. > > > As for the use in insert_step_resume_breakpoint_at_sal, I must admit > > I've got lost in the code again -- I don't know how previous version > > of the patch did not cause any problems. > > I think this is because calls to insert_step_resume_breakpoint_at_sal > are always followed by calls to keep_going -- and due to the behaviour > you described above, with your old patch keep_going would always > insert the step-resume breakpoint anyway. > > In fact, I think with the change to keep_going to always insert all > breakpoints there is no need for insert_step_resume_breakpoint_at_sal > to call insert_breakpoints at all anymore. This should probably > best be removed. I've checked, and indeed, keep_going is called in all cases insert_step_resume_breakpoint_at_sal is called. > If I've counted correctly, with the changes I've described we've > completely eliminated the need for breakpoints_meant_to_be_inserted. Yeah, that's even better result that I expected. > Would you mind updating your patch accordingly? Sure, the revised patch is below. The delta against previous patch version is attached. OK? Thanks, 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. * 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/infrun.c | 59 ++++++++++++++++----------------------------------------- 1 files changed, 17 insertions(+), 42 deletions(-) diff --git a/gdb/infrun.c b/gdb/infrun.c index 85d889a..08b0cf3 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 (); @@ -634,7 +630,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); @@ -783,12 +779,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; @@ -884,7 +875,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(). */ @@ -1334,10 +1324,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 @@ -1380,11 +1367,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 @@ -1670,7 +1653,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 (breakpoint_inserted_here_p (stop_pc)) { ecs->random_signal = 0; if (!breakpoint_thread_match (stop_pc, ecs->ptid)) @@ -1783,7 +1766,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; @@ -1945,7 +1927,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)) @@ -2076,8 +2058,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 @@ -2150,7 +2132,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)) @@ -2176,7 +2157,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) @@ -2186,9 +2166,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. */ @@ -2250,7 +2228,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; @@ -2265,9 +2242,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 @@ -2870,8 +2845,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 @@ -2968,9 +2941,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. */ @@ -2979,7 +2956,6 @@ keep_going (struct execution_control_state *ecs) stop_stepping (ecs); return; } - breakpoints_inserted = 1; } trap_expected = ecs->another_trap; @@ -3172,7 +3148,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 ()) { @@ -3183,7 +3159,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 --Boundary-00=_e5ZRHBx9Uev8Cxb Content-Type: text/x-diff; charset="iso-8859-1"; name="delta.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="delta.diff" Content-length: 3028 diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index d2bd232..81614aa 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -296,8 +296,6 @@ int breakpoint_count; /* Pointer to current exception event record */ static struct exception_event_record *current_exception_event; -static int breakpoints_meant_to_be_inserted_p; - /* This function returns a pointer to the string representation of the pathname of the dynamically-linked library that has just been loaded. @@ -1237,8 +1235,6 @@ insert_breakpoints (void) struct ui_file *tmp_error_stream = mem_fileopen (); make_cleanup_ui_file_delete (tmp_error_stream); - breakpoints_meant_to_be_inserted_p = 1; - /* Explicitly mark the warning -- this will only be printed if there was an error. */ fprintf_unfiltered (tmp_error_stream, "Warning:\n"); @@ -1303,8 +1299,6 @@ remove_breakpoints (void) struct bp_location *b; int val; - breakpoints_meant_to_be_inserted_p = 0; - ALL_BP_LOCATIONS (b) { if (b->inserted) @@ -1318,12 +1312,6 @@ remove_breakpoints (void) } int -breakpoints_meant_to_be_inserted (void) -{ - return breakpoints_meant_to_be_inserted_p; -} - -int remove_hw_watchpoints (void) { struct bp_location *b; diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h index 236cd7b..e4aa72a 100644 --- a/gdb/breakpoint.h +++ b/gdb/breakpoint.h @@ -854,11 +854,4 @@ extern int deprecated_remove_raw_breakpoint (void *); target. */ int watchpoints_triggered (struct target_waitstatus *); -/* Returns non-zero if insert_breakpoints was previously called, - and remove_breakpoints was not called after that. - This function allows to figure out if we meant that all breakpoints - be inserted in inferior. If so, any new breakpoints possibly - created must be inserted as well. */ -extern int breakpoints_meant_to_be_inserted (void); - #endif /* !defined (BREAKPOINT_H) */ diff --git a/gdb/infrun.c b/gdb/infrun.c index 9a1c24f..08b0cf3 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -1324,7 +1324,6 @@ handle_inferior_event (struct execution_control_state *ecs) /* Remove breakpoints, SOLIB_ADD might adjust breakpoint addresses via breakpoint_re_set. */ - breakpoints_were_inserted = breakpoints_meant_to_be_inserted (); remove_breakpoints (); /* Check for any newly added shared libraries if we're @@ -1368,8 +1367,7 @@ handle_inferior_event (struct execution_control_state *ecs) for "catch load". */ /* Reinsert breakpoints and continue. */ - if (breakpoints_were_inserted) - insert_breakpoints (); + insert_breakpoints (); } /* If we are skipping through a shell, or through shared library @@ -2847,8 +2845,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_meant_to_be_inserted ()) - insert_breakpoints (); } /* Insert a "step-resume breakpoint" at RETURN_FRAME.pc. This is used --Boundary-00=_e5ZRHBx9Uev8Cxb--