Re: [PATCH] Fix some i386 unwinder inconcistencies

Yao Qi
2011-06-13T02:32:21Z

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.

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

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

--
Yao (é½å°§)