* [PATCH 2/6] PR breakpoints/7143 - Watchpoint does not trigger when first set
2014-02-25 20:32 [PATCH 0/6] Fix a bunch of run control bugs Pedro Alves
@ 2014-02-25 20:32 ` Pedro Alves
2014-03-05 15:35 ` Pedro Alves
2014-02-25 20:32 ` [PATCH 1/6] PR gdb/16575: stale breakpoint instructions in the code cache Pedro Alves
` (5 subsequent siblings)
6 siblings, 1 reply; 17+ messages in thread
From: Pedro Alves @ 2014-02-25 20:32 UTC (permalink / raw)
To: gdb-patches
Say the program is stopped at a breakpoint, and the user sets a
watchpoint. When the program is next resumed, GDB will first step
over the breakpoint, as explained in the manual:
@value {GDBN} normally ignores breakpoints when it resumes
execution, until at least one instruction has been executed. If it
it did not do this, you would be unable to proceed past a breakpoint
without first disabling the breakpoint. This rule applies whether
or not the breakpoint already existed when your program stopped.
However, GDB currently also removes watchpoints, catchpoints, etc.,
and that means that the first instruction off the breakpoint does not
trigger the watchpoint, catchpoint, etc.
testsuite/gdb.base/watchpoint.exp has a kfail for this.
The PR proposes installing watchpoints only when stepping over a
breakpoint, but that misses catchpoints, etc.
A better fix would instead work from the opposite direction -- remove
only real breakpoints, leaving all other kinds of breakpoints
inserted.
But, going further, it's really a waste to constantly remove/insert
all breakpoints when stepping over a single breakpoint (generating a
pair of RSP z/Z packets for each breakpoint), so the fix goes a step
further and makes GDB remove _only_ the breakpoint being stepped over,
leaving all others installed. This then has the added benefit of
reducing breakpoint-related RSP traffic substancialy when there are
many breakpoints set.
gdb/
2014-02-25 Pedro Alves <palves@redhat.com>
PR breakpoints/7143
* breakpoint.c (should_be_inserted): Don't insert breakpoints that
are being stepped over.
(breakpoint_address_match): Make extern.
* breakpoint.h (breakpoint_address_match): New declaration.
* inferior.h (stepping_over_breakpoint_at): New declaration.
* infrun.c (step_over_aspace, step_over_address): New globals.
(stepping_over_breakpoint_at): New function.
(handle_inferior_event): Clear step_over_aspace and
step_over_address when trap_expected is cleared.
(proceed): Adjust step-over handling to set or clear
step_over_aspace and step_over_address instead of removing all
breakpoints.
(handle_signal_stop): Clear step_over_aspace and step_over_address
when trap_expected is cleared.
(process_event_stop_test): Clear step_over_aspace,
step_over_address and trap_expected if handling a GDB_SIGNAL_TRAP.
Clear step_over_aspace and step_over_address when trap_expected is
cleared.
(proceed): Adjust step-over handling to set step_over_aspace and
step_over_address instead of removing all breakpoints.
gdb/testsuite/
2014-02-25 Pedro Alves <palves@redhat.com>
PR breakpoints/7143
* gdb.base/watchpoint.exp: Mention bugzilla bug number instead of
old gnats gdb/38. Remove kfail. Adjust to use gdb_test instead
of gdb_test_multiple.
---
gdb/breakpoint.c | 19 ++--
gdb/breakpoint.h | 9 ++
gdb/inferior.h | 6 ++
gdb/infrun.c | 170 +++++++++++++++++++++-------------
gdb/testsuite/gdb.base/watchpoint.exp | 13 +--
gdb/testsuite/gdb.cp/annota2.exp | 3 -
gdb/testsuite/gdb.cp/annota3.exp | 3 -
7 files changed, 133 insertions(+), 90 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index ef81443..b462bad 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -165,11 +165,6 @@ static void describe_other_breakpoints (struct gdbarch *,
struct program_space *, CORE_ADDR,
struct obj_section *, int);
-static int breakpoint_address_match (struct address_space *aspace1,
- CORE_ADDR addr1,
- struct address_space *aspace2,
- CORE_ADDR addr2);
-
static int watchpoint_locations_match (struct bp_location *loc1,
struct bp_location *loc2);
@@ -2034,6 +2029,13 @@ should_be_inserted (struct bp_location *bl)
if (bl->pspace->breakpoints_not_allowed)
return 0;
+ /* Don't insert a breakpoint we're trying to step over. */
+ if ((bl->loc_type == bp_loc_software_breakpoint
+ || bl->loc_type == bp_loc_hardware_breakpoint)
+ && stepping_over_breakpoint_at (bl->pspace->aspace,
+ bl->address))
+ return 0;
+
return 1;
}
@@ -6786,12 +6788,9 @@ watchpoint_locations_match (struct bp_location *loc1,
&& loc1->length == loc2->length);
}
-/* Returns true if {ASPACE1,ADDR1} and {ASPACE2,ADDR2} represent the
- same breakpoint location. In most targets, this can only be true
- if ASPACE1 matches ASPACE2. On targets that have global
- breakpoints, the address space doesn't really matter. */
+/* See breakpoint.h. */
-static int
+int
breakpoint_address_match (struct address_space *aspace1, CORE_ADDR addr1,
struct address_space *aspace2, CORE_ADDR addr2)
{
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 4be9f23..42b1492 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1134,6 +1134,15 @@ extern int hardware_watchpoint_inserted_in_range (struct address_space *,
extern int breakpoint_thread_match (struct address_space *,
CORE_ADDR, ptid_t);
+/* Returns true if {ASPACE1,ADDR1} and {ASPACE2,ADDR2} represent the
+ same breakpoint location. In most targets, this can only be true
+ if ASPACE1 matches ASPACE2. On targets that have global
+ breakpoints, the address space doesn't really matter. */
+
+extern int
+ breakpoint_address_match (struct address_space *aspace1, CORE_ADDR addr1,
+ struct address_space *aspace2, CORE_ADDR addr2);
+
extern void until_break_command (char *, int, int);
/* Initialize a struct bp_location. */
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 43e035b..37542ef 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -230,6 +230,12 @@ void set_step_info (struct frame_info *frame, struct symtab_and_line sal);
extern void clear_exit_convenience_vars (void);
+/* Returns true if we're trying to step over a breakpoint at ADDRESS
+ in ASPACE. */
+
+extern int stepping_over_breakpoint_at (struct address_space *aspace,
+ CORE_ADDR address);
+
/* From infcmd.c */
extern void post_create_inferior (struct target_ops *, int);
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 5d60a90..25b40fb 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -985,6 +985,38 @@ static int stepping_past_singlestep_breakpoint;
breakpoint in the thread which hit the breakpoint, but then continue
stepping the thread user has selected. */
static ptid_t deferred_step_ptid;
+
+/* If stepping over a breakpoint (not displaced stepping), this is the
+ address (and address space) where that breakpoint is inserted.
+ When not stepping over a breakpoint, STEP_OVER_ASPACE is NULL.
+
+ (Note: presently GDB can only step over a breakpoint at any given
+ time. Given threads that can't run code in the same address space
+ as the breakpoint's can't really miss the breakpoint, GDB could be
+ taught to step-over at most one breakpoint per address space (so
+ this info could move to the address space object if/when GDB is
+ extended). Even with that, the set of breakpoints being stepped
+ over will normally be much small than the set of all breakpoints,
+ so a separate list instead of a flag in the breakpoint locations
+ themselves saves memory. This also saves complexity and run-time,
+ as otherwise we'd have to go through all breakpoint locations
+ clearing their flag whenever we start a new sequence. Similar
+ considerations weigh against storing this info in the thread
+ object.) */
+static struct address_space *step_over_aspace;
+static CORE_ADDR step_over_address;
+
+/* See inferior.h. */
+
+int
+stepping_over_breakpoint_at (struct address_space *aspace,
+ CORE_ADDR address)
+{
+ return (step_over_aspace != NULL
+ && breakpoint_address_match (aspace, address,
+ step_over_aspace, step_over_address));
+}
+
\f
/* Displaced stepping. */
@@ -1863,8 +1895,11 @@ a command like `return' or `jump' to continue execution."));
remove_single_step_breakpoints ();
singlestep_breakpoints_inserted_p = 0;
- insert_breakpoints ();
+ step_over_aspace = NULL;
+ step_over_address = 0;
tp->control.trap_expected = 0;
+
+ insert_breakpoints ();
}
if (should_resume)
@@ -2232,22 +2267,26 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step)
tp = inferior_thread ();
if (force_step)
+ tp->control.trap_expected = 1;
+
+ /* If we need to step over a breakpoint, and we're not using
+ displaced stepping to do so, insert all breakpoints (watchpoints,
+ etc.) but the one we're stepping over, step one instruction, and
+ then re-insert the breakpoint when that step is finished. */
+ if (tp->control.trap_expected && !use_displaced_stepping (gdbarch))
{
- tp->control.trap_expected = 1;
- /* If displaced stepping is enabled, we can step over the
- breakpoint without hitting it, so leave all breakpoints
- inserted. Otherwise we need to disable all breakpoints, step
- one instruction, and then re-add them when that step is
- finished. */
- if (!use_displaced_stepping (gdbarch))
- remove_breakpoints ();
+ struct regcache *regcache = get_current_regcache ();
+
+ step_over_aspace = get_regcache_aspace (regcache);
+ step_over_address = regcache_read_pc (regcache);
+ }
+ else
+ {
+ step_over_aspace = NULL;
+ step_over_address = 0;
}
- /* We can insert breakpoints if we're not trying to step over one,
- or if we are stepping over one but we're using displaced stepping
- to do so. */
- if (! tp->control.trap_expected || use_displaced_stepping (gdbarch))
- insert_breakpoints ();
+ insert_breakpoints ();
if (!non_stop)
{
@@ -3943,6 +3982,8 @@ handle_signal_stop (struct execution_control_state *ecs)
}
ecs->event_thread->control.trap_expected = 0;
+ step_over_aspace = NULL;
+ step_over_address = 0;
context_switch (deferred_step_ptid);
deferred_step_ptid = null_ptid;
@@ -4068,35 +4109,17 @@ handle_signal_stop (struct execution_control_state *ecs)
singlestep_breakpoints_inserted_p = 0;
}
- /* If the arch can displace step, don't remove the
- breakpoints. */
- thread_regcache = get_thread_regcache (ecs->ptid);
- if (!use_displaced_stepping (get_regcache_arch (thread_regcache)))
- remove_status = remove_breakpoints ();
-
- /* Did we fail to remove breakpoints? If so, try
- to set the PC past the bp. (There's at least
- one situation in which we can fail to remove
- the bp's: On HP-UX's that use ttrace, we can't
- change the address space of a vforking child
- process until the child exits (well, okay, not
- then either :-) or execs. */
- if (remove_status != 0)
- error (_("Cannot step over breakpoint hit in wrong thread"));
- else
- { /* Single step */
- if (!non_stop)
- {
- /* Only need to require the next event from this
- thread in all-stop mode. */
- waiton_ptid = ecs->ptid;
- infwait_state = infwait_thread_hop_state;
- }
-
- ecs->event_thread->stepping_over_breakpoint = 1;
- keep_going (ecs);
- return;
+ if (!non_stop)
+ {
+ /* Only need to require the next event from this thread
+ in all-stop mode. */
+ waiton_ptid = ecs->ptid;
+ infwait_state = infwait_thread_hop_state;
}
+
+ ecs->event_thread->stepping_over_breakpoint = 1;
+ keep_going (ecs);
+ return;
}
}
@@ -4384,6 +4407,8 @@ handle_signal_stop (struct execution_control_state *ecs)
ecs->event_thread->step_after_step_resume_breakpoint = 1;
/* Reset trap_expected to ensure breakpoints are re-inserted. */
ecs->event_thread->control.trap_expected = 0;
+ step_over_aspace = NULL;
+ step_over_address = 0;
/* If we were nexting/stepping some other thread, switch to
it, so that we don't continue it, losing control. */
@@ -4416,6 +4441,8 @@ handle_signal_stop (struct execution_control_state *ecs)
insert_hp_step_resume_breakpoint_at_frame (frame);
/* Reset trap_expected to ensure breakpoints are re-inserted. */
ecs->event_thread->control.trap_expected = 0;
+ step_over_aspace = NULL;
+ step_over_address = 0;
keep_going (ecs);
return;
}
@@ -4461,6 +4488,15 @@ process_event_stop_test (struct execution_control_state *ecs)
frame = get_current_frame ();
gdbarch = get_frame_arch (frame);
+ if (ecs->event_thread->control.trap_expected
+ && ecs->event_thread->suspend.stop_signal == GDB_SIGNAL_TRAP)
+ {
+ /* We got our trap. */
+ ecs->event_thread->control.trap_expected = 0;
+ step_over_aspace = NULL;
+ step_over_address = 0;
+ }
+
what = bpstat_what (ecs->event_thread->control.stop_bpstat);
if (what.call_dummy)
@@ -5324,6 +5360,8 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs)
Clear the trap_expected flag before switching back -- this is
what keep_going would do as well, if we called it. */
ecs->event_thread->control.trap_expected = 0;
+ step_over_aspace = NULL;
+ step_over_address = 0;
if (debug_infrun)
fprintf_unfiltered (gdb_stdlog,
@@ -5782,6 +5820,9 @@ keep_going (struct execution_control_state *ecs)
}
else
{
+ volatile struct gdb_exception e;
+ struct regcache *regcache = get_current_regcache ();
+
/* Either the trap was not expected, but we are continuing
anyway (if we got a signal, the user asked it be passed to
the child)
@@ -5795,33 +5836,32 @@ keep_going (struct execution_control_state *ecs)
already inserted breakpoints. Therefore, we don't
care if breakpoints were already inserted, or not. */
- if (ecs->event_thread->stepping_over_breakpoint)
+ /* If we need to step over a breakpoint, and we're not using
+ displaced stepping to do so, insert all breakpoints
+ (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->event_thread->stepping_over_breakpoint
+ && !use_displaced_stepping (get_regcache_arch (regcache)))
{
- struct regcache *thread_regcache = get_thread_regcache (ecs->ptid);
+ /* Can't step over more than one breakpoint simultaneously
+ without displaced stepping. */
+ gdb_assert (step_over_aspace == NULL);
- if (!use_displaced_stepping (get_regcache_arch (thread_regcache)))
- {
- /* Since we can't do a displaced step, we have to remove
- the breakpoint while we step it. To keep things
- simple, we remove them all. */
- remove_breakpoints ();
- }
+ step_over_aspace = get_regcache_aspace (regcache);
+ step_over_address = regcache_read_pc (regcache);
}
- else
- {
- volatile struct gdb_exception e;
- /* Stop stepping if inserting breakpoints fails. */
- TRY_CATCH (e, RETURN_MASK_ERROR)
- {
- insert_breakpoints ();
- }
- if (e.reason < 0)
- {
- exception_print (gdb_stderr, e);
- stop_stepping (ecs);
- return;
- }
+ /* Stop stepping if inserting breakpoints fails. */
+ TRY_CATCH (e, RETURN_MASK_ERROR)
+ {
+ insert_breakpoints ();
+ }
+ if (e.reason < 0)
+ {
+ exception_print (gdb_stderr, e);
+ stop_stepping (ecs);
+ return;
}
ecs->event_thread->control.trap_expected
diff --git a/gdb/testsuite/gdb.base/watchpoint.exp b/gdb/testsuite/gdb.base/watchpoint.exp
index 80d75cb..1123824 100644
--- a/gdb/testsuite/gdb.base/watchpoint.exp
+++ b/gdb/testsuite/gdb.base/watchpoint.exp
@@ -550,21 +550,16 @@ proc test_complex_watchpoint {} {
proc test_watchpoint_and_breakpoint {} {
global gdb_prompt
- # This is a test for PR gdb/38, which involves setting a
+ # This is a test for PR breakpoints/7143, which involves setting a
# watchpoint right after you've reached a breakpoint.
if [runto func3] then {
gdb_breakpoint [gdb_get_line_number "second x assignment"]
gdb_continue_to_breakpoint "second x assignment"
gdb_test "watch x" ".*atchpoint \[0-9\]+: x"
- gdb_test_multiple "next" "next after watch x" {
- -re ".*atchpoint \[0-9\]+: x\r\n\r\nOld value = 0\r\nNew value = 1\r\n.*$gdb_prompt $" {
- pass "next after watch x"
- }
- -re "\[0-9\]+\[\t \]+y = 1;\r\n$gdb_prompt $" {
- kfail "gdb/38" "next after watch x"
- }
- }
+ gdb_test "next" \
+ ".*atchpoint \[0-9\]+: x\r\n\r\nOld value = 0\r\nNew value = 1\r\n.*" \
+ "next after watch x"
gdb_test_no_output "delete \$bpnum" "delete watch x"
}
diff --git a/gdb/testsuite/gdb.cp/annota2.exp b/gdb/testsuite/gdb.cp/annota2.exp
index 00a6067..6fbf4b5 100644
--- a/gdb/testsuite/gdb.cp/annota2.exp
+++ b/gdb/testsuite/gdb.cp/annota2.exp
@@ -165,9 +165,6 @@ gdb_test_multiple "next" "watch triggered on a.x" {
-re "\r\n\032\032post-prompt\r\n\r\n\032\032starting\r\n${frames_invalid}${breakpoints_invalid}\r\n\032\032watchpoint 3\r\n.*atchpoint 3: a.x\r\n\r\nOld value = 0\r\nNew value = 1\r\n\r\n\032\032frame-begin 0 $hex\r\n\r\n\032\032frame-function-name\r\nmain\r\n\032\032frame-args\r\n \\(\\)\r\n\032\032frame-source-begin\r\n at \r\n\032\032frame-source-file\r\n.*$srcfile\r\n\032\032frame-source-file-end\r\n:\r\n\032\032frame-source-line\r\n$decimal\r\n\032\032frame-source-end\r\n\r\n\r\n\032\032source .*$srcfile.*beg:$hex\r\n\r\n\032\032frame-end\r\n\r\n\032\032stopped\r\n.*$gdb_prompt$" {
pass "watch triggered on a.x"
}
- -re "\r\n\032\032post-prompt\r\n\r\n\032\032starting\r\n${frames_invalid}\r\n\032\032source .*$srcfile.*beg:$hex\r\n\r\n\032\032frame-end\r\n\r\n\032\032stopped\r\n$gdb_prompt$" {
- kfail "gdb/38" "watch triggered on a.x"
- }
}
diff --git a/gdb/testsuite/gdb.cp/annota3.exp b/gdb/testsuite/gdb.cp/annota3.exp
index 957d371..bbf9a1e 100644
--- a/gdb/testsuite/gdb.cp/annota3.exp
+++ b/gdb/testsuite/gdb.cp/annota3.exp
@@ -169,9 +169,6 @@ gdb_test_multiple "next" "watch triggered on a.x" {
-re "\r\n\032\032post-prompt\r\n\r\n\032\032starting\r\n\r\n\032\032watchpoint 3\r\n.*atchpoint 3: a.x\r\n\r\nOld value = 0\r\nNew value = 1\r\n\r\n(\032\032frame-begin 0 0x\[0-9a-z\]+\r\n|)main \\(\\) at .*$srcfile:$decimal\r\n\r\n\032\032source .*$srcfile.*beg:$hex\r\n\r\n\032\032stopped\r\n.*$gdb_prompt$" {
pass "watch triggered on a.x"
}
- -re "\r\n\032\032post-prompt\r\n\r\n\032\032starting\r\n\r\n\032\032source .*$srcfile.*beg:$hex\r\n\r\n\032\032stopped\r\n$gdb_prompt$" {
- kfail "gdb/38" "watch triggered on a.x"
- }
}
#
--
1.7.11.7
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 2/6] PR breakpoints/7143 - Watchpoint does not trigger when first set
2014-02-25 20:32 ` [PATCH 2/6] PR breakpoints/7143 - Watchpoint does not trigger when first set Pedro Alves
@ 2014-03-05 15:35 ` Pedro Alves
0 siblings, 0 replies; 17+ messages in thread
From: Pedro Alves @ 2014-03-05 15:35 UTC (permalink / raw)
To: gdb-patches
On 02/25/2014 08:32 PM, Pedro Alves wrote:
> + if (ecs->event_thread->stepping_over_breakpoint
> + && !use_displaced_stepping (get_regcache_arch (regcache)))
> {
> - struct regcache *thread_regcache = get_thread_regcache (ecs->ptid);
> + /* Can't step over more than one breakpoint simultaneously
> + without displaced stepping. */
> + gdb_assert (step_over_aspace == NULL);
While working on further changes on top of this series, I noticed
something wrong. Then I decided to reorder the patches a little,
and add another watchpoint-related test, which then managed to
trigger this assertion, because we
- stepped a thread
- got to the thread_hop code without clearing step_over_aspace.
v2 will clear step_over_aspace earlier to avoid that. It's still
a good idea to do that even though the series ends up eliminating
the thread hop code.
--
Pedro Alves
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/6] PR gdb/16575: stale breakpoint instructions in the code cache
2014-02-25 20:32 [PATCH 0/6] Fix a bunch of run control bugs Pedro Alves
2014-02-25 20:32 ` [PATCH 2/6] PR breakpoints/7143 - Watchpoint does not trigger when first set Pedro Alves
@ 2014-02-25 20:32 ` Pedro Alves
2014-03-04 4:02 ` Yao Qi
2014-03-05 15:14 ` Pedro Alves
2014-02-25 20:32 ` [PATCH 3/6] Fix missing breakpoing/watchpoint hits, eliminate deferred_step_ptid Pedro Alves
` (4 subsequent siblings)
6 siblings, 2 replies; 17+ messages in thread
From: Pedro Alves @ 2014-02-25 20:32 UTC (permalink / raw)
To: gdb-patches
In non-stop mode, or rather, breakpoints always-inserted mode, the
code cache can easily end up with stale breakpoint instructions:
All it takes is filling a cache line when breakpoints already exist in
that memory region, and then delete the breakpoint.
Vis. (from the new test):
(gdb) set breakpoint always-inserted on
(gdb) b 23
Breakpoint 2 at 0x400540: file ../../../src/gdb/testsuite/gdb.base/breakpoint-shadow.c, line 23.
(gdb) b 24
Breakpoint 3 at 0x400547: file ../../../src/gdb/testsuite/gdb.base/breakpoint-shadow.c, line 24.
disass main
Dump of assembler code for function main:
0x000000000040053c <+0>: push %rbp
0x000000000040053d <+1>: mov %rsp,%rbp
=> 0x0000000000400540 <+4>: movl $0x1,-0x4(%rbp)
0x0000000000400547 <+11>: movl $0x2,-0x4(%rbp)
0x000000000040054e <+18>: mov $0x0,%eax
0x0000000000400553 <+23>: pop %rbp
0x0000000000400554 <+24>: retq
End of assembler dump.
So far so good. Now flush the code cache:
(gdb) set code-cache off
(gdb) set code-cache on
Requesting a disassembly works as expected, breakpoint shadowing is
applied:
(gdb) disass main
Dump of assembler code for function main:
0x000000000040053c <+0>: push %rbp
0x000000000040053d <+1>: mov %rsp,%rbp
=> 0x0000000000400540 <+4>: movl $0x1,-0x4(%rbp)
0x0000000000400547 <+11>: movl $0x2,-0x4(%rbp)
0x000000000040054e <+18>: mov $0x0,%eax
0x0000000000400553 <+23>: pop %rbp
0x0000000000400554 <+24>: retq
End of assembler dump.
However, now delete the breakpoints:
(gdb) delete
Delete all breakpoints? (y or n) y
And disassembly shows the old breakpoint instructions:
(gdb) disass main
Dump of assembler code for function main:
0x000000000040053c <+0>: push %rbp
0x000000000040053d <+1>: mov %rsp,%rbp
=> 0x0000000000400540 <+4>: int3
0x0000000000400541 <+5>: rex.RB cld
0x0000000000400543 <+7>: add %eax,(%rax)
0x0000000000400545 <+9>: add %al,(%rax)
0x0000000000400547 <+11>: int3
0x0000000000400548 <+12>: rex.RB cld
0x000000000040054a <+14>: add (%rax),%al
0x000000000040054c <+16>: add %al,(%rax)
0x000000000040054e <+18>: mov $0x0,%eax
0x0000000000400553 <+23>: pop %rbp
0x0000000000400554 <+24>: retq
End of assembler dump.
Those breakpoint instructions are no longer installed in target memory
they're stale in the code cache. Easily confirmed by just disabling
the code cache:
(gdb) set code-cache off
(gdb) disass main
Dump of assembler code for function main:
0x000000000040053c <+0>: push %rbp
0x000000000040053d <+1>: mov %rsp,%rbp
=> 0x0000000000400540 <+4>: movl $0x1,-0x4(%rbp)
0x0000000000400547 <+11>: movl $0x2,-0x4(%rbp)
0x000000000040054e <+18>: mov $0x0,%eax
0x0000000000400553 <+23>: pop %rbp
0x0000000000400554 <+24>: retq
End of assembler dump.
I stumbled upon this when writing a patch to infrun.c, that made
handle_inferior_event&co fill in the cache before breakpoints were
removed from the target. Recall that wait_for_inferior flushes the
dcache for every event. So in that case, always-inserted mode was not
necessary to trigger this. It's just a convenient way to expose the
issue.
The dcache works at the raw memory level. We need to update it
whenever memory is written, no matter what kind of target memory
object was originally passed down by the caller. The issue is that
the dcache update code isn't reached when a caller explicitly writes
raw memory. Breakpoint insertion/removal is one such case --
mem-break.c uses target_write_read_memory/target_write_raw_memory.
The fix is to move the dcache update code from memory_xfer_partial_1
to raw_memory_xfer_partial so that it's always reachable.
When we do that, we can actually simplify a series of things.
memory_xfer_partial_1 no longer needs to handle writes for any kind of
memory object, and therefore dcache_xfer_memory no longer needs to
handle writes either. So the latter (dcache_xfer_memory) and its
callees can be simplified to only care are reads. While we're
touching dcache_xfer_memory's prototype, might as well rename it to
reflect that fact that it only handles reads, and make it follow the
new target_xfer_status/xfered_len style. This made me notice that
dcache_xfer_memory loses the real error status if a memory read fails:
we could have failed to read due to TARGET_XFER_E_UNAVAILABLE, for
instance, but we always return TARGET_XFER_E_UNAVAILABLE, hence the
FIXME note. I felt that fixing that fell out of the scope of this
patch.
Currently dcache_xfer_memory handles the case of a write failing. The
whole cache line is invalidated when that happens. However,
dcache_update, the sole mechanism for handling writes that will remain
after the patch, does not presently handle that scenario. That's a
bug. The patch makes it handle that, by passing down the
target_xfer_status status from the caller, so that it can better
decide what to do itself. While I was changing the function's
prototype, I constified the myaddr parameter, getting rid of the need
for the cast as seen in its existing caller.
Tested on x86_64 Fedora 17, native and gdbserver.
I think this one should go to 7.7.1 too.
gdb/
2014-02-25 Pedro Alves <palves@redhat.com>
PR gdb/16575
* dcache.c (dcache_poke_byte): Constify ptr parameter. Return
void. Update comment.
(dcache_xfer_memory): Delete.
(dcache_read_memory_partial): New, based on the read bits of
dcache_xfer_memory.
(dcache_update): Add status parameter. Use ULONGEST for len, and
adjust. Discard cache lines if the reason for the update was
error.
* dcache.h (dcache_xfer_memory): Delete declaration.
(dcache_read_memory_partial): New declaration.
(dcache_update): Update prototype.
* target.c (raw_memory_xfer_partial): Update the dcache here.
(memory_xfer_partial_1): Don't handle dcache writes here.
gdb/testsuite/
2014-02-25 Pedro Alves <palves@redhat.com>
PR gdb/16575
* gdb.base/breakpoint-shadow.exp (compare_disassembly): New
procedure.
(top level): Adjust to use it. Add tests that exercise breakpoint
interaction with the code-cache.
---
gdb/dcache.c | 100 ++++++++++++---------------
gdb/dcache.h | 15 ++--
gdb/target.c | 53 ++++++--------
gdb/testsuite/gdb.base/breakpoint-shadow.exp | 38 +++++++---
4 files changed, 102 insertions(+), 104 deletions(-)
diff --git a/gdb/dcache.c b/gdb/dcache.c
index ea51f5b..d3b546b 100644
--- a/gdb/dcache.c
+++ b/gdb/dcache.c
@@ -413,24 +413,20 @@ dcache_peek_byte (DCACHE *dcache, CORE_ADDR addr, gdb_byte *ptr)
/* Write the byte at PTR into ADDR in the data cache.
- The caller is responsible for also promptly writing the data
- through to target memory.
+ The caller should have written the data through to target memory
+ already.
- If addr is not in cache, this function does nothing; writing to
- an area of memory which wasn't present in the cache doesn't cause
- it to be loaded in.
+ If ADDR is not in cache, this function does nothing; writing to an
+ area of memory which wasn't present in the cache doesn't cause it
+ to be loaded in. */
- Always return 1 (meaning success) to simplify dcache_xfer_memory. */
-
-static int
-dcache_poke_byte (DCACHE *dcache, CORE_ADDR addr, gdb_byte *ptr)
+static void
+dcache_poke_byte (DCACHE *dcache, CORE_ADDR addr, const gdb_byte *ptr)
{
struct dcache_block *db = dcache_hit (dcache, addr);
if (db)
db->data[XFORM (dcache, addr)] = *ptr;
-
- return 1;
}
static int
@@ -467,26 +463,17 @@ dcache_init (void)
}
-/* Read or write LEN bytes from inferior memory at MEMADDR, transferring
- to or from debugger address MYADDR. Write to inferior if SHOULD_WRITE is
- nonzero.
+/* Read LEN bytes from dcache memory at MEMADDR, transferring to
+ debugger address MYADDR. If the data is presently cached, this
+ fills the cache. Arguments/return are like the target_xfer_partial
+ interface. */
- Return the number of bytes actually transfered, or -1 if the
- transfer is not supported or otherwise fails. Return of a non-negative
- value less than LEN indicates that no further transfer is possible.
- NOTE: This is different than the to_xfer_partial interface, in which
- positive values less than LEN mean further transfers may be possible. */
-
-int
-dcache_xfer_memory (struct target_ops *ops, DCACHE *dcache,
- CORE_ADDR memaddr, gdb_byte *myaddr,
- int len, int should_write)
+enum target_xfer_status
+dcache_read_memory_partial (struct target_ops *ops, DCACHE *dcache,
+ CORE_ADDR memaddr, gdb_byte *myaddr,
+ ULONGEST len, ULONGEST *xfered_len)
{
- int i;
- int res;
- int (*xfunc) (DCACHE *dcache, CORE_ADDR addr, gdb_byte *ptr);
-
- xfunc = should_write ? dcache_poke_byte : dcache_peek_byte;
+ ULONGEST i;
/* If this is a different inferior from what we've recorded,
flush the cache. */
@@ -497,35 +484,27 @@ dcache_xfer_memory (struct target_ops *ops, DCACHE *dcache,
dcache->ptid = inferior_ptid;
}
- /* Do write-through first, so that if it fails, we don't write to
- the cache at all. */
-
- if (should_write)
- {
- res = target_write (ops, TARGET_OBJECT_RAW_MEMORY,
- NULL, myaddr, memaddr, len);
- if (res <= 0)
- return res;
- /* Update LEN to what was actually written. */
- len = res;
- }
-
for (i = 0; i < len; i++)
{
- if (!xfunc (dcache, memaddr + i, myaddr + i))
+ if (!dcache_peek_byte (dcache, memaddr + i, myaddr + i))
{
/* That failed. Discard its cache line so we don't have a
partially read line. */
dcache_invalidate_line (dcache, memaddr + i);
- /* If we're writing, we still wrote LEN bytes. */
- if (should_write)
- return len;
- else
- return i;
+ break;
}
}
-
- return len;
+
+ if (i == 0)
+ {
+ /* FIXME: We lose the real error status. */
+ return TARGET_XFER_E_IO;
+ }
+ else
+ {
+ *xfered_len = i;
+ return TARGET_XFER_OK;
+ }
}
/* FIXME: There would be some benefit to making the cache write-back and
@@ -537,17 +516,26 @@ dcache_xfer_memory (struct target_ops *ops, DCACHE *dcache,
"logically" connected but not actually a single call to one of the
memory transfer functions. */
-/* Just update any cache lines which are already present. This is called
- by memory_xfer_partial in cases where the access would otherwise not go
- through the cache. */
+/* Just update any cache lines which are already present. This is
+ called by the target_xfer_partial machinery when writing raw
+ memory. */
void
-dcache_update (DCACHE *dcache, CORE_ADDR memaddr, gdb_byte *myaddr, int len)
+dcache_update (DCACHE *dcache, enum target_xfer_status status,
+ CORE_ADDR memaddr, const gdb_byte *myaddr,
+ ULONGEST len)
{
- int i;
+ ULONGEST i;
for (i = 0; i < len; i++)
- dcache_poke_byte (dcache, memaddr + i, myaddr + i);
+ if (status == TARGET_XFER_OK)
+ dcache_poke_byte (dcache, memaddr + i, myaddr + i);
+ else
+ {
+ /* Discard the whole cache line so we don't have a partially
+ valid line. */
+ dcache_invalidate_line (dcache, memaddr + i);
+ }
}
/* Print DCACHE line INDEX. */
diff --git a/gdb/dcache.h b/gdb/dcache.h
index 780dc30..020abd6 100644
--- a/gdb/dcache.h
+++ b/gdb/dcache.h
@@ -32,12 +32,13 @@ DCACHE *dcache_init (void);
/* Free a DCACHE. */
void dcache_free (DCACHE *);
-/* Simple to call from <remote>_xfer_memory. */
-
-int dcache_xfer_memory (struct target_ops *ops, DCACHE *cache, CORE_ADDR mem,
- gdb_byte *my, int len, int should_write);
-
-void dcache_update (DCACHE *dcache, CORE_ADDR memaddr, gdb_byte *myaddr,
- int len);
+enum target_xfer_status
+ dcache_read_memory_partial (struct target_ops *ops, DCACHE *dcache,
+ CORE_ADDR memaddr, gdb_byte *myaddr,
+ ULONGEST len, ULONGEST *xfered_len);
+
+void dcache_update (DCACHE *dcache, enum target_xfer_status status,
+ CORE_ADDR memaddr, const gdb_byte *myaddr,
+ ULONGEST len);
#endif /* DCACHE_H */
diff --git a/gdb/target.c b/gdb/target.c
index e4bf2e9..cbf8f64 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1111,6 +1111,23 @@ raw_memory_xfer_partial (struct target_ops *ops, gdb_byte *readbuf,
}
while (ops != NULL);
+ /* The cache works at the raw memory level. Make sure the cache
+ gets updated with raw contents no matter what kind of memory
+ object was originally being written. Note we do write-through
+ first, so that if it fails, we don't write to the cache contents
+ that never made it to the target. */
+ if (writebuf != NULL
+ && !ptid_equal (inferior_ptid, null_ptid)
+ && target_dcache_init_p ()
+ && (stack_cache_enabled_p () || code_cache_enabled_p ()))
+ {
+ DCACHE *dcache = target_dcache_get ();
+
+ /* Note that writing to an area of memory which wasn't present
+ in the cache doesn't cause it to be loaded in. */
+ dcache_update (dcache, res, memaddr, writebuf, *xfered_len);
+ }
+
return res;
}
@@ -1262,6 +1279,7 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
inf = NULL;
if (inf != NULL
+ && readbuf != NULL
/* The dcache reads whole cache lines; that doesn't play well
with reading from a trace buffer, because reading outside of
the collected memory range fails. */
@@ -1271,23 +1289,9 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
|| (code_cache_enabled_p () && object == TARGET_OBJECT_CODE_MEMORY)))
{
DCACHE *dcache = target_dcache_get_or_init ();
- int l;
- if (readbuf != NULL)
- l = dcache_xfer_memory (ops, dcache, memaddr, readbuf, reg_len, 0);
- else
- /* FIXME drow/2006-08-09: If we're going to preserve const
- correctness dcache_xfer_memory should take readbuf and
- writebuf. */
- l = dcache_xfer_memory (ops, dcache, memaddr, (void *) writebuf,
- reg_len, 1);
- if (l <= 0)
- return TARGET_XFER_E_IO;
- else
- {
- *xfered_len = (ULONGEST) l;
- return TARGET_XFER_OK;
- }
+ return dcache_read_memory_partial (ops, dcache, memaddr, readbuf,
+ reg_len, xfered_len);
}
/* If none of those methods found the memory we wanted, fall back
@@ -1303,23 +1307,6 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
res = raw_memory_xfer_partial (ops, readbuf, writebuf, memaddr, reg_len,
xfered_len);
- /* Make sure the cache gets updated no matter what - if we are writing
- to the stack. Even if this write is not tagged as such, we still need
- to update the cache. */
-
- if (res == TARGET_XFER_OK
- && inf != NULL
- && writebuf != NULL
- && target_dcache_init_p ()
- && !region->attrib.cache
- && ((stack_cache_enabled_p () && object != TARGET_OBJECT_STACK_MEMORY)
- || (code_cache_enabled_p () && object != TARGET_OBJECT_CODE_MEMORY)))
- {
- DCACHE *dcache = target_dcache_get ();
-
- dcache_update (dcache, memaddr, (void *) writebuf, reg_len);
- }
-
/* If we still haven't got anything, return the last error. We
give up. */
return res;
diff --git a/gdb/testsuite/gdb.base/breakpoint-shadow.exp b/gdb/testsuite/gdb.base/breakpoint-shadow.exp
index 74f7c8f..1414ec0 100644
--- a/gdb/testsuite/gdb.base/breakpoint-shadow.exp
+++ b/gdb/testsuite/gdb.base/breakpoint-shadow.exp
@@ -44,14 +44,36 @@ gdb_test_multiple "disass main" $test {
gdb_test "b [gdb_get_line_number "break-first"]" "Breakpoint \[0-9\] at .*" "First breakpoint placed"
gdb_test "b [gdb_get_line_number "break-second"]" "Breakpoint \[0-9\] at .*" "Second breakpoint placed"
-set test "disassembly with breakpoints"
-gdb_test_multiple "disass main" $test {
- -re $match {
- set got $expect_out(1,string)
- if [string equal -nocase $orig $got] {
- pass $test
- } else {
- fail $test
+# Disassemble main, and compare the output to the original output
+# before breakpoints were inserted. TEST is used as test message.
+
+proc test_disassembly {test} {
+ global match orig
+
+ gdb_test_multiple "disass main" $test {
+ -re $match {
+ set got $expect_out(1,string)
+ if [string equal -nocase $orig $got] {
+ pass $test
+ } else {
+ fail $test
+ }
}
}
}
+
+test_disassembly "disassembly with breakpoints"
+
+# Now check the interaction between the code cache and breakpoint
+# always-inserted mode.
+
+# Recreate the code cache when breakpoints are already inserted.
+gdb_test_no_output "set code-cache off"
+gdb_test_no_output "set code-cache on"
+
+test_disassembly "disassembly with breakpoints, fresh code cache"
+
+# Delete breakpoints. This should update the code cache as well.
+delete_breakpoints
+
+test_disassembly "disassembly without breakpoints, no stale breakpoints"
--
1.7.11.7
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 1/6] PR gdb/16575: stale breakpoint instructions in the code cache
2014-02-25 20:32 ` [PATCH 1/6] PR gdb/16575: stale breakpoint instructions in the code cache Pedro Alves
@ 2014-03-04 4:02 ` Yao Qi
2014-03-05 15:13 ` Pedro Alves
2014-03-05 15:14 ` Pedro Alves
1 sibling, 1 reply; 17+ messages in thread
From: Yao Qi @ 2014-03-04 4:02 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On 02/26/2014 04:32 AM, Pedro Alves wrote:
> The dcache works at the raw memory level. We need to update it
> whenever memory is written, no matter what kind of target memory
> object was originally passed down by the caller. The issue is that
> the dcache update code isn't reached when a caller explicitly writes
> raw memory. Breakpoint insertion/removal is one such case --
> mem-break.c uses target_write_read_memory/target_write_raw_memory.
>
> The fix is to move the dcache update code from memory_xfer_partial_1
> to raw_memory_xfer_partial so that it's always reachable.
I went through this patch, and it looks right to me. When I wrote code
cache code, I remember that there may be something about the interaction
between breakpoint and code cache, but I forgot to dig it deeper.
Thanks for fixing it.
--
Yao (é½å°§)
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/6] PR gdb/16575: stale breakpoint instructions in the code cache
2014-03-04 4:02 ` Yao Qi
@ 2014-03-05 15:13 ` Pedro Alves
0 siblings, 0 replies; 17+ messages in thread
From: Pedro Alves @ 2014-03-05 15:13 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On 03/04/2014 03:59 AM, Yao Qi wrote:
> On 02/26/2014 04:32 AM, Pedro Alves wrote:
>> The dcache works at the raw memory level. We need to update it
>> whenever memory is written, no matter what kind of target memory
>> object was originally passed down by the caller. The issue is that
>> the dcache update code isn't reached when a caller explicitly writes
>> raw memory. Breakpoint insertion/removal is one such case --
>> mem-break.c uses target_write_read_memory/target_write_raw_memory.
>>
>> The fix is to move the dcache update code from memory_xfer_partial_1
>> to raw_memory_xfer_partial so that it's always reachable.
>
> I went through this patch, and it looks right to me. When I wrote code
> cache code, I remember that there may be something about the interaction
> between breakpoint and code cache, but I forgot to dig it deeper.
> Thanks for fixing it.
Thanks for taking a look.
I'm applying this one now, as below (some typos in the commit log
fixed), and sending a v2 for the rest of the series.
---
PR gdb/16575: stale breakpoint instructions in the code cache
In non-stop mode, or rather, breakpoints always-inserted mode, the
code cache can easily end up with stale breakpoint instructions:
All it takes is filling a cache line when breakpoints already exist in
that memory region, and then delete the breakpoint.
Vis. (from the new test):
(gdb) set breakpoint always-inserted on
(gdb) b 23
Breakpoint 2 at 0x400540: file ../../../src/gdb/testsuite/gdb.base/breakpoint-shadow.c, line 23.
(gdb) b 24
Breakpoint 3 at 0x400547: file ../../../src/gdb/testsuite/gdb.base/breakpoint-shadow.c, line 24.
disass main
Dump of assembler code for function main:
0x000000000040053c <+0>: push %rbp
0x000000000040053d <+1>: mov %rsp,%rbp
=> 0x0000000000400540 <+4>: movl $0x1,-0x4(%rbp)
0x0000000000400547 <+11>: movl $0x2,-0x4(%rbp)
0x000000000040054e <+18>: mov $0x0,%eax
0x0000000000400553 <+23>: pop %rbp
0x0000000000400554 <+24>: retq
End of assembler dump.
So far so good. Now flush the code cache:
(gdb) set code-cache off
(gdb) set code-cache on
Requesting a disassembly works as expected, breakpoint shadowing is
applied:
(gdb) disass main
Dump of assembler code for function main:
0x000000000040053c <+0>: push %rbp
0x000000000040053d <+1>: mov %rsp,%rbp
=> 0x0000000000400540 <+4>: movl $0x1,-0x4(%rbp)
0x0000000000400547 <+11>: movl $0x2,-0x4(%rbp)
0x000000000040054e <+18>: mov $0x0,%eax
0x0000000000400553 <+23>: pop %rbp
0x0000000000400554 <+24>: retq
End of assembler dump.
However, now delete the breakpoints:
(gdb) delete
Delete all breakpoints? (y or n) y
And disassembly shows the old breakpoint instructions:
(gdb) disass main
Dump of assembler code for function main:
0x000000000040053c <+0>: push %rbp
0x000000000040053d <+1>: mov %rsp,%rbp
=> 0x0000000000400540 <+4>: int3
0x0000000000400541 <+5>: rex.RB cld
0x0000000000400543 <+7>: add %eax,(%rax)
0x0000000000400545 <+9>: add %al,(%rax)
0x0000000000400547 <+11>: int3
0x0000000000400548 <+12>: rex.RB cld
0x000000000040054a <+14>: add (%rax),%al
0x000000000040054c <+16>: add %al,(%rax)
0x000000000040054e <+18>: mov $0x0,%eax
0x0000000000400553 <+23>: pop %rbp
0x0000000000400554 <+24>: retq
End of assembler dump.
Those breakpoint instructions are no longer installed in target memory
they're stale in the code cache. Easily confirmed by just disabling
the code cache:
(gdb) set code-cache off
(gdb) disass main
Dump of assembler code for function main:
0x000000000040053c <+0>: push %rbp
0x000000000040053d <+1>: mov %rsp,%rbp
=> 0x0000000000400540 <+4>: movl $0x1,-0x4(%rbp)
0x0000000000400547 <+11>: movl $0x2,-0x4(%rbp)
0x000000000040054e <+18>: mov $0x0,%eax
0x0000000000400553 <+23>: pop %rbp
0x0000000000400554 <+24>: retq
End of assembler dump.
I stumbled upon this when writing a patch to infrun.c, that made
handle_inferior_event & co fill in the cache before breakpoints were
removed from the target. Recall that wait_for_inferior flushes the
dcache for every event. So in that case, always-inserted mode was not
necessary to trigger this. It's just a convenient way to expose the
issue.
The dcache works at the raw memory level. We need to update it
whenever memory is written, no matter what kind of target memory
object was originally passed down by the caller. The issue is that
the dcache update code isn't reached when a caller explicitly writes
raw memory. Breakpoint insertion/removal is one such case --
mem-break.c uses target_write_read_memory/target_write_raw_memory.
The fix is to move the dcache update code from memory_xfer_partial_1
to raw_memory_xfer_partial so that it's always reachable.
When we do that, we can actually simplify a series of things.
memory_xfer_partial_1 no longer needs to handle writes for any kind of
memory object, and therefore dcache_xfer_memory no longer needs to
handle writes either. So the latter (dcache_xfer_memory) and its
callees can be simplified to only care about reads. While we're
touching dcache_xfer_memory's prototype, might as well rename it to
reflect that fact that it only handles reads, and make it follow the
new target_xfer_status/xfered_len style. This made me notice that
dcache_xfer_memory loses the real error status if a memory read fails:
we could have failed to read due to TARGET_XFER_E_UNAVAILABLE, for
instance, but we always return TARGET_XFER_E_IO, hence the FIXME note.
I felt that fixing that fell out of the scope of this patch.
Currently dcache_xfer_memory handles the case of a write failing. The
whole cache line is invalidated when that happens. However,
dcache_update, the sole mechanism for handling writes that will remain
after the patch, does not presently handle that scenario. That's a
bug. The patch makes it handle that, by passing down the
target_xfer_status status from the caller, so that it can better
decide what to do itself. While I was changing the function's
prototype, I constified the myaddr parameter, getting rid of the need
for the cast as seen in its existing caller.
Tested on x86_64 Fedora 17, native and gdbserver.
gdb/
2014-03-05 Pedro Alves <palves@redhat.com>
PR gdb/16575
* dcache.c (dcache_poke_byte): Constify ptr parameter. Return
void. Update comment.
(dcache_xfer_memory): Delete.
(dcache_read_memory_partial): New, based on the read bits of
dcache_xfer_memory.
(dcache_update): Add status parameter. Use ULONGEST for len, and
adjust. Discard cache lines if the reason for the update was
error.
* dcache.h (dcache_xfer_memory): Delete declaration.
(dcache_read_memory_partial): New declaration.
(dcache_update): Update prototype.
* target.c (raw_memory_xfer_partial): Update the dcache here.
(memory_xfer_partial_1): Don't handle dcache writes here.
gdb/testsuite/
2014-03-05 Pedro Alves <palves@redhat.com>
PR gdb/16575
* gdb.base/breakpoint-shadow.exp (compare_disassembly): New
procedure.
(top level): Adjust to use it. Add tests that exercise breakpoint
interaction with the code-cache.
---
gdb/ChangeLog | 17 +++++
gdb/dcache.c | 100 ++++++++++++---------------
gdb/dcache.h | 15 ++--
gdb/target.c | 53 ++++++--------
gdb/testsuite/ChangeLog | 8 +++
gdb/testsuite/gdb.base/breakpoint-shadow.exp | 38 +++++++---
6 files changed, 127 insertions(+), 104 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 8108e50..b9ac372 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,20 @@
+2014-03-05 Pedro Alves <palves@redhat.com>
+
+ PR gdb/16575
+ * dcache.c (dcache_poke_byte): Constify ptr parameter. Return
+ void. Update comment.
+ (dcache_xfer_memory): Delete.
+ (dcache_read_memory_partial): New, based on the read bits of
+ dcache_xfer_memory.
+ (dcache_update): Add status parameter. Use ULONGEST for len, and
+ adjust. Discard cache lines if the reason for the update was
+ error.
+ * dcache.h (dcache_xfer_memory): Delete declaration.
+ (dcache_read_memory_partial): New declaration.
+ (dcache_update): Update prototype.
+ * target.c (raw_memory_xfer_partial): Update the dcache here.
+ (memory_xfer_partial_1): Don't handle dcache writes here.
+
2014-03-05 Mike Frysinger <vapier@gentoo.org>
* remote-sim.c (gdbsim_load): Add const to prog.
diff --git a/gdb/dcache.c b/gdb/dcache.c
index ea51f5b..d3b546b 100644
--- a/gdb/dcache.c
+++ b/gdb/dcache.c
@@ -413,24 +413,20 @@ dcache_peek_byte (DCACHE *dcache, CORE_ADDR addr, gdb_byte *ptr)
/* Write the byte at PTR into ADDR in the data cache.
- The caller is responsible for also promptly writing the data
- through to target memory.
+ The caller should have written the data through to target memory
+ already.
- If addr is not in cache, this function does nothing; writing to
- an area of memory which wasn't present in the cache doesn't cause
- it to be loaded in.
+ If ADDR is not in cache, this function does nothing; writing to an
+ area of memory which wasn't present in the cache doesn't cause it
+ to be loaded in. */
- Always return 1 (meaning success) to simplify dcache_xfer_memory. */
-
-static int
-dcache_poke_byte (DCACHE *dcache, CORE_ADDR addr, gdb_byte *ptr)
+static void
+dcache_poke_byte (DCACHE *dcache, CORE_ADDR addr, const gdb_byte *ptr)
{
struct dcache_block *db = dcache_hit (dcache, addr);
if (db)
db->data[XFORM (dcache, addr)] = *ptr;
-
- return 1;
}
static int
@@ -467,26 +463,17 @@ dcache_init (void)
}
-/* Read or write LEN bytes from inferior memory at MEMADDR, transferring
- to or from debugger address MYADDR. Write to inferior if SHOULD_WRITE is
- nonzero.
+/* Read LEN bytes from dcache memory at MEMADDR, transferring to
+ debugger address MYADDR. If the data is presently cached, this
+ fills the cache. Arguments/return are like the target_xfer_partial
+ interface. */
- Return the number of bytes actually transfered, or -1 if the
- transfer is not supported or otherwise fails. Return of a non-negative
- value less than LEN indicates that no further transfer is possible.
- NOTE: This is different than the to_xfer_partial interface, in which
- positive values less than LEN mean further transfers may be possible. */
-
-int
-dcache_xfer_memory (struct target_ops *ops, DCACHE *dcache,
- CORE_ADDR memaddr, gdb_byte *myaddr,
- int len, int should_write)
+enum target_xfer_status
+dcache_read_memory_partial (struct target_ops *ops, DCACHE *dcache,
+ CORE_ADDR memaddr, gdb_byte *myaddr,
+ ULONGEST len, ULONGEST *xfered_len)
{
- int i;
- int res;
- int (*xfunc) (DCACHE *dcache, CORE_ADDR addr, gdb_byte *ptr);
-
- xfunc = should_write ? dcache_poke_byte : dcache_peek_byte;
+ ULONGEST i;
/* If this is a different inferior from what we've recorded,
flush the cache. */
@@ -497,35 +484,27 @@ dcache_xfer_memory (struct target_ops *ops, DCACHE *dcache,
dcache->ptid = inferior_ptid;
}
- /* Do write-through first, so that if it fails, we don't write to
- the cache at all. */
-
- if (should_write)
- {
- res = target_write (ops, TARGET_OBJECT_RAW_MEMORY,
- NULL, myaddr, memaddr, len);
- if (res <= 0)
- return res;
- /* Update LEN to what was actually written. */
- len = res;
- }
-
for (i = 0; i < len; i++)
{
- if (!xfunc (dcache, memaddr + i, myaddr + i))
+ if (!dcache_peek_byte (dcache, memaddr + i, myaddr + i))
{
/* That failed. Discard its cache line so we don't have a
partially read line. */
dcache_invalidate_line (dcache, memaddr + i);
- /* If we're writing, we still wrote LEN bytes. */
- if (should_write)
- return len;
- else
- return i;
+ break;
}
}
-
- return len;
+
+ if (i == 0)
+ {
+ /* FIXME: We lose the real error status. */
+ return TARGET_XFER_E_IO;
+ }
+ else
+ {
+ *xfered_len = i;
+ return TARGET_XFER_OK;
+ }
}
/* FIXME: There would be some benefit to making the cache write-back and
@@ -537,17 +516,26 @@ dcache_xfer_memory (struct target_ops *ops, DCACHE *dcache,
"logically" connected but not actually a single call to one of the
memory transfer functions. */
-/* Just update any cache lines which are already present. This is called
- by memory_xfer_partial in cases where the access would otherwise not go
- through the cache. */
+/* Just update any cache lines which are already present. This is
+ called by the target_xfer_partial machinery when writing raw
+ memory. */
void
-dcache_update (DCACHE *dcache, CORE_ADDR memaddr, gdb_byte *myaddr, int len)
+dcache_update (DCACHE *dcache, enum target_xfer_status status,
+ CORE_ADDR memaddr, const gdb_byte *myaddr,
+ ULONGEST len)
{
- int i;
+ ULONGEST i;
for (i = 0; i < len; i++)
- dcache_poke_byte (dcache, memaddr + i, myaddr + i);
+ if (status == TARGET_XFER_OK)
+ dcache_poke_byte (dcache, memaddr + i, myaddr + i);
+ else
+ {
+ /* Discard the whole cache line so we don't have a partially
+ valid line. */
+ dcache_invalidate_line (dcache, memaddr + i);
+ }
}
/* Print DCACHE line INDEX. */
diff --git a/gdb/dcache.h b/gdb/dcache.h
index 780dc30..020abd6 100644
--- a/gdb/dcache.h
+++ b/gdb/dcache.h
@@ -32,12 +32,13 @@ DCACHE *dcache_init (void);
/* Free a DCACHE. */
void dcache_free (DCACHE *);
-/* Simple to call from <remote>_xfer_memory. */
-
-int dcache_xfer_memory (struct target_ops *ops, DCACHE *cache, CORE_ADDR mem,
- gdb_byte *my, int len, int should_write);
-
-void dcache_update (DCACHE *dcache, CORE_ADDR memaddr, gdb_byte *myaddr,
- int len);
+enum target_xfer_status
+ dcache_read_memory_partial (struct target_ops *ops, DCACHE *dcache,
+ CORE_ADDR memaddr, gdb_byte *myaddr,
+ ULONGEST len, ULONGEST *xfered_len);
+
+void dcache_update (DCACHE *dcache, enum target_xfer_status status,
+ CORE_ADDR memaddr, const gdb_byte *myaddr,
+ ULONGEST len);
#endif /* DCACHE_H */
diff --git a/gdb/target.c b/gdb/target.c
index 6cd9741..f7868c0 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1074,6 +1074,23 @@ raw_memory_xfer_partial (struct target_ops *ops, gdb_byte *readbuf,
}
while (ops != NULL);
+ /* The cache works at the raw memory level. Make sure the cache
+ gets updated with raw contents no matter what kind of memory
+ object was originally being written. Note we do write-through
+ first, so that if it fails, we don't write to the cache contents
+ that never made it to the target. */
+ if (writebuf != NULL
+ && !ptid_equal (inferior_ptid, null_ptid)
+ && target_dcache_init_p ()
+ && (stack_cache_enabled_p () || code_cache_enabled_p ()))
+ {
+ DCACHE *dcache = target_dcache_get ();
+
+ /* Note that writing to an area of memory which wasn't present
+ in the cache doesn't cause it to be loaded in. */
+ dcache_update (dcache, res, memaddr, writebuf, *xfered_len);
+ }
+
return res;
}
@@ -1225,6 +1242,7 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
inf = NULL;
if (inf != NULL
+ && readbuf != NULL
/* The dcache reads whole cache lines; that doesn't play well
with reading from a trace buffer, because reading outside of
the collected memory range fails. */
@@ -1234,23 +1252,9 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
|| (code_cache_enabled_p () && object == TARGET_OBJECT_CODE_MEMORY)))
{
DCACHE *dcache = target_dcache_get_or_init ();
- int l;
- if (readbuf != NULL)
- l = dcache_xfer_memory (ops, dcache, memaddr, readbuf, reg_len, 0);
- else
- /* FIXME drow/2006-08-09: If we're going to preserve const
- correctness dcache_xfer_memory should take readbuf and
- writebuf. */
- l = dcache_xfer_memory (ops, dcache, memaddr, (void *) writebuf,
- reg_len, 1);
- if (l <= 0)
- return TARGET_XFER_E_IO;
- else
- {
- *xfered_len = (ULONGEST) l;
- return TARGET_XFER_OK;
- }
+ return dcache_read_memory_partial (ops, dcache, memaddr, readbuf,
+ reg_len, xfered_len);
}
/* If none of those methods found the memory we wanted, fall back
@@ -1266,23 +1270,6 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
res = raw_memory_xfer_partial (ops, readbuf, writebuf, memaddr, reg_len,
xfered_len);
- /* Make sure the cache gets updated no matter what - if we are writing
- to the stack. Even if this write is not tagged as such, we still need
- to update the cache. */
-
- if (res == TARGET_XFER_OK
- && inf != NULL
- && writebuf != NULL
- && target_dcache_init_p ()
- && !region->attrib.cache
- && ((stack_cache_enabled_p () && object != TARGET_OBJECT_STACK_MEMORY)
- || (code_cache_enabled_p () && object != TARGET_OBJECT_CODE_MEMORY)))
- {
- DCACHE *dcache = target_dcache_get ();
-
- dcache_update (dcache, memaddr, (void *) writebuf, reg_len);
- }
-
/* If we still haven't got anything, return the last error. We
give up. */
return res;
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 835338f..65b4f95 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,11 @@
+2014-03-05 Pedro Alves <palves@redhat.com>
+
+ PR gdb/16575
+ * gdb.base/breakpoint-shadow.exp (compare_disassembly): New
+ procedure.
+ (top level): Adjust to use it. Add tests that exercise breakpoint
+ interaction with the code-cache.
+
2014-02-26 Ludovic Courtès <ludo@gnu.org>
* gdb.guile/scm-value.exp (test_value_in_inferior): Add
diff --git a/gdb/testsuite/gdb.base/breakpoint-shadow.exp b/gdb/testsuite/gdb.base/breakpoint-shadow.exp
index 74f7c8f..1414ec0 100644
--- a/gdb/testsuite/gdb.base/breakpoint-shadow.exp
+++ b/gdb/testsuite/gdb.base/breakpoint-shadow.exp
@@ -44,14 +44,36 @@ gdb_test_multiple "disass main" $test {
gdb_test "b [gdb_get_line_number "break-first"]" "Breakpoint \[0-9\] at .*" "First breakpoint placed"
gdb_test "b [gdb_get_line_number "break-second"]" "Breakpoint \[0-9\] at .*" "Second breakpoint placed"
-set test "disassembly with breakpoints"
-gdb_test_multiple "disass main" $test {
- -re $match {
- set got $expect_out(1,string)
- if [string equal -nocase $orig $got] {
- pass $test
- } else {
- fail $test
+# Disassemble main, and compare the output to the original output
+# before breakpoints were inserted. TEST is used as test message.
+
+proc test_disassembly {test} {
+ global match orig
+
+ gdb_test_multiple "disass main" $test {
+ -re $match {
+ set got $expect_out(1,string)
+ if [string equal -nocase $orig $got] {
+ pass $test
+ } else {
+ fail $test
+ }
}
}
}
+
+test_disassembly "disassembly with breakpoints"
+
+# Now check the interaction between the code cache and breakpoint
+# always-inserted mode.
+
+# Recreate the code cache when breakpoints are already inserted.
+gdb_test_no_output "set code-cache off"
+gdb_test_no_output "set code-cache on"
+
+test_disassembly "disassembly with breakpoints, fresh code cache"
+
+# Delete breakpoints. This should update the code cache as well.
+delete_breakpoints
+
+test_disassembly "disassembly without breakpoints, no stale breakpoints"
--
1.7.11.7
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/6] PR gdb/16575: stale breakpoint instructions in the code cache
2014-02-25 20:32 ` [PATCH 1/6] PR gdb/16575: stale breakpoint instructions in the code cache Pedro Alves
2014-03-04 4:02 ` Yao Qi
@ 2014-03-05 15:14 ` Pedro Alves
1 sibling, 0 replies; 17+ messages in thread
From: Pedro Alves @ 2014-03-05 15:14 UTC (permalink / raw)
To: gdb-patches
On 02/25/2014 08:32 PM, Pedro Alves wrote:
> I think this one should go to 7.7.1 too.
Pushed to the 7.7 branch, as below. It's mostly unchanged compared to
the mainline version, except it needed tweaks to some prototypes, because
the branch doesn't have all the to_xfer_partial interface changes that
have gone into mainline recently (xfer_status, xfer_len).
-----
PR gdb/16575: stale breakpoint instructions in the code cache
In non-stop mode, or rather, breakpoints always-inserted mode, the
code cache can easily end up with stale breakpoint instructions:
All it takes is filling a cache line when breakpoints already exist in
that memory region, and then delete the breakpoint.
Vis. (from the new test):
(gdb) set breakpoint always-inserted on
(gdb) b 23
Breakpoint 2 at 0x400540: file ../../../src/gdb/testsuite/gdb.base/breakpoint-shadow.c, line 23.
(gdb) b 24
Breakpoint 3 at 0x400547: file ../../../src/gdb/testsuite/gdb.base/breakpoint-shadow.c, line 24.
disass main
Dump of assembler code for function main:
0x000000000040053c <+0>: push %rbp
0x000000000040053d <+1>: mov %rsp,%rbp
=> 0x0000000000400540 <+4>: movl $0x1,-0x4(%rbp)
0x0000000000400547 <+11>: movl $0x2,-0x4(%rbp)
0x000000000040054e <+18>: mov $0x0,%eax
0x0000000000400553 <+23>: pop %rbp
0x0000000000400554 <+24>: retq
End of assembler dump.
So far so good. Now flush the code cache:
(gdb) set code-cache off
(gdb) set code-cache on
Requesting a disassembly works as expected, breakpoint shadowing is
applied:
(gdb) disass main
Dump of assembler code for function main:
0x000000000040053c <+0>: push %rbp
0x000000000040053d <+1>: mov %rsp,%rbp
=> 0x0000000000400540 <+4>: movl $0x1,-0x4(%rbp)
0x0000000000400547 <+11>: movl $0x2,-0x4(%rbp)
0x000000000040054e <+18>: mov $0x0,%eax
0x0000000000400553 <+23>: pop %rbp
0x0000000000400554 <+24>: retq
End of assembler dump.
However, now delete the breakpoints:
(gdb) delete
Delete all breakpoints? (y or n) y
And disassembly shows the old breakpoint instructions:
(gdb) disass main
Dump of assembler code for function main:
0x000000000040053c <+0>: push %rbp
0x000000000040053d <+1>: mov %rsp,%rbp
=> 0x0000000000400540 <+4>: int3
0x0000000000400541 <+5>: rex.RB cld
0x0000000000400543 <+7>: add %eax,(%rax)
0x0000000000400545 <+9>: add %al,(%rax)
0x0000000000400547 <+11>: int3
0x0000000000400548 <+12>: rex.RB cld
0x000000000040054a <+14>: add (%rax),%al
0x000000000040054c <+16>: add %al,(%rax)
0x000000000040054e <+18>: mov $0x0,%eax
0x0000000000400553 <+23>: pop %rbp
0x0000000000400554 <+24>: retq
End of assembler dump.
Those breakpoint instructions are no longer installed in target memory
they're stale in the code cache. Easily confirmed by just disabling
the code cache:
(gdb) set code-cache off
(gdb) disass main
Dump of assembler code for function main:
0x000000000040053c <+0>: push %rbp
0x000000000040053d <+1>: mov %rsp,%rbp
=> 0x0000000000400540 <+4>: movl $0x1,-0x4(%rbp)
0x0000000000400547 <+11>: movl $0x2,-0x4(%rbp)
0x000000000040054e <+18>: mov $0x0,%eax
0x0000000000400553 <+23>: pop %rbp
0x0000000000400554 <+24>: retq
End of assembler dump.
I stumbled upon this when writing a patch to infrun.c, that made
handle_inferior_event & co fill in the cache before breakpoints were
removed from the target. Recall that wait_for_inferior flushes the
dcache for every event. So in that case, always-inserted mode was not
necessary to trigger this. It's just a convenient way to expose the
issue.
The dcache works at the raw memory level. We need to update it
whenever memory is written, no matter what kind of target memory
object was originally passed down by the caller. The issue is that
the dcache update code isn't reached when a caller explicitly writes
raw memory. Breakpoint insertion/removal is one such case --
mem-break.c uses target_write_read_memory/target_write_raw_memory.
The fix is to move the dcache update code from memory_xfer_partial_1
to raw_memory_xfer_partial so that it's always reachable.
When we do that, we can actually simplify a series of things.
memory_xfer_partial_1 no longer needs to handle writes for any kind of
memory object, and therefore dcache_xfer_memory no longer needs to
handle writes either. So the latter (dcache_xfer_memory) and its
callees can be simplified to only care about reads. While we're
touching dcache_xfer_memory's prototype, might as well rename it to
reflect that fact that it only handles reads.
Currently dcache_xfer_memory handles the case of a write failing. The
whole cache line is invalidated when that happens. However,
dcache_update, the sole mechanism for handling writes that will remain
after the patch, does not presently handle that scenario. That's a
bug. The patch makes it handle that, by passing down the
to_xfer_partial result from the caller, so that it can better decide
what to do itself. While I was changing the function's prototype, I
constified the myaddr parameter, getting rid of the need for the cast
as seen in its existing caller.
Tested on x86_64 Fedora 17, native and gdbserver.
gdb/
2014-03-05 Pedro Alves <palves@redhat.com>
PR gdb/16575
* dcache.c (dcache_poke_byte): Constify ptr parameter. Return
void. Update comment.
(dcache_xfer_memory): Delete.
(dcache_read_memory_partial): New, based on the read bits of
dcache_xfer_memory.
(dcache_update): Add status parameter. Use ULONGEST for len, and
adjust. Discard cache lines if the reason for the update was
error.
* dcache.h (dcache_xfer_memory): Delete declaration.
(dcache_read_memory_partial): New declaration.
(dcache_update): Update prototype.
* target.c (raw_memory_xfer_partial): Update the dcache here.
(memory_xfer_partial_1): Don't handle dcache writes here.
gdb/testsuite/
2014-03-05 Pedro Alves <palves@redhat.com>
PR gdb/16575
* gdb.base/breakpoint-shadow.exp (compare_disassembly): New
procedure.
(top level): Adjust to use it. Add tests that exercise breakpoint
interaction with the code-cache.
---
gdb/ChangeLog | 17 +++++
gdb/dcache.c | 88 ++++++++++----------------
gdb/dcache.h | 12 ++--
gdb/target.c | 49 ++++++--------
gdb/testsuite/ChangeLog | 8 ++
gdb/testsuite/gdb.base/breakpoint-shadow.exp | 38 +++++++++--
6 files changed, 114 insertions(+), 98 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 6518d22..30c7d99 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,20 @@
+2014-03-05 Pedro Alves <palves@redhat.com>
+
+ PR gdb/16575
+ * dcache.c (dcache_poke_byte): Constify ptr parameter. Return
+ void. Update comment.
+ (dcache_xfer_memory): Delete.
+ (dcache_read_memory_partial): New, based on the read bits of
+ dcache_xfer_memory.
+ (dcache_update): Add status parameter. Use ULONGEST for len, and
+ adjust. Discard cache lines if the reason for the update was
+ error.
+ * dcache.h (dcache_xfer_memory): Delete declaration.
+ (dcache_read_memory_partial): New declaration.
+ (dcache_update): Update prototype.
+ * target.c (raw_memory_xfer_partial): Update the dcache here.
+ (memory_xfer_partial_1): Don't handle dcache writes here.
+
2014-02-26 Pedro Alves <palves@redhat.com>
PR breakpoints/16292
diff --git a/gdb/dcache.c b/gdb/dcache.c
index ea51f5b..87da3c4 100644
--- a/gdb/dcache.c
+++ b/gdb/dcache.c
@@ -413,24 +413,20 @@ dcache_peek_byte (DCACHE *dcache, CORE_ADDR addr, gdb_byte *ptr)
/* Write the byte at PTR into ADDR in the data cache.
- The caller is responsible for also promptly writing the data
- through to target memory.
+ The caller should have written the data through to target memory
+ already.
- If addr is not in cache, this function does nothing; writing to
- an area of memory which wasn't present in the cache doesn't cause
- it to be loaded in.
+ If ADDR is not in cache, this function does nothing; writing to an
+ area of memory which wasn't present in the cache doesn't cause it
+ to be loaded in. */
- Always return 1 (meaning success) to simplify dcache_xfer_memory. */
-
-static int
-dcache_poke_byte (DCACHE *dcache, CORE_ADDR addr, gdb_byte *ptr)
+static void
+dcache_poke_byte (DCACHE *dcache, CORE_ADDR addr, const gdb_byte *ptr)
{
struct dcache_block *db = dcache_hit (dcache, addr);
if (db)
db->data[XFORM (dcache, addr)] = *ptr;
-
- return 1;
}
static int
@@ -467,26 +463,16 @@ dcache_init (void)
}
-/* Read or write LEN bytes from inferior memory at MEMADDR, transferring
- to or from debugger address MYADDR. Write to inferior if SHOULD_WRITE is
- nonzero.
-
- Return the number of bytes actually transfered, or -1 if the
- transfer is not supported or otherwise fails. Return of a non-negative
- value less than LEN indicates that no further transfer is possible.
- NOTE: This is different than the to_xfer_partial interface, in which
- positive values less than LEN mean further transfers may be possible. */
+/* Read LEN bytes from dcache memory at MEMADDR, transferring to
+ debugger address MYADDR. If the data is presently cached, this
+ fills the cache. Arguments/return are like the target_xfer_partial
+ interface. */
int
-dcache_xfer_memory (struct target_ops *ops, DCACHE *dcache,
- CORE_ADDR memaddr, gdb_byte *myaddr,
- int len, int should_write)
+dcache_read_memory_partial (struct target_ops *ops, DCACHE *dcache,
+ CORE_ADDR memaddr, gdb_byte *myaddr, ULONGEST len)
{
- int i;
- int res;
- int (*xfunc) (DCACHE *dcache, CORE_ADDR addr, gdb_byte *ptr);
-
- xfunc = should_write ? dcache_poke_byte : dcache_peek_byte;
+ ULONGEST i;
/* If this is a different inferior from what we've recorded,
flush the cache. */
@@ -497,35 +483,18 @@ dcache_xfer_memory (struct target_ops *ops, DCACHE *dcache,
dcache->ptid = inferior_ptid;
}
- /* Do write-through first, so that if it fails, we don't write to
- the cache at all. */
-
- if (should_write)
- {
- res = target_write (ops, TARGET_OBJECT_RAW_MEMORY,
- NULL, myaddr, memaddr, len);
- if (res <= 0)
- return res;
- /* Update LEN to what was actually written. */
- len = res;
- }
-
for (i = 0; i < len; i++)
{
- if (!xfunc (dcache, memaddr + i, myaddr + i))
+ if (!dcache_peek_byte (dcache, memaddr + i, myaddr + i))
{
/* That failed. Discard its cache line so we don't have a
partially read line. */
dcache_invalidate_line (dcache, memaddr + i);
- /* If we're writing, we still wrote LEN bytes. */
- if (should_write)
- return len;
- else
- return i;
+ break;
}
}
-
- return len;
+
+ return i == 0 ? -1 : i;
}
/* FIXME: There would be some benefit to making the cache write-back and
@@ -537,17 +506,26 @@ dcache_xfer_memory (struct target_ops *ops, DCACHE *dcache,
"logically" connected but not actually a single call to one of the
memory transfer functions. */
-/* Just update any cache lines which are already present. This is called
- by memory_xfer_partial in cases where the access would otherwise not go
- through the cache. */
+/* Just update any cache lines which are already present. This is
+ called by the target_xfer_partial machinery when writing raw
+ memory. */
void
-dcache_update (DCACHE *dcache, CORE_ADDR memaddr, gdb_byte *myaddr, int len)
+dcache_update (DCACHE *dcache, int status,
+ CORE_ADDR memaddr, const gdb_byte *myaddr,
+ ULONGEST len)
{
- int i;
+ ULONGEST i;
for (i = 0; i < len; i++)
- dcache_poke_byte (dcache, memaddr + i, myaddr + i);
+ if (status > 0)
+ dcache_poke_byte (dcache, memaddr + i, myaddr + i);
+ else
+ {
+ /* Discard the whole cache line so we don't have a partially
+ valid line. */
+ dcache_invalidate_line (dcache, memaddr + i);
+ }
}
/* Print DCACHE line INDEX. */
diff --git a/gdb/dcache.h b/gdb/dcache.h
index 780dc30..c240a18 100644
--- a/gdb/dcache.h
+++ b/gdb/dcache.h
@@ -32,12 +32,12 @@ DCACHE *dcache_init (void);
/* Free a DCACHE. */
void dcache_free (DCACHE *);
-/* Simple to call from <remote>_xfer_memory. */
+int dcache_read_memory_partial (struct target_ops *ops, DCACHE *dcache,
+ CORE_ADDR memaddr, gdb_byte *myaddr,
+ ULONGEST len);
-int dcache_xfer_memory (struct target_ops *ops, DCACHE *cache, CORE_ADDR mem,
- gdb_byte *my, int len, int should_write);
-
-void dcache_update (DCACHE *dcache, CORE_ADDR memaddr, gdb_byte *myaddr,
- int len);
+void dcache_update (DCACHE *dcache, int status,
+ CORE_ADDR memaddr, const gdb_byte *myaddr,
+ ULONGEST len);
#endif /* DCACHE_H */
diff --git a/gdb/target.c b/gdb/target.c
index 42a8741..a8eeace 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1418,6 +1418,23 @@ raw_memory_xfer_partial (struct target_ops *ops, void *readbuf,
}
while (ops != NULL);
+ /* The cache works at the raw memory level. Make sure the cache
+ gets updated with raw contents no matter what kind of memory
+ object was originally being written. Note we do write-through
+ first, so that if it fails, we don't write to the cache contents
+ that never made it to the target. */
+ if (writebuf != NULL
+ && !ptid_equal (inferior_ptid, null_ptid)
+ && target_dcache_init_p ()
+ && (stack_cache_enabled_p () || code_cache_enabled_p ()))
+ {
+ DCACHE *dcache = target_dcache_get ();
+
+ /* Note that writing to an area of memory which wasn't present
+ in the cache doesn't cause it to be loaded in. */
+ dcache_update (dcache, res, memaddr, writebuf, len);
+ }
+
return res;
}
@@ -1565,6 +1582,7 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
inf = NULL;
if (inf != NULL
+ && readbuf != NULL
/* The dcache reads whole cache lines; that doesn't play well
with reading from a trace buffer, because reading outside of
the collected memory range fails. */
@@ -1575,18 +1593,8 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
{
DCACHE *dcache = target_dcache_get_or_init ();
- if (readbuf != NULL)
- res = dcache_xfer_memory (ops, dcache, memaddr, readbuf, reg_len, 0);
- else
- /* FIXME drow/2006-08-09: If we're going to preserve const
- correctness dcache_xfer_memory should take readbuf and
- writebuf. */
- res = dcache_xfer_memory (ops, dcache, memaddr, (void *) writebuf,
- reg_len, 1);
- if (res <= 0)
- return -1;
- else
- return res;
+ return dcache_read_memory_partial (ops, dcache, memaddr, readbuf,
+ reg_len);
}
/* If none of those methods found the memory we wanted, fall back
@@ -1597,23 +1605,6 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
object which can be read from more than one valid target. */
res = raw_memory_xfer_partial (ops, readbuf, writebuf, memaddr, reg_len);
- /* Make sure the cache gets updated no matter what - if we are writing
- to the stack. Even if this write is not tagged as such, we still need
- to update the cache. */
-
- if (res > 0
- && inf != NULL
- && writebuf != NULL
- && target_dcache_init_p ()
- && !region->attrib.cache
- && ((stack_cache_enabled_p () && object != TARGET_OBJECT_STACK_MEMORY)
- || (code_cache_enabled_p () && object != TARGET_OBJECT_CODE_MEMORY)))
- {
- DCACHE *dcache = target_dcache_get ();
-
- dcache_update (dcache, memaddr, (void *) writebuf, res);
- }
-
/* If we still haven't got anything, return the last error. We
give up. */
return res;
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 8168ec6..ecf8ad3 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,11 @@
+2014-03-05 Pedro Alves <palves@redhat.com>
+
+ PR gdb/16575
+ * gdb.base/breakpoint-shadow.exp (compare_disassembly): New
+ procedure.
+ (top level): Adjust to use it. Add tests that exercise breakpoint
+ interaction with the code-cache.
+
2014-02-26 Pedro Alves <pedro@codesourcery.com>
Pedro Alves <palves@redhat.com>
diff --git a/gdb/testsuite/gdb.base/breakpoint-shadow.exp b/gdb/testsuite/gdb.base/breakpoint-shadow.exp
index 74f7c8f..1414ec0 100644
--- a/gdb/testsuite/gdb.base/breakpoint-shadow.exp
+++ b/gdb/testsuite/gdb.base/breakpoint-shadow.exp
@@ -44,14 +44,36 @@ gdb_test_multiple "disass main" $test {
gdb_test "b [gdb_get_line_number "break-first"]" "Breakpoint \[0-9\] at .*" "First breakpoint placed"
gdb_test "b [gdb_get_line_number "break-second"]" "Breakpoint \[0-9\] at .*" "Second breakpoint placed"
-set test "disassembly with breakpoints"
-gdb_test_multiple "disass main" $test {
- -re $match {
- set got $expect_out(1,string)
- if [string equal -nocase $orig $got] {
- pass $test
- } else {
- fail $test
+# Disassemble main, and compare the output to the original output
+# before breakpoints were inserted. TEST is used as test message.
+
+proc test_disassembly {test} {
+ global match orig
+
+ gdb_test_multiple "disass main" $test {
+ -re $match {
+ set got $expect_out(1,string)
+ if [string equal -nocase $orig $got] {
+ pass $test
+ } else {
+ fail $test
+ }
}
}
}
+
+test_disassembly "disassembly with breakpoints"
+
+# Now check the interaction between the code cache and breakpoint
+# always-inserted mode.
+
+# Recreate the code cache when breakpoints are already inserted.
+gdb_test_no_output "set code-cache off"
+gdb_test_no_output "set code-cache on"
+
+test_disassembly "disassembly with breakpoints, fresh code cache"
+
+# Delete breakpoints. This should update the code cache as well.
+delete_breakpoints
+
+test_disassembly "disassembly without breakpoints, no stale breakpoints"
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/6] Fix missing breakpoing/watchpoint hits, eliminate deferred_step_ptid.
2014-02-25 20:32 [PATCH 0/6] Fix a bunch of run control bugs Pedro Alves
2014-02-25 20:32 ` [PATCH 2/6] PR breakpoints/7143 - Watchpoint does not trigger when first set Pedro Alves
2014-02-25 20:32 ` [PATCH 1/6] PR gdb/16575: stale breakpoint instructions in the code cache Pedro Alves
@ 2014-02-25 20:32 ` Pedro Alves
2014-03-04 6:05 ` Yao Qi
2014-02-25 20:33 ` [PATCH 5/6] Handle multiple step-overs Pedro Alves
` (3 subsequent siblings)
6 siblings, 1 reply; 17+ messages in thread
From: Pedro Alves @ 2014-02-25 20:32 UTC (permalink / raw)
To: gdb-patches
Consider the case of the user doing "step" in thread 2, while thread 1
had previously stopped for a breakpoint. In order to make progress,
GDB makes thread 1 step over its breakpoint, first, and once that is
over, thread 2 then starts stepping (with thread 1 and all others
running free, by default). If GDB didn't do that, thread 1 would just
trip on the same breakpoint immediately again. This is what the
prepare_to_proceed / deferred_step_ptid code is all about.
However, the current implementation has at least two issues:
First, the deferred_step_ptid code resumes the target with:
resume (1, GDB_SIGNAL_0);
prepare_to_wait (ecs);
return;
Recall we were just stepping over a breakpoint when we get here. That
means that _nothing_ had installed breakpoints yet! If there's
another breakpoint just after the breakpoint that was just stepped,
we'll miss it. The fix for that would be to use keep_going instead.
But, there are more problems. What if the instruction that was just
single-stepped triggers a watchpoint? Currently, GDB just happily
resumes the thread, losing that to...
So the fix must be to let the trap fall through the regular bpstat
handling, and only if no breakpoint, watchpoint, etc. claims the trap,
shall we switch back to the stepped thread.
Now, nowadays, we have code at the tail end of trap handling that does
exactly that -- switch back to the stepped thread
(switch_back_to_the_stepped_thread).
So the deferred_step_ptid code is just standing in the way, and can
simply be eliminated, fixing bugs in the process. Sweet.
The comment about spurious "Switching to ..." made me pause, but is
actually stale nowadays. That isn't needed anymore.
previous_inferior_ptid used to be re-set at each (internal) event, but
now it's only touched in proceed and normal stop.
The two tests added by this patch fail without the fix.
Tested on x86_64 Fedora 17 (also against my software single-stepping
on x86 branch).
gdb/
2014-02-25 Pedro Alves <palves@redhat.com>
* infrun.c (previous_inferior_ptid): Adjust comment.
(deferred_step_ptid): Delete.
(infrun_thread_ptid_changed, prepare_to_proceed)
(init_wait_for_inferior): Adjust.
(handle_signal_stop): Delete deferred_step_ptid handling.
gdb/testsuite/
2014-02-25 Pedro Alves <palves@redhat.com>
* gdb.threads/step-over-lands-on-breakpoint.c: New file.
* gdb.threads/step-over-lands-on-breakpoint.exp: New file.
* gdb.threads/step-over-trips-on-watchpoint.c: New file.
* gdb.threads/step-over-trips-on-watchpoint.exp: New file.
---
gdb/infrun.c | 62 +------------------
.../gdb.threads/step-over-lands-on-breakpoint.c | 65 ++++++++++++++++++++
.../gdb.threads/step-over-lands-on-breakpoint.exp | 64 ++++++++++++++++++++
.../gdb.threads/step-over-trips-on-watchpoint.c | 65 ++++++++++++++++++++
.../gdb.threads/step-over-trips-on-watchpoint.exp | 70 ++++++++++++++++++++++
5 files changed, 267 insertions(+), 59 deletions(-)
create mode 100644 gdb/testsuite/gdb.threads/step-over-lands-on-breakpoint.c
create mode 100644 gdb/testsuite/gdb.threads/step-over-lands-on-breakpoint.exp
create mode 100644 gdb/testsuite/gdb.threads/step-over-trips-on-watchpoint.c
create mode 100644 gdb/testsuite/gdb.threads/step-over-trips-on-watchpoint.exp
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 25b40fb..8bfce34 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -127,9 +127,9 @@ show_step_stop_if_no_debug (struct ui_file *file, int from_tty,
int sync_execution = 0;
-/* wait_for_inferior and normal_stop use this to notify the user
- when the inferior stopped in a different thread than it had been
- running in. */
+/* proceed and normal_stop use this to notify the user when the
+ inferior stopped in a different thread than it had been running
+ in. */
static ptid_t previous_inferior_ptid;
@@ -977,15 +977,6 @@ static CORE_ADDR singlestep_pc;
static ptid_t saved_singlestep_ptid;
static int stepping_past_singlestep_breakpoint;
-/* If not equal to null_ptid, this means that after stepping over breakpoint
- is finished, we need to switch to deferred_step_ptid, and step it.
-
- The use case is when one thread has hit a breakpoint, and then the user
- has switched to another thread and issued 'step'. We need to step over
- breakpoint in the thread which hit the breakpoint, but then continue
- stepping the thread user has selected. */
-static ptid_t deferred_step_ptid;
-
/* If stepping over a breakpoint (not displaced stepping), this is the
address (and address space) where that breakpoint is inserted.
When not stepping over a breakpoint, STEP_OVER_ASPACE is NULL.
@@ -1615,9 +1606,6 @@ infrun_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid)
if (ptid_equal (singlestep_ptid, old_ptid))
singlestep_ptid = new_ptid;
- if (ptid_equal (deferred_step_ptid, old_ptid))
- deferred_step_ptid = new_ptid;
-
for (displaced = displaced_step_inferior_states;
displaced;
displaced = displaced->next)
@@ -2138,10 +2126,6 @@ prepare_to_proceed (int step)
if (breakpoint_here_p (get_regcache_aspace (regcache),
regcache_read_pc (regcache)))
{
- /* If stepping, remember current thread to switch back to. */
- if (step)
- deferred_step_ptid = inferior_ptid;
-
/* Switch back to WAIT_PID thread. */
switch_to_thread (wait_ptid);
@@ -2419,7 +2403,6 @@ init_wait_for_inferior (void)
clear_proceed_status ();
stepping_past_singlestep_breakpoint = 0;
- deferred_step_ptid = null_ptid;
target_last_wait_ptid = minus_one_ptid;
@@ -3959,45 +3942,6 @@ handle_signal_stop (struct execution_control_state *ecs)
}
}
- if (!ptid_equal (deferred_step_ptid, null_ptid))
- {
- /* In non-stop mode, there's never a deferred_step_ptid set. */
- gdb_assert (!non_stop);
-
- /* If we stopped for some other reason than single-stepping, ignore
- the fact that we were supposed to switch back. */
- if (ecs->event_thread->suspend.stop_signal == GDB_SIGNAL_TRAP)
- {
- if (debug_infrun)
- fprintf_unfiltered (gdb_stdlog,
- "infrun: handling deferred step\n");
-
- /* 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;
- }
-
- ecs->event_thread->control.trap_expected = 0;
- step_over_aspace = NULL;
- step_over_address = 0;
-
- context_switch (deferred_step_ptid);
- deferred_step_ptid = null_ptid;
- /* Suppress spurious "Switching to ..." message. */
- previous_inferior_ptid = inferior_ptid;
-
- resume (1, GDB_SIGNAL_0);
- prepare_to_wait (ecs);
- return;
- }
-
- deferred_step_ptid = null_ptid;
- }
-
/* See if a thread hit a thread-specific breakpoint that was meant for
another thread. If so, then step that thread past the breakpoint,
and continue it. */
diff --git a/gdb/testsuite/gdb.threads/step-over-lands-on-breakpoint.c b/gdb/testsuite/gdb.threads/step-over-lands-on-breakpoint.c
new file mode 100644
index 0000000..882ae82
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/step-over-lands-on-breakpoint.c
@@ -0,0 +1,65 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2014 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#include <pthread.h>
+#include <unistd.h>
+#include <stdlib.h>
+
+pthread_barrier_t barrier;
+pthread_t child_thread;
+
+volatile unsigned int counter = 1;
+
+void *
+child_function (void *arg)
+{
+ pthread_barrier_wait (&barrier);
+
+ while (counter > 0)
+ {
+ counter++;
+
+ asm (" nop"); /* set breakpoint child here */
+ asm (" nop"); /* set breakpoint after step-over here */
+ usleep (1);
+ }
+
+ pthread_exit (NULL);
+}
+
+static int
+wait_threads (void)
+{
+ return 1; /* in wait_threads */
+}
+
+int
+main ()
+{
+ int res;
+ long i;
+
+ pthread_barrier_init (&barrier, NULL, 2);
+
+ res = pthread_create (&child_thread, NULL, child_function, NULL);
+ pthread_barrier_wait (&barrier);
+ wait_threads (); /* set wait-thread breakpoint here */
+
+ pthread_join (child_thread, NULL);
+
+ exit (EXIT_SUCCESS);
+}
diff --git a/gdb/testsuite/gdb.threads/step-over-lands-on-breakpoint.exp b/gdb/testsuite/gdb.threads/step-over-lands-on-breakpoint.exp
new file mode 100644
index 0000000..c781570
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/step-over-lands-on-breakpoint.exp
@@ -0,0 +1,64 @@
+# Copyright (C) 2014 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+# Test that when a step-over lands on a breakpoint, that breakpoint
+# hit is reported.
+
+standard_testfile
+set executable ${testfile}
+
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
+ executable [list debug "incdir=${objdir}"]] != "" } {
+ return -1
+}
+
+# Cover both stepping and non-stepping execution commands.
+foreach command {"step" "next" "continue" } {
+ with_test_prefix $command {
+ clean_restart $executable
+
+ if ![runto_main] {
+ continue
+ }
+
+ gdb_breakpoint [gdb_get_line_number "set wait-thread breakpoint here"]
+ gdb_continue_to_breakpoint "run to wait-thread breakpoint"
+ gdb_test "info threads" "2 .*\\\* 1.*" "info threads shows all threads"
+
+ gdb_test "set scheduler-locking on"
+
+ delete_breakpoints
+
+ gdb_breakpoint [gdb_get_line_number "set breakpoint child here"]
+ gdb_test "thread 2" "Switching to .*"
+ gdb_continue_to_breakpoint "run to breakpoint in thread 2"
+ gdb_test "p counter = 0" " = 0" "unbreak loop in thread 2"
+
+ # Set a breakpoint exactly where the step-over will land.
+ gdb_breakpoint [gdb_get_line_number "breakpoint after step-over here"]
+
+ # Switch back to thread 1 and disable scheduler locking.
+ gdb_test "thread 1" "Switching to .*"
+ gdb_test "set scheduler-locking off"
+ gdb_test "info breakpoints"
+ gdb_test "set debug infrun 1"
+
+ # Thread 2 is still stopped at a breakpoint that needs to be
+ # stepped over before proceeding thread 1. However, right
+ # where the step-over lands there's another breakpoint
+ # installed, which should trap and be reported to the user.
+ gdb_test "$command" "step-over here.*"
+ }
+}
diff --git a/gdb/testsuite/gdb.threads/step-over-trips-on-watchpoint.c b/gdb/testsuite/gdb.threads/step-over-trips-on-watchpoint.c
new file mode 100644
index 0000000..b72e238
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/step-over-trips-on-watchpoint.c
@@ -0,0 +1,65 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2014 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#include <pthread.h>
+#include <unistd.h>
+#include <stdlib.h>
+
+pthread_barrier_t barrier;
+pthread_t child_thread;
+
+volatile unsigned int counter = 1;
+volatile unsigned int watch_me;
+
+void *
+child_function (void *arg)
+{
+ pthread_barrier_wait (&barrier);
+
+ while (counter > 0)
+ {
+ counter++;
+
+ watch_me = 1; /* set breakpoint child here */
+ usleep (1);
+ }
+
+ pthread_exit (NULL);
+}
+
+static int
+wait_threads (void)
+{
+ return 1; /* in wait_threads */
+}
+
+int
+main ()
+{
+ int res;
+ long i;
+
+ pthread_barrier_init (&barrier, NULL, 2);
+
+ res = pthread_create (&child_thread, NULL, child_function, NULL);
+ pthread_barrier_wait (&barrier);
+ wait_threads (); /* set wait-thread breakpoint here */
+
+ pthread_join (child_thread, NULL);
+
+ exit (EXIT_SUCCESS);
+}
diff --git a/gdb/testsuite/gdb.threads/step-over-trips-on-watchpoint.exp b/gdb/testsuite/gdb.threads/step-over-trips-on-watchpoint.exp
new file mode 100644
index 0000000..1e8fe40
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/step-over-trips-on-watchpoint.exp
@@ -0,0 +1,70 @@
+# Copyright (C) 2014 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+# Test that when a step-over trips on a watchpoint, that watchpoint is
+# reported.
+
+standard_testfile
+set executable ${testfile}
+
+# This test verifies that a watchpoint is detected in a multithreaded
+# program so the test is only meaningful on a system with hardware
+# watchpoints.
+if {[skip_hw_watchpoint_tests]} {
+ return 0
+}
+
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
+ executable [list debug "incdir=${objdir}"]] != "" } {
+ return -1
+}
+
+# Cover both stepping and non-stepping execution commands.
+foreach command {"step" "next" "continue" } {
+ with_test_prefix $command {
+ clean_restart $executable
+
+ if ![runto_main] {
+ continue
+ }
+
+ gdb_breakpoint [gdb_get_line_number "set wait-thread breakpoint here"]
+ gdb_continue_to_breakpoint "run to wait-thread breakpoint"
+ gdb_test "info threads" "2 .*\\\* 1.*" "info threads shows all threads"
+
+ gdb_test "set scheduler-locking on"
+
+ delete_breakpoints
+
+ gdb_breakpoint [gdb_get_line_number "set breakpoint child here"]
+ gdb_test "thread 2" "Switching to .*"
+ gdb_continue_to_breakpoint "run to breakpoint in thread 2"
+ gdb_test "p counter = 0" " = 0" "unbreak loop in thread 2"
+ gdb_test "p watch_me = 0" " = 0" "clear watch_me"
+ gdb_test "watch watch_me" "Hardware watchpoint .*"
+
+ # Switch back to thread 1 and disable scheduler locking.
+ gdb_test "thread 1" "Switching to .*"
+ gdb_test "set scheduler-locking off"
+ gdb_test "info breakpoints"
+ gdb_test "set debug infrun 1"
+
+ # Thread 2 is still stopped at a breakpoint that needs to be
+ # stepped over before proceeding thread 1. However, right
+ # where the step-over lands there's another breakpoint
+ # installed, which should trap and be reported to the user.
+ gdb_test "$command" "Hardware watchpoint.*: watch_me.*New value = 1.*"
+ }
+}
--
1.7.11.7
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 3/6] Fix missing breakpoing/watchpoint hits, eliminate deferred_step_ptid.
2014-02-25 20:32 ` [PATCH 3/6] Fix missing breakpoing/watchpoint hits, eliminate deferred_step_ptid Pedro Alves
@ 2014-03-04 6:05 ` Yao Qi
2014-03-05 16:10 ` Pedro Alves
0 siblings, 1 reply; 17+ messages in thread
From: Yao Qi @ 2014-03-04 6:05 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
I don't have much comments on the patch, just some nits on tests:
On 02/26/2014 04:32 AM, Pedro Alves wrote:
> + gdb_test "set scheduler-locking off"
Use "gdb_test_no_output"?
> + gdb_test "info breakpoints"
> + gdb_test "set debug infrun 1"
I don't see the purpose of doing these two tests.
They apply to step-over-lands-on-watchpoint.exp too.
--
Yao (é½å°§)
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/6] Fix missing breakpoing/watchpoint hits, eliminate deferred_step_ptid.
2014-03-04 6:05 ` Yao Qi
@ 2014-03-05 16:10 ` Pedro Alves
0 siblings, 0 replies; 17+ messages in thread
From: Pedro Alves @ 2014-03-05 16:10 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On 03/04/2014 06:03 AM, Yao Qi wrote:
> I don't have much comments on the patch, just some nits on tests:
>
> On 02/26/2014 04:32 AM, Pedro Alves wrote:
>> + gdb_test "set scheduler-locking off"
>
> Use "gdb_test_no_output"?
>
>> + gdb_test "info breakpoints"
>> + gdb_test "set debug infrun 1"
>
> I don't see the purpose of doing these two tests.
It was handy to have them there while working on the patch,
and I forgot to remove them.
>
> They apply to step-over-lands-on-watchpoint.exp too.
>
Fixed in v2.
Thanks!
--
Pedro Alves
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 5/6] Handle multiple step-overs.
2014-02-25 20:32 [PATCH 0/6] Fix a bunch of run control bugs Pedro Alves
` (2 preceding siblings ...)
2014-02-25 20:32 ` [PATCH 3/6] Fix missing breakpoing/watchpoint hits, eliminate deferred_step_ptid Pedro Alves
@ 2014-02-25 20:33 ` Pedro Alves
2014-02-25 20:33 ` [PATCH 4/6] Fix for even more missed events; eliminate thread-hop code Pedro Alves
` (2 subsequent siblings)
6 siblings, 0 replies; 17+ messages in thread
From: Pedro Alves @ 2014-02-25 20:33 UTC (permalink / raw)
To: gdb-patches
This test fails with current mainline.
If the program stopped for a breakpoint in thread 1, and then the user
switches to thread 2, and resumes the program, GDB first switches back
to thread 1 to step it over the breakpoint, in order to make progress.
However, that logic only considers the last reported event, assuming
only one thread needs that stepping over dance.
That's actually not true when we play with scheduler-locking. The
patch adds an example to the testsuite of multiple threads needing a
step-over before the stepping thread can be resumed. With current
mainline, the program re-traps the same breakpoint it had already
trapped before.
E.g.:
Breakpoint 2, main () at ../../../src/gdb/testsuite/gdb.threads/multiple-step-overs.c:99
99 wait_threads (); /* set wait-threads breakpoint here */
(gdb) PASS: gdb.threads/multiple-step-overs.exp: step: continue to breakpoint: run to breakpoint
info threads
Id Target Id Frame
3 Thread 0x7ffff77c9700 (LWP 4310) "multiple-step-o" 0x00000000004007ca in child_function_3 (arg=0x1) at ../../../src/gdb/testsuite/gdb.threads/multiple-step-overs.c:43
2 Thread 0x7ffff7fca700 (LWP 4309) "multiple-step-o" 0x0000000000400827 in child_function_2 (arg=0x0) at ../../../src/gdb/testsuite/gdb.threads/multiple-step-overs.c:60
* 1 Thread 0x7ffff7fcb740 (LWP 4305) "multiple-step-o" main () at ../../../src/gdb/testsuite/gdb.threads/multiple-step-overs.c:99
(gdb) PASS: gdb.threads/multiple-step-overs.exp: step: info threads shows all threads
set scheduler-locking on
(gdb) PASS: gdb.threads/multiple-step-overs.exp: step: set scheduler-locking on
break 44
Breakpoint 3 at 0x4007d3: file ../../../src/gdb/testsuite/gdb.threads/multiple-step-overs.c, line 44.
(gdb) break 61
Breakpoint 4 at 0x40082d: file ../../../src/gdb/testsuite/gdb.threads/multiple-step-overs.c, line 61.
(gdb) thread 3
[Switching to thread 3 (Thread 0x7ffff77c9700 (LWP 4310))]
#0 0x00000000004007ca in child_function_3 (arg=0x1) at ../../../src/gdb/testsuite/gdb.threads/multiple-step-overs.c:43
43 (*myp) ++;
(gdb) PASS: gdb.threads/multiple-step-overs.exp: step: thread 3
continue
Continuing.
Breakpoint 3, child_function_3 (arg=0x1) at ../../../src/gdb/testsuite/gdb.threads/multiple-step-overs.c:44
44 callme (); /* set breakpoint thread 3 here */
(gdb) PASS: gdb.threads/multiple-step-overs.exp: step: continue to breakpoint: run to breakpoint in thread 3
p *myp = 0
$1 = 0
(gdb) PASS: gdb.threads/multiple-step-overs.exp: step: unbreak loop in thread 3
thread 2
[Switching to thread 2 (Thread 0x7ffff7fca700 (LWP 4309))]
#0 0x0000000000400827 in child_function_2 (arg=0x0) at ../../../src/gdb/testsuite/gdb.threads/multiple-step-overs.c:60
60 (*myp) ++;
(gdb) PASS: gdb.threads/multiple-step-overs.exp: step: thread 2
continue
Continuing.
Breakpoint 4, child_function_2 (arg=0x0) at ../../../src/gdb/testsuite/gdb.threads/multiple-step-overs.c:61
61 callme (); /* set breakpoint thread 2 here */
(gdb) PASS: gdb.threads/multiple-step-overs.exp: step: continue to breakpoint: run to breakpoint in thread 2
p *myp = 0
$2 = 0
(gdb) PASS: gdb.threads/multiple-step-overs.exp: step: unbreak loop in thread 2
thread 1
[Switching to thread 1 (Thread 0x7ffff7fcb740 (LWP 4305))]
#0 main () at ../../../src/gdb/testsuite/gdb.threads/multiple-step-overs.c:99
99 wait_threads (); /* set wait-threads breakpoint here */
(gdb) PASS: gdb.threads/multiple-step-overs.exp: step: thread 1
set scheduler-locking off
(gdb) PASS: gdb.threads/multiple-step-overs.exp: step: set scheduler-locking off
At this point all thread are stopped for a breakpoint that needs stepping over.
(gdb) step
Breakpoint 2, main () at ../../../src/gdb/testsuite/gdb.threads/multiple-step-overs.c:99
99 wait_threads (); /* set wait-threads breakpoint here */
(gdb) FAIL: gdb.threads/multiple-step-overs.exp: step
But that "step" retriggers the same breakpoint instead of making
progress.
The patch teaches GDB to step over all breakpoints of all threads
before resuming the stepping thread.
Tested on x86_64 Fedora 17, against pristine mainline, and also my
branch that implements software single-stepping on x86.
gdb/
2014-02-25 Pedro Alves <palves@redhat.com>
* infrun.c (prepare_to_proceed): Delete.
(thread_still_needs_step_over): New function.
(find_thread_needs_step_over): New function.
(proceed): If the current thread needs a step-over, set its
steping_over_breakpoint flag. Adjust to use
find_thread_needs_step_over instead of prepare_to_proceed.
(process_event_stop_test): For BPSTAT_WHAT_STOP_NOISY and
BPSTAT_WHAT_STOP_SILENT, assume the thread stopped for a
breakpoint.
(switch_back_to_stepped_thread): Step over breakpoints of all
threads not the stepping thread, before switching back to the
stepping thread.
gdb/testsuite/
2014-02-25 Pedro Alves <palves@redhat.com>
* gdb.threads/multiple-step-overs.c: New file.
* gdb.threads/multiple-step-overs.exp: New file.
* gdb.threads/signal-while-stepping-over-bp-other-thread.exp:
Adjust expected infrun debug output.
---
gdb/infrun.c | 238 ++++++++++++---------
gdb/testsuite/gdb.threads/multiple-step-overs.c | 105 +++++++++
gdb/testsuite/gdb.threads/multiple-step-overs.exp | 80 +++++++
.../signal-while-stepping-over-bp-other-thread.exp | 2 +-
4 files changed, 326 insertions(+), 99 deletions(-)
create mode 100644 gdb/testsuite/gdb.threads/multiple-step-overs.c
create mode 100644 gdb/testsuite/gdb.threads/multiple-step-overs.exp
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 1ea8d04..bde40c5 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -90,8 +90,6 @@ static int currently_stepping_or_nexting_callback (struct thread_info *tp,
static void xdb_handle_command (char *args, int from_tty);
-static int prepare_to_proceed (int);
-
static void print_exited_reason (int exitstatus);
static void print_signal_exited_reason (enum gdb_signal siggnal);
@@ -2053,75 +2051,74 @@ clear_proceed_status (void)
}
}
-/* Check the current thread against the thread that reported the most recent
- event. If a step-over is required return TRUE and set the current thread
- to the old thread. Otherwise return FALSE.
-
- This should be suitable for any targets that support threads. */
+/* Returns true if TP is still stopped at a breakpoint that needs
+ stepping-over in order to make progress. If the breakpoint is gone
+ meanwhile, we can skip the whole step-over dance. */
static int
-prepare_to_proceed (int step)
+thread_still_needs_step_over (struct thread_info *tp)
+{
+ if (tp->stepping_over_breakpoint)
+ {
+ struct regcache *regcache = get_thread_regcache (tp->ptid);
+
+ if (breakpoint_here_p (get_regcache_aspace (regcache),
+ regcache_read_pc (regcache)))
+ return 1;
+
+ tp->stepping_over_breakpoint = 0;
+ }
+
+ return 0;
+}
+
+/* Look a thread other than EXCEPT that has previously reported a
+ breakpoint event, and thus needs a step-over in order to make
+ progress. Returns NULL is none is found. STEP indicates whether
+ we're about to step the current thread, in order to decide whether
+ "set scheduler-locking step" applies. */
+
+static struct thread_info *
+find_thread_needs_step_over (int step, struct thread_info *except)
{
- ptid_t wait_ptid;
- struct target_waitstatus wait_status;
int schedlock_enabled;
+ struct thread_info *tp, *current;
/* With non-stop mode on, threads are always handled individually. */
gdb_assert (! non_stop);
- /* Get the last target status returned by target_wait(). */
- get_last_target_status (&wait_ptid, &wait_status);
-
- /* Make sure we were stopped at a breakpoint. */
- if (wait_status.kind != TARGET_WAITKIND_STOPPED
- || (wait_status.value.sig != GDB_SIGNAL_TRAP
- && wait_status.value.sig != GDB_SIGNAL_ILL
- && wait_status.value.sig != GDB_SIGNAL_SEGV
- && wait_status.value.sig != GDB_SIGNAL_EMT))
- {
- return 0;
- }
-
schedlock_enabled = (scheduler_mode == schedlock_on
|| (scheduler_mode == schedlock_step
&& step));
- /* Don't switch over to WAIT_PTID if scheduler locking is on. */
- if (schedlock_enabled)
- return 0;
-
- /* Don't switch over if we're about to resume some other process
- other than WAIT_PTID's, and schedule-multiple is off. */
- if (!sched_multi
- && ptid_get_pid (wait_ptid) != ptid_get_pid (inferior_ptid))
- return 0;
+ current = inferior_thread ();
- /* Switched over from WAIT_PID. */
- if (!ptid_equal (wait_ptid, minus_one_ptid)
- && !ptid_equal (inferior_ptid, wait_ptid))
+ /* If scheduler locking applies, we can avoid iterating over all
+ threads. */
+ if (schedlock_enabled)
{
- struct regcache *regcache = get_thread_regcache (wait_ptid);
+ if (except != current
+ && thread_still_needs_step_over (current))
+ return current;
- if (breakpoint_here_p (get_regcache_aspace (regcache),
- regcache_read_pc (regcache)))
- {
- /* Switch back to WAIT_PID thread. */
- switch_to_thread (wait_ptid);
+ return NULL;
+ }
- if (debug_infrun)
- fprintf_unfiltered (gdb_stdlog,
- "infrun: prepare_to_proceed (step=%d), "
- "switched to [%s]\n",
- step, target_pid_to_str (inferior_ptid));
+ ALL_THREADS (tp)
+ {
+ /* Ignore the EXCEPT thread. */
+ if (tp == except)
+ continue;
+ /* Ignore threads of processes we're not resuming. */
+ if (!sched_multi
+ && ptid_get_pid (tp->ptid) != ptid_get_pid (inferior_ptid))
+ continue;
- /* We return 1 to indicate that there is a breakpoint here,
- so we need to step over it before continuing to avoid
- hitting it straight away. */
- return 1;
- }
+ if (thread_still_needs_step_over (tp))
+ return tp;
}
- return 0;
+ return NULL;
}
/* Basic routine for continuing the program in various fashions.
@@ -2144,8 +2141,6 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step)
struct thread_info *tp;
CORE_ADDR pc;
struct address_space *aspace;
- /* GDB may force the inferior to step due to various reasons. */
- int force_step = 0;
/* If we're stopped at a fork/vfork, follow the branch set by the
"set follow-fork-mode" command; otherwise, we'll just proceed
@@ -2173,6 +2168,9 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step)
if (step < 0)
stop_after_trap = 1;
+ /* Fill in with reasonable starting values. */
+ init_thread_stepping_state (tp);
+
if (addr == (CORE_ADDR) -1)
{
if (pc == stop_pc && breakpoint_here_p (aspace, pc)
@@ -2185,14 +2183,13 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step)
Note, we don't do this in reverse, because we won't
actually be executing the breakpoint insn anyway.
We'll be (un-)executing the previous instruction. */
-
- force_step = 1;
+ tp->stepping_over_breakpoint = 1;
else if (gdbarch_single_step_through_delay_p (gdbarch)
&& gdbarch_single_step_through_delay (gdbarch,
get_current_frame ()))
/* We stepped onto an instruction that needs to be stepped
again before re-inserting the breakpoint, do so. */
- force_step = 1;
+ tp->stepping_over_breakpoint = 1;
}
else
{
@@ -2211,6 +2208,8 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step)
;
else
{
+ struct thread_info *step_over;
+
/* In a multi-threaded task we may select another thread and
then continue or step.
@@ -2219,31 +2218,29 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step)
execution (i.e. it will report a breakpoint hit incorrectly).
So we must step over it first.
- prepare_to_proceed checks the current thread against the
- thread that reported the most recent event. If a step-over
- is required it returns TRUE and sets the current thread to
- the old thread. */
-
- /* Store the prev_pc for the stepping thread too, needed by
- switch_back_to_stepping thread. */
- tp->prev_pc = regcache_read_pc (get_current_regcache ());
-
- if (prepare_to_proceed (step))
+ Look for a thread other than the current (TP) that reported a
+ breakpoint hit and hasn't been resumed yet since. */
+ step_over = find_thread_needs_step_over (step, tp);
+ if (step_over != NULL)
{
- force_step = 1;
- /* The current thread changed. */
- tp = inferior_thread ();
+ if (debug_infrun)
+ fprintf_unfiltered (gdb_stdlog,
+ "infrun: need to step-over [%s] first\n",
+ target_pid_to_str (step_over->ptid));
+
+ /* Store the prev_pc for the stepping thread too, needed by
+ switch_back_to_stepping thread. */
+ tp->prev_pc = regcache_read_pc (get_current_regcache ());
+ switch_to_thread (step_over->ptid);
+ tp = step_over;
}
}
- if (force_step)
- tp->control.trap_expected = 1;
-
/* If we need to step over a breakpoint, and we're not using
displaced stepping to do so, insert all breakpoints (watchpoints,
etc.) but the one we're stepping over, step one instruction, and
then re-insert the breakpoint when that step is finished. */
- if (tp->control.trap_expected && !use_displaced_stepping (gdbarch))
+ if (tp->stepping_over_breakpoint && !use_displaced_stepping (gdbarch))
{
struct regcache *regcache = get_current_regcache ();
@@ -2258,6 +2255,8 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step)
insert_breakpoints ();
+ tp->control.trap_expected = tp->stepping_over_breakpoint;
+
if (!non_stop)
{
/* Pass the last stop signal to the thread we're resuming,
@@ -2321,14 +2320,11 @@ 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 ());
- /* Fill in with reasonable starting values. */
- init_thread_stepping_state (tp);
-
/* Reset to normal state. */
init_infwait_state ();
/* Resume inferior. */
- resume (force_step || step || bpstat_should_step (),
+ resume (tp->control.trap_expected || step || bpstat_should_step (),
tp->suspend.stop_signal);
/* Wait for it to stop (if not standalone)
@@ -4468,8 +4464,10 @@ process_event_stop_test (struct execution_control_state *ecs)
fprintf_unfiltered (gdb_stdlog, "infrun: BPSTAT_WHAT_STOP_NOISY\n");
stop_print_frame = 1;
- /* We are about to nuke the step_resume_breakpointt via the
- cleanup chain, so no need to worry about it here. */
+ /* Assume the thread stopped for a breapoint. We'll still check
+ whether a/the breakpoint is there when the thread is next
+ resumed. */
+ ecs->event_thread->stepping_over_breakpoint = 1;
stop_stepping (ecs);
return;
@@ -4479,9 +4477,10 @@ process_event_stop_test (struct execution_control_state *ecs)
fprintf_unfiltered (gdb_stdlog, "infrun: BPSTAT_WHAT_STOP_SILENT\n");
stop_print_frame = 0;
- /* We are about to nuke the step_resume_breakpoin via the
- cleanup chain, so no need to worry about it here. */
-
+ /* Assume the thread stopped for a breapoint. We'll still check
+ whether a/the breakpoint is there when the thread is next
+ resumed. */
+ ecs->event_thread->stepping_over_breakpoint = 1;
stop_stepping (ecs);
return;
@@ -5106,25 +5105,68 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs)
if (!non_stop)
{
struct thread_info *tp;
-
- tp = iterate_over_threads (currently_stepping_or_nexting_callback,
- ecs->event_thread);
- if (tp)
+ struct thread_info *stepping_thread;
+
+ /* If any thread is blocked on some internal breakpoint, and we
+ simply need to step over that breakpoint to get it going
+ again, do that first. */
+
+ /* However, if we see an event for the stepping thread, then we
+ know all other threads have been moved past their breakpoints
+ already. Let the caller check whether the step is finished,
+ etc., before deciding to move it past a breakpoint. */
+ if (ecs->event_thread->control.step_range_end)
+ return 0;
+
+ /* Check if the current thread is blocking on an incomplete
+ step-over, interrupted by a random signal. */
+ if (ecs->event_thread->control.trap_expected
+ && ecs->event_thread->suspend.stop_signal != GDB_SIGNAL_TRAP)
{
- struct frame_info *frame;
- struct gdbarch *gdbarch;
+ if (debug_infrun)
+ {
+ fprintf_unfiltered (gdb_stdlog,
+ "infrun: need to finish step-over of [%s]\n",
+ target_pid_to_str (ecs->event_thread->ptid));
+ }
+ keep_going (ecs);
+ return 1;
+ }
- /* However, if the current thread is blocked on some internal
- breakpoint, and we simply need to step over that breakpoint
- to get it going again, do that first. */
- if ((ecs->event_thread->control.trap_expected
- && ecs->event_thread->suspend.stop_signal != GDB_SIGNAL_TRAP)
- || ecs->event_thread->stepping_over_breakpoint)
+ stepping_thread
+ = iterate_over_threads (currently_stepping_or_nexting_callback,
+ ecs->event_thread);
+
+ /* Check if any other thread except the stepping thread that
+ needs to start a step-over. Do that before actually
+ proceeding with step/next/etc. */
+ tp = find_thread_needs_step_over (stepping_thread != NULL,
+ stepping_thread);
+ if (tp != NULL)
+ {
+ if (debug_infrun)
{
- keep_going (ecs);
- return 1;
+ fprintf_unfiltered (gdb_stdlog,
+ "infrun: need to step-over [%s]\n",
+ target_pid_to_str (tp->ptid));
}
+ gdb_assert (!tp->control.trap_expected);
+ gdb_assert (!tp->control.step_range_end);
+
+ ecs->ptid = tp->ptid;
+ ecs->event_thread = tp;
+ switch_to_thread (ecs->ptid);
+ keep_going (ecs);
+ return 1;
+ }
+
+ tp = stepping_thread;
+ if (tp != NULL)
+ {
+ struct frame_info *frame;
+ struct gdbarch *gdbarch;
+
/* If the stepping thread exited, then don't try to switch
back and resume it, which could fail in several different
ways depending on the target. Instead, just keep going.
@@ -5687,7 +5729,7 @@ 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->event_thread->stepping_over_breakpoint
+ if (thread_still_needs_step_over (ecs->event_thread)
&& !use_displaced_stepping (get_regcache_arch (regcache)))
{
/* Can't step over more than one breakpoint simultaneously
diff --git a/gdb/testsuite/gdb.threads/multiple-step-overs.c b/gdb/testsuite/gdb.threads/multiple-step-overs.c
new file mode 100644
index 0000000..523a88a
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/multiple-step-overs.c
@@ -0,0 +1,105 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2009-2014 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#include <pthread.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <signal.h>
+
+unsigned int args[2];
+
+pthread_barrier_t barrier;
+pthread_t child_thread_2, child_thread_3;
+
+void
+callme (void)
+{
+}
+
+void *
+child_function_3 (void *arg)
+{
+ int my_number = (long) arg;
+ volatile int *myp = (int *) &args[my_number];
+
+ pthread_barrier_wait (&barrier);
+
+ while (*myp > 0)
+ {
+ (*myp) ++;
+ callme (); /* set breakpoint thread 3 here */
+ }
+
+ pthread_exit (NULL);
+}
+
+void *
+child_function_2 (void *arg)
+{
+ int my_number = (long) arg;
+ volatile int *myp = (int *) &args[my_number];
+
+ pthread_barrier_wait (&barrier);
+
+ while (*myp > 0)
+ {
+ (*myp) ++;
+ callme (); /* set breakpoint thread 2 here */
+ }
+
+ pthread_exit (NULL);
+}
+
+static int
+wait_threads (void)
+{
+ return 1; /* in wait_threads */
+}
+
+int
+main ()
+{
+ int res;
+ long i;
+
+ /* Call these early so that PLTs for these are resolved soon,
+ instead of in the threads. RTLD_NOW should work as well. */
+ usleep (0);
+ pthread_barrier_init (&barrier, NULL, 1);
+ pthread_barrier_wait (&barrier);
+
+ pthread_barrier_init (&barrier, NULL, 2);
+
+ i = 0;
+ args[i] = 1;
+ res = pthread_create (&child_thread_2,
+ NULL, child_function_2, (void *) i);
+ pthread_barrier_wait (&barrier);
+ callme ();
+
+ i = 1;
+ args[i] = 1;
+ res = pthread_create (&child_thread_3,
+ NULL, child_function_3, (void *) i);
+ pthread_barrier_wait (&barrier);
+ wait_threads (); /* set wait-threads breakpoint here */
+
+ pthread_join (child_thread_2, NULL);
+ pthread_join (child_thread_3, NULL);
+
+ exit(EXIT_SUCCESS);
+}
diff --git a/gdb/testsuite/gdb.threads/multiple-step-overs.exp b/gdb/testsuite/gdb.threads/multiple-step-overs.exp
new file mode 100644
index 0000000..4426586
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/multiple-step-overs.exp
@@ -0,0 +1,80 @@
+# Copyright (C) 2011-2014 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+# Test that GDB steps over all breakpoints of threads not the stepping
+# thread, before actually proceeding with the stepped thread.
+
+standard_testfile
+set executable ${testfile}
+
+if [target_info exists gdb,nosignals] {
+ verbose "Skipping ${testfile}.exp because of nosignals."
+ return -1
+}
+
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
+ executable [list debug "incdir=${objdir}"]] != "" } {
+ return -1
+}
+
+# Prepare environment for test. PREFIX is used as prefix in test
+# messages.
+
+proc setup { prefix } {
+ global executable
+
+ with_test_prefix $prefix {
+ clean_restart $executable
+
+ if ![runto_main] {
+ return -1
+ }
+
+ gdb_breakpoint [gdb_get_line_number "set wait-threads breakpoint here"]
+ gdb_continue_to_breakpoint "run to breakpoint"
+ gdb_test "info threads" "3 .* 2 .*\\\* 1.*" "info threads shows all threads"
+
+ gdb_test_no_output "set scheduler-locking on"
+
+ gdb_breakpoint [gdb_get_line_number "set breakpoint thread 3 here"]
+ gdb_breakpoint [gdb_get_line_number "set breakpoint thread 2 here"]
+
+ gdb_test "thread 3" "Switching.*"
+ gdb_continue_to_breakpoint "run to breakpoint in thread 3"
+ gdb_test "p *myp = 0" " = 0" "unbreak loop in thread 3"
+
+ gdb_test "thread 2" "Switching.*"
+ gdb_continue_to_breakpoint "run to breakpoint in thread 2"
+ gdb_test "p *myp = 0" " = 0" "unbreak loop in thread 2"
+
+ # Switch back to thread 1 and disable scheduler locking.
+ gdb_test "thread 1" "Switching.*"
+ gdb_test "set scheduler-locking off"
+
+ # Now all 3 threads are stopped for a breakpoint that needs to
+ # be stepped over before thread 1 is resumed.
+ }
+}
+
+setup "step"
+gdb_test "step" "in wait_threads .*"
+
+setup "next"
+gdb_test "set debug infrun 1" ".*"
+gdb_test "next" "pthread_join.*"
+
+setup "continue"
+gdb_breakpoint [gdb_get_line_number "EXIT_SUCCESS"]
+gdb_test "continue" "EXIT_SUCCESS.*"
diff --git a/gdb/testsuite/gdb.threads/signal-while-stepping-over-bp-other-thread.exp b/gdb/testsuite/gdb.threads/signal-while-stepping-over-bp-other-thread.exp
index bf5ea60..c46dad2 100644
--- a/gdb/testsuite/gdb.threads/signal-while-stepping-over-bp-other-thread.exp
+++ b/gdb/testsuite/gdb.threads/signal-while-stepping-over-bp-other-thread.exp
@@ -107,7 +107,7 @@ gdb_test "set debug infrun 1"
gdb_test \
"step" \
- ".*prepare_to_proceed \\(step=1\\), switched to.*signal arrived while stepping over breakpoint.*switching back to stepped thread.*stepped to a different line.*callme.*" \
+ ".*need to step-over.*resume \\(step=1.*signal arrived while stepping over breakpoint.*switching back to stepped thread.*stepped to a different line.*callme.*" \
"step"
set cnt_after [get_value "args\[$my_number\]" "get count after step"]
--
1.7.11.7
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH 4/6] Fix for even more missed events; eliminate thread-hop code.
2014-02-25 20:32 [PATCH 0/6] Fix a bunch of run control bugs Pedro Alves
` (3 preceding siblings ...)
2014-02-25 20:33 ` [PATCH 5/6] Handle multiple step-overs Pedro Alves
@ 2014-02-25 20:33 ` Pedro Alves
2014-03-05 15:45 ` Pedro Alves
2014-02-25 21:08 ` [PATCH 6/6] Make signal-while-stepping-over-bp-other-thread.exp run against remote targets too Pedro Alves
2014-02-26 13:36 ` [PATCH 3.5/6] Multiple Ada task-specific breakpoints at the same address Pedro Alves
6 siblings, 1 reply; 17+ messages in thread
From: Pedro Alves @ 2014-02-25 20:33 UTC (permalink / raw)
To: gdb-patches
Even with deferred_step_ptid out of the way, GDB can still lose
watchpoints on software single-step targets.
Here in the thread-hop code:
if (!ptid_equal (singlestep_ptid, ecs->ptid)
&& in_thread_list (singlestep_ptid))
{
/* If the PC of the thread we were trying to single-step
has changed, discard this event (which we were going
to ignore anyway), and pretend we saw that thread
trap. This prevents us continuously moving the
single-step breakpoint forward, one instruction at a
time. If the PC has changed, then the thread we were
trying to single-step has trapped or been signalled,
but the event has not been reported to GDB yet.
There might be some cases where this loses signal
information, if a signal has arrived at exactly the
same time that the PC changed, but this is the best
we can do with the information available. Perhaps we
should arrange to report all events for all threads
when they stop, or to re-poll the remote looking for
this particular thread (i.e. temporarily enable
schedlock). */
CORE_ADDR new_singlestep_pc
= regcache_read_pc (get_thread_regcache (singlestep_ptid));
if (new_singlestep_pc != singlestep_pc)
{
enum gdb_signal stop_signal;
if (debug_infrun)
fprintf_unfiltered (gdb_stdlog, "infrun: unexpected thread,"
" but expected thread advanced also\n");
/* The current context still belongs to
singlestep_ptid. Don't swap here, since that's
the context we want to use. Just fudge our
state and continue. */
stop_signal = ecs->event_thread->suspend.stop_signal;
ecs->event_thread->suspend.stop_signal = GDB_SIGNAL_0;
ecs->ptid = singlestep_ptid;
ecs->event_thread = find_thread_ptid (ecs->ptid);
ecs->event_thread->suspend.stop_signal = stop_signal;
stop_pc = new_singlestep_pc;
}
else
{
if (debug_infrun)
fprintf_unfiltered (gdb_stdlog,
"infrun: unexpected thread\n");
thread_hop_needed = 1;
stepping_past_singlestep_breakpoint = 1;
saved_singlestep_ptid = singlestep_ptid;
}
}
}
Note we either end up with thread_hop_needed, ignoring the watchpoint
SIGTRAP, or switch to the stepping thread, again ignoring that the
SIGTRAP could be for some other event.
So the fix is similar to the deferred_step_ptid fix -- defer the
thread hop to _after_ the SIGTRAP had a change of passing through the
regular bpstat handling. If the wrong thread hits a breakpoint, we'll
just end up with BPSTAT_WHAT_SINGLE, and if nothing causes a stop,
keep_going starts a step-over.
But in order to do that, we need to make bpstat aware of single-step
breakpoints. So this patch starts convering the single-step
breakpoints to real breakpoints. It doesn't put them in the regular
breakpoint chain yet though. More changes would be needed for that.
The stepping_past_singlestep_breakpoint mechanism is really not
necessary -- just setting the thread to step over a breakpoint with
thread->stepping_over_breakpoint=1 has the exact same effect.
Special care is still needed to handle the case of PC of the thread we
were trying to single-step having changed, like in the old code. We
can't just keep_going and re-step it, as in that case we can over-step
the thread (if it was already done with the step, but hsn't reported
it yet, we'd ask it to step even further). That's now handled in
switch_back_to_stepped_thread. As bonus, we're now using a technique
that doesn't lose signals, unlike the old code -- insert a breakpoint
at PC, and resume, which either reports the breakpoint immediately, or
any pending signal.
Tested on x86_64 Fedora 17, against pristine mainline, and against a
branch that implements software single-step on x86.
gdb/
2014-02-21 Pedro Alves <palves@redhat.com>
* breakpoint.c (single_step_breakpoints): Change type to struct
breakpoint array.
(single_step_gdbarch): Delete.
(bpstat_stop_status): Handle single-step breakpoints.
(bpstat_what, bptype_string, print_one_breakpoint_location)
(init_bp_location): Handle bp_single_step.
(init_momentary_breakpoint): New, factored out from ...
(set_momentary_breakpoint): ... this. Rewrite.
(init_momentary_breakpoint_at_pc): New, factored out from ...
(set_momentary_breakpoint_at_pc): ... this. Rewrite.
(insert_single_step_breakpoint, remove_single_step_breakpoints)
(cancel_single_step_breakpoints, detach_single_step_breakpoints):
Rewrite/adjust to insert/remove real breakpoints instead of using
deprecated_insert_raw_breakpoint and
deprecated_remove_raw_breakpoint.
(single_step_breakpoint_inserted_here_p): Adjust.
* breakpoint.h (enum bptype) <bp_single_step>: New breakpoint
type.
* infrun.c (saved_singlestep_ptid)
(stepping_past_singlestep_breakpoint): Delete.
(resume): Remove stepping_past_singlestep_breakpoint handling.
(proceed): Store the prev_pc of the stepping thread too.
(init_wait_for_inferior): Adjust. Clear singlestep_ptid and
singlestep_pc.
(enum infwait_states): Delete infwait_thread_hop_state.
(handle_inferior_event): Adjust.
(handle_signal_stop): Delete stepping_past_singlestep_breakpoint
handling and the thread-hop code. Defer removing single-step
breakpoints till after bpstat handling.
(switch_back_to_stepped_thread): Handle the case of the stepped
thread having advanced already.
---
gdb/breakpoint.c | 191 +++++++++++++++++++++++-----------
gdb/breakpoint.h | 1 +
gdb/infrun.c | 308 +++++++++++++++++++------------------------------------
3 files changed, 238 insertions(+), 262 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index b462bad..811823d 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -587,6 +587,11 @@ static CORE_ADDR bp_location_shadow_len_after_address_max;
by a target. */
VEC(bp_location_p) *moribund_locations = NULL;
+/* One (or perhaps two) breakpoints used for software single
+ stepping. */
+
+static struct breakpoint *single_step_breakpoints[2];
+
/* Number of last breakpoint made. */
static int breakpoint_count;
@@ -5375,6 +5380,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))
@@ -5387,6 +5393,27 @@ bpstat_stop_status (struct address_space *aspace,
}
}
+ /* Check if a software single-step breakpoint explains the stop. */
+ if (ws->kind == TARGET_WAITKIND_STOPPED
+ && ws->value.sig == GDB_SIGNAL_TRAP)
+ {
+ int i;
+
+ for (i = 0; i < 2; i++)
+ {
+ struct breakpoint *bp = single_step_breakpoints[i];
+
+ if (bp != NULL
+ && breakpoint_location_address_match (bp->loc, aspace, bp_addr))
+ {
+ bs = bpstat_alloc (bp->loc, &bs_link);
+ bs->stop = 0;
+ bs->print = 0;
+ bs->print_it = print_it_noop;
+ }
+ }
+ }
+
/* A bit of special processing for shlib breakpoints. We need to
process solib loading here, so that the lists of loaded and
unloaded libraries are correct before we handle "catch load" and
@@ -5530,6 +5557,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:
@@ -5888,6 +5916,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"},
@@ -6079,6 +6108,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:
@@ -6957,6 +6987,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:
@@ -8884,33 +8915,43 @@ enable_breakpoints_after_startup (void)
}
-/* Set a breakpoint that will evaporate an end of command
- at address specified by SAL.
- Restrict it to frame FRAME if FRAME is nonzero. */
+/* Initialize a momentary breakpoint of type TYPE at address specified
+ by SAL. If FRAME_ID is valid, the breakpoint is restricted to that
+ frame. The breakpoint is not inserted in the breakpoint chain. */
-struct breakpoint *
-set_momentary_breakpoint (struct gdbarch *gdbarch, struct symtab_and_line sal,
- struct frame_id frame_id, enum bptype type)
+static struct breakpoint *
+init_momentary_breakpoint (struct breakpoint *b, struct gdbarch *gdbarch,
+ struct symtab_and_line sal, struct frame_id frame_id,
+ enum bptype type)
{
- struct breakpoint *b;
-
/* If FRAME_ID is valid, it should be a real frame, not an inlined or
tail-called one. */
gdb_assert (!frame_id_artificial_p (frame_id));
- b = set_raw_breakpoint (gdbarch, sal, type, &momentary_breakpoint_ops);
+ init_raw_breakpoint (b, gdbarch, sal, type, &momentary_breakpoint_ops);
b->enable_state = bp_enabled;
b->disposition = disp_donttouch;
b->frame_id = frame_id;
- /* If we're debugging a multi-threaded program, then we want
- momentary breakpoints to be active in only a single thread of
- control. */
- if (in_thread_list (inferior_ptid))
- b->thread = pid_to_thread_id (inferior_ptid);
+ b->thread = pid_to_thread_id (inferior_ptid);
+ gdb_assert (b->thread != 0);
- update_global_location_list_nothrow (1);
+ return b;
+}
+/* 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,
+ struct frame_id frame_id, enum bptype type)
+{
+ struct breakpoint *b = XNEW (struct breakpoint);
+
+ init_momentary_breakpoint (b, gdbarch, sal, frame_id, type);
+ add_to_breakpoint_chain (b);
+ update_global_location_list_nothrow (1);
return b;
}
@@ -8962,9 +9003,12 @@ clone_momentary_breakpoint (struct breakpoint *orig)
return momentary_breakpoint_from_master (orig, orig->type, orig->ops);
}
-struct breakpoint *
-set_momentary_breakpoint_at_pc (struct gdbarch *gdbarch, CORE_ADDR pc,
- enum bptype type)
+/* Initialize a momentary breakpoint of type TYPE at address PC. */
+
+static void
+init_momentary_breakpoint_at_pc (struct breakpoint *b,
+ struct gdbarch *gdbarch, CORE_ADDR pc,
+ enum bptype type)
{
struct symtab_and_line sal;
@@ -8973,7 +9017,21 @@ set_momentary_breakpoint_at_pc (struct gdbarch *gdbarch, CORE_ADDR pc,
sal.section = find_pc_overlay (pc);
sal.explicit_pc = 1;
- return set_momentary_breakpoint (gdbarch, sal, null_frame_id, type);
+ init_momentary_breakpoint (b, gdbarch, sal, null_frame_id, type);
+}
+
+/* Set a momentary breakpoint of type TYPE at address PC. */
+
+struct breakpoint *
+set_momentary_breakpoint_at_pc (struct gdbarch *gdbarch, CORE_ADDR pc,
+ enum bptype type)
+{
+ struct breakpoint *b = XNEW (struct breakpoint);
+
+ init_momentary_breakpoint_at_pc (b, gdbarch, pc, type);
+ add_to_breakpoint_chain (b);
+ update_global_location_list_nothrow (1);
+ return b;
}
\f
@@ -15027,12 +15085,6 @@ deprecated_remove_raw_breakpoint (struct gdbarch *gdbarch, void *bp)
return ret;
}
-/* One (or perhaps two) breakpoints used for software single
- stepping. */
-
-static void *single_step_breakpoints[2];
-static struct gdbarch *single_step_gdbarch[2];
-
/* Create and insert a breakpoint for software single step. */
void
@@ -15040,18 +15092,21 @@ insert_single_step_breakpoint (struct gdbarch *gdbarch,
struct address_space *aspace,
CORE_ADDR next_pc)
{
- void **bpt_p;
+ struct breakpoint **bp_p;
+ struct breakpoint *bp;
+ enum errors bp_err = GDB_NO_ERROR;
+ volatile struct gdb_exception ex;
+ int val;
+ struct bp_location *bl;
if (single_step_breakpoints[0] == NULL)
{
- bpt_p = &single_step_breakpoints[0];
- single_step_gdbarch[0] = gdbarch;
+ bp_p = &single_step_breakpoints[0];
}
else
{
gdb_assert (single_step_breakpoints[1] == NULL);
- bpt_p = &single_step_breakpoints[1];
- single_step_gdbarch[1] = gdbarch;
+ bp_p = &single_step_breakpoints[1];
}
/* NOTE drow/2006-04-11: A future improvement to this function would
@@ -15060,11 +15115,26 @@ insert_single_step_breakpoint (struct gdbarch *gdbarch,
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. */
+ bp = XNEW (struct breakpoint);
+ init_momentary_breakpoint_at_pc (bp, gdbarch, next_pc, bp_single_step);
+ bl = bp->loc;
- *bpt_p = deprecated_insert_raw_breakpoint (gdbarch, aspace, next_pc);
- if (*bpt_p == NULL)
- error (_("Could not insert single-step breakpoint at %s"),
+ bl->target_info.placed_address = bl->address;
+ bl->target_info.placed_address_space = bl->pspace->aspace;
+ bl->target_info.length = bl->length;
+
+ TRY_CATCH (ex, RETURN_MASK_ALL)
+ {
+ val = bp->ops->insert_location (bl);
+ }
+ if (ex.reason < 0 || val != 0)
+ {
+ delete_breakpoint (bp);
+ error (_("Could not insert single-step breakpoint at %s"),
paddress (gdbarch, next_pc));
+ }
+ else
+ *bp_p = bp;
}
/* Check if the breakpoints used for software single stepping
@@ -15082,21 +15152,20 @@ single_step_breakpoints_inserted (void)
void
remove_single_step_breakpoints (void)
{
- gdb_assert (single_step_breakpoints[0] != NULL);
+ int i;
- /* 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;
+ gdb_assert (single_step_breakpoints[0] != NULL);
- if (single_step_breakpoints[1] != NULL)
+ for (i = 0; i < 2; i++)
{
- deprecated_remove_raw_breakpoint (single_step_gdbarch[1],
- single_step_breakpoints[1]);
- single_step_gdbarch[1] = NULL;
- single_step_breakpoints[1] = NULL;
+ struct breakpoint *bp = single_step_breakpoints[i];
+
+ if (bp != NULL)
+ {
+ bp->ops->remove_location (bp->loc);
+ delete_breakpoint (bp);
+ single_step_breakpoints[i] = NULL;
+ }
}
}
@@ -15111,12 +15180,15 @@ 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;
- }
+ {
+ struct breakpoint *bp = single_step_breakpoints[i];
+
+ if (bp != NULL)
+ {
+ delete_breakpoint (bp);
+ single_step_breakpoints[i] = NULL;
+ }
+ }
}
/* Detach software single-step breakpoints from INFERIOR_PTID without
@@ -15128,9 +15200,12 @@ 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]);
+ {
+ struct breakpoint *bp = single_step_breakpoints[i];
+
+ if (bp != NULL)
+ bp->ops->remove_location (bp->loc);
+ }
}
/* Check whether a software single-step breakpoint is inserted at
@@ -15144,11 +15219,9 @@ single_step_breakpoint_inserted_here_p (struct address_space *aspace,
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))
+ struct breakpoint *bp = single_step_breakpoints[i];
+
+ if (bp != NULL && breakpoint_location_address_match (bp->loc, aspace, pc))
return 1;
}
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 42b1492..109afbe 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -58,6 +58,7 @@ 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 */
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 8bfce34..1ea8d04 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -972,11 +972,6 @@ static ptid_t singlestep_ptid;
/* PC when we started this single-step. */
static CORE_ADDR singlestep_pc;
-/* If another thread hit the singlestep breakpoint, we save the original
- thread here so that we can resume single-stepping it later. */
-static ptid_t saved_singlestep_ptid;
-static int stepping_past_singlestep_breakpoint;
-
/* If stepping over a breakpoint (not displaced stepping), this is the
address (and address space) where that breakpoint is inserted.
When not stepping over a breakpoint, STEP_OVER_ASPACE is NULL.
@@ -1905,24 +1900,8 @@ a command like `return' or `jump' to continue execution."));
resume_ptid = user_visible_resume_ptid (step);
/* Maybe resume a single thread after all. */
- if (singlestep_breakpoints_inserted_p
- && stepping_past_singlestep_breakpoint)
- {
- /* The situation here is as follows. In thread T1 we wanted to
- single-step. Lacking hardware single-stepping we've
- set breakpoint at the PC of the next instruction -- call it
- P. After resuming, we've hit that breakpoint in thread T2.
- Now we've removed original breakpoint, inserted breakpoint
- at P+1, and try to step to advance T2 past breakpoint.
- We need to step only T2, as if T1 is allowed to freely run,
- it can run past P, and if other threads are allowed to run,
- they can hit breakpoint at P+1, and nested hits of single-step
- breakpoints is not something we'd want -- that's complicated
- to support, and has no value. */
- resume_ptid = inferior_ptid;
- }
- else if ((step || singlestep_breakpoints_inserted_p)
- && tp->control.trap_expected)
+ if ((step || singlestep_breakpoints_inserted_p)
+ && tp->control.trap_expected)
{
/* We're allowing a thread to run past a breakpoint it has
hit, by single-stepping the thread with the breakpoint
@@ -2187,6 +2166,7 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step)
gdbarch = get_regcache_arch (regcache);
aspace = get_regcache_aspace (regcache);
pc = regcache_read_pc (regcache);
+ tp = inferior_thread ();
if (step > 0)
step_start_function = find_pc_function (pc);
@@ -2243,13 +2223,19 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step)
thread that reported the most recent event. If a step-over
is required it returns TRUE and sets the current thread to
the old thread. */
+
+ /* Store the prev_pc for the stepping thread too, needed by
+ switch_back_to_stepping thread. */
+ tp->prev_pc = regcache_read_pc (get_current_regcache ());
+
if (prepare_to_proceed (step))
- force_step = 1;
+ {
+ force_step = 1;
+ /* The current thread changed. */
+ tp = inferior_thread ();
+ }
}
- /* prepare_to_proceed may change the current thread. */
- tp = inferior_thread ();
-
if (force_step)
tp->control.trap_expected = 1;
@@ -2402,8 +2388,6 @@ init_wait_for_inferior (void)
clear_proceed_status ();
- stepping_past_singlestep_breakpoint = 0;
-
target_last_wait_ptid = minus_one_ptid;
previous_inferior_ptid = inferior_ptid;
@@ -2411,6 +2395,9 @@ 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
@@ -2421,7 +2408,6 @@ init_wait_for_inferior (void)
enum infwait_states
{
infwait_normal_state,
- infwait_thread_hop_state,
infwait_step_watch_state,
infwait_nonstep_watch_state
};
@@ -3332,11 +3318,6 @@ handle_inferior_event (struct execution_control_state *ecs)
switch (infwait_state)
{
- case infwait_thread_hop_state:
- if (debug_infrun)
- fprintf_unfiltered (gdb_stdlog, "infrun: infwait_thread_hop_state\n");
- break;
-
case infwait_normal_state:
if (debug_infrun)
fprintf_unfiltered (gdb_stdlog, "infrun: infwait_normal_state\n");
@@ -3907,166 +3888,6 @@ handle_signal_stop (struct execution_control_state *ecs)
return;
}
- if (stepping_past_singlestep_breakpoint)
- {
- gdb_assert (singlestep_breakpoints_inserted_p);
- gdb_assert (ptid_equal (singlestep_ptid, ecs->ptid));
- gdb_assert (!ptid_equal (singlestep_ptid, saved_singlestep_ptid));
-
- stepping_past_singlestep_breakpoint = 0;
-
- /* We've either finished single-stepping past the single-step
- breakpoint, or stopped for some other reason. It would be nice if
- we could tell, but we can't reliably. */
- if (ecs->event_thread->suspend.stop_signal == GDB_SIGNAL_TRAP)
- {
- if (debug_infrun)
- fprintf_unfiltered (gdb_stdlog,
- "infrun: stepping_past_"
- "singlestep_breakpoint\n");
- /* Pull the single step breakpoints out of the target. */
- if (!ptid_equal (ecs->ptid, inferior_ptid))
- context_switch (ecs->ptid);
- remove_single_step_breakpoints ();
- singlestep_breakpoints_inserted_p = 0;
-
- ecs->event_thread->control.trap_expected = 0;
-
- context_switch (saved_singlestep_ptid);
- if (deprecated_context_hook)
- deprecated_context_hook (pid_to_thread_id (saved_singlestep_ptid));
-
- resume (1, GDB_SIGNAL_0);
- prepare_to_wait (ecs);
- return;
- }
- }
-
- /* See if a thread hit a thread-specific breakpoint that was meant for
- another thread. If so, then step that thread past the breakpoint,
- and continue it. */
-
- if (ecs->event_thread->suspend.stop_signal == GDB_SIGNAL_TRAP)
- {
- int thread_hop_needed = 0;
- struct address_space *aspace =
- get_regcache_aspace (get_thread_regcache (ecs->ptid));
-
- /* Check if a regular breakpoint has been hit before checking
- for a potential single step breakpoint. Otherwise, GDB will
- not see this breakpoint hit when stepping onto breakpoints. */
- if (regular_breakpoint_inserted_here_p (aspace, stop_pc))
- {
- if (!breakpoint_thread_match (aspace, stop_pc, ecs->ptid))
- thread_hop_needed = 1;
- }
- else if (singlestep_breakpoints_inserted_p)
- {
- /* We have not context switched yet, so this should be true
- no matter which thread hit the singlestep breakpoint. */
- gdb_assert (ptid_equal (inferior_ptid, singlestep_ptid));
- if (debug_infrun)
- fprintf_unfiltered (gdb_stdlog, "infrun: software single step "
- "trap for %s\n",
- target_pid_to_str (ecs->ptid));
-
- /* The call to in_thread_list is necessary because PTIDs sometimes
- change when we go from single-threaded to multi-threaded. If
- the singlestep_ptid is still in the list, assume that it is
- really different from ecs->ptid. */
- if (!ptid_equal (singlestep_ptid, ecs->ptid)
- && in_thread_list (singlestep_ptid))
- {
- /* If the PC of the thread we were trying to single-step
- has changed, discard this event (which we were going
- to ignore anyway), and pretend we saw that thread
- trap. This prevents us continuously moving the
- single-step breakpoint forward, one instruction at a
- time. If the PC has changed, then the thread we were
- trying to single-step has trapped or been signalled,
- but the event has not been reported to GDB yet.
-
- There might be some cases where this loses signal
- information, if a signal has arrived at exactly the
- same time that the PC changed, but this is the best
- we can do with the information available. Perhaps we
- should arrange to report all events for all threads
- when they stop, or to re-poll the remote looking for
- this particular thread (i.e. temporarily enable
- schedlock). */
-
- CORE_ADDR new_singlestep_pc
- = regcache_read_pc (get_thread_regcache (singlestep_ptid));
-
- if (new_singlestep_pc != singlestep_pc)
- {
- enum gdb_signal stop_signal;
-
- if (debug_infrun)
- fprintf_unfiltered (gdb_stdlog, "infrun: unexpected thread,"
- " but expected thread advanced also\n");
-
- /* The current context still belongs to
- singlestep_ptid. Don't swap here, since that's
- the context we want to use. Just fudge our
- state and continue. */
- stop_signal = ecs->event_thread->suspend.stop_signal;
- ecs->event_thread->suspend.stop_signal = GDB_SIGNAL_0;
- ecs->ptid = singlestep_ptid;
- ecs->event_thread = find_thread_ptid (ecs->ptid);
- ecs->event_thread->suspend.stop_signal = stop_signal;
- stop_pc = new_singlestep_pc;
- }
- else
- {
- if (debug_infrun)
- fprintf_unfiltered (gdb_stdlog,
- "infrun: unexpected thread\n");
-
- thread_hop_needed = 1;
- stepping_past_singlestep_breakpoint = 1;
- saved_singlestep_ptid = singlestep_ptid;
- }
- }
- }
-
- if (thread_hop_needed)
- {
- struct regcache *thread_regcache;
- int remove_status = 0;
-
- if (debug_infrun)
- fprintf_unfiltered (gdb_stdlog, "infrun: thread_hop_needed\n");
-
- /* Switch context before touching inferior memory, the
- previous thread may have exited. */
- if (!ptid_equal (inferior_ptid, ecs->ptid))
- context_switch (ecs->ptid);
-
- /* Saw a breakpoint, but it was hit by the wrong thread.
- Just continue. */
-
- if (singlestep_breakpoints_inserted_p)
- {
- /* Pull the single step breakpoints out of the target. */
- remove_single_step_breakpoints ();
- singlestep_breakpoints_inserted_p = 0;
- }
-
- if (!non_stop)
- {
- /* Only need to require the next event from this thread
- in all-stop mode. */
- waiton_ptid = ecs->ptid;
- infwait_state = infwait_thread_hop_state;
- }
-
- ecs->event_thread->stepping_over_breakpoint = 1;
- keep_going (ecs);
- return;
- }
- }
-
/* See if something interesting happened to the non-current thread. If
so, then switch to that thread. */
if (!ptid_equal (ecs->ptid, inferior_ptid))
@@ -4084,13 +3905,6 @@ handle_signal_stop (struct execution_control_state *ecs)
frame = get_current_frame ();
gdbarch = get_frame_arch (frame);
- if (singlestep_breakpoints_inserted_p)
- {
- /* Pull the single step breakpoints out of the target. */
- remove_single_step_breakpoints ();
- singlestep_breakpoints_inserted_p = 0;
- }
-
if (ecs->stepped_after_stopped_by_watchpoint)
stopped_by_watchpoint = 0;
else
@@ -4124,6 +3938,13 @@ handle_signal_stop (struct execution_control_state *ecs)
disable all watchpoints and breakpoints. */
int hw_step = 1;
+ if (singlestep_breakpoints_inserted_p)
+ {
+ /* Pull the single step breakpoints out of the target. */
+ remove_single_step_breakpoints ();
+ singlestep_breakpoints_inserted_p = 0;
+ }
+
if (!target_have_steppable_watchpoint)
{
remove_breakpoints ();
@@ -4206,6 +4027,13 @@ handle_signal_stop (struct execution_control_state *ecs)
if (ecs->event_thread->control.step_range_end == 0
&& step_through_delay)
{
+ if (singlestep_breakpoints_inserted_p)
+ {
+ /* Pull the single step breakpoints out of the target. */
+ remove_single_step_breakpoints ();
+ singlestep_breakpoints_inserted_p = 0;
+ }
+
/* The user issued a continue when stopped at a breakpoint.
Set up for another trap and get out of here. */
ecs->event_thread->stepping_over_breakpoint = 1;
@@ -4301,6 +4129,13 @@ handle_signal_stop (struct execution_control_state *ecs)
stopped_by_random_signal = 1;
+ if (singlestep_breakpoints_inserted_p)
+ {
+ /* Pull the single step breakpoints out of the target. */
+ remove_single_step_breakpoints ();
+ singlestep_breakpoints_inserted_p = 0;
+ }
+
if (signal_print[ecs->event_thread->suspend.stop_signal])
{
printed = 1;
@@ -4441,6 +4276,18 @@ process_event_stop_test (struct execution_control_state *ecs)
step_over_address = 0;
}
+ /* If the wrong thread hits a single-step breakpoint, we want bpstat
+ to return at least BPSTAT_WHAT_SINGLE. If the right thread hit
+ one though, the step is done, so remove them now avoiding a
+ needless step-over. */
+ if (singlestep_breakpoints_inserted_p
+ && ptid_equal (ecs->ptid, singlestep_ptid))
+ {
+ /* Pull the single step breakpoints out of the target. */
+ remove_single_step_breakpoints ();
+ singlestep_breakpoints_inserted_p = 0;
+ }
+
what = bpstat_what (ecs->event_thread->control.stop_bpstat);
if (what.call_dummy)
@@ -4448,6 +4295,13 @@ process_event_stop_test (struct execution_control_state *ecs)
stop_stack_dummy = what.call_dummy;
}
+ /* No longer need these. See above. */
+ if (singlestep_breakpoints_inserted_p)
+ {
+ remove_single_step_breakpoints ();
+ singlestep_breakpoints_inserted_p = 0;
+ }
+
/* If we hit an internal event that triggers symbol changes, the
current frame will be invalidated within bpstat_what (e.g., if we
hit an internal solib event). Re-fetch it. */
@@ -5257,6 +5111,9 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs)
ecs->event_thread);
if (tp)
{
+ struct frame_info *frame;
+ struct gdbarch *gdbarch;
+
/* However, if the current thread is blocked on some internal
breakpoint, and we simply need to step over that breakpoint
to get it going again, do that first. */
@@ -5314,7 +5171,52 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs)
ecs->event_thread = tp;
ecs->ptid = tp->ptid;
context_switch (ecs->ptid);
- keep_going (ecs);
+
+ stop_pc = regcache_read_pc (get_thread_regcache (ecs->ptid));
+ frame = get_current_frame ();
+ gdbarch = get_frame_arch (frame);
+
+ /* If the PC of the thread we were trying to single-step has
+ changed, then the thread we were trying to single-step
+ has trapped or been signalled, but the event has not been
+ reported to GDB yet. Re-poll the remote looking for this
+ particular thread (i.e. temporarily enable schedlock):
+
+ - set a break at the current PC
+ - resuming that particular thread, only (by setting
+ trap expected)
+
+ This prevents us continuously moving the single-step
+ breakpoint forward, one instruction at a time, thus
+ overstepping. */
+
+ if (gdbarch_software_single_step_p (gdbarch)
+ && stop_pc != tp->prev_pc)
+ {
+ if (debug_infrun)
+ fprintf_unfiltered (gdb_stdlog,
+ "infrun: expected thread advanced also\n");
+
+ 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);
+ }
+ else
+ {
+ if (debug_infrun)
+ fprintf_unfiltered (gdb_stdlog,
+ "infrun: expected thread still "
+ "hasn't advanced\n");
+ keep_going (ecs);
+ }
+
return 1;
}
}
--
1.7.11.7
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 4/6] Fix for even more missed events; eliminate thread-hop code.
2014-02-25 20:33 ` [PATCH 4/6] Fix for even more missed events; eliminate thread-hop code Pedro Alves
@ 2014-03-05 15:45 ` Pedro Alves
0 siblings, 0 replies; 17+ messages in thread
From: Pedro Alves @ 2014-03-05 15:45 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On 02/25/2014 08:32 PM, Pedro Alves wrote:
> But in order to do that, we need to make bpstat aware of single-step
> breakpoints. So this patch starts convering the single-step
> breakpoints to real breakpoints. It doesn't put them in the regular
> breakpoint chain yet though. More changes would be needed for that.
Unfortunately we're still not ready for this. watch-cond-infcall.exp
revealed that we need to remove single-step breakpoints before running
the infcall for the condition, otherwise nasty things happen, like
GDB referencing already deleted breakpoints following the bpstat
struct. I didn't notice this before as the failure is intermittent,
but valgrind showed it clearly. Also, patch #5's thread_still_needs_step_over
function call from keep_going meant that the end result of the series
would be that we'd no longer move the thread past the single-step
breakpoint, actually, as the single-step breakpoint (for the other
thread) was always removed before keep_going was reached...
int
thread_still_needs_step_over (struct thread_info *tp)
{
if (tp->stepping_over_breakpoint)
{
struct regcache *regcache = get_thread_regcache (tp->ptid);
if (breakpoint_here_p (get_regcache_aspace (regcache),
regcache_read_pc (regcache)))
v2 simplifies the patch further, and no longer needs to convert
single-step breakpoints to real breakpoints. I'll get back to
that separately...
--
Pedro Alves
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 6/6] Make signal-while-stepping-over-bp-other-thread.exp run against remote targets too.
2014-02-25 20:32 [PATCH 0/6] Fix a bunch of run control bugs Pedro Alves
` (4 preceding siblings ...)
2014-02-25 20:33 ` [PATCH 4/6] Fix for even more missed events; eliminate thread-hop code Pedro Alves
@ 2014-02-25 21:08 ` Pedro Alves
2014-02-26 13:36 ` [PATCH 3.5/6] Multiple Ada task-specific breakpoints at the same address Pedro Alves
6 siblings, 0 replies; 17+ messages in thread
From: Pedro Alves @ 2014-02-25 21:08 UTC (permalink / raw)
To: gdb-patches
Use pthread_kill instead of the host's "kill". The reason the test
wasn't written that way to begin with, is that done this way, before
the previous fixes to make GDB step-over all other threads before the
stepping thread, the test would fail...
Tested on x86_64 Fedora 17, native and gdbserver.
2014-02-25 Pedro Alves <palves@redhat.com>
* gdb.threads/signal-while-stepping-over-bp-other-thread.c (main):
Use pthread_kill to signal thread 2.
* gdb.threads/signal-while-stepping-over-bp-other-thread.exp:
Adjust to make the test send itself a signal rather than using the
host's "kill" command.
---
.../signal-while-stepping-over-bp-other-thread.c | 2 ++
.../signal-while-stepping-over-bp-other-thread.exp | 14 ++------------
2 files changed, 4 insertions(+), 12 deletions(-)
diff --git a/gdb/testsuite/gdb.threads/signal-while-stepping-over-bp-other-thread.c b/gdb/testsuite/gdb.threads/signal-while-stepping-over-bp-other-thread.c
index a4634f2..8839a6f 100644
--- a/gdb/testsuite/gdb.threads/signal-while-stepping-over-bp-other-thread.c
+++ b/gdb/testsuite/gdb.threads/signal-while-stepping-over-bp-other-thread.c
@@ -138,6 +138,8 @@ main ()
pthread_barrier_wait (&barrier);
callme (); /* set wait-thread-3 breakpoint here */
+ pthread_kill (child_thread_2, SIGUSR1);
+
pthread_join (child_thread_2, NULL);
pthread_join (child_thread_3, NULL);
diff --git a/gdb/testsuite/gdb.threads/signal-while-stepping-over-bp-other-thread.exp b/gdb/testsuite/gdb.threads/signal-while-stepping-over-bp-other-thread.exp
index c46dad2..2a30604 100644
--- a/gdb/testsuite/gdb.threads/signal-while-stepping-over-bp-other-thread.exp
+++ b/gdb/testsuite/gdb.threads/signal-while-stepping-over-bp-other-thread.exp
@@ -26,11 +26,6 @@ if [target_info exists gdb,nosignals] {
return -1
}
-# Test uses host "kill".
-if { [is_remote target] } {
- return -1
-}
-
if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
executable [list debug "incdir=${objdir}"]] != "" } {
return -1
@@ -67,11 +62,6 @@ gdb_breakpoint [gdb_get_line_number "set wait-thread-3 breakpoint here"]
gdb_continue_to_breakpoint "run to breakpoint"
gdb_test "info threads" "" "info threads with thread 3"
-set testpid [get_value "pid" "get pid of inferior"]
-if { $testpid == -1 } {
- return -1
-}
-
gdb_test "set scheduler-locking on"
gdb_breakpoint [gdb_get_line_number "set breakpoint child_two here"]
@@ -94,8 +84,8 @@ gdb_test "p *myp = 0" " = 0" "force loop break in thread 2"
# core.
gdb_test "handle SIGUSR1 print nostop pass" "" ""
-# Queue a signal in thread 2.
-remote_exec host "kill -SIGUSR1 ${testpid}"
+gdb_test "thread 1" "" "switch to thread 1 to queue signal in thread 2"
+gdb_test "next 2" "pthread_join .*" "queue signal in thread 2"
gdb_test "thread 3" "" "switch to thread 3 for stepping"
set my_number [get_value "my_number" "get my_number"]
--
1.7.11.7
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH 3.5/6] Multiple Ada task-specific breakpoints at the same address.
2014-02-25 20:32 [PATCH 0/6] Fix a bunch of run control bugs Pedro Alves
` (5 preceding siblings ...)
2014-02-25 21:08 ` [PATCH 6/6] Make signal-while-stepping-over-bp-other-thread.exp run against remote targets too Pedro Alves
@ 2014-02-26 13:36 ` Pedro Alves
2014-02-26 14:19 ` Joel Brobecker
6 siblings, 1 reply; 17+ messages in thread
From: Pedro Alves @ 2014-02-26 13:36 UTC (permalink / raw)
To: gdb-patches, Joel Brobecker
There's one more...
I already had this patch, but somehow missed that it would be
needed in this series, so left it out. But it turns out that
patch 4 exposes a latent Ada task-specific breakpoints bug,
resulting in a testsuite regression that I failed to notice
when posting this series.
So here's that patch, now with a test that fails against
current mainline, even without the rest of the series.
Is this OK?
------------
Subject: [PATCH] Multiple Ada task-specific breakpoints at the same address.
With the test changed as in the patch, against current mainline, we get:
(gdb) PASS: gdb.ada/tasks.exp: info tasks before inserting breakpoint
break break_me task 1
Breakpoint 2 at 0x4030b0: file /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.ada/tasks/foo.adb, line 27.
(gdb) PASS: gdb.ada/tasks.exp: break break_me task 1
break break_me task 3
Note: breakpoint 2 also set at pc 0x4030b0.
Breakpoint 3 at 0x4030b0: file /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.ada/tasks/foo.adb, line 27.
(gdb) PASS: gdb.ada/tasks.exp: break break_me task 3
continue
Continuing.
[Switching to Thread 0x7ffff7dc7700 (LWP 27133)]
Breakpoint 2, foo.break_me () at /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.ada/tasks/foo.adb:27
27 null;
(gdb) FAIL: gdb.ada/tasks.exp: continue to breakpoint
info tasks
ID TID P-ID Pri State Name
1 63b010 48 Waiting on RV with 3 main_task
2 63bd80 1 48 Accept or Select Term task_list(1)
* 3 63f510 1 48 Accepting RV with 1 task_list(2)
4 642ca0 1 48 Accept or Select Term task_list(3)
(gdb) PASS: gdb.ada/tasks.exp: info tasks after hitting breakpoint
The breakpoint that caused a stop is breakpoint 3, but GDB end up
reporting (and running breakpoint commands of) "Breakpoint 2" instead.
The issue is that the bpstat_check_breakpoint_conditions logic of
"wrong thread" is missing the "wrong task" check. This is usually
harmless, because the thread hop code in infrun.c code that handles
wrong-task-hitting-breakpoint does check for task-specific breakpoints
(within breakpoint_thread_match):
/* Check if a regular breakpoint has been hit before checking
for a potential single step breakpoint. Otherwise, GDB will
not see this breakpoint hit when stepping onto breakpoints. */
if (regular_breakpoint_inserted_here_p (aspace, stop_pc))
{
if (!breakpoint_thread_match (aspace, stop_pc, ecs->ptid))
thread_hop_needed = 1;
}
IOW, usually, when one only has a task specific breakpoint at a given
address, things work correctly. Put another task-specific or
non-task-specific breakpoint there, and things break.
A patch that eliminates the special thread hop code in infrun.c is
what exposed this, as after that GDB solely relies on
bpstat_check_breakpoint_conditions to know whether the right or wrong
task hit a breakpoint. IOW, given the latent bug, Ada task-specific
breakpoints become non-task-specific, and that is caught by the
testsuite, as:
break break_me task 3
Breakpoint 2 at 0x4030b0: file /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.ada/tasks/foo.adb, line 27.
(gdb) PASS: gdb.ada/tasks.exp: break break_me task 3
continue
Continuing.
[Switching to Thread 0x7ffff7fcb700 (LWP 17122)]
Breakpoint 2, foo.break_me () at /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.ada/tasks/foo.adb:27
27 null;
(gdb) PASS: gdb.ada/tasks.exp: continue to breakpoint
info tasks
ID TID P-ID Pri State Name
1 63b010 48 Waiting on RV with 2 main_task
* 2 63bd80 1 48 Accepting RV with 1 task_list(1)
3 63f510 1 48 Accept or Select Term task_list(2)
4 642ca0 1 48 Accept or Select Term task_list(3)
(gdb) FAIL: gdb.ada/tasks.exp: info tasks after hitting breakpoint
It was after seeing this that I thought of how to expose the bug with
current mainline.
Tested on x86_64 Fedora 17.
gdb/
2014-02-26 Pedro Alves <palves@redhat.com>
* breakpoint.c (bpstat_check_breakpoint_conditions): Handle
task-specific breakpoints.
gdb/testsuite/
2014-02-26 Pedro Alves <palves@redhat.com>
* gdb.ada/tasks.exp: Set a task-specific breakpoint at break_me
that won't ever trigger. Make sure that GDB reports the correct
breakpoint that caused the stop.
---
gdb/breakpoint.c | 10 ++++++----
gdb/testsuite/gdb.ada/tasks.exp | 28 ++++++++++++++++++++++++----
2 files changed, 30 insertions(+), 8 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index b462bad..fd33a3d 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5161,7 +5161,6 @@ bpstat_check_watchpoint (bpstat bs)
static void
bpstat_check_breakpoint_conditions (bpstat bs, ptid_t ptid)
{
- int thread_id = pid_to_thread_id (ptid);
const struct bp_location *bl;
struct breakpoint *b;
int value_is_zero = 0;
@@ -5186,9 +5185,12 @@ bpstat_check_breakpoint_conditions (bpstat bs, ptid_t ptid)
return;
}
- /* If this is a thread-specific breakpoint, don't waste cpu evaluating the
- condition if this isn't the specified thread. */
- if (b->thread != -1 && b->thread != thread_id)
+ /* If this is a thread/task-specific breakpoint, don't waste cpu
+ evaluating the condition if this isn't the specified
+ thread/task. */
+ if ((b->thread != -1 && b->thread != pid_to_thread_id (ptid))
+ || (b->task != 0 && b->task != ada_get_task_number (ptid)))
+
{
bs->stop = 0;
return;
diff --git a/gdb/testsuite/gdb.ada/tasks.exp b/gdb/testsuite/gdb.ada/tasks.exp
index 710deb0..9ffd000 100644
--- a/gdb/testsuite/gdb.ada/tasks.exp
+++ b/gdb/testsuite/gdb.ada/tasks.exp
@@ -37,15 +37,35 @@ gdb_test "info tasks" \
"\r\n"] \
"info tasks before inserting breakpoint"
-# Now, insert a breakpoint that should stop only if task 3 stops.
-gdb_test "break break_me task 3" "Breakpoint .* at .*"
+# Insert a breakpoint that should stop only if task 1 stops. Since
+# task 1 never calls break_me, this shouldn't actually ever trigger.
+# The fact this this breakpoint is created _before_ the next one
+# matter. GDB used to have a bug where it would report the first
+# breakpoint in the list that matched the triggered-breakpoint's
+# address, no matter which task it was specific to.
+gdb_test "break break_me task 1" "Breakpoint .* at .*"
+
+# Now, insert a breakpoint that should stop only if task 3 stops, and
+# extract its number.
+set bp_number -1
+set test "break break_me task 3"
+gdb_test_multiple $test $test {
+ -re "Breakpoint (.*) at .*$gdb_prompt $" {
+ set bp_number $expect_out(1,string)
+ pass $test
+ }
+}
+
+if {$bp_number < 0} {
+ return
+}
# Continue to that breakpoint. Task 2 should hit it first, and GDB
# is expected to ignore that hit and resume the execution. Only then
# task 3 will hit our breakpoint, and GDB is expected to stop at that
-# point.
+# point. Also make sure that GDB reports the correct breakpoint number.
gdb_test "continue" \
- ".*Breakpoint.*, foo.break_me \\(\\).*" \
+ ".*Breakpoint $bp_number, foo.break_me \\(\\).*" \
"continue to breakpoint"
# Check that it is indeed task 3 that hit the breakpoint by checking
--
1.7.11.7
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 3.5/6] Multiple Ada task-specific breakpoints at the same address.
2014-02-26 13:36 ` [PATCH 3.5/6] Multiple Ada task-specific breakpoints at the same address Pedro Alves
@ 2014-02-26 14:19 ` Joel Brobecker
2014-02-26 14:27 ` Pedro Alves
0 siblings, 1 reply; 17+ messages in thread
From: Joel Brobecker @ 2014-02-26 14:19 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
Hi Pedro,
> gdb/
> 2014-02-26 Pedro Alves <palves@redhat.com>
>
> * breakpoint.c (bpstat_check_breakpoint_conditions): Handle
> task-specific breakpoints.
Interesting bug.
> gdb/testsuite/
> 2014-02-26 Pedro Alves <palves@redhat.com>
>
> * gdb.ada/tasks.exp: Set a task-specific breakpoint at break_me
> that won't ever trigger. Make sure that GDB reports the correct
> breakpoint that caused the stop.
It all looks good to me. Just a few nits I happen to notice:
> +# Insert a breakpoint that should stop only if task 1 stops. Since
> +# task 1 never calls break_me, this shouldn't actually ever trigger.
> +# The fact this this breakpoint is created _before_ the next one
^^^^^^^^^
that this?
> +# matter. GDB used to have a bug where it would report the first
^^^^^^
matters
Thank you,
--
Joel
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 3.5/6] Multiple Ada task-specific breakpoints at the same address.
2014-02-26 14:19 ` Joel Brobecker
@ 2014-02-26 14:27 ` Pedro Alves
0 siblings, 0 replies; 17+ messages in thread
From: Pedro Alves @ 2014-02-26 14:27 UTC (permalink / raw)
To: Joel Brobecker; +Cc: gdb-patches
On 02/26/2014 02:19 PM, Joel Brobecker wrote:
> It all looks good to me. Just a few nits I happen to notice:
>
>> +# Insert a breakpoint that should stop only if task 1 stops. Since
>> +# task 1 never calls break_me, this shouldn't actually ever trigger.
>> +# The fact this this breakpoint is created _before_ the next one
> ^^^^^^^^^
> that this?
Indeed.
>
>> +# matter. GDB used to have a bug where it would report the first
> ^^^^^^
> matters
Righto.
Pushed with those changes. Thanks!
----------
Multiple Ada task-specific breakpoints at the same address.
With the test changed as in the patch, against current mainline, we get:
(gdb) PASS: gdb.ada/tasks.exp: info tasks before inserting breakpoint
break break_me task 1
Breakpoint 2 at 0x4030b0: file /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.ada/tasks/foo.adb, line 27.
(gdb) PASS: gdb.ada/tasks.exp: break break_me task 1
break break_me task 3
Note: breakpoint 2 also set at pc 0x4030b0.
Breakpoint 3 at 0x4030b0: file /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.ada/tasks/foo.adb, line 27.
(gdb) PASS: gdb.ada/tasks.exp: break break_me task 3
continue
Continuing.
[Switching to Thread 0x7ffff7dc7700 (LWP 27133)]
Breakpoint 2, foo.break_me () at /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.ada/tasks/foo.adb:27
27 null;
(gdb) FAIL: gdb.ada/tasks.exp: continue to breakpoint
info tasks
ID TID P-ID Pri State Name
1 63b010 48 Waiting on RV with 3 main_task
2 63bd80 1 48 Accept or Select Term task_list(1)
* 3 63f510 1 48 Accepting RV with 1 task_list(2)
4 642ca0 1 48 Accept or Select Term task_list(3)
(gdb) PASS: gdb.ada/tasks.exp: info tasks after hitting breakpoint
The breakpoint that caused a stop is breakpoint 3, but GDB end up
reporting (and running breakpoint commands of) "Breakpoint 2" instead.
The issue is that the bpstat_check_breakpoint_conditions logic of
"wrong thread" is missing the "wrong task" check. This is usually
harmless, because the thread hop code in infrun.c code that handles
wrong-task-hitting-breakpoint does check for task-specific breakpoints
(within breakpoint_thread_match):
/* Check if a regular breakpoint has been hit before checking
for a potential single step breakpoint. Otherwise, GDB will
not see this breakpoint hit when stepping onto breakpoints. */
if (regular_breakpoint_inserted_here_p (aspace, stop_pc))
{
if (!breakpoint_thread_match (aspace, stop_pc, ecs->ptid))
thread_hop_needed = 1;
}
IOW, usually, when one only has a task specific breakpoint at a given
address, things work correctly. Put another task-specific or
non-task-specific breakpoint there, and things break.
A patch that eliminates the special thread hop code in infrun.c is
what exposed this, as after that GDB solely relies on
bpstat_check_breakpoint_conditions to know whether the right or wrong
task hit a breakpoint. IOW, given the latent bug, Ada task-specific
breakpoints become non-task-specific, and that is caught by the
testsuite, as:
break break_me task 3
Breakpoint 2 at 0x4030b0: file /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.ada/tasks/foo.adb, line 27.
(gdb) PASS: gdb.ada/tasks.exp: break break_me task 3
continue
Continuing.
[Switching to Thread 0x7ffff7fcb700 (LWP 17122)]
Breakpoint 2, foo.break_me () at /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.ada/tasks/foo.adb:27
27 null;
(gdb) PASS: gdb.ada/tasks.exp: continue to breakpoint
info tasks
ID TID P-ID Pri State Name
1 63b010 48 Waiting on RV with 2 main_task
* 2 63bd80 1 48 Accepting RV with 1 task_list(1)
3 63f510 1 48 Accept or Select Term task_list(2)
4 642ca0 1 48 Accept or Select Term task_list(3)
(gdb) FAIL: gdb.ada/tasks.exp: info tasks after hitting breakpoint
It was after seeing this that I thought of how to expose the bug with
current mainline.
Tested on x86_64 Fedora 17.
gdb/
2014-02-26 Pedro Alves <palves@redhat.com>
* breakpoint.c (bpstat_check_breakpoint_conditions): Handle
task-specific breakpoints.
gdb/testsuite/
2014-02-26 Pedro Alves <palves@redhat.com>
* gdb.ada/tasks.exp: Set a task-specific breakpoint at break_me
that won't ever trigger. Make sure that GDB reports the correct
breakpoint that caused the stop.
---
gdb/ChangeLog | 5 +++++
gdb/breakpoint.c | 10 ++++++----
gdb/testsuite/ChangeLog | 6 ++++++
gdb/testsuite/gdb.ada/tasks.exp | 28 ++++++++++++++++++++++++----
4 files changed, 41 insertions(+), 8 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 16f4619..3a02a49 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2014-02-26 Pedro Alves <palves@redhat.com>
+
+ * breakpoint.c (bpstat_check_breakpoint_conditions): Handle
+ task-specific breakpoints.
+
2014-02-25 Pedro Alves <palves@redhat.com>
* ia64-linux-nat.c (ia64_linux_xfer_partial): Reimplement
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index ef81443..45c3417 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5159,7 +5159,6 @@ bpstat_check_watchpoint (bpstat bs)
static void
bpstat_check_breakpoint_conditions (bpstat bs, ptid_t ptid)
{
- int thread_id = pid_to_thread_id (ptid);
const struct bp_location *bl;
struct breakpoint *b;
int value_is_zero = 0;
@@ -5184,9 +5183,12 @@ bpstat_check_breakpoint_conditions (bpstat bs, ptid_t ptid)
return;
}
- /* If this is a thread-specific breakpoint, don't waste cpu evaluating the
- condition if this isn't the specified thread. */
- if (b->thread != -1 && b->thread != thread_id)
+ /* If this is a thread/task-specific breakpoint, don't waste cpu
+ evaluating the condition if this isn't the specified
+ thread/task. */
+ if ((b->thread != -1 && b->thread != pid_to_thread_id (ptid))
+ || (b->task != 0 && b->task != ada_get_task_number (ptid)))
+
{
bs->stop = 0;
return;
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index d54ed98..09cc8a3 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2014-02-26 Pedro Alves <palves@redhat.com>
+
+ * gdb.ada/tasks.exp: Set a task-specific breakpoint at break_me
+ that won't ever trigger. Make sure that GDB reports the correct
+ breakpoint that caused the stop.
+
2014-02-25 Jan Kratochvil <jan.kratochvil@redhat.com>
PR gdb/16626
diff --git a/gdb/testsuite/gdb.ada/tasks.exp b/gdb/testsuite/gdb.ada/tasks.exp
index 710deb0..088be6d 100644
--- a/gdb/testsuite/gdb.ada/tasks.exp
+++ b/gdb/testsuite/gdb.ada/tasks.exp
@@ -37,15 +37,35 @@ gdb_test "info tasks" \
"\r\n"] \
"info tasks before inserting breakpoint"
-# Now, insert a breakpoint that should stop only if task 3 stops.
-gdb_test "break break_me task 3" "Breakpoint .* at .*"
+# Insert a breakpoint that should stop only if task 1 stops. Since
+# task 1 never calls break_me, this shouldn't actually ever trigger.
+# The fact that this breakpoint is created _before_ the next one
+# matters. GDB used to have a bug where it would report the first
+# breakpoint in the list that matched the triggered-breakpoint's
+# address, no matter which task it was specific to.
+gdb_test "break break_me task 1" "Breakpoint .* at .*"
+
+# Now, insert a breakpoint that should stop only if task 3 stops, and
+# extract its number.
+set bp_number -1
+set test "break break_me task 3"
+gdb_test_multiple $test $test {
+ -re "Breakpoint (.*) at .*$gdb_prompt $" {
+ set bp_number $expect_out(1,string)
+ pass $test
+ }
+}
+
+if {$bp_number < 0} {
+ return
+}
# Continue to that breakpoint. Task 2 should hit it first, and GDB
# is expected to ignore that hit and resume the execution. Only then
# task 3 will hit our breakpoint, and GDB is expected to stop at that
-# point.
+# point. Also make sure that GDB reports the correct breakpoint number.
gdb_test "continue" \
- ".*Breakpoint.*, foo.break_me \\(\\).*" \
+ ".*Breakpoint $bp_number, foo.break_me \\(\\).*" \
"continue to breakpoint"
# Check that it is indeed task 3 that hit the breakpoint by checking
--
1.7.11.7
^ permalink raw reply [flat|nested] 17+ messages in thread