This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA] mn10300 prologue analyzer fixes
- From: Kevin Buettner <kevinb at redhat dot com>
- To: Michael Snyder <msnyder at redhat dot com>
- Cc: gdb-patches at sources dot redhat dot com
- Date: Thu, 16 Feb 2006 15:46:24 -0700
- Subject: Re: [RFA] mn10300 prologue analyzer fixes
- References: <20051207153245.49b0342c@ironwood.lan>
I just noticed that this is still sitting in one of my source trees
waiting to be committed.
Ping?
On Wed, 7 Dec 2005 15:32:45 -0700
Kevin Buettner <kevinb@redhat.com> 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?
>
> * 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;