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.

1 files changed, 1 insertions, 36 deletions

diff --git a/gdb/dwarf2/frame-tailcall.c b/gdb/dwarf2/frame-tailcall.c
index 16dba2b..2d219f1 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);