This is the mail archive of the gdb-patches@sources.redhat.com mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [offbyone RFC] Merge i386newframe


   Date: Thu, 13 Mar 2003 14:05:23 -0500
   From: Andrew Cagney <ac131313 at redhat dot com>

   > 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.

There defenitely is a case where the frame ID isn't constant through
the lifetime of the frame in my current implementation.  I think I can
fix that though...

   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).

Hmm.  I was under the impression that we have this function because on
some targets (the i386 is one of them) the frame hasn't been setup yet
when we've stopped on the first instruction of a function.

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

I think that would be a good idea.

   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.

But I believe the current implementation is more expressive, since the
PC is a ultimately a memory address.  I did consider your
implementation too, and did find it difficult too choose :-).

   >  /* 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).

Yup!

   > +#define regcache_cooked_write_unsigned regcache_raw_write_unsigned

And this is temporary too.

   >  
   >  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.

My implementation of this function suggests that, maybe, it isn't such
a good idea to delete it entirely.

   > +  /* 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 :-)?

Well, being CISC, the i386 has these instructions that, as a
side-effect, based on the direction flag, increase or decrease the
value of the %ecx register.  My copy of the i386 processor supplement
of the system V ABI (Fourth edition) says on page 3-12 that "the
direction flag must be set to forward before entry and upon exit from
a function."  This bit of code tries the implement the "upon exit" bit.

   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).

I felt that it is somehow different from a "saved" registers.  But
your phrasing makes me believe it's more correct to reset from the
register unwind code.

   > +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.

Ahh, Michael changed my comment here.

   > +     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'.

Might work.  I'll have to think about that a bit more.

   > +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?

I'm not sure whether we need/want these sigtramp-specific unwinders.
Read ahead.

   > +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.

Hmm, the pc_in_sigtramp test isn't exactly cheap on some targets, so
if we can do without I think that would be preferable.

Mark


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]