Re: [PATCH] Fix some i386 unwinder inconcistencies

[Mark Kettenis <mark.kettenis@xs4all.nl>]

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