Re: entryval tail call frames $sp adjustment vs. gdbarches [Re: New ARI warning Thu Oct 13 01:55:36 UTC 2011]

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

Hello Ulrich,

On Wed, 19 Oct 2011 17:51:45 +0200, Ulrich Weigand wrote:
> Similarly, when injecting the tailcall PC values, you hook into the
> prev_register routine

struct frame_unwind has only the prev_register method.


>  -- which assumes that unwind_pc will end up
> doing an unwind_register of pc_regnum.  This assumptium would be
> unnecessary if you just hooked into frame_unwind_pc directly instead.

This way the tail call frames are implemented only in the specific DWARF
unwinder.  They do not need to be hooked into the generic unwinding code.

There are now two phases - first phase, before dwarf2_tailcall_frame_unwind
gets installed, the outer non-tailcall frame gets unwound.

Then runs the second phase - with dwarf2_tailcall_frame_unwind installed.
Only then the PC + SP registers simulation gets into effect.  Moving this code
somewhere outside of dwarf2_tailcall_frame_unwind seems incorrect to me.


> Now as to SP, what happens is that you detect the rule for the CFA
> at the entry point is
>    CFA = <sp_register> + <some_offset>
> 
> Tailcall unwinding doesn't directly know the value of sp_register, but
> it does know the CFA, so you hook into prev_register to return a value
> of sp_register that is designed such that the above computation will
> yield the desired CFA.
> 
> However -- nothing in that design depends on the CFA being relative to
> the SP register in particular.  You could avoid this by detecting the
> more general rule
>   CFA = <some_register> + <some_offset>
> 
> and then remember both the register and the offset, and hook into 
> prev_register for whatever the register is -- no need at all to
> enforce that this must be gdbarch_sp_regnum ...
> 
> (You could then even generalize this to even more generic expressions
> to compute the CFA, if necessary for certain platforms.)

This depends on some chosen general approach and personal opinions IMO.

The most logical way would be to use gdbarch-specific SP adjustment.
Attaching below a proof of concept.  It can then drop the whole CFI analysis
from dwarf2-frame.c .  The major disadvantage is it requires gdbarch specific
code for each arch needing SP adjustment.

Another problem is one IMO needs to create a new gdbarch method (or possibly
a gdbarch value).  One cannot reuse / extend gdbarch_unwind_sp as it has
a chicken-and-egg problem with detecting whether we unwind into
TAILCALL_FRAME.  There would be needed some non-standard detection into which
frame type the unwind is being made.  It also violates the first / second
phase separation.


Therefore I originally chose the gdbarch-independent way with CFI analysis.
But I find it possibly fragile.  It is not such a concern if the SP adjustment
fails.  As it is only SP adjustment - not CFA adjustment - even if unwound SP
is wrong still frame relative objects are relative to CFA (not SP) so it does
not matter much.  Moreover tailcall frames do not have any active frame
objects.

It is more a concern if the fragile CFI detection would mistakenly corrupt
some arbitrary register by an inappropriate adjustment.  This was the reason
why I made the autodetection as strict as possible, without any flexibility
outside of what was required for existing known gdbarches needing the SP
adjustment.

I do not have an idea on which gdbarch can the current detection fail (making
itself inactive) and/or on which gdbarch your way could corrupt a register.
I just followed some such design idea.


Thanks,
Jan


Brief proof-of-concept for the gdbarch specific way:

--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2184,6 +2184,12 @@ amd64_gen_return_address (struct gdbarch *gdbarch,
   value->kind = axs_lvalue_memory;
 }
 
+static CORE_ADDR
+amd64_unwind_tailcall (struct gdbarch *gdbarch, struct frame_info *this_frame,
+		       CORE_ADDR prev_sp)
+{
+  return prev_sp - 8;
+}
 
 /* Signal trampolines.  */
 
@@ -2690,6 +2696,8 @@ amd64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   set_gdbarch_relocate_instruction (gdbarch, amd64_relocate_instruction);
 
   set_gdbarch_gen_return_address (gdbarch, amd64_gen_return_address);
+
+  set_gdbarch_unwind_tailcall (gdbarch, amd64_unwind_tailcall);
 }
 
 /* Provide a prototype to silence -Wmissing-prototypes.  */
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -794,6 +794,13 @@ default_gen_return_address (struct gdbarch *gdbarch,
   error (_("This architecture has no method to collect a return address."));
 }
 
+CORE_ADDR
+default_unwind_tailcall (struct gdbarch *gdbarch, struct frame_info *this_frame,
+			 CORE_ADDR prev_sp)
+{
+  return prev_sp;
+}
+
 /* */
 
 /* -Wmissing-prototypes */
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -169,6 +169,10 @@ extern void default_gen_return_address (struct gdbarch *gdbarch,
 					struct axs_value *value,
 					CORE_ADDR scope);
 
+extern CORE_ADDR default_unwind_tailcall (struct gdbarch *gdbarch,
+					  struct frame_info *this_frame,
+					  CORE_ADDR prev_sp);
+
 extern const char *default_auto_charset (void);
 extern const char *default_auto_wide_charset (void);
 
--- a/gdb/dwarf2-frame-tailcall.c
+++ b/gdb/dwarf2-frame-tailcall.c
@@ -56,13 +56,10 @@ struct tailcall_cache
      in CHAIN.  */
   CORE_ADDR prev_pc;
 
-  /* Compensate SP in caller frames appropriately.  prev_sp and
-     entry_cfa_sp_offset are valid only if PREV_SP_P.  PREV_SP is SP at the top
-     (caller) frame.  ENTRY_CFA_SP_OFFSET is shift of SP in tail call frames
-     against next_bottom_frame SP.  */
+  /* Compensate SP in caller frames appropriately.  prev_sp is valid iff
+     PREV_SP_P.  PREV_SP is SP at the top (caller) frame.  */
   unsigned prev_sp_p : 1;
   CORE_ADDR prev_sp;
-  LONGEST entry_cfa_sp_offset;
 };
 
 /* hash_f for htab_create_alloc of cache_htab.  */
@@ -281,7 +278,8 @@ dwarf2_tailcall_prev_register_first (struct frame_info *this_frame,
       if (next_levels == cache->chain_levels - 1)
 	addr = cache->prev_sp;
       else
-	addr = dwarf2_frame_cfa (this_frame) - cache->entry_cfa_sp_offset;
+	addr = gdbarch_unwind_tailcall (this_gdbarch, this_frame,
+					cache->prev_sp);
     }
   else
     return NULL;
@@ -353,17 +351,11 @@ tailcall_frame_sniffer (const struct frame_unwind *self,
 /* The initial "sniffer" whether THIS_FRAME is a bottom (callee) frame of a new
    chain to create.  Keep TAILCALL_CACHEP NULL if it did not find any chain,
    initialize it otherwise.  No tail call chain is created if there are no
-   unambiguous virtual tail call frames to report.
-   
-   ENTRY_CFA_SP_OFFSETP is NULL if no special SP handling is possible,
-   otherwise *ENTRY_CFA_SP_OFFSETP is the number of bytes to subtract from tail
-   call frames frame base to get the SP value there - to simulate return
-   address pushed on the stack.  */
+   unambiguous virtual tail call frames to report.  */
 
 void
 dwarf2_tailcall_sniffer_first (struct frame_info *this_frame,
-			       void **tailcall_cachep,
-			       const LONGEST *entry_cfa_sp_offsetp)
+			       void **tailcall_cachep)
 {
   CORE_ADDR prev_pc = 0, prev_sp = 0;	/* GCC warning.  */
   int prev_sp_p = 0;
@@ -391,8 +383,6 @@ dwarf2_tailcall_sniffer_first (struct frame_info *this_frame,
       /* call_site_find_chain can throw an exception.  */
       chain = call_site_find_chain (prev_gdbarch, prev_pc, this_pc);
 
-      if (entry_cfa_sp_offsetp == NULL)
-	break;
       sp_regnum = gdbarch_sp_regnum (prev_gdbarch);
       if (sp_regnum == -1)
 	break;
@@ -420,10 +410,7 @@ dwarf2_tailcall_sniffer_first (struct frame_info *this_frame,
   cache->chain_levels = pretended_chain_levels (chain);
   cache->prev_sp_p = prev_sp_p;
   if (cache->prev_sp_p)
-    {
-      cache->prev_sp = prev_sp;
-      cache->entry_cfa_sp_offset = *entry_cfa_sp_offsetp;
-    }
+    cache->prev_sp = prev_sp;
   gdb_assert (cache->chain_levels > 0);
 }
 
--- a/gdb/dwarf2-frame-tailcall.h
+++ b/gdb/dwarf2-frame-tailcall.h
@@ -27,8 +27,7 @@ struct frame_unwind;
 
 extern void
   dwarf2_tailcall_sniffer_first (struct frame_info *this_frame,
-				 void **tailcall_cachep,
-				 const LONGEST *entry_cfa_sp_offsetp);
+				 void **tailcall_cachep);
 
 extern struct value *
   dwarf2_tailcall_prev_register_first (struct frame_info *this_frame,
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -400,11 +400,7 @@ Not implemented: computing unwound register using explicit value operator"));
 }
 
 
-/* Execute FDE program from INSN_PTR possibly up to INSN_END or up to inferior
-   PC.  Modify FS state accordingly.  Return current INSN_PTR where the
-   execution has stopped, one can resume it on the next call.  */
-
-static const gdb_byte *
+static void
 execute_cfa_program (struct dwarf2_fde *fde, const gdb_byte *insn_ptr,
 		     const gdb_byte *insn_end, struct gdbarch *gdbarch,
 		     CORE_ADDR pc, struct dwarf2_frame_state *fs)
@@ -687,14 +683,9 @@ bad CFI data; mismatched DW_CFA_restore_state at %s"),
 	}
     }
 
-  if (fs->initial.reg == NULL)
-    {
-      /* Don't allow remember/restore between CIE and FDE programs.  */
-      dwarf2_frame_state_free_regs (fs->regs.prev);
-      fs->regs.prev = NULL;
-    }
-
-  return insn_ptr;
+  /* Don't allow remember/restore between CIE and FDE programs.  */
+  dwarf2_frame_state_free_regs (fs->regs.prev);
+  fs->regs.prev = NULL;
 }
 
 
@@ -1007,8 +998,6 @@ dwarf2_frame_cache (struct frame_info *this_frame, void **this_cache)
   struct dwarf2_fde *fde;
   volatile struct gdb_exception ex;
   CORE_ADDR entry_pc;
-  LONGEST entry_cfa_sp_offset;
-  int entry_cfa_sp_offset_p = 0;
   const gdb_byte *instr;
 
   if (*this_cache)
@@ -1061,25 +1050,8 @@ dwarf2_frame_cache (struct frame_info *this_frame, void **this_cache)
   fs->initial = fs->regs;
   fs->initial.reg = dwarf2_frame_state_copy_regs (&fs->regs);
 
-  if (get_frame_func_if_available (this_frame, &entry_pc))
-    {
-      /* Decode the insns in the FDE up to the entry PC.  */
-      instr = execute_cfa_program (fde, fde->instructions, fde->end, gdbarch,
-				   entry_pc, fs);
-
-      if (fs->regs.cfa_how == CFA_REG_OFFSET
-	  && (gdbarch_dwarf2_reg_to_regnum (gdbarch, fs->regs.cfa_reg)
-	      == gdbarch_sp_regnum (gdbarch)))
-	{
-	  entry_cfa_sp_offset = fs->regs.cfa_offset;
-	  entry_cfa_sp_offset_p = 1;
-	}
-    }
-  else
-    instr = fde->instructions;
-
   /* Then decode the insns in the FDE up to our target PC.  */
-  execute_cfa_program (fde, instr, fde->end, gdbarch,
+  execute_cfa_program (fde, fde->instructions, fde->end, gdbarch,
 		       get_frame_pc (this_frame), fs);
 
   TRY_CATCH (ex, RETURN_MASK_ERROR)
@@ -1222,9 +1194,7 @@ incomplete CFI data; unspecified registers (e.g., %s) at %s"),
 
   /* Try to find a virtual tail call frames chain with bottom (callee) frame
      starting at THIS_FRAME.  */
-  dwarf2_tailcall_sniffer_first (this_frame, &cache->tailcall_cache,
-				 (entry_cfa_sp_offset_p
-				  ? &entry_cfa_sp_offset : NULL));
+  dwarf2_tailcall_sniffer_first (this_frame, &cache->tailcall_cache);
 
   return cache;
 }
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -831,6 +831,10 @@ v:int:has_dos_based_file_system:::0:0::0
 # places that the return address may be found.
 m:void:gen_return_address:struct agent_expr *ax, struct axs_value *value, CORE_ADDR scope:ax, value, scope::default_gen_return_address::0
 
+# Simulate SP register for TAILCALL_FRAME.  PREV_SP is the first previous
+# non-tailcall frame.
+m:CORE_ADDR:unwind_tailcall:struct frame_info *this_frame, CORE_ADDR prev_sp:this_frame, prev_sp::default_unwind_tailcall::0
+
 EOF
 }