Re: [PATCH] Find tailcall frames before inline frames

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

* Tom Tromey <tromey@adacore.com> [2020-02-20 08:58:20 -0700]:

> A customer reported a failure to unwind in a certain core dump.  A
> lengthy investigation showed that the problem came from the
> interaction between the tailcall and inline frame sniffers.
> 
> Normally, the regular DWARF unwinder may discover a chain of tail
> calls ending in the current frame.  In this case, it sets a member on
> the dwarf2_frame_cache object, so that a subsequent call into the
> tailcall sniffer will create the tailcall frames.
> 
> However, in this scenario, what happened is that the DWARF unwinder
> did find tailcall frames -- but then the PC of the first such frame
> was recognized and claimed by the inline frame sniffer.

I'm trying to understand the setup you have here in the hope I might
be able to craft a test case for this - given that I'm not convinced
the new placement of the tail call sniffer is safe.

Was the setup something like:

                    ,-- f3 tail calls to f4.
                    |
                    |
  f1 --> f2 --> f3 --> f4 --> f5 --> f6

  |_______________|
  All inlined in f1

Was there anything else special about this case?  I feel like there
must have been, but I don't really understand the problem description.

Thanks,
Andrew

> 
> This then caused unwinding to go astray further up the stack.
> 
> This patch fixes the problem by arranging for the tailcall sniffer to
> be called before the inline sniffer.  This way, if a DWARF frame has
> tailcall information, the tailcalls will always be processed first.
> This is safe to do, because the tailcall sniffer can only claim a
> frame if the previous frame did in fact find this information.  (So,
> for example, if no DWARF frame is ever found, then this sniffer will
> never trigger.)
> 
> This patch also partially reverts:
> 
>     commit 1ec56e88aa9b052ab10b806d82fbdbc8d153d977
>     Author: Pedro Alves <palves@redhat.com>
>     Date:   Fri Nov 22 13:17:46 2013 +0000
> 
> 	Eliminate dwarf2_frame_cache recursion, don't unwind from the dwarf2 sniffer (move dwarf2_tailcall_sniffer_first elsewhere).
> 
> That patch moved the call to dwarf2_tailcall_sniffer_first out of
> dwarf2_frame_cache, and into dwarf2_frame_prev_register.  However, in
> this situation, this is too late -- by the time
> dwarf2_frame_prev_register is called, the frame in question is already
> recognized by the inline frame sniffer.
> 
> Rather than fully revert that patch, though, this just arranges to
> call dwarf2_tailcall_sniffer_first from dwarf2_frame_cache -- which is
> called shortly after the DWARF frame sniffer succeeds, via
> compute_frame_id.
> 
> I don't know how to write a test case for this.
> 
> gdb/ChangeLog
> 2020-02-20  Tom Tromey  <tromey@adacore.com>
> 
> 	* dwarf2/frame.c (struct dwarf2_frame_cache)
> 	<checked_tailcall_bottom, entry_cfa_sp_offset,
> 	entry_cfa_sp_offset_p>: Remove members.
> 	(dwarf2_frame_cache): Call dwarf2_tailcall_sniffer_first.
> 	(dwarf2_frame_prev_register): Don't call
> 	dwarf2_tailcall_sniffer_first.
> 	(dwarf2_append_unwinders): Don't append tailcall unwinder.
> 	* frame-unwind.c (add_unwinder): New fuction.
> 	(frame_unwind_init): Use it.  Add tailcall unwinder.
> ---
>  gdb/ChangeLog      | 12 ++++++++++++
>  gdb/dwarf2/frame.c | 34 ++++++++--------------------------
>  gdb/frame-unwind.c | 33 +++++++++++++++++++++++++++------
>  3 files changed, 47 insertions(+), 32 deletions(-)
> 
> diff --git a/gdb/dwarf2/frame.c b/gdb/dwarf2/frame.c
> index b240a25e2d8..74488f9a8aa 100644
> --- a/gdb/dwarf2/frame.c
> +++ b/gdb/dwarf2/frame.c
> @@ -959,22 +959,12 @@ struct dwarf2_frame_cache
>    /* The .text offset.  */
>    CORE_ADDR text_offset;
>  
> -  /* True if we already checked whether this frame is the bottom frame
> -     of a virtual tail call frame chain.  */
> -  int checked_tailcall_bottom;
> -
>    /* If not NULL then this frame is the bottom frame of a TAILCALL_FRAME
>       sequence.  If NULL then it is a normal case with no TAILCALL_FRAME
>       involved.  Non-bottom frames of a virtual tail call frames chain use
>       dwarf2_tailcall_frame_unwind unwinder so this field does not apply for
>       them.  */
>    void *tailcall_cache;
> -
> -  /* The number of bytes to subtract from TAILCALL_FRAME frames frame
> -     base to get the SP, to simulate the return address pushed on the
> -     stack.  */
> -  LONGEST entry_cfa_sp_offset;
> -  int entry_cfa_sp_offset_p;
>  };
>  
>  static struct dwarf2_frame_cache *
> @@ -1037,6 +1027,8 @@ dwarf2_frame_cache (struct frame_info *this_frame, void **this_cache)
>       in an address that's within the range of FDE locations.  This
>       is due to the possibility of the function occupying non-contiguous
>       ranges.  */
> +  LONGEST entry_cfa_sp_offset;
> +  int entry_cfa_sp_offset_p = 0;
>    if (get_frame_func_if_available (this_frame, &entry_pc)
>        && fde->initial_location <= entry_pc
>        && entry_pc < fde->initial_location + fde->address_range)
> @@ -1049,8 +1041,8 @@ dwarf2_frame_cache (struct frame_info *this_frame, void **this_cache)
>  	  && (dwarf_reg_to_regnum (gdbarch, fs.regs.cfa_reg)
>  	      == gdbarch_sp_regnum (gdbarch)))
>  	{
> -	  cache->entry_cfa_sp_offset = fs.regs.cfa_offset;
> -	  cache->entry_cfa_sp_offset_p = 1;
> +	  entry_cfa_sp_offset = fs.regs.cfa_offset;
> +	  entry_cfa_sp_offset_p = 1;
>  	}
>      }
>    else
> @@ -1195,6 +1187,10 @@ incomplete CFI data; unspecified registers (e.g., %s) at %s"),
>        && fs.regs.reg[fs.retaddr_column].how == DWARF2_FRAME_REG_UNDEFINED)
>      cache->undefined_retaddr = 1;
>  
> +  dwarf2_tailcall_sniffer_first (this_frame, &cache->tailcall_cache,
> +				 (entry_cfa_sp_offset_p
> +				  ? &entry_cfa_sp_offset : NULL));
> +
>    return cache;
>  }
>  
> @@ -1239,16 +1235,6 @@ dwarf2_frame_prev_register (struct frame_info *this_frame, void **this_cache,
>    CORE_ADDR addr;
>    int realnum;
>  
> -  /* Check whether THIS_FRAME is the bottom frame of a virtual tail
> -     call frame chain.  */
> -  if (!cache->checked_tailcall_bottom)
> -    {
> -      cache->checked_tailcall_bottom = 1;
> -      dwarf2_tailcall_sniffer_first (this_frame, &cache->tailcall_cache,
> -				     (cache->entry_cfa_sp_offset_p
> -				      ? &cache->entry_cfa_sp_offset : NULL));
> -    }
> -
>    /* Non-bottom frames of a virtual tail call frames chain use
>       dwarf2_tailcall_frame_unwind unwinder so this code does not apply for
>       them.  If dwarf2_tailcall_prev_register_first does not have specific value
> @@ -1410,10 +1396,6 @@ static const struct frame_unwind dwarf2_signal_frame_unwind =
>  void
>  dwarf2_append_unwinders (struct gdbarch *gdbarch)
>  {
> -  /* TAILCALL_FRAME must be first to find the record by
> -     dwarf2_tailcall_sniffer_first.  */
> -  frame_unwind_append_unwinder (gdbarch, &dwarf2_tailcall_frame_unwind);
> -
>    frame_unwind_append_unwinder (gdbarch, &dwarf2_frame_unwind);
>    frame_unwind_append_unwinder (gdbarch, &dwarf2_signal_frame_unwind);
>  }
> diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
> index 35f2e82c57d..3334c472d02 100644
> --- a/gdb/frame-unwind.c
> +++ b/gdb/frame-unwind.c
> @@ -27,6 +27,7 @@
>  #include "gdb_obstack.h"
>  #include "target.h"
>  #include "gdbarch.h"
> +#include "dwarf2/frame-tailcall.h"
>  
>  static struct gdbarch_data *frame_unwind_data;
>  
> @@ -43,6 +44,18 @@ struct frame_unwind_table
>    struct frame_unwind_table_entry **osabi_head;
>  };
>  
> +/* A helper function to add an unwinder to a list.  LINK says where to
> +   install the new unwinder.  The new link is returned.  */
> +
> +static struct frame_unwind_table_entry **
> +add_unwinder (struct obstack *obstack, const struct frame_unwind *unwinder,
> +	      struct frame_unwind_table_entry **link)
> +{
> +  *link = OBSTACK_ZALLOC (obstack, struct frame_unwind_table_entry);
> +  (*link)->unwinder = unwinder;
> +  return &(*link)->next;
> +}
> +
>  static void *
>  frame_unwind_init (struct obstack *obstack)
>  {
> @@ -51,13 +64,21 @@ frame_unwind_init (struct obstack *obstack)
>  
>    /* Start the table out with a few default sniffers.  OSABI code
>       can't override this.  */
> -  table->list = OBSTACK_ZALLOC (obstack, struct frame_unwind_table_entry);
> -  table->list->unwinder = &dummy_frame_unwind;
> -  table->list->next = OBSTACK_ZALLOC (obstack,
> -				      struct frame_unwind_table_entry);
> -  table->list->next->unwinder = &inline_frame_unwind;
> +  struct frame_unwind_table_entry **link = &table->list;
> +
> +  link = add_unwinder (obstack, &dummy_frame_unwind, link);
> +  /* The DWARF tailcall sniffer must come before the inline sniffer.
> +     Otherwise, we can end up in a situation where a DWARF frame finds
> +     tailcall information, but then the inline sniffer claims a frame
> +     before the tailcall sniffer, resulting in confusion.  This is
> +     safe to do always because the tailcall sniffer can only ever be
> +     activated if the newer frame was created using the DWARF
> +     unwinder, and it also found tailcall information.  */
> +  link = add_unwinder (obstack, &dwarf2_tailcall_frame_unwind, link);
> +  link = add_unwinder (obstack, &inline_frame_unwind, link);
> +
>    /* The insertion point for OSABI sniffers.  */
> -  table->osabi_head = &table->list->next->next;
> +  table->osabi_head = link;
>    return table;
>  }
>  
> -- 
> 2.21.1
>