[PATCH 5/5] gdb: Fix Python unwinders and inline frames

[Andrew Burgess <andrew.burgess@embecosm.com>]

I observed that when making use of a Python frame unwinder, if the
frame sniffing code accessed any registers within an inline frame then
GDB would crash with this error:

  gdb/frame.c:579: internal-error: frame_id get_frame_id(frame_info*): Assertion `fi->level == 0' failed.

The problem is that, when in the Python unwinder I write this:

  pending_frame.read_register ("register-name")

This is translated internally into a call to `value_of_register',
which in turn becomes a call to `value_of_register_lazy'.

Usually this isn't a problem, `value_of_register_lazy' requires the
next frame (more inner) to have a valid frame_id, which will be the
case (if we're sniffing frame #1, then frame #0 will have had its
frame-id figured out).

Unfortunately if frame #0 is inline within frame #1, then the frame-id
for frame #0 _is_ same as the frame-id for frame #1 - the frame-id for
frame #0 requires us to first compute the frame-id for frame #1.  As a
result it is not same to call `value_of_register_lazy' before the
frame-id of the current frame is known.

The solution I propose here is that `value_of_register' stops making a
lazy register value (the step that requires the frame-id, and thus
causes the problems), and instead calls `get_frame_register_value'.
This avoids the need to calculate the frame-id and so avoids the
problem.

This bug has crept in because we allowed calls to the function
`value_of_register_lazy' at a time when the frame-id of the frame
passed to the function didn't have its frame-id calculated.  This is
only ever a problem for inline frames.  To prevent bugs like this
getting into GDB in the future I've added an assert to the function
`value_of_register_lazy' to require the current frame have its id
calculated.

If we are calling from outside of the frame sniffer then this will
_always_ be true, so is not a problem.  If we are calling this
function from within a frame sniffer then it will always trigger.

gdb/ChangeLog:

	* findvar.c (value_of_register): Use get_frame_register_value
	rather than value_of_register_lazy.
	(value_of_register_lazy): Add new assert, and update comments.

gdb/testsuite/ChangeLog:

	* gdb.python/py-unwind-inline.c: New file.
	* gdb.python/py-unwind-inline.exp: New file.
	* gdb.python/py-unwind-inline.py: New file.
---
 gdb/ChangeLog                                 |  6 ++
 gdb/findvar.c                                 | 28 ++++++--
 gdb/testsuite/ChangeLog                       |  6 ++
 gdb/testsuite/gdb.python/py-unwind-inline.c   | 37 ++++++++++
 gdb/testsuite/gdb.python/py-unwind-inline.exp | 49 +++++++++++++
 gdb/testsuite/gdb.python/py-unwind-inline.py  | 71 +++++++++++++++++++
 6 files changed, 192 insertions(+), 5 deletions(-)
 create mode 100644 gdb/testsuite/gdb.python/py-unwind-inline.c
 create mode 100644 gdb/testsuite/gdb.python/py-unwind-inline.exp
 create mode 100644 gdb/testsuite/gdb.python/py-unwind-inline.py

diff --git a/gdb/findvar.c b/gdb/findvar.c
index c7cd31ce1a6..e5445b34930 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -263,16 +263,13 @@ struct value *
 value_of_register (int regnum, struct frame_info *frame)
 {
   struct gdbarch *gdbarch = get_frame_arch (frame);
-  struct value *reg_val;
 
   /* User registers lie completely outside of the range of normal
      registers.  Catch them early so that the target never sees them.  */
   if (regnum >= gdbarch_num_cooked_regs (gdbarch))
     return value_of_user_reg (regnum, frame);
 
-  reg_val = value_of_register_lazy (frame, regnum);
-  value_fetch_lazy (reg_val);
-  return reg_val;
+  return get_frame_register_value (frame, regnum);
 }
 
 /* Return a `value' with the contents of (virtual or cooked) register
@@ -292,7 +289,28 @@ value_of_register_lazy (struct frame_info *frame, int regnum)
 
   next_frame = get_next_frame_sentinel_okay (frame);
 
-  /* We should have a valid next frame.  */
+  /* If NEXT_FRAME is inline within FRAME, then calculating the frame-id
+     for the next frame will require that FRAME has a valid frame-id.
+     Usually this is not a problem, however, if this function is ever
+     called from a frame sniffer, the small window of time when not all
+     frames have yet had their frame-id calculated, this function will
+     trigger an assert within get_frame_id that a frame at a level > 0
+     doesn't have a frame id.
+
+     Instead of waiting for an inline frame to reveal an invalid use of
+     this function, we instead demand that FRAME must have had its frame-id
+     calculated before we use this function.
+
+     The one time it is safe to call this function when FRAME does not yet
+     have a frame id is when FRAME is at level 0, in this case the
+     assertion in get_frame_id doesn't fire, and instead get_frame_id will
+     correctly compute the frame id for us.  */
+  gdb_assert (frame_relative_level (frame) == 0
+	      || frame_id_computed_p (frame));
+
+  /* We should have a valid next frame too.  Given that FRAME is more
+     outer than NEXT_FRAME, and we know that FRAME has its ID calculated,
+     then this must always be true.  */
   gdb_assert (frame_id_p (get_frame_id (next_frame)));
 
   reg_val = allocate_value_lazy (register_type (gdbarch, regnum));
diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.c b/gdb/testsuite/gdb.python/py-unwind-inline.c
new file mode 100644
index 00000000000..0cfe06dd273
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-unwind-inline.c
@@ -0,0 +1,37 @@
+/* Copyright 2019-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/>.  */
+
+volatile int global_var;
+
+int  __attribute__ ((noinline))
+bar ()
+{
+  return global_var;
+}
+
+static inline int __attribute__ ((always_inline))
+foo ()
+{
+  return bar ();
+}
+
+int
+main ()
+{
+  int ans;
+  global_var = 0;
+  ans = foo ();
+  return ans;
+}
diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.exp b/gdb/testsuite/gdb.python/py-unwind-inline.exp
new file mode 100644
index 00000000000..f7c65f6afc8
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-unwind-inline.exp
@@ -0,0 +1,49 @@
+# Copyright (C) 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 script tests GDB's handling of using a Python unwinder in the
+# presence of inlined frames.
+
+load_lib gdb-python.exp
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
+	  debug] } {
+    return -1
+}
+
+# Skip all tests if Python scripting is not enabled.
+if { [skip_python_tests] } { continue }
+
+# The following tests require execution.
+if ![runto_main] then {
+    fail "can't run to main"
+    return 0
+}
+
+set pyfile [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
+
+gdb_breakpoint "foo"
+
+gdb_test "source ${pyfile}" "Python script imported" \
+        "import python scripts"
+
+gdb_continue_to_breakpoint "foo"
+
+gdb_test_sequence "backtrace"  "backtrace with dummy unwinder" {
+    "\\r\\n#0  foo \\(\\)"
+    "\\r\\n#1  main \\(\\)"
+}
diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.py b/gdb/testsuite/gdb.python/py-unwind-inline.py
new file mode 100644
index 00000000000..7206a0b2830
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-unwind-inline.py
@@ -0,0 +1,71 @@
+# Copyright (C) 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/>.
+
+# A dummy stack unwinder used for testing the Python unwinders when we
+# have inline frames.  This unwinder will never claim any frames,
+# instead, all it does it try to read all registers possible target
+# registers as part of the frame sniffing process..
+
+import gdb
+from gdb.unwinder import Unwinder
+
+apb_global = None
+
+class dummy_unwinder (Unwinder):
+    """ A dummy unwinder that looks at a bunch of registers as part of
+    the unwinding process."""
+
+    class frame_id (object):
+        """ Basic frame id."""
+
+        def __init__ (self, sp, pc):
+            """ Create the frame id."""
+            self.sp = sp
+            self.pc = pc
+
+    def __init__ (self):
+        """Create the unwinder."""
+        Unwinder.__init__ (self, "dummy stack unwinder")
+        self.void_ptr_t = gdb.lookup_type("void").pointer()
+        self.regs = None
+
+    def get_regs (self, pending_frame):
+        """Return a list of register names that should be read.  Only
+        gathers the list once, then caches the result."""
+        if (self.regs != None):
+            return self.regs
+
+        # Collect the names of all registers to read.
+        self.regs = list (pending_frame.architecture ()
+                          .register_names ())
+
+        return self.regs
+
+    def __call__ (self, pending_frame):
+        """Actually performs the unwind, or at least sniffs this frame
+        to see if the unwinder should claim it, which is never does."""
+        try:
+            for r in (self.get_regs (pending_frame)):
+                v = pending_frame.read_register (r).cast (self.void_ptr_t)
+        except:
+            print ("Dummy unwinder, exception")
+            raise
+
+        return None
+
+# Register the ComRV stack unwinder.
+gdb.unwinder.register_unwinder (None, dummy_unwinder (), True)
+
+print ("Python script imported")
-- 
2.25.4