[RFA] Stop infrun from tracking breakpoint insertion status.
Vladimir Prus
vladimir@codesourcery.com
Thu Nov 22 15:21:00 GMT 2007
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: delta.diff
Type: text/x-diff
Size: 3028 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20071122/db794dd5/attachment.bin>
More information about the Gdb-patches
mailing list