This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
Re: [offbyone RFC] Merge i386newframe
- From: Mark Kettenis <kettenis at chello dot nl>
- To: ac131313 at redhat dot com
- Cc: mludvig at suse dot cz, gdb-patches at sources dot redhat dot com
- Date: Thu, 13 Mar 2003 23:46:07 +0100 (CET)
- Subject: Re: [offbyone RFC] Merge i386newframe
- References: <3E6FAF64.7070304@suse.cz> <3E70D673.1040504@redhat.com>
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