From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22202 invoked by alias); 24 Nov 2014 23:52:38 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 22190 invoked by uid 89); 24 Nov 2014 23:52:37 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 24 Nov 2014 23:52:36 +0000 Received: from svr-orw-fem-03.mgc.mentorg.com ([147.34.97.39]) by relay1.mentorg.com with esmtp id 1Xt3Qn-0004dG-3F from Sandra_Loosemore@mentor.com for gdb-patches@sourceware.org; Mon, 24 Nov 2014 15:52:33 -0800 Received: from [IPv6:::1] (147.34.91.1) by svr-orw-fem-03.mgc.mentorg.com (147.34.97.39) with Microsoft SMTP Server id 14.3.181.6; Mon, 24 Nov 2014 15:52:32 -0800 Message-ID: <5473C4BA.2050903@codesourcery.com> Date: Mon, 24 Nov 2014 23:52:00 -0000 From: Sandra Loosemore User-Agent: Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/20130329 Thunderbird/17.0.5 MIME-Version: 1.0 To: Yao Qi CC: Subject: Re: [patch, nios2] clean up prologue/epilogue detection code References: <545EA0C1.6050104@codesourcery.com> <87bnoeif74.fsf@codesourcery.com> In-Reply-To: <87bnoeif74.fsf@codesourcery.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2014-11/txt/msg00615.txt.bz2 On 11/10/2014 08:33 PM, Yao Qi wrote: > These fixes should be in separate patches. I'd like to see this patch > is split to a series which includes the following patches, > > 1. add new helpers and rewrite existing functions with these new > helpers, > 2. permit more than one stack adjustment instruction to appear in > epilogue > 3. detect some additional prologue instruction patterns, Hmmm, OK (although this required rewriting code for part 1 that is promptly going to get thrown away again with parts 2 and 3). > There should be an empty line between the end of comment and the function. Fixed throughout the patch. >> + >> +/* Match and disassemble an ADD-type instruction, with 3 register operands. >> + Returns true on success, and fills in the operand pointers. */ >> +static int >> +nios2_match_add (uint32_t insn, >> + const struct nios2_opcode *op, >> + unsigned long mach, >> + int *ra, int *rb, int *rc) > > Is there any reason you put these parameters in different lines? We can > place them on the same line like: > > static int > nios2_match_add (uint32_t insn, const struct nios2_opcode *op, > unsigned long mach, int *ra, int *rb, int *rc) > > then the function can be shorter. Fixed here and elsewhere. > An empty line is needed between declaration and statement. Fixed. >> + /* We found a whole lot of stack adjustments. Be safe, tell GDB that >> + the epilogue stack unwind is in progress even if we didn't see a >> + return yet. */ >> + if (ninsns == max_insns) >> + return 1; > > I am confused by the comments and code. Infer from your code above that > GCC emits arbitrary number of sp-adjusting continuous instructions in > epilogue, is that true? No, GCC doesn't emit an arbitrary number of SP-adjusting instructions, but what should GDB do if it gets some code that looks like that? I've rewritten the comment to make it more clear that this is a "shouldn't happen" situation. > Looks the epilogue detection isn't precise nor accurate. Supposing we > have an epilogue like this below, > > 1. ADDI sp, sp, n > 2. RET > > if pc points at insn #1, nios2_in_epilogue_p returns true, which isn't > precise, because the stack frame isn't destroyed at the moment pc points > at insn #1. gdbarch in_function_epilogue_p's name is misleading, and > better to be renamed to stack_frame_destroyed_p (it's on my todo list). > > if pc points at insn #2, nios2_in_epilogue_p returns false, which isn't > accurate. I think that you have gotten confused by the pc and insn variables being updated out of sync in the scanning loop. I have rewritten the loop code and the comments to make it more obvious that scanning for stack adjustments starts at the instruction before current_pc. It doesn't matter if there have been additional stack adjustments in the sequence of instructions before that -- we only need to see one to know that the stack teardown is already in progress at current_pc. > Why do we change from byte_order_for_code to byte_order? Leftover bits from some other changes that aren't ready to commit yet. I've put it back the way it was. New patch set coming up shortly. -Sandra