Re: [offbyone RFC] Merge i386newframe

[Andrew Cagney <ac131313 at redhat dot com>]

> Hi Andrew,
> this patch is a merge of Mark's i386newframe-branch to your offbyone-branch. It works quite well, unwinds through signal frames and frameless functions, but still has some strange regressions that aren't present in i386newframe branch.
>
> For example this program:
> int var;
> void foo ()
> {
>   var = 0x1234;
> }
> int main ()
> {
>   foo ();
>   return 0;
> }
>
> Running it in the GDB:
> (gdb) b main
> Breakpoint 1 at 0x804832c: file tst.c, line 8.
> (gdb) r
> Starting program: tst
>
> Breakpoint 1, main () at tst.c:8
> 8         foo ();
> (gdb) n

I suspect Daniel's answered this.  The frame ID needs to be constant 
through out the lifetime of the frame.  Getting that right isn't 
trivial.  However, getting it right can receive a bonus: d10v now 
passing mips_pro.exp.

> Program exited normally.
> (gdb)
>
> Why it didn't stop on "return 0" but exitted immediately instead? Did you observe this behaviour in offbyone branch too, or is it caused by this patch? It seems to have something to do with symbols, linetables, ... but I can't see how it could be related to this patch. It doesn't matter if I compile the program with stabs or dwarf2 debug info. Ideas?
>
> Anyway comments on the code are welcome. As soon as i386newframe is merged to offbyone I'll start porting x86-64 on top of it as well.

Definitly getting close!


> Index: frame.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/frame.c,v
> retrieving revision 1.71.2.7
> diff -u -p -r1.71.2.7 frame.c
> --- frame.c	11 Mar 2003 23:00:26 -0000	1.71.2.7
> +++ frame.c	12 Mar 2003 16:28:15 -0000
> @@ -1282,7 +1282,6 @@ get_prev_frame (struct frame_info *this_
>    /* Only try to do the unwind once.  */
>    if (this_frame->prev_p)
>      return this_frame->prev;
> -  this_frame->prev_p = 1;
>  
>    /* If we're inside the entry file, it isn't valid.  Don't apply this
>       test to a dummy frame - dummy frame PC's typically land in the
> @@ -1351,6 +1350,11 @@ get_prev_frame (struct frame_info *this_
>    prev_frame = FRAME_OBSTACK_ZALLOC (struct frame_info);
>    prev_frame->level = this_frame->level + 1;
>  
> +  /* Link it in.  */
> +  this_frame->prev = prev_frame;
> +  prev_frame->next = this_frame;
> +  this_frame->prev_p = 1;
> +
>    /* Try to unwind the PC.  If that doesn't work, assume we've reached
>       the oldest frame and simply return.  Is there a better sentinal
>       value?  The unwound PC value is then used to initialize the new
> @@ -1466,10 +1470,6 @@ get_prev_frame (struct frame_info *this_
>       (HP/UX) still reply on EXTRA_FRAME_INFO and, hence, still poke at
>       the "struct frame_info" object directly.  */
>    prev_frame->frame = prev_frame->id.base;
> -
> -  /* Link it in.  */
> -  this_frame->prev = prev_frame;
> -  prev_frame->next = this_frame;
>  
>    /* FIXME: cagney/2002-01-19: This call will go away.  Instead of
>       initializing extra info, all frames will use the frame_cache

The need for the above suggests code trying to walk up the frame chain 
when it shouldn't need to.  Do you have more details?

>  static CORE_ADDR
>  i386_saved_pc_after_call (struct frame_info *frame)
>  {
> -  if (get_frame_type (frame) == SIGTRAMP_FRAME)
> -    return i386_sigtramp_saved_pc (frame);
> +  char buf[4];
>  
> -  return read_memory_unsigned_integer (read_register (SP_REGNUM), 4);
> +  /* Our frame unwinder handles this just fine.  */
> +  frame_unwind_register (frame, PC_REGNUM, buf);
> +  return extract_address (buf, 4);
>  }

Idea's for what to do with this architecture method welcome.

I believe the intent is for this method to have relatively low overhead 
(when measured by the number of target interactions).  Hence, it should 
avoid doing prologue analysis (which frame_unwind_register() will trigger).

Perhaphs it should be superseeded by a method that takes a regcache 
instead of a frame (making the non-analysis of the prologue clearer)?

Alternatively, it could be defaulted to gdbarch_unwind_pc() making its 
implementation optional (by those that complain that GDB next's too slow 
:-)?

Anyway, the call:
frame_unwind_unsigned_register (frame, PC_REGNUM, &ulongest);
would simplify the code a little.



>  /* Return PC of first real instruction.  */
>  
>  static CORE_ADDR
> @@ -855,37 +690,423 @@ i386_push_return_address (CORE_ADDR pc, 
>    write_memory (sp - 4, buf, 4);
>    return sp - 4;
>  }
> +
> +#include "frame-unwind.h"

Should eventually be moved to the start of the file (yes, that came with 
the original patch).

> +#define regcache_cooked_write_unsigned regcache_raw_write_unsigned
> +
> +#ifdef I386_REGNO_TO_SYMMETRY
> +#error "The Sequent Symmetry is no longer supported."
> +#endif
> +
> +/* According to the System V ABI, the registers %ebp, %ebx, %edi, %esi
> +   and %esp "belong" to the calling function.  */
> +
> +/* The maximum number of saved registers.  This should include all
> +   registers mentioned above, and %eip.  */
> +#define I386_NUM_SAVED_REGISTERS	9
> +
> +struct i386_frame_cache
> +{
> +  CORE_ADDR saved_regs[I386_NUM_SAVED_REGISTERS];
> +  CORE_ADDR saved_sp;
> +  CORE_ADDR return_pc;
> +  CORE_ADDR base;
> +  int frameless;
> +};
> +
> +/* Parse the first few instructions the function to see what registers
> +   were stored.
> +   
> +   We handle these cases:
> +
> +   The startup sequence can be at the start of the function, or the
> +   function can start with a branch to startup code at the end.
> +
> +   %ebp can be set up with either the 'enter' instruction, or "pushl
> +   %ebp, movl %esp, %ebp" (`enter' is too slow to be useful, but was
> +   once used in the System V compiler).
> +
> +   Local space is allocated just below the saved %ebp by either the
> +   'enter' instruction, or by "subl $<size>, %esp".  'enter' has a 16
> +   bit unsigned argument for space to allocate, and the 'addl'
> +   instruction could have either a signed byte, or 32 bit immediate.
> +
> +   Next, the registers used by this function are pushed.  With the
> +   System V compiler they will always be in the order: %edi, %esi,
> +   %ebx (and sometimes a harmless bug causes it to also save but not
> +   restore %eax); however, the code below is willing to see the pushes
> +   in any order, and will handle up to 8 of them.
> + 
> +   If the setup sequence is at the end of the function, then the next
> +   instruction will be a branch back to the start.  */
> +
> +static struct i386_frame_cache *
> +i386_frame_cache (struct frame_info *frame, void **cachep)
> +{
> +  struct i386_frame_cache *cache;
> +  CORE_ADDR pc, start_pc;
> +
> +  if (*cachep)
> +    return *cachep;
> +
> +  cache = FRAME_OBSTACK_ZALLOC (struct i386_frame_cache);
> +
> +  pc = frame_pc_unwind (frame);
> +  start_pc = get_pc_function_start (pc);
> +
> +  long locals = -1;
> +
> +  if (start_pc != 0)
> +    locals = i386_get_frame_setup (start_pc);
> +
> +  if (locals >= 0 && pc > start_pc)
> +    {
> +      ULONGEST fp;
> +      CORE_ADDR addr;
> +      unsigned char op;
> +      int i;
> +
> +      frame_unwind_unsigned_register (frame, FP_REGNUM, &fp);
> +      cache->base = fp;
> +
> +      addr = fp - 4 - locals;
> +      for (i = 0; i < I386_NUM_SAVED_REGISTERS; i++)
> +        {
> +          op = codestream_get ();
> +          if (op < 0x50 || op > 0x57)
> +    	break;
> +
> +          cache->saved_regs[op - 0x50] = addr;
> +          addr -= 4;
> +        }
> +
> +      cache->saved_regs[PC_REGNUM] = fp + 4;
> +      cache->saved_regs[FP_REGNUM] = fp;
> +      cache->saved_sp = fp + 8;
> +    }
> +  else
> +    {
> +      ULONGEST sp;
> +    
> +      /* This function is either frameless or we're at the start
> +         of the function and this function's frame hasn't been
> +         setup yet.  In the latter case only %eip and %esp have
> +         changed, and we can determine their previous values.  We
> +         pretend we can do the same in the former case.  */
> +      cache->frameless = 1;
> +    
> +      frame_unwind_unsigned_register (frame, SP_REGNUM, &sp);
> +      cache->saved_regs[PC_REGNUM] = sp;
> +      cache->saved_sp = cache->saved_regs[PC_REGNUM] + 4;
> +      cache->base = sp;
> +    }
> +
> +  *cachep = cache;
> +  return cache;
> +}
>  
>  static void
> -i386_do_pop_frame (struct frame_info *frame)
> +i386_frame_pop (struct frame_info *frame, void **cachep,
> +		struct regcache *regcache)

Hmm, I've deleted this function.  Instead a frame pop relies 100% on 
register unwind.  Need to figure out the problem here.

>  {
> -  CORE_ADDR fp;
> +  CORE_ADDR fp = get_frame_base (frame);
>    int regnum;
> -  char regbuf[I386_MAX_REGISTER_SIZE];
> +  char buf[4];
> +  ULONGEST val;
>  
> -  fp = get_frame_base (frame);
> -  i386_frame_init_saved_regs (frame);
> +  gdb_assert (get_frame_type (frame) != DUMMY_FRAME);
>  
> -  for (regnum = 0; regnum < NUM_REGS; regnum++)
> +  for (regnum = 0; regnum < I386_NUM_SAVED_REGISTERS; regnum++)
>      {
> -      CORE_ADDR addr;
> -      addr = get_frame_saved_regs (frame)[regnum];
> -      if (addr)
> +      frame_unwind_register (frame, regnum, buf);
> +      regcache_cooked_write (regcache, regnum, buf);
> +    }
> +
> +  /* Reset the direction flag.  */
> +  regcache_cooked_read_unsigned (regcache, PS_REGNUM, &val);
> +  val &= ~(1 << 10);
> +  regcache_cooked_write_unsigned (regcache, PS_REGNUM, val);

Ah, ok.  So what the heck is the direction flag and why would someone 
want to clear it (Yes I've even looked in the ia32 spec :-)?

If the previous frame's direction flag should have been reset then the 
register unwind code should have done that (wonder if dwarf2cfi is 
powerful enough to specify this).

> +static CORE_ADDR
> +i386_unwind_pc (struct gdbarch *gdbarch, struct frame_info *next_frame)
> +{
> +  ULONGEST pc;
> +  frame_unwind_unsigned_register (next_frame, PC_REGNUM, &pc);
> +  return (CORE_ADDR) pc;
> +}

Yes.

> +static void
> +i386_frame_id_unwind (struct frame_info *next_frame, void **cachep,
> +		      struct frame_id *id)
> +{
> +  struct i386_frame_cache *cache = i386_frame_cache (next_frame, cachep);
> +  
> +  gdb_assert (cache);
> +  gdb_assert (get_frame_type (next_frame) != DUMMY_FRAME);
> +
> +  /* Start with a NULL next_frame ID.  */
> +  *id = null_frame_id;
> +
> +  /* Unwind PC first */
> +  id->pc = frame_pc_unwind (next_frame);
> +  cache->return_pc = id->pc;

> +  /* The next_frame's base is the address of a 4-byte word containing the
> +     calling next_frame's address.

It's previous, not next.

> +     Signal trampolines don't have a meaningful next_frame.  The frame
> +     pointer value we use is actually the next_frame pointer of the calling
> +     next_frame -- that is, the frame which was in progress when the signal
> +     trampoline was entered.  GDB mostly treats this next_frame pointer
> +     value as a magic cookie.  We detect the case of a signal
> +     trampoline by testing for get_frame_type() == SIGTRAMP_FRAME,
> +     which is set based on PC_IN_SIGTRAMP.  */

> +  if (get_frame_type (next_frame) == SIGTRAMP_FRAME || cache->frameless)
> +    id->base = get_frame_base (next_frame);

Is this SIGTRAMP_FRAME specific test needed?  Can all the information 
this code needs be obtained using frame_register_unwind().  If this and 
the next frame's base pointer are identical, just unwind the next 
frame's BP and trust the next frame to `do the right thing'.

> +  else
> +    if (!inside_entry_file (get_frame_pc (next_frame)))
> +      id->base = cache->base;
> +}
> +
> +static void
> +i386_frame_register_unwind (struct frame_info *frame, void **cachep,
> +			    int regnum, int *optimizedp,
> +			    enum lval_type *lvalp, CORE_ADDR *addrp,
> +			    int *realnump, void *valuep)
> +{
> +  /* FIXME: kettenis/20030302: I don't understand why the cache isn't
> +     already initialized.  */
> +  struct i386_frame_cache *cache = i386_frame_cache (frame, cachep);
> +
> +  gdb_assert (cache);
> +  gdb_assert (get_frame_type (frame) != DUMMY_FRAME);
> +  gdb_assert (regnum >= 0);
> +
> +  if (regnum == SP_REGNUM && cache->saved_sp)
> +    {
> +      *optimizedp = 0;
> +      *lvalp = not_lval;
> +      *addrp = 0;
> +      *realnump = -1;
> +      if (valuep)
>  	{
> -	  read_memory (addr, regbuf, REGISTER_RAW_SIZE (regnum));
> -	  deprecated_write_register_gen (regnum, regbuf);
> +	  /* Store the value.  */
> +	  store_address (valuep, 4, cache->saved_regs[SP_REGNUM]);
>  	}
> +      return;
>      }
> -  write_register (FP_REGNUM, read_memory_integer (fp, 4));
> -  write_register (PC_REGNUM, read_memory_integer (fp + 4, 4));
> -  write_register (SP_REGNUM, fp + 8);
> -  flush_cached_frames ();
> +
> +  if (regnum == PC_REGNUM)
> +    {
> +      CORE_ADDR pc;
> +      *optimizedp = 0;
> +      *lvalp = not_lval;
> +      *addrp = 0;
> +      *realnump = -1;
> +      pc = i386_frame_pc_unwind (frame, cachep);
> +      if (valuep)
> +        store_address (valuep, 4, pc);
> +    }
> +
> +  if (regnum < I386_NUM_SAVED_REGISTERS && cache->saved_regs[regnum] != 0)
> +    {
> +      *optimizedp = 0;
> +      *lvalp = lval_memory;
> +      *addrp = cache->saved_regs[regnum];
> +      *realnump = -1;
> +      if (valuep)
> +	{
> +	  /* Read the value in from memory.  */
> +	  read_memory (*addrp, valuep,
> +		       register_size (current_gdbarch, regnum));
> +	}
> +      return;
> +    }
> +
> +  frame_register (frame, regnum, optimizedp, lvalp, addrp, realnump, valuep);
> +}
> +
> +static struct i386_frame_cache *
> +i386_sigtramp_cache (struct frame_info *frame, void **cachep)
> +{
> +  struct i386_frame_cache *cache;
> +  CORE_ADDR pc, start_pc, addr;
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (current_gdbarch);
> +  int sc_offset, regnum;
> +
> +  if (*cachep)
> +    return *cachep;
> +
> +  cache = FRAME_OBSTACK_ZALLOC (struct i386_frame_cache);
> +
> +  pc = frame_pc_unwind (frame);
> +  start_pc = get_pc_function_start (pc);
> +  addr = tdep->sigcontext_addr (frame->prev);
> +
> +  for (regnum=0; regnum<=PS_REGNUM; regnum++)
> +  {
> +    /* See <asm/sigcontext.h> for details on how registers are stored 
> +       in the sigcontext.  */
> +    if (regnum < PC_REGNUM)
> +      sc_offset = (11 - regnum);
> +    else if (regnum == PC_REGNUM)
> +      sc_offset = 14;
> +    else if (regnum == PS_REGNUM)
> +      sc_offset = 16;
> +    else
> +      break;
> +
> +    sc_offset *= 4;
> +
> +    if (regnum < sizeof (cache->saved_regs))
> +      cache->saved_regs[regnum] = addr + sc_offset;
> +  }
> +
> +  cache->base = addr;
> +
> +  *cachep = cache;
> +  return cache;
> +}
> +
> +static void
> +i386_sigtramp_register_unwind (struct frame_info *frame, void **cachep,
> +			    int regnum, int *optimizedp,
> +			    enum lval_type *lvalp, CORE_ADDR *addrp,
> +			    int *realnump, void *valuep)
> +{
> +  printf_debug ("## %s(%p, %p, %s(%d))\n", __func__, (void*)frame->frame,
> +		   (void*)frame->pc, i386_register_name(regnum), regnum);
> +
> +  struct i386_frame_cache *cache = i386_sigtramp_cache (frame, cachep);
> +  char buf[4];
> +  CORE_ADDR addr;
> +
> +  gdb_assert (cache);
> +  gdb_assert (regnum >= 0);
> +
> +  if (regnum < I386_NUM_SAVED_REGISTERS && cache->saved_regs[regnum] != 0)
> +  {
> +    *optimizedp = 0;
> +    *lvalp = lval_memory;
> +    *addrp = cache->saved_regs[regnum];
> +    *realnump = -1;
> +    if (valuep)
> +        read_memory (*addrp, valuep,
> +		   register_size (current_gdbarch, regnum));
> +    return;
> +  }
> +  
> +  /* Fall back for non-saved registers.  */
> +  frame_register (frame, regnum, optimizedp, lvalp, addrp, realnump, valuep);
>  }
>  
>  static void
> -i386_pop_frame (void)
> +i386_sigtramp_id_unwind (struct frame_info *next_frame, void **cachep,
> +		      struct frame_id *id)
> +{
> +  printf_debug ("## %s(%p, %p)\n", __func__, (void*)next_frame->frame,
> +		   (void*)next_frame->pc);
> +  struct i386_frame_cache *cache = i386_sigtramp_cache (next_frame, cachep);
> +  
> +  gdb_assert (cache);
> +  gdb_assert (get_frame_type (next_frame) != DUMMY_FRAME);
> +
> +  /* Start with a NULL next_frame ID.  */
> +  *id = null_frame_id;
> +
> +  /* Unwind PC first */
> +  id->pc = frame_pc_unwind (next_frame);
> +  cache->return_pc = id->pc;
> +
> +  /* The next_frame's base is the address of a 4-byte word containing the
> +     calling next_frame's address.
> +
> +     Signal trampolines don't have a meaningful next_frame.  The frame
> +     pointer value we use is actually the next_frame pointer of the calling
> +     next_frame -- that is, the frame which was in progress when the signal
> +     trampoline was entered.  GDB mostly treats this next_frame pointer
> +     value as a magic cookie.  We detect the case of a signal
> +     trampoline by testing for get_frame_type() == SIGTRAMP_FRAME,
> +     which is set based on PC_IN_SIGTRAMP.  */
> +
> +  if (get_frame_type (next_frame) == SIGTRAMP_FRAME || cache->frameless)
> +    id->base = get_frame_base (next_frame);

Hmm, per the normal register unwind, is this needed?

> +static struct frame_unwind i386_sigtramp_unwind = {
> +  i386_sigtramp_id_unwind,
> +  i386_sigtramp_register_unwind
> +};
> +
> +static struct frame_unwind i386_frame_unwind = {
> +  i386_frame_id_unwind,
> +  i386_frame_register_unwind
> +};
> +
> +const struct frame_unwind *
> +i386_frame_p (CORE_ADDR pc)
>  {
> -  generic_pop_current_frame (i386_do_pop_frame);
> +  if (gdbarch_pc_in_sigtramp (current_gdbarch, pc, NULL))
> +    return &i386_sigtramp_unwind;
> +  else
> +    return &i386_frame_unwind;
> +}

Yes, or two separate predicate functions.

Andrew