Re: [PATCH] Fix some i386 unwinder inconcistencies

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

> -      /* 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 (齐尧)