[PATCH 2/3] Fix record-full with "set breakpoint always-inserted on"
Pedro Alves
pedro@palves.net
Fri Nov 27 22:19:57 GMT 2020
Running the gdb.reverse/ tests with the native-gdbserver board, with
"set breakpoint always-inserted" forced "on", like this:
$ make check-parallel \
RUNTESTFLAGS="--target_board=native-gdbserver GDBFLAGS='-ex set\ breakpoint\ always-inserted\ on'" \
TESTS="gdb.reverse/*.exp"
shows a number of regressions, compared to plain native-gdbserver with
always-inserted left to default off:
-# of expected passes 2729
-# of unexpected failures 14
+# of expected passes 2580
+# of unexpected failures 157
The failures all look like this:
(gdb) PASS: gdb.reverse/finish-reverse-bkpt.exp: set breakpoint at void_func's entry
reverse-finish
Run back to call of #0 void_func () at /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.reverse/finish-reverse.c:45
/home/pedro/gdb/binutils-gdb/src/gdb/record-full.c:1789: internal-error: virtual int record_full_target::insert_breakpoint(gdbarch*, bp_target_info*): Assertion `bp.in_targ
et_beneath == in_target_beneath' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n) FAIL: gdb.reverse/finish-reverse-bkpt.exp: reverse-finish from void_func trips breakpoint at entry (GDB internal error)
Resyncing due to internal error.
n
(top-gdb) bt
#0 internal_error (file=0x555555f55668 "src/gdb/record-full.c", line=1789, fmt=0x555555f554c9 "%s: Assertion `%s' failed.") at src/gdbsupport/errors.cc:54
#1 0x0000555555aebda6 in record_full_target::insert_breakpoint (this=0x5555563260e0 <record_full_ops>, gdbarch=0x555556c3f200, bp_tgt=0x555557142f08) at src/gdb/record-full.c:1789
#2 0x0000555555be6730 in target_insert_breakpoint (gdbarch=0x555556c3f200, bp_tgt=0x555557142f08) at src/gdb/target.c:1803
#3 0x00005555556ccb31 in bkpt_insert_location (bl=0x555557142e70) at src/gdb/breakpoint.c:12615
#4 0x00005555556b3d63 in insert_bp_location (bl=0x555557142e70, tmp_error_stream=0x7fffffffd540, disabled_breaks=0x7fffffffd4cc, hw_breakpoint_error=0x7fffffffd4d0, hw_bp_error_explained_already=0x7fffffffd4d4) at src/gdb/breakpoint.c:2634
#5 0x00005555556b4b4b in update_inserted_breakpoint_locations () at src/gdb/breakpoint.c:3003
#6 0x00005555556cbeb4 in update_global_location_list (insert_mode=UGLL_DONT_INSERT) at src/gdb/breakpoint.c:12282
#7 0x00005555556ce962 in delete_breakpoint (bpt=0x555557138e50) at src/gdb/breakpoint.c:13395
#8 0x0000555555c01a77 in delete_thread_breakpoint (bp_p=0x55555640e278) at src/gdb/thread.c:106
#9 0x0000555555c01aac in delete_step_resume_breakpoint (tp=0x55555640e230) at src/gdb/thread.c:115
#10 0x000055555596e0db in delete_thread_infrun_breakpoints (tp=0x55555640e230) at src/gdb/infrun.c:3270
#11 0x000055555596e22c in for_each_just_stopped_thread (func=0x55555596e0bf <delete_thread_infrun_breakpoints(thread_info*)>) at src/gdb/infrun.c:3297
#12 0x000055555596e268 in delete_just_stopped_threads_infrun_breakpoints () at src/gdb/infrun.c:3307
#13 0x00005555559701d3 in fetch_inferior_event () at src/gdb/infrun.c:3920
#14 0x00005555559522e8 in inferior_event_handler (event_type=INF_REG_EVENT) at src/gdb/inf-loop.c:42
#15 0x000055555597ca95 in infrun_async_inferior_event_handler (data=0x0) at src/gdb/infrun.c:9161
#16 0x000055555568803b in check_async_event_handlers () at src/gdb/async-event.c:328
#17 0x0000555555e43b84 in gdb_do_one_event () at src/gdbsupport/event-loop.cc:216
#18 0x00005555559e3560 in start_event_loop () at src/gdb/main.c:347
#19 0x00005555559e36ab in captured_command_loop () at src/gdb/main.c:407
#20 0x00005555559e4f6e in captured_main (data=0x7fffffffdbf0) at src/gdb/main.c:1234
#21 0x00005555559e4fe0 in gdb_main (args=0x7fffffffdbf0) at src/gdb/main.c:1249
#22 0x0000555555627c66 in main (argc=4, argv=0x7fffffffdd08) at src/gdb/gdb.c:32
(top-gdb)
In frame #1, we have:
(top-gdb) p bp.in_target_beneath
$1 = true
(top-gdb) p in_target_beneath
$2 = false
The problem is that:
#1 - GDB inserts a breakpoint when recording / not replaying. That
installs a breakpoint in the real process, and records a copy in
the record_full_breakpoints list, with bp.in_target_beneath true.
#2 - Test does reverse-finish. That sets an internal breakpoint and
due to the breakpoint condition evaluation on the target side,
GDB ends up reinserting previous breakpoints to update conditions
on the target side.
#3 - Now record_full_target::insert_breakpoint is reached, but this
time we're replaying backwards, so we go straight to checking for
a duplicate in the record_full_breakpoints list. A duplicate is
found, which had been inserted in #1. Since we're now replaying,
we skipped the insert on the beneath target this time around, so
we hit the assertion.
A new testcase -- gdb.reverse/break-always-inserted.exp -- is added
which exercises this issue.
The fix is to stop trying to avoid inserting breakpoints in the live
process while recording. Just always pass the insert/remove request
down directly. Back in:
https://sourceware.org/pipermail/gdb-patches/2012-January/088894.html
which was the patch that added the record breakpoint tracking, I
mentioned that the current solution wasn't sufficient for
always-inserted. I think it's time to go with the simpler route, it's
just not worth it to try to be smarter. This is also what
record-btrace does, btw.
Another related issue the testcase exposes triggers even on native
debugging, without gdbserver. That's the use case tested by the
test_reverse_forward procedure in the new test. That test sets a
breakpoint while recording and while exec-direction is set to reverse.
That reaches record_full_target::insert_breakpoint with
RECORD_FULL_IS_REPLAY true and so doesn't propagate the breakpoint to
the target beneath. Then we switch to exec-direction forward, stop
recording and continue. The breakpoint was never installed in the
real process, so the process runs to exit instead of hitting the
breakpoint. The same fix fixes this issue too.
Without the fix we get, with native debugging:
continue
Continuing.
[Inferior 1 (process 40018) exited normally]
(gdb) FAIL: gdb.reverse/break-always-inserted.exp: always-inserted=on: cond-eval=host: test_reverse_forward: continue to breakpoint: foo (the program exited)
And with remote debugging:
FAIL: gdb.reverse/break-always-inserted.exp: always-inserted=on: cond-eval=host: test_reverse_forward: continue to breakpoint: foo (the program exited)
FAIL: gdb.reverse/break-always-inserted.exp: always-inserted=on: cond-eval=target: test_reverse_forward: continue to breakpoint: foo (the program exited)
FAIL: gdb.reverse/break-always-inserted.exp: always-inserted=on: cond-eval=target: test_forward_reverse: setting breakpoint at bar if 1 (GDB internal error)
We get all passes with the fix.
gdb/ChangeLog:
* breakpoint.c (iterate_over_bp_locations): Delete.
* breakpoint.h (iterate_over_bp_locations): Delete.
* record-full.c (record_full_open): No longer call
record_full_init_record_breakpoints.
(struct record_full_breakpoint, record_full_breakpoints)
(record_full_sync_record_breakpoints)
(record_full_init_record_breakpoints): Delete.
(record_full_target::insert_breakpoint)
(record_full_target::remove_breakpoint): No longer keep track of
inserted breakpoints; always pass down the request to the target
beneath.
gdb/testsuite/ChangeLog:
* gdb.reverse/break-always-inserted.exp: New.
---
gdb/breakpoint.c | 13 --
gdb/breakpoint.h | 2 -
gdb/record-full.c | 129 +------------------
.../gdb.reverse/break-always-inserted.exp | 141 +++++++++++++++++++++
4 files changed, 147 insertions(+), 138 deletions(-)
create mode 100644 gdb/testsuite/gdb.reverse/break-always-inserted.exp
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 61fbc3b92b1..e39ca288c93 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2942,19 +2942,6 @@ insert_breakpoints (void)
update_global_location_list (UGLL_INSERT);
}
-/* Invoke CALLBACK for each of bp_location. */
-
-void
-iterate_over_bp_locations (walk_bp_location_callback callback)
-{
- struct bp_location *loc, **loc_tmp;
-
- ALL_BP_LOCATIONS (loc, loc_tmp)
- {
- callback (loc, NULL);
- }
-}
-
/* This is used when we need to synch breakpoint conditions between GDB and the
target. It is the case with deleting and disabling of breakpoints when using
always-inserted mode. */
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 4a65dd2dd43..7f278aa845c 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1288,8 +1288,6 @@ extern void breakpoint_auto_delete (bpstat);
typedef void (*walk_bp_location_callback) (struct bp_location *, void *);
-extern void iterate_over_bp_locations (walk_bp_location_callback);
-
/* Return the chain of command lines to execute when this breakpoint
is hit. */
extern struct command_line *breakpoint_commands (struct breakpoint *b);
diff --git a/gdb/record-full.c b/gdb/record-full.c
index 3b5e6fee7cd..6836cc8a73c 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -951,8 +951,6 @@ record_full_open_1 (const char *name, int from_tty)
push_target (&record_full_ops);
}
-static void record_full_init_record_breakpoints (void);
-
/* Open the process record target. */
static void
@@ -979,8 +977,6 @@ record_full_open (const char *name, int from_tty)
= create_async_event_handler (record_full_async_inferior_event_handler,
NULL, "record-full");
- record_full_init_record_breakpoints ();
-
gdb::observers::record_changed.notify (current_inferior (), 1, "full", NULL);
}
@@ -1693,63 +1689,6 @@ record_full_target::xfer_partial (enum target_object object,
offset, len, xfered_len);
}
-/* This structure represents a breakpoint inserted while the record
- target is active. We use this to know when to install/remove
- breakpoints in/from the target beneath. For example, a breakpoint
- may be inserted while recording, but removed when not replaying nor
- recording. In that case, the breakpoint had not been inserted on
- the target beneath, so we should not try to remove it there. */
-
-struct record_full_breakpoint
-{
- record_full_breakpoint (struct address_space *address_space_,
- CORE_ADDR addr_,
- bool in_target_beneath_)
- : address_space (address_space_),
- addr (addr_),
- in_target_beneath (in_target_beneath_)
- {
- }
-
- /* The address and address space the breakpoint was set at. */
- struct address_space *address_space;
- CORE_ADDR addr;
-
- /* True when the breakpoint has been also installed in the target
- beneath. This will be false for breakpoints set during replay or
- when recording. */
- bool in_target_beneath;
-};
-
-/* The list of breakpoints inserted while the record target is
- active. */
-static std::vector<record_full_breakpoint> record_full_breakpoints;
-
-static void
-record_full_sync_record_breakpoints (struct bp_location *loc, void *data)
-{
- if (loc->loc_type != bp_loc_software_breakpoint)
- return;
-
- if (loc->inserted)
- {
- record_full_breakpoints.emplace_back
- (loc->target_info.placed_address_space,
- loc->target_info.placed_address,
- 1);
- }
-}
-
-/* Sync existing breakpoints to record_full_breakpoints. */
-
-static void
-record_full_init_record_breakpoints (void)
-{
- record_full_breakpoints.clear ();
-
- iterate_over_bp_locations (record_full_sync_record_breakpoints);
-}
-
/* Behavior is conditional on RECORD_FULL_IS_REPLAY. We will not actually
insert or remove breakpoints in the real target when replaying, nor
when recording. */
@@ -1758,43 +1697,10 @@ int
record_full_target::insert_breakpoint (struct gdbarch *gdbarch,
struct bp_target_info *bp_tgt)
{
- bool in_target_beneath = false;
-
- if (!RECORD_FULL_IS_REPLAY)
- {
- /* When recording, we currently always single-step, so we don't
- really need to install regular breakpoints in the inferior.
- However, we do have to insert software single-step
- breakpoints, in case the target can't hardware step. To keep
- things simple, we always insert. */
-
- scoped_restore restore_operation_disable
- = record_full_gdb_operation_disable_set ();
-
- int ret = this->beneath ()->insert_breakpoint (gdbarch, bp_tgt);
- if (ret != 0)
- return ret;
-
- in_target_beneath = true;
- }
-
- /* Use the existing entries if found in order to avoid duplication
- in record_full_breakpoints. */
-
- for (const record_full_breakpoint &bp : record_full_breakpoints)
- {
- if (bp.addr == bp_tgt->placed_address
- && bp.address_space == bp_tgt->placed_address_space)
- {
- gdb_assert (bp.in_target_beneath == in_target_beneath);
- return 0;
- }
- }
+ scoped_restore restore_operation_disable
+ = record_full_gdb_operation_disable_set ();
- record_full_breakpoints.emplace_back (bp_tgt->placed_address_space,
- bp_tgt->placed_address,
- in_target_beneath);
- return 0;
+ return this->beneath ()->insert_breakpoint (gdbarch, bp_tgt);
}
/* "remove_breakpoint" method for process record target. */
@@ -1804,33 +1710,10 @@ record_full_target::remove_breakpoint (struct gdbarch *gdbarch,
struct bp_target_info *bp_tgt,
enum remove_bp_reason reason)
{
- for (auto iter = record_full_breakpoints.begin ();
- iter != record_full_breakpoints.end ();
- ++iter)
- {
- struct record_full_breakpoint &bp = *iter;
-
- if (bp.addr == bp_tgt->placed_address
- && bp.address_space == bp_tgt->placed_address_space)
- {
- if (bp.in_target_beneath)
- {
- scoped_restore restore_operation_disable
- = record_full_gdb_operation_disable_set ();
-
- int ret = this->beneath ()->remove_breakpoint (gdbarch, bp_tgt,
- reason);
- if (ret != 0)
- return ret;
- }
-
- if (reason == REMOVE_BREAKPOINT)
- unordered_remove (record_full_breakpoints, iter);
- return 0;
- }
- }
+ scoped_restore restore_operation_disable
+ = record_full_gdb_operation_disable_set ();
- gdb_assert_not_reached ("removing unknown breakpoint");
+ return this->beneath ()->remove_breakpoint (gdbarch, bp_tgt, reason);
}
/* "can_execute_reverse" method for process record target. */
diff --git a/gdb/testsuite/gdb.reverse/break-always-inserted.exp b/gdb/testsuite/gdb.reverse/break-always-inserted.exp
new file mode 100644
index 00000000000..9e99c6efca2
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/break-always-inserted.exp
@@ -0,0 +1,141 @@
+# Copyright 2020 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/>.
+
+# This file is part of the GDB testsuite.
+
+# Test that:
+#
+# - setting a breakpoint when exec-direction is reverse isn't lost
+# when exec-direction is set back to forward, and recording is
+# stopped. That used to happen with "set breakpoint always-inserted
+# on".
+#
+# - setting a breakpoint when exec-direction is forward and then
+# re-inserting the breakpoint with exec-direction backward. This
+# used to internal error with with "set breakpoint always-inserted
+# on" and "set breakpoint condition-evalution target".
+
+if ![supports_reverse] {
+ return
+}
+
+standard_testfile break-reverse.c
+
+if { [build_executable "failed to prepare" $testfile $srcfile] } {
+ return -1
+}
+
+set foo_location [gdb_get_line_number "break in foo" ]
+set bar_location [gdb_get_line_number "break in bar" ]
+
+proc prepare {always_inserted cond_eval} {
+ global binfile gdb_prompt
+
+ clean_restart $binfile
+
+ gdb_test_no_output "set breakpoint always-inserted $always_inserted"
+
+ if {![runto_main]} {
+ fail "could not run to main"
+ return false
+ }
+
+ # Must do this after running to main, so that we're sure we're
+ # already connected to the remote target at this point, and GDB
+ # knows whether target-side evaluation is possible.
+ set test "set breakpoint condition-evaluation $cond_eval"
+ gdb_test_multiple $test "" {
+ -re "warning: Target does not support breakpoint condition evaluation.\r\nUsing host evaluation mode instead.\r\n$gdb_prompt $" {
+ # Target doesn't support breakpoint condition
+ # evaluation on its side. Skip the test.
+ pass $test
+ return false
+ }
+ -re "^$test\r\n$gdb_prompt $" {
+ pass $test
+ }
+ }
+
+ return true
+}
+
+# Test setting a breakpoint while replaying and then running to it
+# when not replaying nor recording.
+proc_with_prefix test_reverse_forward {always_inserted cond_eval} {
+ global srcfile decimal foo_location
+
+ if {![prepare $always_inserted $cond_eval]} {
+ return
+ }
+
+ if [supports_process_record] {
+ # Activate process record/replay
+ gdb_test_no_output "record" "turn on process record"
+ }
+
+ gdb_test_no_output "set exec-direction reverse"
+
+ # Insert a breakpoint with reverse exec-direction, replaying.
+ gdb_breakpoint "foo"
+
+ # Flip direction to forward, stop recording, and run to the
+ # breakpoint. This used to miss inserting the breakpoint in the
+ # live process.
+ gdb_test_no_output "set exec-direction forward"
+
+ if [supports_process_record] {
+ gdb_test "record stop"
+ }
+
+ gdb_continue_to_breakpoint "foo" ".*$srcfile:$foo_location.*"
+}
+
+# Test setting a breakpoint while recording forward and then
+# re-installing it when replaying.
+proc_with_prefix test_forward_reverse {always_inserted cond_eval} {
+ global srcfile decimal foo_location bar_location
+
+ if {![prepare $always_inserted $cond_eval]} {
+ return
+ }
+
+ if [supports_process_record] {
+ # Activate process record/replay
+ gdb_test_no_output "record" "turn on process record"
+ }
+
+ # Insert a breakpoint with forward exec-direction, recording.
+ gdb_breakpoint "bar"
+ gdb_continue_to_breakpoint "bar" ".*$srcfile:$bar_location.*"
+
+ # Flip direction to reverse, so that we're replaying.
+ gdb_test_no_output "set exec-direction reverse"
+
+ # Force a breakpoint reinsert while replaying. This used to
+ # internal-error with breakpoint always-inserted on.
+ gdb_breakpoint "bar if 1"
+
+ # Run backwards, to force breakpoint insertion with
+ # always-inserted off.
+ gdb_breakpoint "foo"
+ gdb_continue_to_breakpoint "foo" ".*$srcfile:$foo_location.*"
+}
+
+foreach_with_prefix always-inserted {"off" "on"} {
+ foreach_with_prefix cond-eval {"host" "target"} {
+ test_reverse_forward ${always-inserted} ${cond-eval}
+ test_forward_reverse ${always-inserted} ${cond-eval}
+ }
+}
--
2.14.5
More information about the Gdb-patches
mailing list