* [PATCH 0/9] software single-step support rework, fix limitations
@ 2014-09-26 0:39 Pedro Alves
2014-09-26 0:39 ` [PATCH 1/9] Decide whether we may have removed breakpoints based on step_over_info Pedro Alves
` (8 more replies)
0 siblings, 9 replies; 20+ messages in thread
From: Pedro Alves @ 2014-09-26 0:39 UTC (permalink / raw)
To: gdb-patches
This series reworks software single-step support. In a nutshell:
- Makes single-step breakpoints regular breakpoints and puts them on
the global location chain.
- Makes the moribund location machinery aware of software single-step
breakpoints.
- Removes the currently limitation that only 2 single-step breakpoints
can be inserted. Some archs, like PPC, need this.
- Makes software single-step work when stepping through read-only code
regions.
- Adds support for having multiple threads software single-stepping
simultaneously. Currently, in non-stop, all single-step requests
are handled by displaced stepping the threads, which means threads
are serialized, because only one thread can be displaced stepping at
any given time.
- Finally eliminates deprecated_insert_raw_breakpoint and friends.
- Removes a set of run control globals along the way.
- And cleans things up here and there.
Tested on:
- x86_64 Fedora 20
- x86_64 Fedora 20 w/ 'software single-step on x86' series
- PPC64 Fedora 18
Pedro Alves (9):
Decide whether we may have removed breakpoints based on step_over_info
Rewrite non-continuable watchpoints handling
Put single-step breakpoints on the bp_location chain
Remove deprecated_insert_raw_breakpoint and friends
Switch back to stepped thread: clear step-over info
thread.c: cleanup breakpoint deletion
infrun.c: add for_each_just_stopped_thread
Make single-step breakpoints be per-thread
Non-stop + software single-step archs: don't force displaced-stepping
for all single-steps
gdb/breakpoint.c | 402 +++++++++++++-----------------------------------------
gdb/breakpoint.h | 31 ++---
gdb/gdbthread.h | 25 ++++
gdb/infrun.c | 360 ++++++++++++++++++++++--------------------------
gdb/infrun.h | 4 +
gdb/record-full.c | 8 +-
gdb/thread.c | 80 ++++++++---
7 files changed, 364 insertions(+), 546 deletions(-)
--
1.9.3
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 2/9] Rewrite non-continuable watchpoints handling
2014-09-26 0:39 [PATCH 0/9] software single-step support rework, fix limitations Pedro Alves
2014-09-26 0:39 ` [PATCH 1/9] Decide whether we may have removed breakpoints based on step_over_info Pedro Alves
@ 2014-09-26 0:39 ` Pedro Alves
2014-09-26 0:40 ` [PATCH 4/9] Remove deprecated_insert_raw_breakpoint and friends Pedro Alves
` (6 subsequent siblings)
8 siblings, 0 replies; 20+ messages in thread
From: Pedro Alves @ 2014-09-26 0:39 UTC (permalink / raw)
To: gdb-patches
When GDB finds out the target triggered a watchpoint, and the target
has non-continuable watchpoints, GDB sets things up to step past the
instruction that triggered the watchpoint. This is just like stepping
past a breakpoint, but goes through a different mechanism - it resumes
only the thread that needs to step past the watchpoint, but also
switches a "infwait state" global, that has the effect that the next
target_wait only wait for events only from that thread.
This forcing of a ptid to pass to target_wait obviously becomes a
bottleneck if we ever support stepping past different watchpoints
simultaneously (in separate processes).
It's also unnecessary -- the target should only return events for
threads that have been resumed; if no other thread than the one we're
stepping past the watchpoint has been resumed, then those other
threads should not report events. If we couldn't assume that, then
stepping past regular breakpoints would be broken for not likewise
forcing a similar infwait_state.
So this patch eliminates infwait_state, and instead teaches keep_going
to mark step_over_info in a way that has the breakpoints module skip
inserting watchpoints (because we're stepping past one), like it skips
breakpoints when we're stepping past one.
Tested on:
- x86_64 Fedora 20 (continuable watchpoints)
- PPC64 Fedora 18 (non-steppable watchpoints)
gdb/
2014-09-25 Pedro Alves <palves@redhat.com>
* breakpoint.c (should_be_inserted): Don't insert watchpoints if
trying to step past a non-steppable watchpoint.
* gdbthread.h (struct thread_info) <stepping_over_watchpoint>: New
field.
* infrun.c (struct step_over_info): Add new field
'nonsteppable_watchpoint_p' and adjust comments.
(set_step_over_info): New 'nonsteppable_watchpoint_p' parameter.
Adjust.
(clear_step_over_info): Clear nonsteppable_watchpoint_p as well.
(stepping_past_nonsteppable_watchpoint): New function.
(step_over_info_valid_p): Also return true if stepping past a
nonsteppable watchpoint.
(proceed): Adjust call to set_step_over_info. Remove reference to
init_infwait_state.
(init_wait_for_inferior): Remove reference to init_infwait_state.
(waiton_ptid): Delete global.
(struct execution_control_state)
<stepped_after_stopped_by_watchpoint>: Delete field.
(wait_for_inferior, fetch_inferior_event): Always pass
minus_one_ptid to target_wait.
(init_thread_stepping_state): Clear 'stepping_over_watchpoint'
field.
(init_infwait_state): Delete function.
(handle_inferior_event): Remove infwait_state handling.
(handle_signal_stop) <watchpoints handling>: Adjust after
stepped_after_stopped_by_watchpoint removal. Don't remove
breakpoints here nor set infwait_state. Set the thread's
stepping_over_watchpoint flag, and call keep_going instead.
(keep_going): Handle stepping_over_watchpoint. Adjust
set_step_over_info calls.
* infrun.h (stepping_past_nonsteppable_watchpoint): Declare
function.
---
gdb/breakpoint.c | 16 ++++++
gdb/gdbthread.h | 5 ++
gdb/infrun.c | 150 +++++++++++++++++++++----------------------------------
gdb/infrun.h | 4 ++
4 files changed, 82 insertions(+), 93 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index bec7f68..997514e 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2212,6 +2212,22 @@ should_be_inserted (struct bp_location *bl)
return 0;
}
+ /* Don't insert watchpoints if we're trying to step past the
+ instruction that triggered one. */
+ if ((bl->loc_type == bp_loc_hardware_watchpoint)
+ && stepping_past_nonsteppable_watchpoint ())
+ {
+ if (debug_infrun)
+ {
+ fprintf_unfiltered (gdb_stdlog,
+ "infrun: stepping past non-steppable watchpoint. "
+ "skipping watchpoint at %s:%d\n",
+ paddress (bl->gdbarch, bl->address),
+ bl->length);
+ }
+ return 0;
+ }
+
return 1;
}
diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 522b674..54c9fae 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -197,6 +197,11 @@ struct thread_info
/* Should we step over breakpoint next time keep_going is called? */
int stepping_over_breakpoint;
+ /* Should we step over a watchpoint next time keep_going is called?
+ This is needed on targets with non-continuable, non-steppable
+ watchpoints. */
+ int stepping_over_watchpoint;
+
/* Set to TRUE if we should finish single-stepping over a breakpoint
after hitting the current step-resume breakpoint. The context here
is that GDB is to do `next' or `step' while signal arrives.
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 6c8296d..94a58b2 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -373,8 +373,6 @@ static void context_switch (ptid_t ptid);
void init_thread_stepping_state (struct thread_info *tss);
-static void init_infwait_state (void);
-
static const char follow_fork_mode_child[] = "child";
static const char follow_fork_mode_parent[] = "parent";
@@ -968,16 +966,20 @@ static ptid_t singlestep_ptid;
/* PC when we started this single-step. */
static CORE_ADDR singlestep_pc;
-/* Info about an instruction that is being stepped over. Invalid if
- ASPACE is NULL. */
+/* Info about an instruction that is being stepped over. */
struct step_over_info
{
- /* The instruction's address space. */
+ /* If we're stepping past a breakpoint, this is the address space
+ and address of the instruction the breakpoint is set at. We'll
+ skip inserting all breakpoints here. Valid iff ASPACE is
+ non-NULL. */
struct address_space *aspace;
-
- /* The instruction's address. */
CORE_ADDR address;
+
+ /* The instruction being stepped over triggers a nonsteppable
+ watchpoint. If true, we'll skip inserting watchpoints. */
+ int nonsteppable_watchpoint_p;
};
/* The step-over info of the location that is being stepped over.
@@ -1010,10 +1012,12 @@ static struct step_over_info step_over_info;
stepping over. */
static void
-set_step_over_info (struct address_space *aspace, CORE_ADDR address)
+set_step_over_info (struct address_space *aspace, CORE_ADDR address,
+ int nonsteppable_watchpoint_p)
{
step_over_info.aspace = aspace;
step_over_info.address = address;
+ step_over_info.nonsteppable_watchpoint_p = nonsteppable_watchpoint_p;
}
/* Called when we're not longer stepping over a breakpoint / an
@@ -1024,6 +1028,7 @@ clear_step_over_info (void)
{
step_over_info.aspace = NULL;
step_over_info.address = 0;
+ step_over_info.nonsteppable_watchpoint_p = 0;
}
/* See infrun.h. */
@@ -1038,12 +1043,21 @@ stepping_past_instruction_at (struct address_space *aspace,
step_over_info.address));
}
+/* See infrun.h. */
+
+int
+stepping_past_nonsteppable_watchpoint (void)
+{
+ return step_over_info.nonsteppable_watchpoint_p;
+}
+
/* Returns true if step-over info is valid. */
static int
step_over_info_valid_p (void)
{
- return (step_over_info.aspace != NULL);
+ return (step_over_info.aspace != NULL
+ || stepping_past_nonsteppable_watchpoint ());
}
\f
@@ -2006,9 +2020,9 @@ a command like `return' or `jump' to continue execution."));
tp->suspend.stop_signal = GDB_SIGNAL_0;
/* Advise target which signals may be handled silently. If we have
- removed breakpoints because we are stepping over one, we need to
- receive all signals to avoid accidentally skipping a breakpoint
- during execution of a signal handler. */
+ removed breakpoints, we need to receive all signals to avoid
+ accidentally skipping a breakpoint during execution of a signal
+ handler. */
if (step_over_info_valid_p ())
target_pass_signals (0, NULL);
else
@@ -2315,7 +2329,7 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step)
struct regcache *regcache = get_current_regcache ();
set_step_over_info (get_regcache_aspace (regcache),
- regcache_read_pc (regcache));
+ regcache_read_pc (regcache), 0);
}
else
clear_step_over_info ();
@@ -2355,9 +2369,6 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step)
correctly when the inferior is stopped. */
tp->prev_pc = regcache_read_pc (get_current_regcache ());
- /* Reset to normal state. */
- init_infwait_state ();
-
/* Resume inferior. */
resume (tp->control.trap_expected || step || bpstat_should_step (),
tp->suspend.stop_signal);
@@ -2422,7 +2433,6 @@ init_wait_for_inferior (void)
target_last_wait_ptid = minus_one_ptid;
previous_inferior_ptid = inferior_ptid;
- init_infwait_state ();
/* Discard any skipped inlined frames. */
clear_inline_frame_state (minus_one_ptid);
@@ -2443,9 +2453,6 @@ enum infwait_states
infwait_nonstep_watch_state
};
-/* The PTID we'll do a target_wait on.*/
-ptid_t waiton_ptid;
-
/* Current inferior wait state. */
static enum infwait_states infwait_state;
@@ -2465,11 +2472,6 @@ struct execution_control_state
const char *stop_func_name;
int wait_some_more;
- /* We were in infwait_step_watch_state or
- infwait_nonstep_watch_state state, and the thread reported an
- event. */
- int stepped_after_stopped_by_watchpoint;
-
/* True if the event thread hit the single-step breakpoint of
another thread. Thus the event doesn't cause a stop, the thread
needs to be single-stepped past the single-step breakpoint before
@@ -2794,6 +2796,7 @@ wait_for_inferior (void)
struct execution_control_state ecss;
struct execution_control_state *ecs = &ecss;
struct cleanup *old_chain;
+ ptid_t waiton_ptid = minus_one_ptid;
memset (ecs, 0, sizeof (*ecs));
@@ -2849,6 +2852,7 @@ fetch_inferior_event (void *client_data)
struct cleanup *ts_old_chain;
int was_sync = sync_execution;
int cmd_done = 0;
+ ptid_t waiton_ptid = minus_one_ptid;
memset (ecs, 0, sizeof (*ecs));
@@ -2966,6 +2970,7 @@ void
init_thread_stepping_state (struct thread_info *tss)
{
tss->stepping_over_breakpoint = 0;
+ tss->stepping_over_watchpoint = 0;
tss->step_after_step_resume_breakpoint = 0;
}
@@ -3135,13 +3140,6 @@ adjust_pc_after_break (struct execution_control_state *ecs)
}
}
-static void
-init_infwait_state (void)
-{
- waiton_ptid = pid_to_ptid (-1);
- infwait_state = infwait_normal_state;
-}
-
static int
stepped_in_from (struct frame_info *frame, struct frame_id step_frame_id)
{
@@ -3362,40 +3360,6 @@ handle_inferior_event (struct execution_control_state *ecs)
&& ecs->ws.kind != TARGET_WAITKIND_EXITED)
set_executing (ecs->ptid, 0);
- switch (infwait_state)
- {
- case infwait_normal_state:
- if (debug_infrun)
- fprintf_unfiltered (gdb_stdlog, "infrun: infwait_normal_state\n");
- break;
-
- case infwait_step_watch_state:
- if (debug_infrun)
- fprintf_unfiltered (gdb_stdlog,
- "infrun: infwait_step_watch_state\n");
-
- ecs->stepped_after_stopped_by_watchpoint = 1;
- break;
-
- case infwait_nonstep_watch_state:
- if (debug_infrun)
- fprintf_unfiltered (gdb_stdlog,
- "infrun: infwait_nonstep_watch_state\n");
- insert_breakpoints ();
-
- /* FIXME-maybe: is this cleaner than setting a flag? Does it
- handle things like signals arriving and other things happening
- in combination correctly? */
- ecs->stepped_after_stopped_by_watchpoint = 1;
- break;
-
- default:
- internal_error (__FILE__, __LINE__, _("bad switch"));
- }
-
- infwait_state = infwait_normal_state;
- waiton_ptid = pid_to_ptid (-1);
-
switch (ecs->ws.kind)
{
case TARGET_WAITKIND_LOADED:
@@ -3985,7 +3949,9 @@ handle_signal_stop (struct execution_control_state *ecs)
singlestep_breakpoints_inserted_p = 0;
}
- if (ecs->stepped_after_stopped_by_watchpoint)
+ if (ecs->event_thread->suspend.stop_signal == GDB_SIGNAL_TRAP
+ && ecs->event_thread->control.trap_expected
+ && ecs->event_thread->stepping_over_watchpoint)
stopped_by_watchpoint = 0;
else
stopped_by_watchpoint = watchpoints_triggered (&ecs->ws);
@@ -4015,29 +3981,21 @@ handle_signal_stop (struct execution_control_state *ecs)
It is far more common to need to disable a watchpoint to step
the inferior over it. If we have non-steppable watchpoints,
we must disable the current watchpoint; it's simplest to
- disable all watchpoints and breakpoints. */
- int hw_step = 1;
-
- if (!target_have_steppable_watchpoint)
- {
- remove_breakpoints ();
- /* See comment in resume why we need to stop bypassing signals
- while breakpoints have been removed. */
- target_pass_signals (0, NULL);
- }
- /* Single step */
- hw_step = maybe_software_singlestep (gdbarch, stop_pc);
- target_resume (ecs->ptid, hw_step, GDB_SIGNAL_0);
- waiton_ptid = ecs->ptid;
- if (target_have_steppable_watchpoint)
- infwait_state = infwait_step_watch_state;
- else
- infwait_state = infwait_nonstep_watch_state;
- prepare_to_wait (ecs);
+ disable all watchpoints.
+
+ Any breakpoint at PC must also be stepped over -- if there's
+ one, it will have already triggered before the watchpoint
+ triggered, and we either already reported it to the user, or
+ it didn't cause a stop and we called keep_going. In either
+ case, if there was a breakpoint at PC, we must be trying to
+ step past it. */
+ ecs->event_thread->stepping_over_watchpoint = 1;
+ keep_going (ecs);
return;
}
ecs->event_thread->stepping_over_breakpoint = 0;
+ ecs->event_thread->stepping_over_watchpoint = 0;
bpstat_clear (&ecs->event_thread->control.stop_bpstat);
ecs->event_thread->control.stop_step = 0;
stop_print_frame = 1;
@@ -5773,6 +5731,8 @@ keep_going (struct execution_control_state *ecs)
{
volatile struct gdb_exception e;
struct regcache *regcache = get_current_regcache ();
+ int remove_bp;
+ int remove_wps;
/* Either the trap was not expected, but we are continuing
anyway (if we got a signal, the user asked it be passed to
@@ -5792,13 +5752,19 @@ keep_going (struct execution_control_state *ecs)
(watchpoints, etc.) but the one we're stepping over, step one
instruction, and then re-insert the breakpoint when that step
is finished. */
- if ((ecs->hit_singlestep_breakpoint
- || thread_still_needs_step_over (ecs->event_thread))
- && !use_displaced_stepping (get_regcache_arch (regcache)))
+
+ remove_bp = (ecs->hit_singlestep_breakpoint
+ || thread_still_needs_step_over (ecs->event_thread));
+ remove_wps = (ecs->event_thread->stepping_over_watchpoint
+ && !target_have_steppable_watchpoint);
+
+ if (remove_bp && !use_displaced_stepping (get_regcache_arch (regcache)))
{
set_step_over_info (get_regcache_aspace (regcache),
- regcache_read_pc (regcache));
+ regcache_read_pc (regcache), remove_wps);
}
+ else if (remove_wps)
+ set_step_over_info (NULL, 0, remove_wps);
else
clear_step_over_info ();
@@ -5814,9 +5780,7 @@ keep_going (struct execution_control_state *ecs)
return;
}
- ecs->event_thread->control.trap_expected
- = (ecs->event_thread->stepping_over_breakpoint
- || ecs->hit_singlestep_breakpoint);
+ ecs->event_thread->control.trap_expected = (remove_bp || remove_wps);
/* Do not deliver GDB_SIGNAL_TRAP (except when the user
explicitly specifies that such a signal should be delivered
diff --git a/gdb/infrun.h b/gdb/infrun.h
index cc9cb33..13127ed 100644
--- a/gdb/infrun.h
+++ b/gdb/infrun.h
@@ -122,6 +122,10 @@ extern void follow_inferior_reset_breakpoints (void);
extern int stepping_past_instruction_at (struct address_space *aspace,
CORE_ADDR address);
+/* Returns true if we're trying to step past an instruction that
+ triggers a non-steppable watchpoint. */
+extern int stepping_past_nonsteppable_watchpoint (void);
+
extern void set_step_info (struct frame_info *frame,
struct symtab_and_line sal);
--
1.9.3
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/9] Decide whether we may have removed breakpoints based on step_over_info
2014-09-26 0:39 [PATCH 0/9] software single-step support rework, fix limitations Pedro Alves
@ 2014-09-26 0:39 ` Pedro Alves
2014-09-28 12:52 ` Yao Qi
2014-09-26 0:39 ` [PATCH 2/9] Rewrite non-continuable watchpoints handling Pedro Alves
` (7 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Pedro Alves @ 2014-09-26 0:39 UTC (permalink / raw)
To: gdb-patches
... instead of trap_expected.
Gets rid of one singlestep_breakpoints_inserted_p reference, and is
generally more to the point.
gdb/
2014-09-22 Pedro Alves <palves@redhat.com>
* infrun.c (step_over_info_valid_p): New function.
(resume): Use step_over_info_valid_p instead of checking the
threads's trap_expected flag. Add debug output.
---
gdb/infrun.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 5e123be..6c8296d 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1038,6 +1038,14 @@ stepping_past_instruction_at (struct address_space *aspace,
step_over_info.address));
}
+/* Returns true if step-over info is valid. */
+
+static int
+step_over_info_valid_p (void)
+{
+ return (step_over_info.aspace != NULL);
+}
+
\f
/* Displaced stepping. */
@@ -1903,7 +1911,8 @@ a command like `return' or `jump' to continue execution."));
once we arrive back at the step-resume breakpoint, actually step
over the breakpoint we originally wanted to step over. */
if (singlestep_breakpoints_inserted_p
- && tp->control.trap_expected && sig != GDB_SIGNAL_0)
+ && sig != GDB_SIGNAL_0
+ && step_over_info_valid_p ())
{
/* If we have nested signals or a pending signal is delivered
immediately after a handler returns, might might already have
@@ -1997,13 +2006,10 @@ a command like `return' or `jump' to continue execution."));
tp->suspend.stop_signal = GDB_SIGNAL_0;
/* Advise target which signals may be handled silently. If we have
- removed breakpoints because we are stepping over one (which can
- happen only if we are not using displaced stepping), we need to
+ removed breakpoints because we are stepping over one, we need to
receive all signals to avoid accidentally skipping a breakpoint
during execution of a signal handler. */
- if ((step || singlestep_breakpoints_inserted_p)
- && tp->control.trap_expected
- && !use_displaced_stepping (gdbarch))
+ if (step_over_info_valid_p ())
target_pass_signals (0, NULL);
else
target_pass_signals ((int) GDB_SIGNAL_LAST, signal_pass);
--
1.9.3
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 5/9] Switch back to stepped thread: clear step-over info
2014-09-26 0:39 [PATCH 0/9] software single-step support rework, fix limitations Pedro Alves
` (3 preceding siblings ...)
2014-09-26 0:40 ` [PATCH 9/9] Non-stop + software single-step archs: don't force displaced-stepping for all single-steps Pedro Alves
@ 2014-09-26 0:40 ` Pedro Alves
2014-09-30 16:33 ` Pedro Alves
2014-09-26 0:40 ` [PATCH 3/9] Put single-step breakpoints on the bp_location chain Pedro Alves
` (3 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Pedro Alves @ 2014-09-26 0:40 UTC (permalink / raw)
To: gdb-patches
This path misses clearing the step-over info, because it bypasses
keep_going.
The testsuite caught this while I was working on making software
single-step breakpoints per-thread. Unfortunately, I didn't keep the
logs around and I don't recall ATM which test tripped on this.
gdb/
2014-09-25 Pedro Alves <palves@redhat.com>
* infrun.c (switch_back_to_stepped_thread) <expect thread advanced
also>: Clear step-over info.
---
gdb/infrun.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 94a58b2..e0df9bf 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -5275,6 +5275,13 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs)
fprintf_unfiltered (gdb_stdlog,
"infrun: expected thread advanced also\n");
+ /* Clear this before trying to insert the sss
+ breakpoint, in case we were previously trying to step
+ over this location in another thread, otherwise the
+ breakpoint ends up _not_ installed. It's what
+ keep_going would do too, if we called it. */
+ clear_step_over_info ();
+
insert_single_step_breakpoint (get_frame_arch (frame),
get_frame_address_space (frame),
stop_pc);
--
1.9.3
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 8/9] Make single-step breakpoints be per-thread
2014-09-26 0:39 [PATCH 0/9] software single-step support rework, fix limitations Pedro Alves
` (5 preceding siblings ...)
2014-09-26 0:40 ` [PATCH 3/9] Put single-step breakpoints on the bp_location chain Pedro Alves
@ 2014-09-26 0:40 ` Pedro Alves
2014-09-26 1:18 ` [PATCH 6/9] thread.c: cleanup breakpoint deletion Pedro Alves
2014-09-26 1:36 ` [PATCH 7/9] infrun.c: add for_each_just_stopped_thread Pedro Alves
8 siblings, 0 replies; 20+ messages in thread
From: Pedro Alves @ 2014-09-26 0:40 UTC (permalink / raw)
To: gdb-patches
This patch finally makes each thread have its own set of single-step
breakpoints. This paves the way to have multiple threads software
single-stepping, though this patch doesn't flip that switch on yet.
That'll be done on a subsequent patch.
gdb/
2014-09-25 Pedro Alves <palves@redhat.com>
* breakpoint.c (single_step_breakpoints): Delete global.
(insert_single_step_breakpoint): Adjust to store the breakpoint
pointer in the current thread.
(single_step_breakpoints_inserted, remove_single_step_breakpoints)
(cancel_single_step_breakpoints): Delete functions.
(breakpoint_has_location_inserted_here): Make extern.
(single_step_breakpoint_inserted_here_p): Adjust to walk the
breakpoint list.
* breakpoint.h (breakpoint_has_location_inserted_here): New
declaration.
(single_step_breakpoints_inserted, remove_single_step_breakpoints)
(cancel_single_step_breakpoints): Remove declarations.
* gdbthread.h (struct thread_control_state)
<single_step_breakpoints>: New field.
(delete_single_step_breakpoints)
(thread_has_single_step_breakpoints_set)
(thread_has_single_step_breakpoint_here): New declarations.
* infrun.c (follow_exec): Also clear the single-step breakpoints.
(singlestep_breakpoints_inserted_p, singlestep_ptid)
(singlestep_pc): Delete globals.
(infrun_thread_ptid_changed, maybe_software_singlestep): Remove
references to removed globals.
(resume): No longer force displaced stepping for all single-steps,
in non-stop mode. Adjust to use
thread_has_single_step_breakpoints_set and
delete_single_step_breakpoints.
(init_wait_for_inferior): Remove references to removed globals.
(delete_step_resume_breakpoint_callback): Delete single-step
breakpoints too.
(delete_stopped_threads_single_step_breakpoints): New function.
(adjust_pc_after_break): Adjust to use
thread_has_single_step_breakpoints_set.
(handle_inferior_event): Remove references to removed globals.
Use delete_stopped_threads_single_step_breakpoints.
(handle_signal_stop): Adjust to per-thread single-step
breakpoints. Swap test order to do cheaper tests first.
(switch_back_to_stepped_thread): Extend debug output. Remove
references to removed globals.
* record-full.c (record_full_wait_1): Adjust to per-thread
single-step breakpoints.
* thread.c (delete_single_step_breakpoints)
(thread_has_single_step_breakpoints_set)
(thread_has_single_step_breakpoint_here): New functions.
(clear_thread_inferior_resources): Also delete the thread's
single-step breakpoints.
---
gdb/breakpoint.c | 62 ++++++++---------------------
gdb/breakpoint.h | 10 +++--
gdb/gdbthread.h | 20 ++++++++++
gdb/infrun.c | 114 ++++++++++++++++++++++--------------------------------
gdb/record-full.c | 8 ++--
gdb/thread.c | 31 +++++++++++++++
6 files changed, 125 insertions(+), 120 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index bd3e08a..dacb867 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -326,11 +326,6 @@ static struct breakpoint_ops bkpt_probe_breakpoint_ops;
/* Dynamic printf class type. */
struct breakpoint_ops dprintf_breakpoint_ops;
-/* One (or perhaps two) breakpoints used for software single
- stepping. */
-
-static struct breakpoint *single_step_breakpoints;
-
/* The style in which to perform a dynamic printf. This is a user
option because different output options have different tradeoffs;
if GDB does the printing, there is better error handling if there
@@ -15310,55 +15305,24 @@ insert_single_step_breakpoint (struct gdbarch *gdbarch,
struct symtab_and_line sal;
CORE_ADDR pc = next_pc;
- if (single_step_breakpoints == NULL)
- single_step_breakpoints = new_single_step_breakpoint (tp->num, gdbarch);
+ if (tp->control.single_step_breakpoints == NULL)
+ {
+ tp->control.single_step_breakpoints
+ = new_single_step_breakpoint (tp->num, gdbarch);
+ }
sal = find_pc_line (pc, 0);
sal.pc = pc;
sal.section = find_pc_overlay (pc);
sal.explicit_pc = 1;
- add_location_to_breakpoint (single_step_breakpoints, &sal);
+ add_location_to_breakpoint (tp->control.single_step_breakpoints, &sal);
update_global_location_list (UGLL_INSERT);
}
-/* Check if the breakpoints used for software single stepping
- were inserted or not. */
+/* See breakpoint.h. */
int
-single_step_breakpoints_inserted (void)
-{
- return (single_step_breakpoints != NULL);
-}
-
-/* Remove and delete any breakpoints used for software single step. */
-
-void
-remove_single_step_breakpoints (void)
-{
- gdb_assert (single_step_breakpoints != NULL);
-
- delete_breakpoint (single_step_breakpoints);
-}
-
-/* Delete software single step breakpoints without removing them from
- the inferior. This is intended to be used if the inferior's address
- space where they were inserted is already gone, e.g. after exit or
- exec. */
-
-void
-cancel_single_step_breakpoints (void)
-{
- /* We don't really need to (or should) delete them here. After an
- exit, breakpoint_init_inferior deletes it. After an exec,
- update_breakpoints_after_exec does it. Just clear our
- reference. */
- single_step_breakpoints = NULL;
-}
-
-/* Check whether any location of BP is inserted at PC. */
-
-static int
breakpoint_has_location_inserted_here (struct breakpoint *bp,
struct address_space *aspace,
CORE_ADDR pc)
@@ -15380,9 +15344,15 @@ int
single_step_breakpoint_inserted_here_p (struct address_space *aspace,
CORE_ADDR pc)
{
- return (single_step_breakpoints != NULL
- && breakpoint_has_location_inserted_here (single_step_breakpoints,
- aspace, pc));
+ struct breakpoint *bpt;
+
+ ALL_BREAKPOINTS (bpt)
+ {
+ if (bpt->type == bp_single_step
+ && breakpoint_has_location_inserted_here (bpt, aspace, pc))
+ return 1;
+ }
+ return 0;
}
/* Returns 0 if 'bp' is NOT a syscall catchpoint,
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 47c7f7b..792a0a3 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1126,6 +1126,12 @@ extern int regular_breakpoint_inserted_here_p (struct address_space *,
extern int software_breakpoint_inserted_here_p (struct address_space *,
CORE_ADDR);
+/* Check whether any location of BP is inserted at PC. */
+
+extern int breakpoint_has_location_inserted_here (struct breakpoint *bp,
+ struct address_space *aspace,
+ CORE_ADDR pc);
+
extern int single_step_breakpoint_inserted_here_p (struct address_space *,
CORE_ADDR);
@@ -1460,10 +1466,6 @@ extern void delete_command (char *arg, int from_tty);
extern void insert_single_step_breakpoint (struct gdbarch *,
struct address_space *,
CORE_ADDR);
-extern int single_step_breakpoints_inserted (void);
-extern void remove_single_step_breakpoints (void);
-extern void cancel_single_step_breakpoints (void);
-
/* Check if any hardware watchpoints have triggered, according to the
target. */
int watchpoints_triggered (struct target_waitstatus *);
diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 54c9fae..c775e2e 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -52,6 +52,13 @@ struct thread_control_state
/* Exception-resume breakpoint. */
struct breakpoint *exception_resume_breakpoint;
+ /* Breakpoints used for software single stepping. Plural, because
+ it may have multiple locations. E.g., if stepping over a
+ conditional branch instruction we can't decode the condition for,
+ we'll need to put a breakpoint at the branch destination, and
+ another at the instruction after the branch. */
+ struct breakpoint *single_step_breakpoints;
+
/* Range to single step within.
If this is nonzero, respond to a single-step signal by continuing
@@ -285,6 +292,19 @@ extern void delete_step_resume_breakpoint (struct thread_info *);
/* Delete an exception_resume_breakpoint from the thread database. */
extern void delete_exception_resume_breakpoint (struct thread_info *);
+/* Delete the single-step breakpoints of thread TP, if any. */
+extern void delete_single_step_breakpoints (struct thread_info *tp);
+
+/* Check if the thread has software single stepping breakpoints
+ set. */
+extern int thread_has_single_step_breakpoints_set (struct thread_info *tp);
+
+/* Check whether the thread has software single stepping breakpoints
+ set at PC. */
+extern int thread_has_single_step_breakpoint_here (struct thread_info *tp,
+ struct address_space *aspace,
+ CORE_ADDR addr);
+
/* Translate the integer thread id (GDB's homegrown id, not the system's)
into a "pid" (which may be overloaded with extra thread information). */
extern ptid_t thread_id_to_pid (int);
diff --git a/gdb/infrun.c b/gdb/infrun.c
index d6d0948..4ff5b07 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -842,6 +842,7 @@ follow_exec (ptid_t pid, char *execd_pathname)
statement through an exec(). */
th->control.step_resume_breakpoint = NULL;
th->control.exception_resume_breakpoint = NULL;
+ th->control.single_step_breakpoints = NULL;
th->control.step_range_start = 0;
th->control.step_range_end = 0;
@@ -955,17 +956,6 @@ follow_exec (ptid_t pid, char *execd_pathname)
matically get reset there in the new process.). */
}
-/* Non-zero if we just simulating a single-step. This is needed
- because we cannot remove the breakpoints in the inferior process
- until after the `wait' in `wait_for_inferior'. */
-static int singlestep_breakpoints_inserted_p = 0;
-
-/* The thread we inserted single-step breakpoints for. */
-static ptid_t singlestep_ptid;
-
-/* PC when we started this single-step. */
-static CORE_ADDR singlestep_pc;
-
/* Info about an instruction that is being stepped over. */
struct step_over_info
@@ -1655,9 +1645,6 @@ infrun_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid)
if (ptid_equal (inferior_ptid, old_ptid))
inferior_ptid = new_ptid;
- if (ptid_equal (singlestep_ptid, old_ptid))
- singlestep_ptid = new_ptid;
-
for (displaced = displaced_step_inferior_states;
displaced;
displaced = displaced->next)
@@ -1732,11 +1719,6 @@ maybe_software_singlestep (struct gdbarch *gdbarch, CORE_ADDR pc)
&& gdbarch_software_single_step (gdbarch, get_current_frame ()))
{
hw_step = 0;
- /* Do not pull these breakpoints until after a `wait' in
- `wait_for_inferior'. */
- singlestep_breakpoints_inserted_p = 1;
- singlestep_ptid = inferior_ptid;
- singlestep_pc = pc;
}
return hw_step;
}
@@ -1924,7 +1906,7 @@ a command like `return' or `jump' to continue execution."));
at the current address, deliver the signal without stepping, and
once we arrive back at the step-resume breakpoint, actually step
over the breakpoint we originally wanted to step over. */
- if (singlestep_breakpoints_inserted_p
+ if (thread_has_single_step_breakpoints_set (tp)
&& sig != GDB_SIGNAL_0
&& step_over_info_valid_p ())
{
@@ -1939,8 +1921,7 @@ a command like `return' or `jump' to continue execution."));
tp->step_after_step_resume_breakpoint = 1;
}
- remove_single_step_breakpoints ();
- singlestep_breakpoints_inserted_p = 0;
+ delete_single_step_breakpoints (tp);
clear_step_over_info ();
tp->control.trap_expected = 0;
@@ -1951,7 +1932,7 @@ a command like `return' or `jump' to continue execution."));
/* If STEP is set, it's a request to use hardware stepping
facilities. But in that case, we should never
use singlestep breakpoint. */
- gdb_assert (!(singlestep_breakpoints_inserted_p && step));
+ gdb_assert (!(thread_has_single_step_breakpoints_set (tp) && step));
/* Decide the set of threads to ask the target to resume. Start
by assuming everything will be resumed, than narrow the set
@@ -1967,7 +1948,7 @@ a command like `return' or `jump' to continue execution."));
set_running (resume_ptid, 1);
/* Maybe resume a single thread after all. */
- if ((step || singlestep_breakpoints_inserted_p)
+ if ((step || thread_has_single_step_breakpoints_set (tp))
&& tp->control.trap_expected)
{
/* We're allowing a thread to run past a breakpoint it has
@@ -2436,9 +2417,6 @@ init_wait_for_inferior (void)
/* Discard any skipped inlined frames. */
clear_inline_frame_state (minus_one_ptid);
-
- singlestep_ptid = null_ptid;
- singlestep_pc = 0;
}
\f
@@ -2614,6 +2592,7 @@ delete_thread_infrun_breakpoints (struct thread_info *tp)
{
delete_step_resume_breakpoint (tp);
delete_exception_resume_breakpoint (tp);
+ delete_single_step_breakpoints (tp);
}
/* If the target still has execution, call FUNC for each thread that
@@ -2655,6 +2634,15 @@ delete_just_stopped_threads_infrun_breakpoints (void)
for_each_just_stopped_thread (delete_thread_infrun_breakpoints);
}
+/* Delete the single-step breakpoints of the threads that just
+ stopped. */
+
+static void
+delete_just_stopped_threads_single_step_breakpoints (void)
+{
+ for_each_just_stopped_thread (delete_single_step_breakpoints);
+}
+
/* A cleanup wrapper. */
static void
@@ -3138,7 +3126,7 @@ adjust_pc_after_break (struct execution_control_state *ecs)
software breakpoint. In this case (prev_pc == breakpoint_pc),
we also need to back up to the breakpoint address. */
- if (singlestep_breakpoints_inserted_p
+ if (thread_has_single_step_breakpoints_set (ecs->event_thread)
|| !ptid_equal (ecs->ptid, inferior_ptid)
|| !currently_stepping (ecs->event_thread)
|| ecs->event_thread->prev_pc == breakpoint_pc)
@@ -3524,8 +3512,6 @@ Cannot fill $_exitsignal with the correct signal number.\n"));
gdb_flush (gdb_stdout);
target_mourn_inferior ();
- singlestep_breakpoints_inserted_p = 0;
- cancel_single_step_breakpoints ();
stop_print_frame = 0;
stop_waiting (ecs);
return;
@@ -3618,12 +3604,7 @@ Cannot fill $_exitsignal with the correct signal number.\n"));
detach_breakpoints (ecs->ws.value.related_pid);
}
- if (singlestep_breakpoints_inserted_p)
- {
- /* Pull the single step breakpoints out of the target. */
- remove_single_step_breakpoints ();
- singlestep_breakpoints_inserted_p = 0;
- }
+ delete_just_stopped_threads_single_step_breakpoints ();
/* In case the event is caught by a catchpoint, remember that
the event is to be followed at the next resume of the thread,
@@ -3710,9 +3691,6 @@ Cannot fill $_exitsignal with the correct signal number.\n"));
if (!ptid_equal (ecs->ptid, inferior_ptid))
context_switch (ecs->ptid);
- singlestep_breakpoints_inserted_p = 0;
- cancel_single_step_breakpoints ();
-
stop_pc = regcache_read_pc (get_thread_regcache (ecs->ptid));
/* Do whatever is necessary to the parent branch of the vfork. */
@@ -3778,14 +3756,7 @@ Cannot fill $_exitsignal with the correct signal number.\n"));
fprintf_unfiltered (gdb_stdlog, "infrun: TARGET_WAITKIND_NO_HISTORY\n");
/* Reverse execution: target ran out of history info. */
- /* Pull the single step breakpoints out of the target. */
- if (singlestep_breakpoints_inserted_p)
- {
- if (!ptid_equal (ecs->ptid, inferior_ptid))
- context_switch (ecs->ptid);
- remove_single_step_breakpoints ();
- singlestep_breakpoints_inserted_p = 0;
- }
+ delete_just_stopped_threads_single_step_breakpoints ();
stop_pc = regcache_read_pc (get_thread_regcache (ecs->ptid));
observer_notify_no_history ();
stop_waiting (ecs);
@@ -3924,37 +3895,47 @@ handle_signal_stop (struct execution_control_state *ecs)
gdbarch = get_frame_arch (frame);
/* Pull the single step breakpoints out of the target. */
- if (singlestep_breakpoints_inserted_p)
+ if (ecs->event_thread->suspend.stop_signal == GDB_SIGNAL_TRAP
+ && gdbarch_software_single_step_p (gdbarch))
{
+ struct regcache *regcache;
+ struct address_space *aspace;
+ CORE_ADDR pc;
+
+ regcache = get_thread_regcache (ecs->ptid);
+ aspace = get_regcache_aspace (regcache);
+ pc = regcache_read_pc (regcache);
+
/* However, before doing so, if this single-step breakpoint was
actually for another thread, set this thread up for moving
past it. */
- if (!ptid_equal (ecs->ptid, singlestep_ptid)
- && ecs->event_thread->suspend.stop_signal == GDB_SIGNAL_TRAP)
+ if (!thread_has_single_step_breakpoint_here (ecs->event_thread,
+ aspace, pc))
{
- struct regcache *regcache;
- struct address_space *aspace;
- CORE_ADDR pc;
-
- regcache = get_thread_regcache (ecs->ptid);
- aspace = get_regcache_aspace (regcache);
- pc = regcache_read_pc (regcache);
if (single_step_breakpoint_inserted_here_p (aspace, pc))
{
if (debug_infrun)
{
fprintf_unfiltered (gdb_stdlog,
- "infrun: [%s] hit step over single-step"
- " breakpoint of [%s]\n",
- target_pid_to_str (ecs->ptid),
- target_pid_to_str (singlestep_ptid));
+ "infrun: [%s] hit another thread's "
+ "single-step breakpoint\n",
+ target_pid_to_str (ecs->ptid));
}
ecs->hit_singlestep_breakpoint = 1;
}
}
+ else
+ {
+ if (debug_infrun)
+ {
+ fprintf_unfiltered (gdb_stdlog,
+ "infrun: [%s] hit its "
+ "single-step breakpoint\n",
+ target_pid_to_str (ecs->ptid));
+ }
+ }
- remove_single_step_breakpoints ();
- singlestep_breakpoints_inserted_p = 0;
+ delete_just_stopped_threads_single_step_breakpoints ();
}
if (ecs->event_thread->suspend.stop_signal == GDB_SIGNAL_TRAP
@@ -5281,7 +5262,9 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs)
{
if (debug_infrun)
fprintf_unfiltered (gdb_stdlog,
- "infrun: expected thread advanced also\n");
+ "infrun: expected thread advanced also: prev_pc=%s, stop_pc=%s\n",
+ paddress (gdbarch, tp->prev_pc),
+ paddress (gdbarch, stop_pc));
/* Clear this before trying to insert the sss
breakpoint, in case we were previously trying to step
@@ -5293,10 +5276,7 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs)
insert_single_step_breakpoint (get_frame_arch (frame),
get_frame_address_space (frame),
stop_pc);
- singlestep_breakpoints_inserted_p = 1;
ecs->event_thread->control.trap_expected = 1;
- singlestep_ptid = inferior_ptid;
- singlestep_pc = stop_pc;
resume (0, GDB_SIGNAL_0);
prepare_to_wait (ecs);
diff --git a/gdb/record-full.c b/gdb/record-full.c
index 119361f..a2257ed 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -961,7 +961,7 @@ record_full_resume (struct target_ops *ops, ptid_t ptid, int step,
else
{
/* This arch support soft sigle step. */
- if (single_step_breakpoints_inserted ())
+ if (thread_has_single_step_breakpoints_set (inferior_thread ()))
{
/* This is a soft single step. */
record_full_resume_step = 1;
@@ -1085,6 +1085,8 @@ record_full_wait_1 (struct target_ops *ops,
while (1)
{
+ struct thread_info *tp;
+
ret = ops->beneath->to_wait (ops->beneath, ptid, status, options);
if (status->kind == TARGET_WAITKIND_IGNORE)
{
@@ -1095,8 +1097,8 @@ record_full_wait_1 (struct target_ops *ops,
return ret;
}
- if (single_step_breakpoints_inserted ())
- remove_single_step_breakpoints ();
+ ALL_NON_EXITED_THREADS (tp)
+ delete_single_step_breakpoints (tp);
if (record_full_resume_step)
return ret;
diff --git a/gdb/thread.c b/gdb/thread.c
index ea4a9db..f5ce07e 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -111,6 +111,15 @@ delete_exception_resume_breakpoint (struct thread_info *tp)
delete_thread_breakpoint (&tp->control.exception_resume_breakpoint);
}
+/* See gdbthread.h. */
+
+void
+delete_single_step_breakpoints (struct thread_info *tp)
+{
+ if (tp != NULL)
+ delete_thread_breakpoint (&tp->control.single_step_breakpoints);
+}
+
/* Delete the breakpoint pointed at by BP_P at the next stop, if
there's one. */
@@ -124,6 +133,27 @@ delete_at_next_stop (struct breakpoint **bp)
}
}
+/* See gdbthread.h. */
+
+int
+thread_has_single_step_breakpoints_set (struct thread_info *tp)
+{
+ return tp->control.single_step_breakpoints != NULL;
+}
+
+/* See gdbthread.h. */
+
+int
+thread_has_single_step_breakpoint_here (struct thread_info *tp,
+ struct address_space *aspace,
+ CORE_ADDR addr)
+{
+ struct breakpoint *ss_bps = tp->control.single_step_breakpoints;
+
+ return (ss_bps != NULL
+ && breakpoint_has_location_inserted_here (ss_bps, aspace, addr));
+}
+
static void
clear_thread_inferior_resources (struct thread_info *tp)
{
@@ -133,6 +163,7 @@ clear_thread_inferior_resources (struct thread_info *tp)
be stopped at the moment. */
delete_at_next_stop (&tp->control.step_resume_breakpoint);
delete_at_next_stop (&tp->control.exception_resume_breakpoint);
+ delete_at_next_stop (&tp->control.single_step_breakpoints);
delete_longjmp_breakpoint_at_next_stop (tp->num);
--
1.9.3
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/9] Remove deprecated_insert_raw_breakpoint and friends
2014-09-26 0:39 [PATCH 0/9] software single-step support rework, fix limitations Pedro Alves
2014-09-26 0:39 ` [PATCH 1/9] Decide whether we may have removed breakpoints based on step_over_info Pedro Alves
2014-09-26 0:39 ` [PATCH 2/9] Rewrite non-continuable watchpoints handling Pedro Alves
@ 2014-09-26 0:40 ` Pedro Alves
2014-09-26 0:40 ` [PATCH 9/9] Non-stop + software single-step archs: don't force displaced-stepping for all single-steps Pedro Alves
` (5 subsequent siblings)
8 siblings, 0 replies; 20+ messages in thread
From: Pedro Alves @ 2014-09-26 0:40 UTC (permalink / raw)
To: gdb-patches
There are no users of deprecated_{insert,remove}_raw_breakpoint left.
gdb/
2014-09-25 Pedro Alves <palves@redhat.com>
* breakpoint.c (regular_breakpoint_inserted_here_p): Inline ...
(breakpoint_inserted_here_p): ... here. Remove special case for
software single-step breakpoints.
(find_non_raw_software_breakpoint_inserted_here): Inline ...
(software_breakpoint_inserted_here_p): ... here. Remove special
case for software single-step breakpoints.
(bp_target_info_copy_insertion_state)
(deprecated_insert_raw_breakpoint)
(deprecated_remove_raw_breakpoint): Delete functions.
* breakpoint.h (deprecated_insert_raw_breakpoint)
(deprecated_remove_raw_breakpoint): Remove declarations.
---
gdb/breakpoint.c | 144 +++----------------------------------------------------
gdb/breakpoint.h | 8 ----
2 files changed, 7 insertions(+), 145 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 0064d88..bd3e08a 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -4242,14 +4242,10 @@ moribund_breakpoint_here_p (struct address_space *aspace, CORE_ADDR pc)
return 0;
}
-/* Returns non-zero if there's a breakpoint inserted at PC, which is
- inserted using regular breakpoint_chain / bp_location array
- mechanism. This does not check for single-step breakpoints, which
- are inserted and removed using direct target manipulation. */
+/* Returns non-zero iff there's a breakpoint inserted at PC. */
int
-regular_breakpoint_inserted_here_p (struct address_space *aspace,
- CORE_ADDR pc)
+breakpoint_inserted_here_p (struct address_space *aspace, CORE_ADDR pc)
{
struct bp_location *bl, **blp_tmp;
@@ -4273,27 +4269,12 @@ regular_breakpoint_inserted_here_p (struct address_space *aspace,
return 0;
}
-/* Returns non-zero iff there's either regular breakpoint
- or a single step breakpoint inserted at PC. */
+/* This function returns non-zero iff there is a software breakpoint
+ inserted at PC. */
int
-breakpoint_inserted_here_p (struct address_space *aspace, CORE_ADDR pc)
-{
- if (regular_breakpoint_inserted_here_p (aspace, pc))
- return 1;
-
- if (single_step_breakpoint_inserted_here_p (aspace, pc))
- return 1;
-
- return 0;
-}
-
-/* Ignoring deprecated raw breakpoints, return non-zero iff there is a
- software breakpoint inserted at PC. */
-
-static struct bp_location *
-find_non_raw_software_breakpoint_inserted_here (struct address_space *aspace,
- CORE_ADDR pc)
+software_breakpoint_inserted_here_p (struct address_space *aspace,
+ CORE_ADDR pc)
{
struct bp_location *bl, **blp_tmp;
@@ -4311,27 +4292,10 @@ find_non_raw_software_breakpoint_inserted_here (struct address_space *aspace,
&& !section_is_mapped (bl->section))
continue; /* unmapped overlay -- can't be a match */
else
- return bl;
+ return 1;
}
}
- return NULL;
-}
-
-/* This function returns non-zero iff there is a software breakpoint
- inserted at PC. */
-
-int
-software_breakpoint_inserted_here_p (struct address_space *aspace,
- CORE_ADDR pc)
-{
- if (find_non_raw_software_breakpoint_inserted_here (aspace, pc) != NULL)
- return 1;
-
- /* Also check for software single-step breakpoints. */
- if (single_step_breakpoint_inserted_here_p (aspace, pc))
- return 1;
-
return 0;
}
@@ -13322,19 +13286,6 @@ bkpt_re_set (struct breakpoint *b)
breakpoint_re_set_default (b);
}
-/* Copy SRC's shadow buffer and whatever else we'd set if we actually
- inserted DEST, so we can remove it later, in case SRC is removed
- first. */
-
-static void
-bp_target_info_copy_insertion_state (struct bp_target_info *dest,
- const struct bp_target_info *src)
-{
- dest->shadow_len = src->shadow_len;
- memcpy (dest->shadow_contents, src->shadow_contents, src->shadow_len);
- dest->placed_size = src->placed_size;
-}
-
static int
bkpt_insert_location (struct bp_location *bl)
{
@@ -15348,87 +15299,6 @@ invalidate_bp_value_on_memory_change (struct inferior *inferior,
}
}
-/* Create and insert a raw software breakpoint at PC. Return an
- identifier, which should be used to remove the breakpoint later.
- In general, places which call this should be using something on the
- breakpoint chain instead; this function should be eliminated
- someday. */
-
-void *
-deprecated_insert_raw_breakpoint (struct gdbarch *gdbarch,
- struct address_space *aspace, CORE_ADDR pc)
-{
- struct bp_target_info *bp_tgt;
- struct bp_location *bl;
-
- bp_tgt = XCNEW (struct bp_target_info);
-
- bp_tgt->placed_address_space = aspace;
- bp_tgt->placed_address = pc;
-
- /* If an unconditional non-raw breakpoint is already inserted at
- that location, there's no need to insert another. However, with
- target-side evaluation of breakpoint conditions, if the
- breakpoint that is currently inserted on the target is
- conditional, we need to make it unconditional. Note that a
- breakpoint with target-side commands is not reported even if
- unconditional, so we need to remove the commands from the target
- as well. */
- bl = find_non_raw_software_breakpoint_inserted_here (aspace, pc);
- if (bl != NULL
- && VEC_empty (agent_expr_p, bl->target_info.conditions)
- && VEC_empty (agent_expr_p, bl->target_info.tcommands))
- {
- bp_target_info_copy_insertion_state (bp_tgt, &bl->target_info);
- return bp_tgt;
- }
-
- if (target_insert_breakpoint (gdbarch, bp_tgt) != 0)
- {
- /* Could not insert the breakpoint. */
- xfree (bp_tgt);
- return NULL;
- }
-
- return bp_tgt;
-}
-
-/* Remove a breakpoint BP inserted by
- deprecated_insert_raw_breakpoint. */
-
-int
-deprecated_remove_raw_breakpoint (struct gdbarch *gdbarch, void *bp)
-{
- struct bp_target_info *bp_tgt = bp;
- struct address_space *aspace = bp_tgt->placed_address_space;
- CORE_ADDR address = bp_tgt->placed_address;
- struct bp_location *bl;
- int ret;
-
- bl = find_non_raw_software_breakpoint_inserted_here (aspace, address);
-
- /* Only remove the raw breakpoint if there are no other non-raw
- breakpoints still inserted at this location. Otherwise, we would
- be effectively disabling those breakpoints. */
- if (bl == NULL)
- ret = target_remove_breakpoint (gdbarch, bp_tgt);
- else if (!VEC_empty (agent_expr_p, bl->target_info.conditions)
- || !VEC_empty (agent_expr_p, bl->target_info.tcommands))
- {
- /* The target is evaluating conditions, and when we inserted the
- software single-step breakpoint, we had made the breakpoint
- unconditional and command-less on the target side. Reinsert
- to restore the conditions/commands. */
- ret = target_insert_breakpoint (bl->gdbarch, &bl->target_info);
- }
- else
- ret = 0;
-
- xfree (bp_tgt);
-
- return ret;
-}
-
/* Create and insert a breakpoint for software single step. */
void
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 12f1158..47c7f7b 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1464,14 +1464,6 @@ extern int single_step_breakpoints_inserted (void);
extern void remove_single_step_breakpoints (void);
extern void cancel_single_step_breakpoints (void);
-/* Manage manual breakpoints, separate from the normal chain of
- breakpoints. These functions are used in murky target-specific
- ways. Please do not add more uses! */
-extern void *deprecated_insert_raw_breakpoint (struct gdbarch *,
- struct address_space *,
- CORE_ADDR);
-extern int deprecated_remove_raw_breakpoint (struct gdbarch *, void *);
-
/* Check if any hardware watchpoints have triggered, according to the
target. */
int watchpoints_triggered (struct target_waitstatus *);
--
1.9.3
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 9/9] Non-stop + software single-step archs: don't force displaced-stepping for all single-steps
2014-09-26 0:39 [PATCH 0/9] software single-step support rework, fix limitations Pedro Alves
` (2 preceding siblings ...)
2014-09-26 0:40 ` [PATCH 4/9] Remove deprecated_insert_raw_breakpoint and friends Pedro Alves
@ 2014-09-26 0:40 ` Pedro Alves
2014-09-26 0:40 ` [PATCH 5/9] Switch back to stepped thread: clear step-over info Pedro Alves
` (4 subsequent siblings)
8 siblings, 0 replies; 20+ messages in thread
From: Pedro Alves @ 2014-09-26 0:40 UTC (permalink / raw)
To: gdb-patches
This finally reverts this bit of commit 929dfd4f:
2009-07-31 Pedro Alves <pedro@codesourcery.com>
Julian Brown <julian@codesourcery.com>
...
(resume): If this is a software single-stepping arch, and
displaced-stepping is enabled, use it for all single-step
requests.
...
That means that in non-stop (or really displaced-stepping) mode, on
software single-step archs - even those that only use sss breakpoints
to deal with atomic sequences, like PPC - if we have more than one
thread single-stepping, we'll always serialize the threads'
single-steps, as only one thread may be displaced stepping at a given
time, because there's only one scratch pad.
We originally did that because GDB didn't support having multiple
threads software-single-stepping simultaneously. The previous patches
fixed that limitation, so we can now finally revert this too.
Tested on:
- x86_64 Fedora 20, on top of the 'software single-step on x86'
series.
- PPC64 Fedora 18 (installs gdbarch_software_single_step hook to
deal with atomic sequences).
gdb/
2014-09-25 Pedro Alves <palves@redhat.com>
* infrun.c (resume): Don't force displaced-stepping for all
single-steps on software single-stepping archs.
---
gdb/infrun.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 4ff5b07..e51bc7c 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1846,8 +1846,7 @@ a command like `return' or `jump' to continue execution."));
event, displaced stepping breaks the vfork child similarly as single
step software breakpoint. */
if (use_displaced_stepping (gdbarch)
- && (tp->control.trap_expected
- || (step && gdbarch_software_single_step_p (gdbarch)))
+ && tp->control.trap_expected
&& sig == GDB_SIGNAL_0
&& !current_inferior ()->waiting_for_vfork_done)
{
--
1.9.3
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 3/9] Put single-step breakpoints on the bp_location chain
2014-09-26 0:39 [PATCH 0/9] software single-step support rework, fix limitations Pedro Alves
` (4 preceding siblings ...)
2014-09-26 0:40 ` [PATCH 5/9] Switch back to stepped thread: clear step-over info Pedro Alves
@ 2014-09-26 0:40 ` Pedro Alves
2014-09-28 12:36 ` Yao Qi
2014-09-29 6:33 ` Yao Qi
2014-09-26 0:40 ` [PATCH 8/9] Make single-step breakpoints be per-thread Pedro Alves
` (2 subsequent siblings)
8 siblings, 2 replies; 20+ messages in thread
From: Pedro Alves @ 2014-09-26 0:40 UTC (permalink / raw)
To: gdb-patches
This patch makes single-step breakpoints "real" breakpoints on the
global location list.
There are several benefits to this:
- It removes the currently limitation that only 2 single-step
breakpoints can be inserted. See an example here of a discussion
around a case that wants more than 2, possibly unbounded:
https://sourceware.org/ml/gdb-patches/2014-03/msg00663.html
- makes software single-step work on read-only code regions.
The logic to convert a software breakpoint to a hardware breakpoint
if the memory map says the breakpoint address is in read only memory
is in insert_bp_location. Because software single-step breakpoints
bypass all that go and straight to target_insert_breakpoint, we
can't software single-step over read only memory. This patch should
remove that limitation, though I haven't tested that (should be
testable with a test that forces a code region to read-only with
"mem LOW HIGH ro").
- Paves the way to have multiple threads software single-stepping at
the same time, leaving update_global_location_list to worry about
duplicate locations.
- Makes the moribund location machinery aware of software single-step
breakpoints, paving the way to enable software single-step on
non-stop, instead of forcing displaced stepping for all single
steps.
- It's generaly cleaner.
We no longer have to play games with single-step breakpoints
inserted at the same address as regular breakpoints, like we
recently had to do for 7.8. See this discussion:
https://sourceware.org/ml/gdb-patches/2014-06/msg00052.html.
Tested on x86_64 Fedora 20, on top of my 'single-step breakpoints on
x86' series.
gdb/
2014-09-25 Pedro Alves <palves@redhat.com>
* breakpoint.c (single_step_breakpoints, single_step_gdbarch):
Delete array globals.
(single_step_breakpoints): New global.
(breakpoint_xfer_memory): Remove special handling for single-step
breakpoints.
(update_breakpoints_after_exec): Delete bp_single_step
breakpoints.
(detach_breakpoints): Remove special handling for single-step
breakpoints.
(breakpoint_init_inferior): Delete bp_single_step breakpoints.
(bpstat_stop_status): Add comment.
(bpstat_what, bptype_string, print_one_breakpoint_location)
(init_bp_location): Handle bp_single_step.
(new_single_step_breakpoint): New function.
(set_momentary_breakpoint, bkpt_remove_location): Remove special
handling for single-step breakpoints.
(insert_single_step_breakpoint, single_step_breakpoints_inserted)
(remove_single_step_breakpoints, cancel_single_step_breakpoints):
Rewrite.
(detach_single_step_breakpoints, find_single_step_breakpoint):
Delete functions.
(breakpoint_has_location_inserted_here): New function.
(single_step_breakpoint_inserted_here_p): Rewrite.
* breakpoint.h: Remove FIXME.
(enum bptype) <bp_single_step>: New enum value.
(insert_single_step_breakpoint): Update comment.
---
gdb/breakpoint.c | 216 ++++++++++++++++++++-----------------------------------
gdb/breakpoint.h | 13 ++--
2 files changed, 81 insertions(+), 148 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 997514e..0064d88 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -226,11 +226,6 @@ static void stopat_command (char *arg, int from_tty);
static void tcatch_command (char *arg, int from_tty);
-static void detach_single_step_breakpoints (void);
-
-static int find_single_step_breakpoint (struct address_space *aspace,
- CORE_ADDR pc);
-
static void free_bp_location (struct bp_location *loc);
static void incref_bp_location (struct bp_location *loc);
static void decref_bp_location (struct bp_location **loc);
@@ -334,8 +329,7 @@ struct breakpoint_ops dprintf_breakpoint_ops;
/* One (or perhaps two) breakpoints used for software single
stepping. */
-static void *single_step_breakpoints[2];
-static struct gdbarch *single_step_gdbarch[2];
+static struct breakpoint *single_step_breakpoints;
/* The style in which to perform a dynamic printf. This is a user
option because different output options have different tradeoffs;
@@ -1668,21 +1662,6 @@ breakpoint_xfer_memory (gdb_byte *readbuf, gdb_byte *writebuf,
one_breakpoint_xfer_memory (readbuf, writebuf, writebuf_org,
memaddr, len, &bl->target_info, bl->gdbarch);
}
-
- /* Now process single-step breakpoints. These are not found in the
- bp_location array. */
- for (i = 0; i < 2; i++)
- {
- struct bp_target_info *bp_tgt = single_step_breakpoints[i];
-
- if (bp_tgt != NULL)
- {
- struct gdbarch *gdbarch = single_step_gdbarch[i];
-
- one_breakpoint_xfer_memory (readbuf, writebuf, writebuf_org,
- memaddr, len, bp_tgt, gdbarch);
- }
- }
}
\f
@@ -3790,6 +3769,13 @@ update_breakpoints_after_exec (void)
continue;
}
+ /* Just like single-step breakpoints. */
+ if (b->type == bp_single_step)
+ {
+ delete_breakpoint (b);
+ continue;
+ }
+
/* Longjmp and longjmp-resume breakpoints are also meaningless
after an exec. */
if (b->type == bp_longjmp || b->type == bp_longjmp_resume
@@ -3887,9 +3873,6 @@ detach_breakpoints (ptid_t ptid)
val |= remove_breakpoint_1 (bl, mark_inserted);
}
- /* Detach single-step breakpoints as well. */
- detach_single_step_breakpoints ();
-
do_cleanups (old_chain);
return val;
}
@@ -4159,6 +4142,10 @@ breakpoint_init_inferior (enum inf_context context)
/* Also remove step-resume breakpoints. */
+ case bp_single_step:
+
+ /* Also remove single-step breakpoints. */
+
delete_breakpoint (b);
break;
@@ -5635,6 +5622,7 @@ bpstat_stop_status (struct address_space *aspace,
}
}
+ /* Check if a moribund breakpoint explains the stop. */
for (ix = 0; VEC_iterate (bp_location_p, moribund_locations, ix, loc); ++ix)
{
if (breakpoint_location_address_match (loc, aspace, bp_addr))
@@ -5790,6 +5778,7 @@ bpstat_what (bpstat bs_head)
break;
case bp_breakpoint:
case bp_hardware_breakpoint:
+ case bp_single_step:
case bp_until:
case bp_finish:
case bp_shlib_event:
@@ -6148,6 +6137,7 @@ bptype_string (enum bptype type)
{bp_none, "?deleted?"},
{bp_breakpoint, "breakpoint"},
{bp_hardware_breakpoint, "hw breakpoint"},
+ {bp_single_step, "sw single-step"},
{bp_until, "until"},
{bp_finish, "finish"},
{bp_watchpoint, "watchpoint"},
@@ -6339,6 +6329,7 @@ print_one_breakpoint_location (struct breakpoint *b,
case bp_breakpoint:
case bp_hardware_breakpoint:
+ case bp_single_step:
case bp_until:
case bp_finish:
case bp_longjmp:
@@ -7217,6 +7208,7 @@ init_bp_location (struct bp_location *loc, const struct bp_location_ops *ops,
switch (owner->type)
{
case bp_breakpoint:
+ case bp_single_step:
case bp_until:
case bp_finish:
case bp_longjmp:
@@ -9199,10 +9191,31 @@ enable_breakpoints_after_startup (void)
breakpoint_re_set ();
}
+/* Create a new single-step breakpoint for thread THREAD, with no
+ locations. */
+
+static struct breakpoint *
+new_single_step_breakpoint (int thread, struct gdbarch *gdbarch)
+{
+ struct breakpoint *b = XNEW (struct breakpoint);
+
+ init_raw_breakpoint_without_location (b, gdbarch, bp_single_step,
+ &momentary_breakpoint_ops);
+
+ b->disposition = disp_donttouch;
+ b->frame_id = null_frame_id;
+
+ b->thread = thread;
+ gdb_assert (b->thread != 0);
+
+ add_to_breakpoint_chain (b);
+
+ return b;
+}
-/* Set a breakpoint that will evaporate an end of command
- at address specified by SAL.
- Restrict it to frame FRAME if FRAME is nonzero. */
+/* Set a momentary breakpoint of type TYPE at address specified by
+ SAL. If FRAME_ID is valid, the breakpoint is restricted to that
+ frame. */
struct breakpoint *
set_momentary_breakpoint (struct gdbarch *gdbarch, struct symtab_and_line sal,
@@ -13326,28 +13339,9 @@ static int
bkpt_insert_location (struct bp_location *bl)
{
if (bl->loc_type == bp_loc_hardware_breakpoint)
- return target_insert_hw_breakpoint (bl->gdbarch,
- &bl->target_info);
+ return target_insert_hw_breakpoint (bl->gdbarch, &bl->target_info);
else
- {
- struct bp_target_info *bp_tgt = &bl->target_info;
- int ret;
- int sss_slot;
-
- /* There is no need to insert a breakpoint if an unconditional
- raw/sss breakpoint is already inserted at that location. */
- sss_slot = find_single_step_breakpoint (bp_tgt->placed_address_space,
- bp_tgt->placed_address);
- if (sss_slot >= 0)
- {
- struct bp_target_info *sss_bp_tgt = single_step_breakpoints[sss_slot];
-
- bp_target_info_copy_insertion_state (bp_tgt, sss_bp_tgt);
- return 0;
- }
-
- return target_insert_breakpoint (bl->gdbarch, bp_tgt);
- }
+ return target_insert_breakpoint (bl->gdbarch, &bl->target_info);
}
static int
@@ -13356,19 +13350,7 @@ bkpt_remove_location (struct bp_location *bl)
if (bl->loc_type == bp_loc_hardware_breakpoint)
return target_remove_hw_breakpoint (bl->gdbarch, &bl->target_info);
else
- {
- struct bp_target_info *bp_tgt = &bl->target_info;
- struct address_space *aspace = bp_tgt->placed_address_space;
- CORE_ADDR address = bp_tgt->placed_address;
-
- /* Only remove the breakpoint if there is no raw/sss breakpoint
- still inserted at this location. Otherwise, we would be
- effectively disabling the raw/sss breakpoint. */
- if (single_step_breakpoint_inserted_here_p (aspace, address))
- return 0;
-
- return target_remove_breakpoint (bl->gdbarch, bp_tgt);
- }
+ return target_remove_breakpoint (bl->gdbarch, &bl->target_info);
}
static int
@@ -15454,31 +15436,20 @@ insert_single_step_breakpoint (struct gdbarch *gdbarch,
struct address_space *aspace,
CORE_ADDR next_pc)
{
- void **bpt_p;
+ struct thread_info *tp = inferior_thread ();
+ struct symtab_and_line sal;
+ CORE_ADDR pc = next_pc;
- if (single_step_breakpoints[0] == NULL)
- {
- bpt_p = &single_step_breakpoints[0];
- single_step_gdbarch[0] = gdbarch;
- }
- else
- {
- gdb_assert (single_step_breakpoints[1] == NULL);
- bpt_p = &single_step_breakpoints[1];
- single_step_gdbarch[1] = gdbarch;
- }
+ if (single_step_breakpoints == NULL)
+ single_step_breakpoints = new_single_step_breakpoint (tp->num, gdbarch);
- /* NOTE drow/2006-04-11: A future improvement to this function would
- be to only create the breakpoints once, and actually put them on
- the breakpoint chain. That would let us use set_raw_breakpoint.
- We could adjust the addresses each time they were needed. Doing
- this requires corresponding changes elsewhere where single step
- breakpoints are handled, however. So, for now, we use this. */
+ sal = find_pc_line (pc, 0);
+ sal.pc = pc;
+ sal.section = find_pc_overlay (pc);
+ sal.explicit_pc = 1;
+ add_location_to_breakpoint (single_step_breakpoints, &sal);
- *bpt_p = deprecated_insert_raw_breakpoint (gdbarch, aspace, next_pc);
- if (*bpt_p == NULL)
- error (_("Could not insert single-step breakpoint at %s"),
- paddress (gdbarch, next_pc));
+ update_global_location_list (UGLL_INSERT);
}
/* Check if the breakpoints used for software single stepping
@@ -15487,8 +15458,7 @@ insert_single_step_breakpoint (struct gdbarch *gdbarch,
int
single_step_breakpoints_inserted (void)
{
- return (single_step_breakpoints[0] != NULL
- || single_step_breakpoints[1] != NULL);
+ return (single_step_breakpoints != NULL);
}
/* Remove and delete any breakpoints used for software single step. */
@@ -15496,22 +15466,9 @@ single_step_breakpoints_inserted (void)
void
remove_single_step_breakpoints (void)
{
- gdb_assert (single_step_breakpoints[0] != NULL);
+ gdb_assert (single_step_breakpoints != NULL);
- /* See insert_single_step_breakpoint for more about this deprecated
- call. */
- deprecated_remove_raw_breakpoint (single_step_gdbarch[0],
- single_step_breakpoints[0]);
- single_step_gdbarch[0] = NULL;
- single_step_breakpoints[0] = NULL;
-
- if (single_step_breakpoints[1] != NULL)
- {
- deprecated_remove_raw_breakpoint (single_step_gdbarch[1],
- single_step_breakpoints[1]);
- single_step_gdbarch[1] = NULL;
- single_step_breakpoints[1] = NULL;
- }
+ delete_breakpoint (single_step_breakpoints);
}
/* Delete software single step breakpoints without removing them from
@@ -15522,51 +15479,28 @@ remove_single_step_breakpoints (void)
void
cancel_single_step_breakpoints (void)
{
- int i;
-
- for (i = 0; i < 2; i++)
- if (single_step_breakpoints[i])
- {
- xfree (single_step_breakpoints[i]);
- single_step_breakpoints[i] = NULL;
- single_step_gdbarch[i] = NULL;
- }
+ /* We don't really need to (or should) delete them here. After an
+ exit, breakpoint_init_inferior deletes it. After an exec,
+ update_breakpoints_after_exec does it. Just clear our
+ reference. */
+ single_step_breakpoints = NULL;
}
-/* Detach software single-step breakpoints from INFERIOR_PTID without
- removing them. */
-
-static void
-detach_single_step_breakpoints (void)
-{
- int i;
-
- for (i = 0; i < 2; i++)
- if (single_step_breakpoints[i])
- target_remove_breakpoint (single_step_gdbarch[i],
- single_step_breakpoints[i]);
-}
-
-/* Find the software single-step breakpoint that inserted at PC.
- Returns its slot if found, and -1 if not found. */
+/* Check whether any location of BP is inserted at PC. */
static int
-find_single_step_breakpoint (struct address_space *aspace,
- CORE_ADDR pc)
+breakpoint_has_location_inserted_here (struct breakpoint *bp,
+ struct address_space *aspace,
+ CORE_ADDR pc)
{
- int i;
+ struct bp_location *loc;
- for (i = 0; i < 2; i++)
- {
- struct bp_target_info *bp_tgt = single_step_breakpoints[i];
- if (bp_tgt
- && breakpoint_address_match (bp_tgt->placed_address_space,
- bp_tgt->placed_address,
- aspace, pc))
- return i;
- }
+ for (loc = bp->loc; loc != NULL; loc = loc->next)
+ if (loc->inserted
+ && breakpoint_location_address_match (loc, aspace, pc))
+ return 1;
- return -1;
+ return 0;
}
/* Check whether a software single-step breakpoint is inserted at
@@ -15576,7 +15510,9 @@ int
single_step_breakpoint_inserted_here_p (struct address_space *aspace,
CORE_ADDR pc)
{
- return find_single_step_breakpoint (aspace, pc) >= 0;
+ return (single_step_breakpoints != NULL
+ && breakpoint_has_location_inserted_here (single_step_breakpoints,
+ aspace, pc));
}
/* Returns 0 if 'bp' is NOT a syscall catchpoint,
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index e191c10..12f1158 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -47,18 +47,13 @@ struct linespec_sals;
\f
/* Type of breakpoint. */
-/* FIXME In the future, we should fold all other breakpoint-like
- things into here. This includes:
-
- * single-step (for machines where we have to simulate single
- stepping) (probably, though perhaps it is better for it to look as
- much as possible like a single-step to wait_for_inferior). */
enum bptype
{
bp_none = 0, /* Eventpoint has been deleted */
bp_breakpoint, /* Normal breakpoint */
bp_hardware_breakpoint, /* Hardware assisted breakpoint */
+ bp_single_step, /* Software single-step */
bp_until, /* used by until command */
bp_finish, /* used by finish command */
bp_watchpoint, /* Watchpoint */
@@ -1458,8 +1453,10 @@ extern void add_solib_catchpoint (char *arg, int is_load, int is_temp,
deletes all breakpoints. */
extern void delete_command (char *arg, int from_tty);
-/* Manage a software single step breakpoint (or two). Insert may be
- called twice before remove is called. */
+/* Create and insert a new software single step breakpoint for the
+ current thread. May be called multiple times; each time will add a
+ new location to the set of potential addresses the next instruction
+ is at. */
extern void insert_single_step_breakpoint (struct gdbarch *,
struct address_space *,
CORE_ADDR);
--
1.9.3
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 6/9] thread.c: cleanup breakpoint deletion
2014-09-26 0:39 [PATCH 0/9] software single-step support rework, fix limitations Pedro Alves
` (6 preceding siblings ...)
2014-09-26 0:40 ` [PATCH 8/9] Make single-step breakpoints be per-thread Pedro Alves
@ 2014-09-26 1:18 ` Pedro Alves
2014-09-26 1:36 ` [PATCH 7/9] infrun.c: add for_each_just_stopped_thread Pedro Alves
8 siblings, 0 replies; 20+ messages in thread
From: Pedro Alves @ 2014-09-26 1:18 UTC (permalink / raw)
To: gdb-patches
A little refactoring to reduce duplicate code.
gdb/
2014-09-25 Pedro Alves <palves@redhat.com>
* thread.c (delete_thread_breakpoint): New function.
(delete_step_resume_breakpoint)
(delete_exception_resume_breakpoint): Use it.
(delete_at_next_stop): New function.
(clear_thread_inferior_resources): Use delete_at_next_stop.
---
gdb/thread.c | 49 +++++++++++++++++++++++++++++--------------------
1 file changed, 29 insertions(+), 20 deletions(-)
diff --git a/gdb/thread.c b/gdb/thread.c
index 65890e1..ea4a9db 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -85,23 +85,42 @@ inferior_thread (void)
return tp;
}
-void
-delete_step_resume_breakpoint (struct thread_info *tp)
+/* Delete the breakpoint pointed at by BP_P, if there's one. */
+
+static void
+delete_thread_breakpoint (struct breakpoint **bp_p)
{
- if (tp && tp->control.step_resume_breakpoint)
+ if (*bp_p != NULL)
{
- delete_breakpoint (tp->control.step_resume_breakpoint);
- tp->control.step_resume_breakpoint = NULL;
+ delete_breakpoint (*bp_p);
+ *bp_p = NULL;
}
}
void
+delete_step_resume_breakpoint (struct thread_info *tp)
+{
+ if (tp != NULL)
+ delete_thread_breakpoint (&tp->control.step_resume_breakpoint);
+}
+
+void
delete_exception_resume_breakpoint (struct thread_info *tp)
{
- if (tp && tp->control.exception_resume_breakpoint)
+ if (tp != NULL)
+ delete_thread_breakpoint (&tp->control.exception_resume_breakpoint);
+}
+
+/* Delete the breakpoint pointed at by BP_P at the next stop, if
+ there's one. */
+
+static void
+delete_at_next_stop (struct breakpoint **bp)
+{
+ if (*bp != NULL)
{
- delete_breakpoint (tp->control.exception_resume_breakpoint);
- tp->control.exception_resume_breakpoint = NULL;
+ (*bp)->disposition = disp_del_at_next_stop;
+ *bp = NULL;
}
}
@@ -112,18 +131,8 @@ clear_thread_inferior_resources (struct thread_info *tp)
but not any user-specified thread-specific breakpoints. We can not
delete the breakpoint straight-off, because the inferior might not
be stopped at the moment. */
- if (tp->control.step_resume_breakpoint)
- {
- tp->control.step_resume_breakpoint->disposition = disp_del_at_next_stop;
- tp->control.step_resume_breakpoint = NULL;
- }
-
- if (tp->control.exception_resume_breakpoint)
- {
- tp->control.exception_resume_breakpoint->disposition
- = disp_del_at_next_stop;
- tp->control.exception_resume_breakpoint = NULL;
- }
+ delete_at_next_stop (&tp->control.step_resume_breakpoint);
+ delete_at_next_stop (&tp->control.exception_resume_breakpoint);
delete_longjmp_breakpoint_at_next_stop (tp->num);
--
1.9.3
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 7/9] infrun.c: add for_each_just_stopped_thread
2014-09-26 0:39 [PATCH 0/9] software single-step support rework, fix limitations Pedro Alves
` (7 preceding siblings ...)
2014-09-26 1:18 ` [PATCH 6/9] thread.c: cleanup breakpoint deletion Pedro Alves
@ 2014-09-26 1:36 ` Pedro Alves
8 siblings, 0 replies; 20+ messages in thread
From: Pedro Alves @ 2014-09-26 1:36 UTC (permalink / raw)
To: gdb-patches
This is a preparatory/cleanup patch that does two things:
- Renames 'delete_step_thread_step_resume_breakpoint'. The
"step_resume" part is misnomer these days, as the function deletes
other kinds of breakpoints, not just the step-resume breakpoint. A
following patch will want to make it delete yet another kind of
breakpoint, even.
- Splits out the logic of which threads get those breakpoints deleted
to a separate "for_each"-style function, so that the same following
patch may use it with a different callback.
Tested on x86_64 Fedora 20.
gdb/
2014-09-25 Pedro Alves <palves@redhat.com>
* infrun.c (delete_step_resume_breakpoint_callback): Delete.
(delete_thread_infrun_breakpoints): New function, with parts
salvaged from delete_step_resume_breakpoint_callback.
(delete_step_thread_step_resume_breakpoint): Delete.
(for_each_just_stopped_thread_callback_func): New typedef.
(for_each_just_stopped_thread): New function.
(delete_just_stopped_threads_infrun_breakpoints): New function.
(delete_step_thread_step_resume_breakpoint_cleanup): Rename to ...
(delete_just_stopped_threads_infrun_breakpoints_cleanup):
... this. Adjust.
(wait_for_inferior, fetch_inferior_event): Adjust to renames.
---
gdb/infrun.c | 72 +++++++++++++++++++++++++++++++++---------------------------
1 file changed, 40 insertions(+), 32 deletions(-)
diff --git a/gdb/infrun.c b/gdb/infrun.c
index e0df9bf..d6d0948 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2606,54 +2606,61 @@ infrun_thread_thread_exit (struct thread_info *tp, int silent)
nullify_last_target_wait_ptid ();
}
-/* Callback for iterate_over_threads. */
+/* Delete the step resume, single-step and longjmp/exception resume
+ breakpoints of TP. */
-static int
-delete_step_resume_breakpoint_callback (struct thread_info *info, void *data)
+static void
+delete_thread_infrun_breakpoints (struct thread_info *tp)
{
- if (is_exited (info->ptid))
- return 0;
-
- delete_step_resume_breakpoint (info);
- delete_exception_resume_breakpoint (info);
- return 0;
+ delete_step_resume_breakpoint (tp);
+ delete_exception_resume_breakpoint (tp);
}
-/* In all-stop, delete the step resume breakpoint of any thread that
- had one. In non-stop, delete the step resume breakpoint of the
- thread that just stopped. */
+/* If the target still has execution, call FUNC for each thread that
+ just stopped. In all-stop, that's all the non-exited threads; in
+ non-stop, that's the current thread, only. */
+
+typedef void (*for_each_just_stopped_thread_callback_func)
+ (struct thread_info *tp);
static void
-delete_step_thread_step_resume_breakpoint (void)
+for_each_just_stopped_thread (for_each_just_stopped_thread_callback_func func)
{
- if (!target_has_execution
- || ptid_equal (inferior_ptid, null_ptid))
- /* If the inferior has exited, we have already deleted the step
- resume breakpoints out of GDB's lists. */
+ if (!target_has_execution || ptid_equal (inferior_ptid, null_ptid))
return;
if (non_stop)
{
- /* If in non-stop mode, only delete the step-resume or
- longjmp-resume breakpoint of the thread that just stopped
- stepping. */
- struct thread_info *tp = inferior_thread ();
-
- delete_step_resume_breakpoint (tp);
- delete_exception_resume_breakpoint (tp);
+ /* If in non-stop mode, only the current thread stopped. */
+ func (inferior_thread ());
}
else
- /* In all-stop mode, delete all step-resume and longjmp-resume
- breakpoints of any thread that had them. */
- iterate_over_threads (delete_step_resume_breakpoint_callback, NULL);
+ {
+ struct thread_info *tp;
+
+ /* In all-stop mode, all threads have stopped. */
+ ALL_NON_EXITED_THREADS (tp)
+ {
+ func (tp);
+ }
+ }
+}
+
+/* Delete the step resume and longjmp/exception resume breakpoints of
+ the threads that just stopped. */
+
+static void
+delete_just_stopped_threads_infrun_breakpoints (void)
+{
+ for_each_just_stopped_thread (delete_thread_infrun_breakpoints);
}
/* A cleanup wrapper. */
static void
-delete_step_thread_step_resume_breakpoint_cleanup (void *arg)
+delete_just_stopped_threads_infrun_breakpoints_cleanup (void *arg)
{
- delete_step_thread_step_resume_breakpoint ();
+ delete_just_stopped_threads_infrun_breakpoints ();
}
/* Pretty print the results of target_wait, for debugging purposes. */
@@ -2788,8 +2795,9 @@ wait_for_inferior (void)
fprintf_unfiltered
(gdb_stdlog, "infrun: wait_for_inferior ()\n");
- old_cleanups =
- make_cleanup (delete_step_thread_step_resume_breakpoint_cleanup, NULL);
+ old_cleanups
+ = make_cleanup (delete_just_stopped_threads_infrun_breakpoints_cleanup,
+ NULL);
while (1)
{
@@ -2911,7 +2919,7 @@ fetch_inferior_event (void *client_data)
{
struct inferior *inf = find_inferior_pid (ptid_get_pid (ecs->ptid));
- delete_step_thread_step_resume_breakpoint ();
+ delete_just_stopped_threads_infrun_breakpoints ();
/* We may not find an inferior if this was a process exit. */
if (inf == NULL || inf->control.stop_soon == NO_STOP_QUIETLY)
--
1.9.3
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/9] Put single-step breakpoints on the bp_location chain
2014-09-26 0:40 ` [PATCH 3/9] Put single-step breakpoints on the bp_location chain Pedro Alves
@ 2014-09-28 12:36 ` Yao Qi
2014-09-30 13:01 ` Pedro Alves
2014-09-29 6:33 ` Yao Qi
1 sibling, 1 reply; 20+ messages in thread
From: Yao Qi @ 2014-09-28 12:36 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
Pedro Alves <palves@redhat.com> writes:
> @@ -15496,22 +15466,9 @@ single_step_breakpoints_inserted (void)
> void
> remove_single_step_breakpoints (void)
> {
> - gdb_assert (single_step_breakpoints[0] != NULL);
> + gdb_assert (single_step_breakpoints != NULL);
>
> - /* See insert_single_step_breakpoint for more about this deprecated
> - call. */
> - deprecated_remove_raw_breakpoint (single_step_gdbarch[0],
> - single_step_breakpoints[0]);
> - single_step_gdbarch[0] = NULL;
> - single_step_breakpoints[0] = NULL;
> -
> - if (single_step_breakpoints[1] != NULL)
> - {
> - deprecated_remove_raw_breakpoint (single_step_gdbarch[1],
> - single_step_breakpoints[1]);
> - single_step_gdbarch[1] = NULL;
> - single_step_breakpoints[1] = NULL;
> - }
> + delete_breakpoint (single_step_breakpoints);
We need to set single_step_breakpoints back to NULL. This causes many
fails for arm-linux-gnueabi target, as I tested.
--
Yao (齐尧)
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/9] Decide whether we may have removed breakpoints based on step_over_info
2014-09-26 0:39 ` [PATCH 1/9] Decide whether we may have removed breakpoints based on step_over_info Pedro Alves
@ 2014-09-28 12:52 ` Yao Qi
2014-10-02 18:05 ` Pedro Alves
0 siblings, 1 reply; 20+ messages in thread
From: Yao Qi @ 2014-09-28 12:52 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
Pedro Alves <palves@redhat.com> writes:
> gdb/
> 2014-09-22 Pedro Alves <palves@redhat.com>
>
> * infrun.c (step_over_info_valid_p): New function.
> (resume): Use step_over_info_valid_p instead of checking the
> threads's trap_expected flag. Add debug output.
^^^^^^^^^^^^^^^^
I don't see any debug output added by the code. Maybe a staled changelog entry?
> +/* Returns true if step-over info is valid. */
> +
> +static int
> +step_over_info_valid_p (void)
> +{
> + return (step_over_info.aspace != NULL);
> +}
> +
How about replace "step_over_info.aspace != NULL" in
stepping_past_instruction_at with step_over_info_valid_p too?
> /* Advise target which signals may be handled silently. If we have
> - removed breakpoints because we are stepping over one (which can
> - happen only if we are not using displaced stepping), we need to
> + removed breakpoints because we are stepping over one, we need to
> receive all signals to avoid accidentally skipping a breakpoint
> during execution of a signal handler. */
> - if ((step || singlestep_breakpoints_inserted_p)
> - && tp->control.trap_expected
> - && !use_displaced_stepping (gdbarch))
> + if (step_over_info_valid_p ())
Why do we remove condition (step || singlestep_breakpoints_inserted_p)?
I understand that step_over_info_valid_p is equivalent to
"tp->control.trap_expected && !use_displaced_stepping (gdbarch)", so I
don't know why (step || singlestep_breakpoints_inserted_p) is removed
too.
> target_pass_signals (0, NULL);
> else
> target_pass_signals ((int) GDB_SIGNAL_LAST, signal_pass);
--
Yao (齐尧)
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/9] Put single-step breakpoints on the bp_location chain
2014-09-26 0:40 ` [PATCH 3/9] Put single-step breakpoints on the bp_location chain Pedro Alves
2014-09-28 12:36 ` Yao Qi
@ 2014-09-29 6:33 ` Yao Qi
2014-10-02 17:55 ` Pedro Alves
1 sibling, 1 reply; 20+ messages in thread
From: Yao Qi @ 2014-09-29 6:33 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
Pedro Alves <palves@redhat.com> writes:
> This patch makes single-step breakpoints "real" breakpoints on the
> global location list.
We also need to skip adjusting breakpoint address
(in adjust_breakpoint_address) for bp_single_step, otherwise,
arm-linux-gnueabi gdb has a trouble on stepping through IT block, as
gdb.arch/thumb2-it.exp tested.
--
Yao (齐尧)
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/9] Put single-step breakpoints on the bp_location chain
2014-09-28 12:36 ` Yao Qi
@ 2014-09-30 13:01 ` Pedro Alves
2014-09-30 13:15 ` Pedro Alves
0 siblings, 1 reply; 20+ messages in thread
From: Pedro Alves @ 2014-09-30 13:01 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On 09/28/2014 01:32 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
>
>> @@ -15496,22 +15466,9 @@ single_step_breakpoints_inserted (void)
>> void
>> remove_single_step_breakpoints (void)
>> {
>> - gdb_assert (single_step_breakpoints[0] != NULL);
>> + gdb_assert (single_step_breakpoints != NULL);
>>
>> - /* See insert_single_step_breakpoint for more about this deprecated
>> - call. */
>> - deprecated_remove_raw_breakpoint (single_step_gdbarch[0],
>> - single_step_breakpoints[0]);
>> - single_step_gdbarch[0] = NULL;
>> - single_step_breakpoints[0] = NULL;
>> -
>> - if (single_step_breakpoints[1] != NULL)
>> - {
>> - deprecated_remove_raw_breakpoint (single_step_gdbarch[1],
>> - single_step_breakpoints[1]);
>> - single_step_gdbarch[1] = NULL;
>> - single_step_breakpoints[1] = NULL;
>> - }
>> + delete_breakpoint (single_step_breakpoints);
>
> We need to set single_step_breakpoints back to NULL. This causes many
> fails for arm-linux-gnueabi target, as I tested.
Thanks Yao.
I'll try to figure out why I didn't see that in my testing...
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/9] Put single-step breakpoints on the bp_location chain
2014-09-30 13:01 ` Pedro Alves
@ 2014-09-30 13:15 ` Pedro Alves
0 siblings, 0 replies; 20+ messages in thread
From: Pedro Alves @ 2014-09-30 13:15 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On 09/30/2014 02:01 PM, Pedro Alves wrote:
> On 09/28/2014 01:32 PM, Yao Qi wrote:
>> Pedro Alves <palves@redhat.com> writes:
>>
>>> @@ -15496,22 +15466,9 @@ single_step_breakpoints_inserted (void)
>>> void
>>> remove_single_step_breakpoints (void)
>>> {
>>> - gdb_assert (single_step_breakpoints[0] != NULL);
>>> + gdb_assert (single_step_breakpoints != NULL);
>>>
>>> - /* See insert_single_step_breakpoint for more about this deprecated
>>> - call. */
>>> - deprecated_remove_raw_breakpoint (single_step_gdbarch[0],
>>> - single_step_breakpoints[0]);
>>> - single_step_gdbarch[0] = NULL;
>>> - single_step_breakpoints[0] = NULL;
>>> -
>>> - if (single_step_breakpoints[1] != NULL)
>>> - {
>>> - deprecated_remove_raw_breakpoint (single_step_gdbarch[1],
>>> - single_step_breakpoints[1]);
>>> - single_step_gdbarch[1] = NULL;
>>> - single_step_breakpoints[1] = NULL;
>>> - }
>>> + delete_breakpoint (single_step_breakpoints);
>>
>> We need to set single_step_breakpoints back to NULL. This causes many
>> fails for arm-linux-gnueabi target, as I tested.
>
> Thanks Yao.
>
> I'll try to figure out why I didn't see that in my testing...
Ah, I failed to test this revision of the patch alone on top of
the sss-on-x86 series. A later patch in the series removes this
function, and instead deletes the sss breakpoint through this:
/* Delete the breakpoint pointed at by BP_P, if there's one. */
static void
delete_thread_breakpoint (struct breakpoint **bp_p)
{
if (*bp_p != NULL)
{
delete_breakpoint (*bp_p);
*bp_p = NULL;
}
}
which clears the pointer.
I'll rebase this on top of the x86-on-sss series instead of
the other way around, and re-test it. It's a pain, but
it'll be my penance...
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/9] Switch back to stepped thread: clear step-over info
2014-09-26 0:40 ` [PATCH 5/9] Switch back to stepped thread: clear step-over info Pedro Alves
@ 2014-09-30 16:33 ` Pedro Alves
0 siblings, 0 replies; 20+ messages in thread
From: Pedro Alves @ 2014-09-30 16:33 UTC (permalink / raw)
To: gdb-patches
On 09/26/2014 01:39 AM, Pedro Alves wrote:
> This path misses clearing the step-over info, because it bypasses
> keep_going.
>
> The testsuite caught this while I was working on making software
> single-step breakpoints per-thread. Unfortunately, I didn't keep the
> logs around and I don't recall ATM which test tripped on this.
Got it. It's schedlock.exp, what else...
Without this, the previous part of the series fails with:
Running /home/pedro/gdb/mygit/build/../src/gdb/testsuite/gdb.threads/schedlock.exp ...
FAIL: gdb.threads/schedlock.exp: step to increment (unlocked 0)
FAIL: gdb.threads/schedlock.exp: step to increment (unlocked 1)
FAIL: gdb.threads/schedlock.exp: step to increment (unlocked 2)
FAIL: gdb.threads/schedlock.exp: step to increment (unlocked 3)
FAIL: gdb.threads/schedlock.exp: step to increment (unlocked 4)
FAIL: gdb.threads/schedlock.exp: step to increment (unlocked 5)
FAIL: gdb.threads/schedlock.exp: step to increment (unlocked 6)
FAIL: gdb.threads/schedlock.exp: step to increment (unlocked 7)
FAIL: gdb.threads/schedlock.exp: step to increment (unlocked 8)
FAIL: gdb.threads/schedlock.exp: step to increment (unlocked 9)
...
>
> gdb/
> 2014-09-25 Pedro Alves <palves@redhat.com>
>
> * infrun.c (switch_back_to_stepped_thread) <expect thread advanced
> also>: Clear step-over info.
> ---
> gdb/infrun.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 94a58b2..e0df9bf 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -5275,6 +5275,13 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs)
> fprintf_unfiltered (gdb_stdlog,
> "infrun: expected thread advanced also\n");
>
> + /* Clear this before trying to insert the sss
> + breakpoint, in case we were previously trying to step
> + over this location in another thread, otherwise the
> + breakpoint ends up _not_ installed. It's what
> + keep_going would do too, if we called it. */
> + clear_step_over_info ();
> +
> insert_single_step_breakpoint (get_frame_arch (frame),
> get_frame_address_space (frame),
> stop_pc);
>
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/9] Put single-step breakpoints on the bp_location chain
2014-09-29 6:33 ` Yao Qi
@ 2014-10-02 17:55 ` Pedro Alves
0 siblings, 0 replies; 20+ messages in thread
From: Pedro Alves @ 2014-10-02 17:55 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On 09/29/2014 07:29 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
>
>> This patch makes single-step breakpoints "real" breakpoints on the
>> global location list.
>
> We also need to skip adjusting breakpoint address
> (in adjust_breakpoint_address) for bp_single_step, otherwise,
> arm-linux-gnueabi gdb has a trouble on stepping through IT block, as
> gdb.arch/thumb2-it.exp tested.
Thanks Yao, makes sense. v2 will do this.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/9] Decide whether we may have removed breakpoints based on step_over_info
2014-09-28 12:52 ` Yao Qi
@ 2014-10-02 18:05 ` Pedro Alves
2014-10-06 1:06 ` Yao Qi
0 siblings, 1 reply; 20+ messages in thread
From: Pedro Alves @ 2014-10-02 18:05 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On 09/28/2014 01:48 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
>
>> gdb/
>> 2014-09-22 Pedro Alves <palves@redhat.com>
>>
>> * infrun.c (step_over_info_valid_p): New function.
>> (resume): Use step_over_info_valid_p instead of checking the
>> threads's trap_expected flag. Add debug output.
> ^^^^^^^^^^^^^^^^
> I don't see any debug output added by the code. Maybe a staled changelog entry?
Yeah, I ended up removing that code. Will fix, thanks.
>
>> +/* Returns true if step-over info is valid. */
>> +
>> +static int
>> +step_over_info_valid_p (void)
>> +{
>> + return (step_over_info.aspace != NULL);
>> +}
>> +
>
> How about replace "step_over_info.aspace != NULL" in
> stepping_past_instruction_at with step_over_info_valid_p too?
See patch 2/9. With that, step_over_info_valid_p will return
true if we're stepping over a watchpoint, but aspace will
be NULL.
>
>> /* Advise target which signals may be handled silently. If we have
>> - removed breakpoints because we are stepping over one (which can
>> - happen only if we are not using displaced stepping), we need to
>> + removed breakpoints because we are stepping over one, we need to
>> receive all signals to avoid accidentally skipping a breakpoint
>> during execution of a signal handler. */
>> - if ((step || singlestep_breakpoints_inserted_p)
>> - && tp->control.trap_expected
>> - && !use_displaced_stepping (gdbarch))
>> + if (step_over_info_valid_p ())
>
> Why do we remove condition (step || singlestep_breakpoints_inserted_p)?
> I understand that step_over_info_valid_p is equivalent to
> "tp->control.trap_expected && !use_displaced_stepping (gdbarch)", so I
> don't know why (step || singlestep_breakpoints_inserted_p) is removed
> too.
It ends up unnecessary, and it seems to me to be more to the
point.
I.e., if we have step-over info, then something, somewhere wants a
breakpoint lifted out of the target. No matter whether we're
stepping or continuing the target at this point, we need to receive
all signals so that if the signal handler calls the code that
would trigger the breakpoint/watchpoint, we don't miss it.
Removing this check now avoids having tweak it when
singlestep_breakpoints_inserted_p check global ends up
eliminated by a later patch in the series.
Does that make sense?
>
>> target_pass_signals (0, NULL);
>> else
>> target_pass_signals ((int) GDB_SIGNAL_LAST, signal_pass);
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/9] Decide whether we may have removed breakpoints based on step_over_info
2014-10-02 18:05 ` Pedro Alves
@ 2014-10-06 1:06 ` Yao Qi
2014-10-06 8:42 ` Pedro Alves
0 siblings, 1 reply; 20+ messages in thread
From: Yao Qi @ 2014-10-06 1:06 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
Pedro Alves <palves@redhat.com> writes:
> I.e., if we have step-over info, then something, somewhere wants a
> breakpoint lifted out of the target. No matter whether we're
> stepping or continuing the target at this point, we need to receive
> all signals so that if the signal handler calls the code that
> would trigger the breakpoint/watchpoint, we don't miss it.
>
> Removing this check now avoids having tweak it when
> singlestep_breakpoints_inserted_p check global ends up
> eliminated by a later patch in the series.
>
> Does that make sense?
Yes, it makes sense to me.
I've reviewed the rest of patches, and they are good to me. I've tested
the whole patch set with the changes I suggested in patch 3/9 on
arm-linux-gnueabi target. No regression.
--
Yao (齐尧)
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/9] Decide whether we may have removed breakpoints based on step_over_info
2014-10-06 1:06 ` Yao Qi
@ 2014-10-06 8:42 ` Pedro Alves
0 siblings, 0 replies; 20+ messages in thread
From: Pedro Alves @ 2014-10-06 8:42 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On 10/06/2014 02:02 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
>
>> I.e., if we have step-over info, then something, somewhere wants a
>> breakpoint lifted out of the target. No matter whether we're
>> stepping or continuing the target at this point, we need to receive
>> all signals so that if the signal handler calls the code that
>> would trigger the breakpoint/watchpoint, we don't miss it.
>>
>> Removing this check now avoids having tweak it when
>> singlestep_breakpoints_inserted_p check global ends up
>> eliminated by a later patch in the series.
>>
>> Does that make sense?
>
> Yes, it makes sense to me.
>
> I've reviewed the rest of patches, and they are good to me. I've tested
> the whole patch set with the changes I suggested in patch 3/9 on
> arm-linux-gnueabi target. No regression.
Excellent. Thank you very much, Yao.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2014-10-06 8:42 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-26 0:39 [PATCH 0/9] software single-step support rework, fix limitations Pedro Alves
2014-09-26 0:39 ` [PATCH 1/9] Decide whether we may have removed breakpoints based on step_over_info Pedro Alves
2014-09-28 12:52 ` Yao Qi
2014-10-02 18:05 ` Pedro Alves
2014-10-06 1:06 ` Yao Qi
2014-10-06 8:42 ` Pedro Alves
2014-09-26 0:39 ` [PATCH 2/9] Rewrite non-continuable watchpoints handling Pedro Alves
2014-09-26 0:40 ` [PATCH 4/9] Remove deprecated_insert_raw_breakpoint and friends Pedro Alves
2014-09-26 0:40 ` [PATCH 9/9] Non-stop + software single-step archs: don't force displaced-stepping for all single-steps Pedro Alves
2014-09-26 0:40 ` [PATCH 5/9] Switch back to stepped thread: clear step-over info Pedro Alves
2014-09-30 16:33 ` Pedro Alves
2014-09-26 0:40 ` [PATCH 3/9] Put single-step breakpoints on the bp_location chain Pedro Alves
2014-09-28 12:36 ` Yao Qi
2014-09-30 13:01 ` Pedro Alves
2014-09-30 13:15 ` Pedro Alves
2014-09-29 6:33 ` Yao Qi
2014-10-02 17:55 ` Pedro Alves
2014-09-26 0:40 ` [PATCH 8/9] Make single-step breakpoints be per-thread Pedro Alves
2014-09-26 1:18 ` [PATCH 6/9] thread.c: cleanup breakpoint deletion Pedro Alves
2014-09-26 1:36 ` [PATCH 7/9] infrun.c: add for_each_just_stopped_thread Pedro Alves
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).