From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 521 invoked by alias); 28 Feb 2006 18:49:28 -0000 Received: (qmail 513 invoked by uid 22791); 28 Feb 2006 18:49:27 -0000 X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (66.187.233.31) by sourceware.org (qpsmtpd/0.31) with ESMTP; Tue, 28 Feb 2006 18:49:26 +0000 Received: from int-mx1.corp.redhat.com (int-mx1.corp.redhat.com [172.16.52.254]) by mx1.redhat.com (8.12.11/8.12.11) with ESMTP id k1SInOPq025029 for ; Tue, 28 Feb 2006 13:49:24 -0500 Received: from potter.sfbay.redhat.com (potter.sfbay.redhat.com [172.16.27.15]) by int-mx1.corp.redhat.com (8.11.6/8.11.6) with ESMTP id k1SInN102018; Tue, 28 Feb 2006 13:49:23 -0500 Received: from [172.16.24.50] (bluegiant.sfbay.redhat.com [172.16.24.50]) by potter.sfbay.redhat.com (8.12.8/8.12.8) with ESMTP id k1SInMKt004669; Tue, 28 Feb 2006 13:49:22 -0500 Message-ID: <44049B32.8070503@redhat.com> Date: Tue, 28 Feb 2006 18:53:00 -0000 From: Michael Snyder User-Agent: Mozilla Thunderbird 1.0.7-1.4.1 (X11/20050929) MIME-Version: 1.0 To: Kevin Buettner CC: gdb-patches@sources.redhat.com Subject: Re: [RFA] mn10300 prologue analyzer fixes References: <20051207153245.49b0342c@ironwood.lan> In-Reply-To: <20051207153245.49b0342c@ironwood.lan> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2006-02/txt/msg00509.txt.bz2 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; > >