public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] Fix inline frame unwinding breakage
@ 2020-04-14 21:31 Luis Machado
  2020-04-14 21:38 ` Luis Machado
  0 siblings, 1 reply; 24+ messages in thread
From: Luis Machado @ 2020-04-14 21:31 UTC (permalink / raw)
  To: gdb-patches; +Cc: tromey

There has been some breakage for aarch64-linux, arm-linux and s390-linux in
terms of inline frame unwinding. There may be other targets, but these are
the ones i'm aware of.

The following testcases started to show numerous failures and trigger internal
errors in GDB after commit 1009d92fc621bc4d017029b90a5bfab16e17fde5,
"Find tailcall frames before inline frames".

gdb.opt/inline-break.exp
gdb.opt/inline-cmds.exp
gdb.python/py-frame-inline.exp
gdb.reverse/insn-reverse.exp

The internal errors were of this kind:

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

After a lengthy investigation to try and find the cause of these assertions,
it seems we're dealing with some fragile/poorly documented code to handle inline
frames and we are attempting to unwind from this fragile section of code.

Before commit 1009d92fc621bc4d017029b90a5bfab16e17fde5, the tailcall sniffer
was invoked from dwarf2_frame_prev_register. By the time we invoke the
dwarf2_frame_prev_register function, we've probably already calculated the
frame id (via compute_frame_id).

After said commit, the call to dwarf2_tailcall_sniffer_first was moved to
dwarf2_frame_cache. This is very early in a frame creation process, and
we're still calculating the frame ID (so compute_frame_id is in the call
stack).

This would be fine for regular frames, but the above testcases all deal
with some inline frames.

The particularity of inline frames is that their frame ID's depend on
the previous frame's ID, and the previous frame's ID relies in the inline
frame's registers. So it is a bit of a messy situation.

We have comments in various parts of the code warning about some of these
particularities.

In the case of dwarf2_tailcall_sniffer_first, we attempt to unwind the PC,
which goes through various functions until we eventually invoke
frame_unwind_got_register. This function will eventually attempt to create
a lazy value for a particular register, and this lazy value will require
a valid frame ID.  Since the inline frame doesn't have a valid frame ID
yet (remember we're still calculating the previous frame's ID so we can tell
what the inline frame ID is) we will call compute_frame_id for the inline
frame (level 0).

We'll eventually hit the assertion above, inside get_frame_id:

--
      /* If we haven't computed the frame id yet, then it must be that
         this is the current frame.  Compute it now, and stash the
         result.  The IDs of other frames are computed as soon as
         they're created, in order to detect cycles.  See
         get_prev_frame_if_no_cycle.  */
      gdb_assert (fi->level == 0);
--

It seems to me we shouldn't have reached this assertion without having the
inline frame ID already calculated. In fact, it seems we even start recursing
a bit when we invoke get_prev_frame_always within inline_frame_this_id. But
a check makes us quit the recursion and proceed to compute the id.

Here's the call stack for context:

<<<< recursion >>>> #1  0x0000aaaaaae1d098 in get_prev_frame_always (this_frame=0xaaaaab85a670) at ../../../repos/binutils-gdb/gdb/frame.c:2124 <<< recursion >>>
    at ../../../repos/binutils-gdb/gdb/inline-frame.c:165
    at ../../../repos/binutils-gdb/gdb/dwarf2/frame.c:1296
    at ../../../repos/binutils-gdb/gdb/aarch64-tdep.c:1114
    at ../../../repos/binutils-gdb/gdb/dwarf2/frame.c:1316
    at ../../../repos/binutils-gdb/gdb/dwarf2/frame-tailcall.c:388
    at ../../../repos/binutils-gdb/gdb/dwarf2/frame.c:1218
<<<< first call >>>> #22 0x0000aaaaaae1d098 in get_prev_frame_always (this_frame=0xaaaaab85a670) at ../../../repos/binutils-gdb/gdb/frame.c:2124

The following patch addresses this by using a function that unwinds the PC
from the next (inline) frame directly as opposed to creating a lazy value
that is bound to the next frame's ID (still not computed).

I've validated this for aarch64-linux and x86_64-linux by running the
testsuite.

Tromey, would you mind checking if this suits your problematic core file
tailcall scenario?

gdb/ChangeLog:

2020-04-14  Luis Machado  <luis.machado@linaro.org>

	* dwarf2/frame-tailcall.c (dwarf2_tailcall_sniffer_first): Use
	get_frame_register instead of gdbarch_unwind_pc.
---
 gdb/dwarf2/frame-tailcall.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/gdb/dwarf2/frame-tailcall.c b/gdb/dwarf2/frame-tailcall.c
index 2d219f13f9..01bb134a5c 100644
--- a/gdb/dwarf2/frame-tailcall.c
+++ b/gdb/dwarf2/frame-tailcall.c
@@ -385,7 +385,9 @@ dwarf2_tailcall_sniffer_first (struct frame_info *this_frame,
       prev_gdbarch = frame_unwind_arch (this_frame);
 
       /* Simulate frame_unwind_pc without setting this_frame->prev_pc.p.  */
-      prev_pc = gdbarch_unwind_pc (prev_gdbarch, this_frame);
+      get_frame_register (this_frame, gdbarch_pc_regnum (prev_gdbarch),
+			  (gdb_byte *) &prev_pc);
+      prev_pc = gdbarch_addr_bits_remove (prev_gdbarch, prev_pc);
 
       /* call_site_find_chain can throw an exception.  */
       chain = call_site_find_chain (prev_gdbarch, prev_pc, this_pc);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 24+ messages in thread
end of thread, other threads:[~2020-06-22 15:49 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-14 21:31 [PATCH] Fix inline frame unwinding breakage Luis Machado
2020-04-14 21:38 ` Luis Machado
2020-04-16 21:15   ` Tom Tromey
2020-04-22  9:37   ` Andrew Burgess
2020-04-22 11:22     ` Luis Machado
2020-04-23 17:51       ` Luis Machado
2020-04-24  9:17         ` Tom de Vries
2020-04-24 10:02           ` Luis Machado
2020-04-24 10:58             ` Luis Machado
2020-04-24 11:08               ` Tom de Vries
2020-04-24 11:37                 ` Luis Machado
2020-04-24 12:23                   ` Tom de Vries
2020-04-24 13:19                     ` Luis Machado
2020-04-24 14:36                       ` Tom de Vries
2020-04-24 14:39                         ` Luis Machado
2020-06-18 16:58   ` Andrew Burgess
2020-06-18 17:29     ` Andrew Burgess
2020-06-18 17:40       ` Andrew Burgess
2020-06-18 18:19         ` Luis Machado
2020-06-18 18:31           ` Andrew Burgess
2020-06-18 18:39             ` Luis Machado
2020-06-22 15:49               ` Andrew Burgess
2020-06-18 17:45       ` Luis Machado
2020-06-18 18:04         ` Andrew Burgess

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).