[binutils-gdb] Fix remaining inline/tailcall unwinding breakage for x86_64

[binutils-gdb] Fix remaining inline/tailcall unwinding breakage for x86_64

[Luis Machado]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=991a3e2e9944a4b3a27bd989ac03c18285bd545d

commit 991a3e2e9944a4b3a27bd989ac03c18285bd545d
Author: Luis Machado <luis.machado@linaro.org>
Date:   Sat Apr 25 00:32:44 2020 -0300

    Fix remaining inline/tailcall unwinding breakage for x86_64
    
    Commit 5939967b355ba6a940887d19847b7893a4506067 fixed inline
    frame unwinding breakage for some targets (aarch64, riscv, s390...)
    but regressed a few amd64 testcases related to tailcalls.
    
    Given the following example situation...
    
    Frame #-1 - sentinel frame
    Frame # 0 - inline frame
    Frame # 1 - normal frame
    
    ... suppose we're at level #1 and call into dwarf2_tailcall_sniffer_first.
    
    We'll attempt to fetch PC, which used to be done via the gdbarch_unwind_pc call
    (before 5939967b355ba6a940887d19847b7893a4506067), but now it is being handled
    by the get_frame_register function.
    
    gdbarch_unwind_pc will attempt to use frame #1's cache to retrieve information
    about the PC. Here's where different architectures behave differently.
    
    x86_64 will find a dwarf rule to retrieve PC from memory, at a CFA + offset
    location. So the PC value is readily available and there is no need to
    create a lazy value.
    
    For aarch64 (and others), GCC doesn't emit an explicit location for PC, so we
    eventually will find that PC is DWARF2_FRAME_REG_UNSPECIFIED. This is known
    and is handled by GDB by assuming GCC really meant DWARF2_FRAME_REG_SAME_VALUE.
    
    This means we'll attempt to fetch the register value from frame #0, via a call
    to frame_unwind_got_register, which will trigger the creation of a lazy value
    that requires a valid frame id for frame #0.
    
    We don't have a valid id for frame #0 yet, so we assert.
    
    Given the above, the following patch attempts to handle the situation without
    being too hacky. We verify if the next frame is an inline frame and if its
    frame id has been computed already. If it hasn't been computed yet, then we
    use the safer get_frame_register function, otherwise we use the regular
    gdbarch_unwind_pc hook.
    
    gdb/ChangeLog:
    
    2020-04-27  Luis Machado  <luis.machado@linaro.org>
    
            * dwarf2/frame-tailcall.c (dwarf2_tailcall_sniffer_first): Handle
            problematic inline frame unwinding situation.
            * frame.c (frame_id_computed_p): New function.
            * frame.h (frame_id_computed_p): New prototype.

Diff:
---
 gdb/ChangeLog               |  7 +++++++
 gdb/dwarf2/frame-tailcall.c | 39 ++++++++++++++++++++++++++++++++++++---
 gdb/frame.c                 |  8 ++++++++
 gdb/frame.h                 |  4 ++++
 4 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 51f4d9595e2..b076a45a9a1 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2020-04-27  Luis Machado  <luis.machado@linaro.org>
+
+	* dwarf2/frame-tailcall.c (dwarf2_tailcall_sniffer_first): Handle
+	problematic inline frame unwinding situation.
+	* frame.c (frame_id_computed_p): New function.
+	* frame.h (frame_id_computed_p): New prototype.
+
 2020-04-26  Tom Tromey  <tom@tromey.com>
 
 	* command.h (enum command_class) <class_pseudo>: Remove.
diff --git a/gdb/dwarf2/frame-tailcall.c b/gdb/dwarf2/frame-tailcall.c
index 01bb134a5c0..16dba2b201a 100644
--- a/gdb/dwarf2/frame-tailcall.c
+++ b/gdb/dwarf2/frame-tailcall.c
@@ -384,10 +384,43 @@ 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.  */
-      get_frame_register (this_frame, gdbarch_pc_regnum (prev_gdbarch),
-			  (gdb_byte *) &prev_pc);
-      prev_pc = gdbarch_addr_bits_remove (prev_gdbarch, prev_pc);
+      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);
 
       /* call_site_find_chain can throw an exception.  */
       chain = call_site_find_chain (prev_gdbarch, prev_pc, this_pc);
diff --git a/gdb/frame.c b/gdb/frame.c
index ac1016b083f..ff27b9f00e9 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -687,6 +687,14 @@ 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 cfc15022ed5..e835d49f9ca 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -236,6 +236,10 @@ 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.  */