diff mbox

[1/9] gdb: Check the selected-frame in frame_find_by_id.

Message ID 3544270eb01ddce088cccf4a2f708af4ec4dbc77.1441996064.git.andrew.burgess@embecosm.com
State New
Headers show

Commit Message

Andrew Burgess Sept. 11, 2015, 6:49 p.m. UTC
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/frame-cmds.c: New file.
	* gdb.base/frame-cmds.exp: New file.
---
 gdb/ChangeLog                         |  4 ++
 gdb/frame.c                           | 18 +++++++--
 gdb/testsuite/ChangeLog               |  7 ++++
 gdb/testsuite/gdb.base/frame-cmds.c   | 34 +++++++++++++++++
 gdb/testsuite/gdb.base/frame-cmds.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/frame-cmds.c
 create mode 100644 gdb/testsuite/gdb.base/frame-cmds.exp
 create mode 100644 gdb/testsuite/gdb.mi/mi-var-frame.c
 create mode 100644 gdb/testsuite/gdb.mi/mi-var-frame.exp

Comments

Pedro Alves Sept. 30, 2015, 1:39 p.m. UTC | #1
Hi Andrew,

On 09/11/2015 07:49 PM, Andrew Burgess wrote:
> 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.

Seems to me that this issue runs deeper.

Two points:

#1 - Consider what happens if something flushes the frame cache while
the user is looking at a manually-created frame.  Anything that
calls registers_changed() and relies on the old selected frame _id_
to re-unwind, re-populate the frame cache, and re-find the old selected
frame object (e.g., see make_cleanup_restore_current_thread).
As is, the selected frame's frame_info object is gone along with the frame cache
flush, so gdb will just unexpectedly lose the selected frame when it tries
to re-find it.  E.g., something around:

thread2 ()
{
  while (1)
    sleep (1);
}

main ()
{
  create thread (thread2);
  join thread2;
}


(gdb) set non-stop on
(gdb) break sleep if 0
(gdb) c -a&
(gdb) thread 1
(gdb) interrupt
(gdb) frame create 0xaddr
// now the breakpoint at sleep triggers
// while we're looking at the created frame.
// gdb switches to thread 2 internally to handle
// the event, re-resumes the thread (bp condition evals false),
// and then tries to restore the previously selected frame, which
// is gone.  so gdb falls back to re-selecting the current frame...


Or simpler, from reading the code, it seems like just "info threads"
should trigger that too.

#2 - As mentioned in my comment here:

 https://sourceware.org/bugzilla/show_bug.cgi?id=18074#c1

I think the fix should consider what should happen to
"backtrace" after "frame create".  Currently, we always start
the backtrace at the current frame, so the created frame does
not appear in the backtrace.  But if the user does "frame create",
and can follow that with "up", shouldn't "bt" after "frame ADDR" to
attempt to backtrace starting at the created frame too?

That would suggest that the fix for this instead would be for
"frame create" to override the source of the current frame.
That then raises the question of how to get out of that
frame.

I'm not saying that that's the best model, but I do think this
should be considered, and the way forward depends on
how frame-related / backtrace commands _should_ work around
"frame create".

Thanks,
Pedro Alves
diff mbox

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d7a8d31..b4fe9cc 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@ 
+2015-09-11  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* frame.c (frame_find_by_id): Also check the selected frame.
+
 2015-09-11  Pierre Langlois  <pierre.langlois@arm.com>
 
 	* aarch64-tdep.c (decode_cb): Move up comment describing the
diff --git a/gdb/frame.c b/gdb/frame.c
index 745e007..114fa6a 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -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;
 }
 
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 1538d15..cf83b3a 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,10 @@ 
+2015-09-11  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* gdb.mi/mi-var-frame.c: New file.
+	* gdb.mi/mi-var-frame.exp: New file.
+	* gdb.base/frame-cmds.c: New file.
+	* gdb.base/frame-cmds.exp: New file.
+
 2015-09-09  Doug Evans  <dje@google.com>
 
 	* gdb.python/py-prettyprint.exp: Check result of run_lang_tests.
diff --git a/gdb/testsuite/gdb.base/frame-cmds.c b/gdb/testsuite/gdb.base/frame-cmds.c
new file mode 100644
index 0000000..53b222d
--- /dev/null
+++ b/gdb/testsuite/gdb.base/frame-cmds.c
@@ -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 ();
+}
diff --git a/gdb/testsuite/gdb.base/frame-cmds.exp b/gdb/testsuite/gdb.base/frame-cmds.exp
new file mode 100644
index 0000000..2518463
--- /dev/null
+++ b/gdb/testsuite/gdb.base/frame-cmds.exp
@@ -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"
diff --git a/gdb/testsuite/gdb.mi/mi-var-frame.c b/gdb/testsuite/gdb.mi/mi-var-frame.c
new file mode 100644
index 0000000..53b222d
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-var-frame.c
@@ -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 ();
+}
diff --git a/gdb/testsuite/gdb.mi/mi-var-frame.exp b/gdb/testsuite/gdb.mi/mi-var-frame.exp
new file mode 100644
index 0000000..5c3f196
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-var-frame.exp
@@ -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"
+}