From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31276 invoked by alias); 11 Nov 2014 03:33:44 -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 31264 invoked by uid 89); 11 Nov 2014 03:33:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 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; Tue, 11 Nov 2014 03:33:41 +0000 Received: from svr-orw-fem-05.mgc.mentorg.com ([147.34.97.43]) by relay1.mentorg.com with esmtp id 1Xo2D4-0005jP-1X from Yao_Qi@mentor.com for gdb-patches@sourceware.org; Mon, 10 Nov 2014 19:33:38 -0800 Received: from GreenOnly (147.34.91.1) by svr-orw-fem-05.mgc.mentorg.com (147.34.97.43) with Microsoft SMTP Server id 14.3.181.6; Mon, 10 Nov 2014 19:33:37 -0800 From: Yao Qi To: Sandra Loosemore CC: , "Qi, Yao" Subject: Re: [patch, nios2] clean up prologue/epilogue detection code References: <545EA0C1.6050104@codesourcery.com> Date: Tue, 11 Nov 2014 03:33:00 -0000 In-Reply-To: <545EA0C1.6050104@codesourcery.com> (Sandra Loosemore's message of "Sat, 8 Nov 2014 16:01:21 -0700") Message-ID: <87bnoeif74.fsf@codesourcery.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2014-11/txt/msg00177.txt.bz2 Sandra Loosemore writes: Hi, Sandra, > In this patch, all explicit matching of opcodes and masks in > nios2_in_epilogue_p, nios2_analyze_prologue, and nios2_get_net_pc is > eliminated in favor of invoking the disassembler (via > nios2_find_opcode_hash) to do that part. A new set of helper > functions are introduced to extract instruction operands according to > the format of the matched instruction opcode. Future ISA extensions > are likely to include multiple encodings of some logical operations, > such as add or subtract, so this organization will allow those details > to be consolidated in the new helper functions instead of handled > inline at call sites for those functions. Also, organizing the > analysis by what the instructions being examined conceptually do > instead of by their format and encoding makes it easier to understand. Great, these helpers look very clear to me. > > This patch also fixes an outstanding bug in the code. Formerly, there > were assumptions that the prologue and epilogue could only include one > stack adjustment each. This used to be true of code emitted by GCC > except for functions with stack frame too large to be addressed via a > 16-bit offset. A change made to GCC earlier this year (r208472) to > correct an ABI conformance issue also means that many functions with > frame pointers now have an additional stack pointer adjustment too. > In lifting the restriction, I made the prologue analyzer a little > smarter in differentiating valid prologue stack adjustments > (decrementing the SP, setting the FP from the SP) from those that can > only appear in epilogues (incrementing the SP, setting the SP from the > FP). 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, > diff --git a/gdb/nios2-tdep.c b/gdb/nios2-tdep.c > index 1b647ac..816b17b 100644 > --- a/gdb/nios2-tdep.c > +++ b/gdb/nios2-tdep.c > @@ -275,46 +275,398 @@ nios2_init_cache (struct nios2_unwind_cache *cache= , CORE_ADDR pc) > nios2_setup_default (cache); > } >=20=20 > +/* Read and identify an instruction at PC. If INSNP is non-null, > + store the instruction word into that location. Return the opcode > + pointer or NULL if the memory couldn't be read or disassembled. */ There should be an empty line between the end of comment and the function. > +static const struct nios2_opcode * > +nios2_fetch_insn (struct gdbarch *gdbarch, CORE_ADDR pc, > + unsigned int *insnp) > +{ > + LONGEST memword; > + unsigned long mach =3D gdbarch_bfd_arch_info (gdbarch)->mach; > + unsigned int insn; > + > + if (!safe_read_memory_integer (pc, NIOS2_OPCODE_SIZE, > + gdbarch_byte_order (gdbarch), &memword)) > + return NULL; > + > + insn =3D (unsigned int) memword; > + if (insnp) > + *insnp =3D insn; > + return nios2_find_opcode_hash (insn, mach); > +} > + > + > +/* Match and disassemble an ADD-type instruction, with 3 register operan= ds. > + 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. > +{ > + if (op->match =3D=3D MATCH_R1_ADD || op->match =3D=3D MATCH_R1_MOV) > + { > + *ra =3D GET_IW_R_A (insn); > + *rb =3D GET_IW_R_B (insn); > + *rc =3D GET_IW_R_C (insn); > + return 1; > + } > + return 0; > +} > + > + > /* Helper function to identify when we're in a function epilogue; > that is, the part of the function from the point at which the > - stack adjustment is made, to the return or sibcall. On Nios II, > - we want to check that the CURRENT_PC is a return-type instruction > - and that the previous instruction is a stack adjustment. > - START_PC is the beginning of the function in question. */ > - > + stack adjustments are made, to the return or sibcall. > + Note that we may have several stack adjustment instructions, and > + this function needs to test whether the stack teardown has already > + started before current_pc, not whether it has completed. */ > static int > nios2_in_epilogue_p (struct gdbarch *gdbarch, > CORE_ADDR current_pc, > CORE_ADDR start_pc) > { > - enum bfd_endian byte_order =3D gdbarch_byte_order (gdbarch); > + unsigned long mach =3D gdbarch_bfd_arch_info (gdbarch)->mach; > + /* Maximum number of possibly-epilogue instructions to check. > + Note that this number should not be too large, else we can > + potentially end up iterating through unmapped memory. */ > + int ninsns, max_insns =3D 5; > + unsigned int insn; > + const struct nios2_opcode *op =3D NULL; > + unsigned int uimm; > + int imm; > + int ra, rb, rc; > + enum branch_condition cond; > + CORE_ADDR pc; >=20=20 > /* There has to be a previous instruction in the function. */ > - if (current_pc > start_pc) > - { > + if (current_pc <=3D start_pc) > + return 0; >=20=20 > - /* Check whether the previous instruction was a stack > - adjustment. */ > - unsigned int insn > - =3D read_memory_unsigned_integer (current_pc - NIOS2_OPCODE_SIZE, > - NIOS2_OPCODE_SIZE, byte_order); > + /* Find the previous instruction before current_pc. */ > + op =3D nios2_fetch_insn (gdbarch, current_pc - NIOS2_OPCODE_SIZE, &ins= n); > + if (op =3D=3D NULL) > + return 0; >=20=20 > - if ((insn & 0xffc0003c) =3D=3D 0xdec00004 /* ADDI sp, sp, */ > - || (insn & 0xffc1ffff) =3D=3D 0xdec1883a /* ADD sp, sp, */ > - || (insn & 0xffc0003f) =3D=3D 0xdec00017) /* LDW sp, constant(sp) */ > - { > - /* Then check if it's followed by a return or a tail > - call. */ > - insn =3D read_memory_unsigned_integer (current_pc, NIOS2_OPCOD= E_SIZE, > - byte_order); > - > - if (insn =3D=3D 0xf800283a /* RET */ > - || insn =3D=3D 0xe800083a /* ERET */ > - || (insn & 0x07ffffff) =3D=3D 0x0000683a /* JMP */ > - || (insn & 0xffc0003f) =3D=3D 6) /* BR */ > - return 1; > - } > + /* Beginning with the previous instruction we just located, check whet= her > + we are in a sequence of at least one stack adjustment instruction. > + Possible instructions here include: > + ADDI sp, sp, n > + ADD sp, sp, rn > + LDW sp, n(sp) */ > + for (ninsns =3D 0, pc =3D current_pc; ninsns < max_insns; ninsns++) > + { > + int ok =3D 0; An empty line is needed between declaration and statement. > + if (nios2_match_addi (insn, op, mach, &ra, &rb, &imm)) > + ok =3D (rb =3D=3D NIOS2_SP_REGNUM); > + else if (nios2_match_add (insn, op, mach, &ra, &rb, &rc)) > + ok =3D (rc =3D=3D NIOS2_SP_REGNUM); > + else if (nios2_match_ldw (insn, op, mach, &ra, &rb, &imm)) > + ok =3D (rb =3D=3D NIOS2_SP_REGNUM); > + if (!ok) > + break; > + /* Fetch the next insn. */ > + op =3D nios2_fetch_insn (gdbarch, pc, &insn); > + if (op =3D=3D NULL) > + return 0; > + pc +=3D op->size; > } > + > + /* No stack adjustments found. */ > + if (ninsns =3D=3D 0) > + return 0; > + > + /* 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 =3D=3D 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? > + > + /* The next instruction following the stack adjustments must be a > + return, jump, or unconditional branch. */ > + if (nios2_match_jmpr (insn, op, mach, &ra) > + || nios2_match_jmpi (insn, op, mach, &uimm) > + || (nios2_match_branch (insn, op, mach, &ra, &rb, &imm, &cond) > + && cond =3D=3D branch_none)) > + return 1; > + > return 0; > } >=20=20 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. Probably, we need do the forward scan first, skipping arbitrary number of sp-adjusting instructions until a return or jump, and then do a backward scan to match a sp-adjusting instruction. In this case, return true, otherwise, return false. Hopefully, this fixes some sw watchpoint related test fails. > nios2_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *bp_addr, > int *bp_size) > { > - /* break encoding: 31->27 26->22 21->17 16->11 10->6 5->0 */ > - /* 00000 00000 0x1d 0x2d 11111 0x3a */ > - /* 00000 00000 11101 101101 11111 111010 */ > - /* In bytes: 00000000 00111011 01101111 11111010 */ > - /* 0x0 0x3b 0x6f 0xfa */ > - static const gdb_byte breakpoint_le[] =3D {0xfa, 0x6f, 0x3b, 0x0}; > - static const gdb_byte breakpoint_be[] =3D {0x0, 0x3b, 0x6f, 0xfa}; > - > - enum bfd_endian byte_order_for_code =3D gdbarch_byte_order_for_code (g= dbarch); > - > - *bp_size =3D 4; > - if (gdbarch_byte_order_for_code (gdbarch) =3D=3D BFD_ENDIAN_BIG) > - return breakpoint_be; > + enum bfd_endian byte_order =3D gdbarch_byte_order (gdbarch); Why do we change from byte_order_for_code to byte_order? > + unsigned long mach =3D gdbarch_bfd_arch_info (gdbarch)->mach; > + > + /* R1 break encoding: > + ((0x1e << 17) | (0x34 << 11) | (0x1f << 6) | (0x3a << 0)) > + 0x003da7fa */ > + static const gdb_byte r1_breakpoint_le[] =3D {0xfa, 0xa7, 0x3d, 0x0}; > + static const gdb_byte r1_breakpoint_be[] =3D {0x0, 0x3d, 0xa7, 0xfa}; > + *bp_size =3D NIOS2_OPCODE_SIZE; > + if (byte_order =3D=3D BFD_ENDIAN_BIG) > + return r1_breakpoint_be; > else > - return breakpoint_le; > + return r1_breakpoint_le; > } >=20=20 --=20 Yao (=E9=BD=90=E5=B0=A7)