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

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

After feedback from Luis, and then discussion here:

  https://sourceware.org/pipermail/gdb-patches/2020-June/169679.html

this is a completely new version of this patch that addresses the
original issue with Python unwinders, as well as addressing the issues
brought up in the discussion above.

Thanks,
Andrew

---

commit c66679e65c2f247a75d84a0166b07fed352879e1
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Mon Jun 8 11:36:13 2020 +0100

    gdb: Python unwinders, inline frames, and tail-call frames
    
    This started with me running into the bug described in python/22748,
    in summary, 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 can't be computed until we have the frame-id for #1.  As
    a result we can't create a lazy register for frame #1 when frame #0 is
    inline.
    
    Initially I proposed a solution inline with that proposed in bugzilla,
    changing value_of_register to avoid creating a lazy register value.
    However, when this was discussed on the mailing list I got this reply:
    
      https://sourceware.org/pipermail/gdb-patches/2020-June/169633.html
    
    Which led me to look at these two patches:
    
      [1] https://sourceware.org/pipermail/gdb-patches/2020-April/167612.html
      [2] https://sourceware.org/pipermail/gdb-patches/2020-April/167930.html
    
    When I considered patches [1] and [2] I saw that all of the issues
    being addressed here were related, and that there was a single
    solution that could address all of these issues.
    
    First I wrote the new test gdb.opt/inline-frame-tailcall.exp, which
    shows that [1] and [2] regress the inline tail-call unwinder, the
    reason for this is that these two patches replace a call to
    gdbarch_unwind_pc with a call to get_frame_register, however, this is
    not correct.  The previous call to gdbarch_unwind_pc takes THIS_FRAME
    and returns the $pc value in the previous frame.  In contrast
    get_frame_register takes THIS_FRAME and returns the value of the $pc
    in THIS_FRAME; these calls are not equivalent.
    
    The reason these patches appear (or do) fix the regressions listed in
    [1] is that the tail call sniffer depends on identifying the address
    of a caller and a callee, GDB then looks for a tail-call sequence that
    takes us from the caller address to the callee, if such a series is
    found then tail-call frames are added.
    
    The bug that was being hit, and which was address in patch [1] is that
    in order to find the address of the caller, GDB ended up creating a
    lazy register value for an inline frame with to frame-id.  The
    solution in patch [1] is to instead take the address of the callee and
    treat this as the address of the caller.  Getting the address of the
    callee works, but we then end up looking for a tail-call series from
    the callee to the callee, which obviously doesn't return any sane
    results, so we don't insert any tail call frames.
    
    The original patch [1] did cause some breakage, so patch [2] undid
    patch [1] in all cases except those where we had an inline frame with
    no frame-id.  It just so happens that there were no tests that fitted
    this description _and_ which required tail-call frames to be
    successfully spotted, as a result patch [2] appeared to work.
    
    The new test inline-frame-tailcall.exp, exposes the flaw in patch [2].
    
    This commit undoes patch [1] and [2], and replaces them with a new
    solution, which is also different to the solution proposed in the
    python/22748 bug report.
    
    In this solution I propose that we introduce some special case logic
    to value_of_register_lazy.  To understand what this logic is we must
    first look at how inline frames unwind registers, this is very simple,
    they do this:
    
      static struct value *
      inline_frame_prev_register (struct frame_info *this_frame,
                                  void **this_cache, int regnum)
      {
        return get_frame_register_value (this_frame, regnum);
      }
    
    And remember:
    
      struct value *
      get_frame_register_value (struct frame_info *frame, int regnum)
      {
        return frame_unwind_register_value (frame->next, regnum);
      }
    
    So in all cases, unwinding a register in an inline frame just asks the
    next frame to unwind the register, this makes sense, as an inline
    frame doesn't really exist, when we unwind a register in an inline
    frame, we're really just asking the next frame for the value of the
    register in the previous, non-inline frame.
    
    So, if we assume that we only get into the missing frame-id situation
    when we try to unwind a register from an inline frame during the frame
    sniffing process, then we can change value_of_register_lazy to not
    create lazy register values for an inline frame.
    
    Imagine this stack setup, where #1 is inline within #2.
    
      #3 -> #2 -> #1 -> #0
            \______/
             inline
    
    Now when trying to figure out the frame-id for #1, we need to compute
    the frame-id for #2.  If the frame sniffer for #2 causes a lazy
    register read in #2, either due to a Python Unwinder, or for the
    tail-call sniffer, then we call value_of_register_lazy passing in
    frame #2.
    
    In value_of_register_lazy, we grab the next frame, which is #1, and we
    used to then ask for the frame-id of #1, which was not computed, and
    this was our bug.
    
    Now, I propose we spot that #1 is an inline frame, and so lookup the
    next frame of #1, which is #0.  As #0 is not inline it will have a
    valid frame-id, and so we create a lazy register value using #0 as the
    next-frame-id.  This will give us the exact same result we had
    previously (thanks to the code we inspected above).
    
    Encoding into value_of_register_lazy the knowledge that reading an
    inline frame register will always just forward to the next frame
    feels.... not ideal, but this seems like the cleanest solution to this
    recursive frame-id computation/sniffing issue that appears to crop
    up.
    
    The following two commits are fully reverted with this commit, these
    correspond to patches [1] and [2] respectively:
    
      commit 5939967b355ba6a940887d19847b7893a4506067
      Date:   Tue Apr 14 17:26:22 2020 -0300
    
          Fix inline frame unwinding breakage
    
      commit 991a3e2e9944a4b3a27bd989ac03c18285bd545d
      Date:   Sat Apr 25 00:32:44 2020 -0300
    
          Fix remaining inline/tailcall unwinding breakage for x86_64
    
    gdb/ChangeLog:
    
            PR python/22748
            * dwarf2/frame-tailcall.c (dwarf2_tailcall_sniffer_first): Remove
            special handling for inline frames.
            * findvar.c (value_of_register_lazy): Skip inline frames when
            creating lazy register values.
            * frame.c (frame_id_computed_p): Delete definition.
            * frame.h (frame_id_computed_p): Delete declaration.
    
    gdb/testsuite/ChangeLog:
    
            PR python/22748
            * gdb.opt/inline-frame-tailcall.c: New file.
            * gdb.opt/inline-frame-tailcall.exp: New file.
            * gdb.python/py-unwind-inline.c: New file.
            * gdb.python/py-unwind-inline.exp: New file.
            * gdb.python/py-unwind-inline.py: New file.

diff --git a/gdb/dwarf2/frame-tailcall.c b/gdb/dwarf2/frame-tailcall.c
index 16dba2b201a..2d219f13f9d 100644
--- a/gdb/dwarf2/frame-tailcall.c
+++ b/gdb/dwarf2/frame-tailcall.c
@@ -384,43 +384,8 @@ dwarf2_tailcall_sniffer_first (struct frame_info *this_frame,
 
       prev_gdbarch = frame_unwind_arch (this_frame);
 
-      /* The dwarf2 tailcall sniffer runs early, at the end of populating the
-	 dwarf2 frame cache for the current frame.  If there exists inline
-	 frames inner (next) to the current frame, there is a good possibility
-	 of that inline frame not having a computed frame id yet.
-
-	 This is because computing such a frame id requires us to walk through
-	 the frame chain until we find the first normal frame after the inline
-	 frame and then compute the normal frame's id first.
-
-	 Some architectures' compilers generate enough register location
-	 information for a dwarf unwinder to fetch PC without relying on inner
-	 frames (x86_64 for example).  In this case the PC is retrieved
-	 according to dwarf rules.
-
-	 But others generate less strict dwarf data for which assumptions are
-	 made (like interpreting DWARF2_FRAME_REG_UNSPECIFIED as
-	 DWARF2_FRAME_REG_SAME_VALUE).  For such cases, GDB may attempt to
-	 create lazy values for registers, and those lazy values must be
-	 created with a valid frame id, but we potentially have no valid id.
-
-	 So, to avoid breakage, if we see a dangerous situation with inline
-	 frames without a computed id, use safer functions to retrieve the
-	 current frame's PC.  Otherwise use the provided dwarf rules.  */
-      frame_info *next_frame = get_next_frame (this_frame);
-
       /* Simulate frame_unwind_pc without setting this_frame->prev_pc.p.  */
-      if (next_frame != nullptr && get_frame_type (next_frame) == INLINE_FRAME
-	  && !frame_id_computed_p (next_frame))
-	{
-	  /* The next frame is an inline frame and its frame id has not been
-	     computed yet.  */
-	  get_frame_register (this_frame, gdbarch_pc_regnum (prev_gdbarch),
-			      (gdb_byte *) &prev_pc);
-	  prev_pc = gdbarch_addr_bits_remove (prev_gdbarch, prev_pc);
-	}
-      else
-	prev_pc = gdbarch_unwind_pc (prev_gdbarch, this_frame);
+      prev_pc = gdbarch_unwind_pc (prev_gdbarch, this_frame);
 
       /* call_site_find_chain can throw an exception.  */
       chain = call_site_find_chain (prev_gdbarch, prev_pc, this_pc);
diff --git a/gdb/findvar.c b/gdb/findvar.c
index c7cd31ce1a6..7e9dab567f6 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -292,6 +292,14 @@ value_of_register_lazy (struct frame_info *frame, int regnum)
 
   next_frame = get_next_frame_sentinel_okay (frame);
 
+  /* In some cases NEXT_FRAME may not have a valid frame-id yet.  This can
+     happen if we end up trying to unwind a register as part of the frame
+     sniffer.  The only time that we get here without a valid frame-id is
+     if NEXT_FRAME is an inline frame.  If this is the case then we can
+     avoid getting into trouble here by skipping past the inline frames.  */
+  while (get_frame_type (next_frame) == INLINE_FRAME)
+    next_frame = get_next_frame_sentinel_okay (next_frame);
+
   /* We should have a valid next frame.  */
   gdb_assert (frame_id_p (get_frame_id (next_frame)));
 
diff --git a/gdb/frame.c b/gdb/frame.c
index ff27b9f00e9..ac1016b083f 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -687,14 +687,6 @@ frame_id_build_wild (CORE_ADDR stack_addr)
   return id;
 }
 
-bool
-frame_id_computed_p (struct frame_info *frame)
-{
-  gdb_assert (frame != nullptr);
-
-  return frame->this_id.p != 0;
-}
-
 int
 frame_id_p (struct frame_id l)
 {
diff --git a/gdb/frame.h b/gdb/frame.h
index e835d49f9ca..cfc15022ed5 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -236,10 +236,6 @@ extern struct frame_id
    as the special identifier address are set to indicate wild cards.  */
 extern struct frame_id frame_id_build_wild (CORE_ADDR stack_addr);
 
-/* Returns true if FRAME's id has been computed.
-   Returns false otherwise.  */
-extern bool frame_id_computed_p (struct frame_info *frame);
-
 /* Returns non-zero when L is a valid frame (a valid frame has a
    non-zero .base).  The outermost frame is valid even without an
    ID.  */
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")