[1/2] gdb: Check the selected-frame in frame_find_by_id.
Commit Message
In gdb we track two different things, the current-frame, and the
selected-frame. The current-frame represents the frame in which we are
currently stopped. If gdb stops at a breakpoint and the user asks for a
backtrace then all the frames in the backtrace will be the ancestors of
the the current-frame.
Also the current-frame does not change until the inferior is set running
again (though it is possible for the user to alter the contents of the
current-frame this is not the same as changing the current-frame).
There is also the selected-frame. This is the frame that the user has
"selected" using the frame selection commands 'frame', 'up', or 'down'
being the most common.
Usually the selected-frame is always an ancestor of the current-frame,
however, there is a feature of the 'frame' command that allows arbitrary
frames to be created, in this case the selected-frame might not be an
ancestor of the current-frame.
The problem this causes is that lazy register values hold the frame-id
for the frame in which the real register value can be obtained. When
gdb needs to fetch the actual register value we try to find the frame
using frame_find_by_id, which currently looks at the current-frame, and
all the ancestors of the current-frame.
When the selected-frame is not an ancestor of the current-frame then
frame_find_by_id will fail to find the required frame, and gdb will fail
with an assertion while trying to fetch the lazy value (the assertion is
that the frame will always be found).
This patch extends frame_find_by_id to also check the selected-frame as
a separate comparison as well as the current-frame and its ancestors.
This resolves the issue.
There are also two new tests added that show this issue. The first is
an mi test using var objects, which is where I first saw this issue.
The second is a slightly simpler test, just creating a new frame then
calling 'info frame' is enough to trigger the assertion.
gdb/ChangeLog:
* frame.c (frame_find_by_id): Also check the selected frame.
gdb/testsuite/ChangeLog:
* gdb.mi/mi-var-frame.c: New file.
* gdb.mi/mi-var-frame.exp: New file.
* gdb.base/create-frame.c: New file.
* gdb.base/create-frame.exp: New file.
---
gdb/ChangeLog | 4 ++
gdb/frame.c | 18 +++++++--
gdb/testsuite/ChangeLog | 7 ++++
gdb/testsuite/gdb.base/create-frame.c | 34 ++++++++++++++++
gdb/testsuite/gdb.base/create-frame.exp | 30 ++++++++++++++
gdb/testsuite/gdb.mi/mi-var-frame.c | 34 ++++++++++++++++
gdb/testsuite/gdb.mi/mi-var-frame.exp | 70 +++++++++++++++++++++++++++++++++
7 files changed, 194 insertions(+), 3 deletions(-)
create mode 100644 gdb/testsuite/gdb.base/create-frame.c
create mode 100644 gdb/testsuite/gdb.base/create-frame.exp
create mode 100644 gdb/testsuite/gdb.mi/mi-var-frame.c
create mode 100644 gdb/testsuite/gdb.mi/mi-var-frame.exp
Comments
Seems that I originally failed to post patch 2/2 from the original
series. So here's both of them again, rebased on latest master.
I believe that these patches should still be applied even in light of
the discussion here:
https://sourceware.org/ml/gdb-patches/2015-07/msg00241.html
as any solution that does come out of that conversation will probably
be limited to changing the user interface of gdb, not the underlying
behaviour. Also that conversation is about future clean up, while
this patch series addresses real, user visible, assertion failures.
--
Andrew Burgess (2):
gdb: Check the selected-frame in frame_find_by_id.
gdb: Select a frame for frame_info.
gdb/ChangeLog | 13 ++++++
gdb/frame.c | 39 ++++++++++++++++--
gdb/frame.h | 5 +++
gdb/stack.c | 10 +++++
gdb/testsuite/ChangeLog | 12 ++++++
gdb/testsuite/gdb.base/create-frame.c | 34 ++++++++++++++++
gdb/testsuite/gdb.base/create-frame.exp | 36 +++++++++++++++++
gdb/testsuite/gdb.mi/mi-var-frame.c | 34 ++++++++++++++++
gdb/testsuite/gdb.mi/mi-var-frame.exp | 70 +++++++++++++++++++++++++++++++++
9 files changed, 250 insertions(+), 3 deletions(-)
create mode 100644 gdb/testsuite/gdb.base/create-frame.c
create mode 100644 gdb/testsuite/gdb.base/create-frame.exp
create mode 100644 gdb/testsuite/gdb.mi/mi-var-frame.c
create mode 100644 gdb/testsuite/gdb.mi/mi-var-frame.exp
Ping!
Thanks,
Andrew
* Andrew Burgess <andrew.burgess@embecosm.com> [2015-07-23 12:07:32 +0100]:
> Seems that I originally failed to post patch 2/2 from the original
> series. So here's both of them again, rebased on latest master.
>
> I believe that these patches should still be applied even in light of
> the discussion here:
> https://sourceware.org/ml/gdb-patches/2015-07/msg00241.html
> as any solution that does come out of that conversation will probably
> be limited to changing the user interface of gdb, not the underlying
> behaviour. Also that conversation is about future clean up, while
> this patch series addresses real, user visible, assertion failures.
>
> --
>
> Andrew Burgess (2):
> gdb: Check the selected-frame in frame_find_by_id.
> gdb: Select a frame for frame_info.
>
> gdb/ChangeLog | 13 ++++++
> gdb/frame.c | 39 ++++++++++++++++--
> gdb/frame.h | 5 +++
> gdb/stack.c | 10 +++++
> gdb/testsuite/ChangeLog | 12 ++++++
> gdb/testsuite/gdb.base/create-frame.c | 34 ++++++++++++++++
> gdb/testsuite/gdb.base/create-frame.exp | 36 +++++++++++++++++
> gdb/testsuite/gdb.mi/mi-var-frame.c | 34 ++++++++++++++++
> gdb/testsuite/gdb.mi/mi-var-frame.exp | 70 +++++++++++++++++++++++++++++++++
> 9 files changed, 250 insertions(+), 3 deletions(-)
> create mode 100644 gdb/testsuite/gdb.base/create-frame.c
> create mode 100644 gdb/testsuite/gdb.base/create-frame.exp
> create mode 100644 gdb/testsuite/gdb.mi/mi-var-frame.c
> create mode 100644 gdb/testsuite/gdb.mi/mi-var-frame.exp
>
> --
> 2.4.0
>
I've now merged these patches into a large patch series here:
https://sourceware.org/ml/gdb-patches/2015-09/msg00248.html
Thanks,
Andrew
* Andrew Burgess <andrew.burgess@embecosm.com> [2015-08-07 13:56:46 +0200]:
> Ping!
>
> Thanks,
> Andrew
>
>
> * Andrew Burgess <andrew.burgess@embecosm.com> [2015-07-23 12:07:32 +0100]:
>
> > Seems that I originally failed to post patch 2/2 from the original
> > series. So here's both of them again, rebased on latest master.
> >
> > I believe that these patches should still be applied even in light of
> > the discussion here:
> > https://sourceware.org/ml/gdb-patches/2015-07/msg00241.html
> > as any solution that does come out of that conversation will probably
> > be limited to changing the user interface of gdb, not the underlying
> > behaviour. Also that conversation is about future clean up, while
> > this patch series addresses real, user visible, assertion failures.
> >
> > --
> >
> > Andrew Burgess (2):
> > gdb: Check the selected-frame in frame_find_by_id.
> > gdb: Select a frame for frame_info.
> >
> > gdb/ChangeLog | 13 ++++++
> > gdb/frame.c | 39 ++++++++++++++++--
> > gdb/frame.h | 5 +++
> > gdb/stack.c | 10 +++++
> > gdb/testsuite/ChangeLog | 12 ++++++
> > gdb/testsuite/gdb.base/create-frame.c | 34 ++++++++++++++++
> > gdb/testsuite/gdb.base/create-frame.exp | 36 +++++++++++++++++
> > gdb/testsuite/gdb.mi/mi-var-frame.c | 34 ++++++++++++++++
> > gdb/testsuite/gdb.mi/mi-var-frame.exp | 70 +++++++++++++++++++++++++++++++++
> > 9 files changed, 250 insertions(+), 3 deletions(-)
> > create mode 100644 gdb/testsuite/gdb.base/create-frame.c
> > create mode 100644 gdb/testsuite/gdb.base/create-frame.exp
> > create mode 100644 gdb/testsuite/gdb.mi/mi-var-frame.c
> > create mode 100644 gdb/testsuite/gdb.mi/mi-var-frame.exp
> >
> > --
> > 2.4.0
> >
@@ -1,3 +1,7 @@
+2015-07-08 Andrew Burgess <andrew.burgess@embecosm.com>
+
+ * frame.c (frame_find_by_id): Also check the selected frame.
+
2015-07-08 Jan Kratochvil <jan.kratochvil@redhat.com>
PR compile/18484
@@ -732,7 +732,7 @@ frame_id_inner (struct gdbarch *gdbarch, struct frame_id l, struct frame_id r)
struct frame_info *
frame_find_by_id (struct frame_id id)
{
- struct frame_info *frame, *prev_frame;
+ struct frame_info *frame, *prev_frame, *selected_frame;
/* ZERO denotes the null frame, let the caller decide what to do
about it. Should it instead return get_current_frame()? */
@@ -761,7 +761,7 @@ frame_find_by_id (struct frame_id id)
prev_frame = get_prev_frame (frame);
if (!prev_frame)
- return NULL;
+ break;
/* As a safety net to avoid unnecessary backtracing while trying
to find an invalid ID, we check for a common situation where
@@ -772,8 +772,20 @@ frame_find_by_id (struct frame_id id)
&& !frame_id_inner (get_frame_arch (frame), id, self)
&& frame_id_inner (get_frame_arch (prev_frame), id,
get_frame_id (prev_frame)))
- return NULL;
+ break;
+ }
+
+ /* We check the selected frame specifically here to handle the case where
+ the selected frame is not an ancestor of the current frame. */
+ selected_frame = get_selected_frame_if_set ();
+ if (selected_frame)
+ {
+ struct frame_id selected_frame_id = get_frame_id (selected_frame);
+
+ if (frame_id_eq (id, selected_frame_id))
+ return frame;
}
+
return NULL;
}
@@ -1,3 +1,10 @@
+2015-07-08 Andrew Burgess <andrew.burgess@embecosm.com>
+
+ * gdb.mi/mi-var-frame.c: New file.
+ * gdb.mi/mi-var-frame.exp: New file.
+ * gdb.base/create-frame.c: New file.
+ * gdb.base/create-frame.exp: New file.
+
2015-07-08 Jan Kratochvil <jan.kratochvil@redhat.com>
PR compile/18484
new file mode 100644
@@ -0,0 +1,34 @@
+/* Copyright 2015 Free Software Foundation, Inc.
+
+ This file is part of GDB.
+
+ 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/>. */
+
+int
+frame_2 (void)
+{
+ return 0;
+}
+
+int
+frame_1 (void)
+{
+ return frame_2 ();
+}
+
+int
+main (void)
+{
+ return frame_1 ();
+}
new file mode 100644
@@ -0,0 +1,30 @@
+# Copyright 2015 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/>.
+
+if { [prepare_for_testing create-frame.exp "create-frame" {create-frame.c} {debug nowarnings}] } {
+ return -1
+}
+set srcfile create-frame.c
+
+runto_main
+gdb_breakpoint frame_2
+gdb_continue_to_breakpoint frame_2
+
+gdb_test "bt" "#0.*#1.*#2.*" "backtrace at breakpoint"
+
+gdb_test_no_output "select-frame 3"
+
+gdb_test "info frame" "Stack level 0, frame at 0x3:.*" \
+ "info frame for created frame"
new file mode 100644
@@ -0,0 +1,34 @@
+/* Copyright 2015 Free Software Foundation, Inc.
+
+ This file is part of GDB.
+
+ 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/>. */
+
+int
+frame_2 (void)
+{
+ return 0;
+}
+
+int
+frame_1 (void)
+{
+ return frame_2 ();
+}
+
+int
+main (void)
+{
+ return frame_1 ();
+}
new file mode 100644
@@ -0,0 +1,70 @@
+# Copyright 2015 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 tests an issue relating to updating var objects in user created
+# frames.
+
+# This test requires a register name for creating a var object.
+# Currently only x86/x86_64 like targets are supported.
+if {![istarget "x86_64-*-*"] && ![istarget "i?86-*-*"]} {
+ return 0
+}
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi"
+
+gdb_exit
+if [mi_gdb_start] {
+ continue
+}
+
+standard_testfile mi-var-frame.c
+
+if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+ untested mi-var-frame.exp
+ return -1
+}
+
+mi_delete_breakpoints
+mi_gdb_reinitialize_dir $srcdir/$subdir
+mi_gdb_load ${binfile}
+
+mi_create_breakpoint "frame_2" \
+ "break-insert into frame_2" \
+ -number 1 -func frame_2 -file ".*${srcfile}"
+
+mi_run_cmd
+mi_expect_stop "breakpoint-hit" "frame_2" "" ".*${srcfile}" \
+ ".*" { "" "disp=\"keep\"" } "run to breakpoint"
+
+mi_gdb_test "-stack-info-depth" \
+ "\\^done,depth=\"3\"" \
+ "stack info-depth"
+
+set register "\$eax"
+
+mi_gdb_test "-var-create R * $register" \
+ "\\^done,name=\"R\",numchild=\"0\",value=\".*\",type=\".*\",has_more=\"0\"" \
+ "create varobj 'R' for register '$register'"
+
+for {set i 0} {$i < 5} {incr i} {
+ mi_gdb_test "-stack-select-frame $i" \
+ {\^done} \
+ "-stack-select-frame $i"
+
+ mi_gdb_test "-var-update R" \
+ "\\^done,changelist=\\\[\\\]" \
+ "update 'R' in frame $i: no change"
+}