This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Fix some i386 unwinder inconcistencies
> Date: Mon, 13 Jun 2011 10:31:59 +0800
> From: Yao Qi <yao@codesourcery.com>
>
> On 06/13/2011 04:57 AM, Mark Kettenis wrote:
> > 2011-06-12 Mark Kettenis <kettenis@gnu.org>
> >
> > * i386-tdep.c (i386_epilogue_frame_cache): Simplify code. Call
> > get_frame_func instead of get_frame_pc to determine the code
> > address used to construct the frame ID.
> > (i386_epilogue_frame_unwind_stop_reason): Fix coding style.
> > (i386_epilogue_frame_this_id): Likewise.
> > (i386_epilogue_frame_prev_register): New function.
> > (i386_epilogue_frame_unwind): Use i386_epilogue_frame_prev_register.
> > (i386_stack_tramp_frame_sniffer): Fix coding style.
> > (i386_stack_tramp_frame_unwind): Use i386_epilogue_frame_prev_register.
> > (i386_gdbarch_init): Fix comments.
> >
>
> Looks like you commit two irrelevant changes (simplification and code
> style/comment fix) together. IMO, each commit should be a
> self-contained, single-purpose change. I don't know this rule applies
> to GDB development or not.
It does. And I should probably have seperated out the change to use
get_frame_func. The rest of the changes are all part of a single
logical change to get rid of the inconcistencies that have crept into
this bit of the code. There is such a thing as splitting a change up
into too many bits.
> > - /* Cache base will be %esp plus cache->sp_offset (-4). */
> > - get_frame_register (this_frame, I386_ESP_REGNUM, buf);
> > - cache->base = extract_unsigned_integer (buf, 4,
> > - byte_order) + cache->sp_offset;
> > + cache->pc = get_frame_func (this_frame);
> >
> > - /* Cache pc will be the frame func. */
> > - cache->pc = get_frame_pc (this_frame);
> > -
> > - /* The saved %esp will be at cache->base plus 8. */
>
> I am not sure why this comment is removed, which is still valid to
> statement below "cache->saved_sp = cache->base + 8;", even it says
> nothing more than the code.
Exactly. It says nothing more than the code itself. That isn't
terribly helpful.
> > + /* At this point the stack looks as if we just entered the
> > + function, with the return address at the top of the
> > + stack. */
> > + sp = get_frame_register_unsigned (this_frame, I386_ESP_REGNUM);
> > + cache->base = sp + cache->sp_offset;
> > cache->saved_sp = cache->base + 8;
> > -
> > - /* The saved %eip will be at cache->base plus 4. */
>
> Why this comment is removed?
Because it didn't really say much more than what the code said either.
I replaced it with a comment that adds some useful information by
saying that the return address lives at the top of the stack.