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



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

I don't think any existing implementations get the edge case right :-(


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.

With CFI, frame or no frame, it is always possible to unwind the PC. A more complex prologue analysier could also manage to unwind the PC correctly in this case (but at the expense of doing prologue analysis).


The function is called when doing a next and has just stepped into a function. To make the next faster (no prologue analysis), and the prologue analyzer easier (avoid most common frameless case).

Thing is, it doesn't do anything for:

	(gdb) stepi
	Stepped into function foo()
	0x10000    add 8 to sp
	(gdb) stepi
	0x10000    store link-register in [sp + 4]

which prologue analyzers should handle but don't `because it is to hard'. A simple minded suggestion is to limit the prologue analyser to the instruction range [func ... current-pc) so that, when still in the prologue, it only records what really happened.

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.

Optional. It should fall back to using standard unwind_pc().


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

I need to deprecate extract_address() people are ment to use extract typed address. Which is even more descriptive :-)



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

Ah, thats the info I needed.



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.

I don't think it is any different. For:


	(gdb) up
	Frame #1 foo()
	(gdb) info register psw

to work correctly, the register unwind code will need to zap that bit. Otherwize GDB will mis-represent the value of the PSW in the calling frame.

I think there is still going to be a problem in the CFI unwinder. The CFI spec as the `architectural' register unwind rule as a loop-hole. Something related to that may need to be added. Wonder if GCC even thought to generate it. Hmm, the throw/catch code must have done something ....

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.

It involves an act of faith. The code needs to trust that the next frame's unwinder is correctly returning this frames register values. Imagine a world where all next frames were as reliable as the sentinel frame ...



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.

For the i386, are there any differences between how a sigtramp and a normal frame are unwound? If there are, I think the i386 should represent them with different frame objects. The above test will only occure once per frame, and then, only if another frame object hasn't already identified the frame.


I think here, the best thing is to implement it correctly, and then when the numbers are in, implement it fast.

BTW, infrun.c and breakpoint.c both call pc_in_sigtramp. Perhaphs they should call: get_frame_type (get_current_frame ()) == SIGTRAMP_FRAME. I think, using the new frame code, it is even possible delay the computation of the frame's ID until it is needed. This makes get_current_frame() very cheap.

Andrew



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