[RFA] mn10300 prologue analyzer fixes

Michael Snyder msnyder@redhat.com
Tue Feb 28 18:53:00 GMT 2006


Kevin Buettner wrote:
> This patch actually contains two distinct fixes, but one of them is
> a one-liner...
> 
> The bulk of the patch is concerned with the pattern match that
> culminates in finding a series of fmov instructions.  As explained in
> the comments, the code attempted to defer the increment of `addr'
> until it was certain that the pattern match succeeded.  Unfortunately,
> it fails to do the right thing in at least two places.  The first is
> just below that comment which used to say "We're committed now."
> The second is in the `for' loop which processes the fmov instructions. 
> The initialization portion of this loop advances `addr' prior to a
> successful read of an fmov instruction.  If / when we do either of these
> increments, but later fail to find an fmov instruction, the remainder
> of the prologue scan will continue with `addr' advanced too far.
> 
> The one-liner portion of the patch concerns the adjustment of
> `stack_extra_size' in the last hunk of the patch.   As I recall,
> the size being extracted is a negative quantity, which necessitates
> changing the sign.  This bug was discovered when attempting to
> backtrace through a frame-pointer-less function.
> 
> Okay?

Looks good, except that the indentation seems staggered in a
couple of block comments.  Might just be a tabs artifact...

> 
> 	* mn10300-tdep.c (mn10300_analyze_prologue):  Implement backtrack
> 	out of pattern match by saving relevant state.  Fix stack size
> 	adjustment bug.
> 
> Index: mn10300-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/mn10300-tdep.c,v
> retrieving revision 1.134
> diff -u -p -r1.134 mn10300-tdep.c
> --- mn10300-tdep.c	8 Sep 2005 22:48:56 -0000	1.134
> +++ mn10300-tdep.c	7 Dec 2005 22:07:09 -0000
> @@ -620,6 +620,17 @@ mn10300_analyze_prologue (struct frame_i
>  	[mov sp,a3]        [mov sp,a3]
>  	[add -SIZE2,sp]    [add -SIZE2,sp]                                 */
>  
> +      /* Remember the address at which we started in the event that we
> +         don't ultimately find an fmov instruction.  Once we're certain
> +	 that we matched one of the above patterns, we'll set
> +	 ``restore_addr'' to the appropriate value.  Note: At one time
> +	 in the past, this code attempted to not adjust ``addr'' until
> +	 there was a fair degree of certainty that the pattern would be
> +	 matched.  However, that code did not wait until an fmov instruction
> +	 was actually encountered.  As a consequence, ``addr'' would
> +	 sometimes be advanced even when no fmov instructions were found.  */
> +      CORE_ADDR restore_addr = addr;
> +
>        /* First, look for add -SIZE,sp (i.e. add imm8,sp  (0xf8feXX)
>                                           or add imm16,sp (0xfafeXXXX)
>                                           or add imm32,sp (0xfcfeXXXXXXXX)) */
> @@ -651,10 +662,10 @@ mn10300_analyze_prologue (struct frame_i
>  		     This is a one byte instruction:  mov sp,aN = 0011 11XX
>  		     where XX is the register number.
>  
> -		     Skip this instruction by incrementing addr. (We're
> -		     committed now.) The "fmov" instructions will have the
> -		     form "fmov fs#,(aN+)" in this case, but that will not
> -		     necessitate a change in the "fmov" parsing logic below. */
> +		     Skip this instruction by incrementing addr.  The "fmov"
> +		     instructions will have the form "fmov fs#,(aN+)" in this
> +		     case, but that will not necessitate a change in the
> +		     "fmov" parsing logic below. */
>  
>  		  addr++;
>  
> @@ -698,6 +709,14 @@ mn10300_analyze_prologue (struct frame_i
>  		      if (buf[0] != 0xf9 && buf[0] != 0xfb)
>  			break;
>  
> +		      /* An fmov instruction has just been seen.  We can
> +		         now really commit to the pattern match.  Set the
> +			 address to restore at the end of this speculative
> +			 bit of code to the actually address that we've
> +			 been incrementing (or not) throughout the
> +			 speculation.  */
> +		      restore_addr = addr;
> +
>  		      /* Get the floating point register number from the 
>  			 2nd and 3rd bytes of the "fmov" instruction:
>  			 Machine Code: 0000 00X0 YYYY 0000 =>
> @@ -719,6 +738,7 @@ mn10300_analyze_prologue (struct frame_i
>  		{
>  		  /* No "fmov" was found. Reread the two bytes at the original
>  		     "addr" to reset the state. */
> +		  addr = restore_addr;
>  		  if (!safe_frame_unwind_memory (fi, addr, buf, 2))
>  		    goto finish_prologue;
>  		}
> @@ -727,8 +747,16 @@ mn10300_analyze_prologue (struct frame_i
>  	     instruction. Handle this below. */
>  	}
>        /* else no "add -SIZE,sp" was found indicating no floating point
> -	 registers are saved in this prologue. Do not increment addr. Pretend
> -	 this bit of code never happened. */
> +	 registers are saved in this prologue.  */
> +
> +      /* In the pattern match code contained within this block, `restore_addr'
> +         is set to the starting address at the very beginning and then
> +	 iteratively to the next address to start scanning at once the
> +	 pattern match has succeeded.  Thus `restore_addr' will contain
> +	 the address to rewind to if the pattern match failed.  If the
> +	 match succeeded, `restore_addr' and `addr' will already have the
> +	 same value.  */
> +      addr = restore_addr;
>      }
>  
>    /* Now see if we set up a frame pointer via "mov sp,a3" */
> @@ -777,7 +805,7 @@ mn10300_analyze_prologue (struct frame_i
>  	goto finish_prologue;
>  
>        /* Note the size of the stack.  */
> -      stack_extra_size += extract_signed_integer (buf, imm_size);
> +      stack_extra_size -= extract_signed_integer (buf, imm_size);
>  
>        /* We just consumed 2 + imm_size bytes.  */
>        addr += 2 + imm_size;
> 
> 



More information about the Gdb-patches mailing list