From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31761 invoked by alias); 20 Nov 2007 20:48:59 -0000 Received: (qmail 31751 invoked by uid 22791); 20 Nov 2007 20:48:58 -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, 20 Nov 2007 20:48:52 +0000 Received: (qmail 27431 invoked from network); 20 Nov 2007 20:48:49 -0000 Received: from unknown (HELO 172.16.unknown.plus.ru) (vladimir@127.0.0.2) by mail.codesourcery.com with ESMTPA; 20 Nov 2007 20:48:49 -0000 From: Vladimir Prus To: "Ulrich Weigand" Subject: Re: [RFA] Stop infrun from tracking breakpoint insertion status. Date: Tue, 20 Nov 2007 20:48:00 -0000 User-Agent: KMail/1.9.6 References: <200711191139.lAJBdRFc027229@d12av02.megacenter.de.ibm.com> In-Reply-To: <200711191139.lAJBdRFc027229@d12av02.megacenter.de.ibm.com> Cc: gdb-patches@sources.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200711202348.45105.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/msg00385.txt.bz2 On Monday 19 November 2007 14:39:27 you wrote: > Vladimir Prus wrote: > > > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > > index e81ec20..ef55a3d 100644 > > --- a/gdb/breakpoint.c > > +++ b/gdb/breakpoint.c > > @@ -296,6 +296,8 @@ 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. > > @@ -1312,6 +1314,12 @@ 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; > > > Is the breakpoints_meant_to_be_inserted_p variable actually ever > set to any non-zero value in this patch? I have looked into why I got no failures with the previous version of the patch. 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. 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. 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. This version of patch makes breakpoints_meant_to_be_inserted actually return 0 and 1 as needed, and adjust keep_going. 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, breakpoints_inserted indicates if infrun.c expects breakpoints to be inserted in the target, and that state is now kept in breakpoint.c * breakpoint.h (breakpoints_meant_to_be_inserted): New declaration. * breakpoint.c (breakpoints_meant_to_be_inserted_p): New. (breakpoints_meant_to_be_inserted): New. * 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. --- gdb/breakpoint.c | 12 +++++++++++ gdb/breakpoint.h | 7 ++++++ gdb/infrun.c | 59 +++++++++++++++++------------------------------------ 3 files changed, 38 insertions(+), 40 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 81614aa..d2bd232 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -296,6 +296,8 @@ 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. @@ -1235,6 +1237,8 @@ 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"); @@ -1299,6 +1303,8 @@ remove_breakpoints (void) struct bp_location *b; int val; + breakpoints_meant_to_be_inserted_p = 0; + ALL_BP_LOCATIONS (b) { if (b->inserted) @@ -1312,6 +1318,12 @@ 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 e4aa72a..236cd7b 100644 --- a/gdb/breakpoint.h +++ b/gdb/breakpoint.h @@ -854,4 +854,11 @@ 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 85d889a..9a1c24f 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,8 @@ 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; + breakpoints_were_inserted = breakpoints_meant_to_be_inserted (); + remove_breakpoints (); /* Check for any newly added shared libraries if we're supposed to be adding them automatically. Switch @@ -1381,10 +1369,7 @@ handle_inferior_event (struct execution_control_state *ecs) /* 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 +1655,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 +1768,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 +1929,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 +2060,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 +2134,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 +2159,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 +2168,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 +2230,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 +2244,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,7 +2847,7 @@ 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) + if (breakpoints_meant_to_be_inserted ()) insert_breakpoints (); } @@ -2968,9 +2945,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 +2960,6 @@ keep_going (struct execution_control_state *ecs) stop_stepping (ecs); return; } - breakpoints_inserted = 1; } trap_expected = ecs->another_trap; @@ -3172,7 +3152,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 +3163,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. */