From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21246 invoked by alias); 9 Aug 2011 18:46:29 -0000 Received: (qmail 21237 invoked by uid 22791); 9 Aug 2011 18:46:28 -0000 X-SWARE-Spam-Status: No, hits=-2.0 required=5.0 tests=AWL,BAYES_00,MSGID_FROM_MTA_HEADER,RP_MATCHES_RCVD,TW_EG,TW_XF X-Spam-Check-By: sourceware.org Received: from mtagate2.uk.ibm.com (HELO mtagate2.uk.ibm.com) (194.196.100.162) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 09 Aug 2011 18:46:12 +0000 Received: from d06nrmr1507.portsmouth.uk.ibm.com (d06nrmr1507.portsmouth.uk.ibm.com [9.149.38.233]) by mtagate2.uk.ibm.com (8.13.1/8.13.1) with ESMTP id p79Ik40l025935 for ; Tue, 9 Aug 2011 18:46:04 GMT Received: from d06av02.portsmouth.uk.ibm.com (d06av02.portsmouth.uk.ibm.com [9.149.37.228]) by d06nrmr1507.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p79Ik4IQ2429154 for ; Tue, 9 Aug 2011 19:46:04 +0100 Received: from d06av02.portsmouth.uk.ibm.com (loopback [127.0.0.1]) by d06av02.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p79Ik3LF021415 for ; Tue, 9 Aug 2011 12:46:04 -0600 Received: from tuxmaker.boeblingen.de.ibm.com (tuxmaker.boeblingen.de.ibm.com [9.152.85.9]) by d06av02.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with SMTP id p79Ik23F021402; Tue, 9 Aug 2011 12:46:02 -0600 Message-Id: <201108091846.p79Ik23F021402@d06av02.portsmouth.uk.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Tue, 09 Aug 2011 20:46:02 +0200 Subject: Re: [try 2nd 5/8] Displaced stepping for Thumb 32-bit insns To: yao@codesourcery.com (Yao Qi) Date: Tue, 09 Aug 2011 18:46:00 -0000 From: "Ulrich Weigand" Cc: gdb-patches@sourceware.org (gdb-patches@sourceware.org) In-Reply-To: <4E3CC3D7.1030603@codesourcery.com> from "Yao Qi" at Aug 06, 2011 12:32:23 PM MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit 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 X-SW-Source: 2011-08/txt/msg00209.txt.bz2 Yao Qi wrote: > Support displaced stepping for Thumb 32-bit insns. There's still a couple of issues I noticed, but overall it is looking quite good now... Thanks! > + /* Rewrite instruction {pli/pld} PC imm12 into: > + Preapre: tmp[0] <- r0, tmp[1] <- r1, r0 <- pc, r1 <- imm12 Typo: Prepare > + {pli/pld} [r0, r1] > + > + Cleanup: r0 <- tmp[0], r1 <- tmp[1]. */ > + > + dsc->tmp[0] = displaced_read_reg (regs, dsc, 0); > + dsc->tmp[1] = displaced_read_reg (regs, dsc, 1); > + > + pc_val = displaced_read_reg (regs, dsc, ARM_PC_REGNUM); > + > + displaced_write_reg (regs, dsc, 0, pc_val, CANNOT_WRITE_PC); > + displaced_write_reg (regs, dsc, 1, imm12, CANNOT_WRITE_PC); > + dsc->u.preload.immed = 0; > + > + /* {pli/pld} [r0, r1] */ > + dsc->modinsn[0] = insn1 & 0xff00; Shouldn't this be something like 0xfff0 instead? We need to keep bit 4 set ... > +static int > +decode_thumb_32bit_ld_mem_hints (struct gdbarch *gdbarch, > + uint16_t insn1, uint16_t insn2, > + struct regcache *regs, > + struct displaced_step_closure *dsc) > +{ > + int rt = bits (insn2, 12, 15); > + int rn = bits (insn1, 0, 3); > + int op1 = bits (insn1, 7, 8); > + int err = 0; > + > + switch (bits (insn1, 5, 6)) > + { > + case 0: /* Load byte and memory hints */ > + if (rt == 0xf) /* PLD/PLI */ > + { > + if (rn == 0xf) > + /* PLD literal or Encoding T3 of PLI(immediate, literal). */ > + return thumb2_copy_preload (gdbarch, insn1, insn2, regs, dsc); > + else > + return thumb_copy_unmodified_32bit (gdbarch, insn1, insn2, > + "pli/pld", dsc); > + } > + else > + return thumb_copy_unmodified_32bit (gdbarch, insn1, insn2, > + "ldrb{reg, immediate}/ldrbt", > + dsc); Hmm. What about literal variants of LDRB/LDRSB ? > + case 1: /* Load halfword and memory hints. */ > + if (rt == 0xf) /* PLD{W} and Unalloc memory hint. */ > + return thumb_copy_unmodified_32bit (gdbarch, insn1, insn2, > + "pld/unalloc memhint", dsc); > + else > + { > + int insn2_bit_8_11 = bits (insn2, 8, 11); > + > + if (rn == 0xf) > + return thumb2_copy_load_literal (gdbarch, insn1, insn2, regs, dsc); copy_load_literal currently only handles full-word loads ... this should really be able to handle half-word loads as well (which means it probably needs a size argument). > + else > + { > + if (op1 == 0x1 || op1 == 0x3) > + /* LDRH/LDRSH (immediate), in which bit 7 of insn1 is 1, > + PC is not used. */ > + return thumb_copy_unmodified_32bit (gdbarch, insn1, insn2, > + "ldrh/ldrht", dsc); > + else if (insn2_bit_8_11 == 0xc > + || (insn2_bit_8_11 & 0x9) == 0x9) > + /* LDRH/LDRSH (imediate), in which bit 7 of insn1 is 0, PC > + can be used. */ > + return thumb2_copy_load_reg_imm (gdbarch, insn1, insn2, regs, > + dsc, 2, 0, bit (insn2, 8), 1); Actually, it cannot ... if RT is PC, we have either UNPREDICTABLE or an Unallocated memory hint; if RN is PC, we have the literal version. It seems everything except literal can just be passed through unmodified, and we do not need to call thumb2_copy_load_reg_imm at all. > + case 2: /* Load word */ > + { > + int insn2_bit_8_11 = bits (insn2, 8, 11); > + > + if (rn == 0xf) > + return thumb2_copy_load_literal (gdbarch, insn1, insn2, regs, dsc); > + else if (op1 == 0x1) /* Encoding T3 */ > + return thumb2_copy_load_reg_imm (gdbarch, insn1, insn2, regs, > + dsc, 4, 0, 0, 1); > + else /* op1 == 0x0 */ > + { > + if (insn2_bit_8_11 == 0xc || (insn2_bit_8_11 & 0x9) == 0x9) > + /* LDR (immediate) */ > + return thumb2_copy_load_reg_imm (gdbarch, insn1, insn2, regs, > + dsc, 4, 0, > + bit (insn2, 8), 1); > + else > + /* LDRT and LDR (register) */ > + return thumb2_copy_load_reg_imm (gdbarch, insn1, insn2, regs, > + dsc, 4, > + bits (insn2, 8, 11) == 0xe, > + 0, 0); LDRT also cannot use PC as target, so we really only need to check for LDR (register) here. Also, this means that thumb2_copy_load_reg_imm doesn't need a user_mode argument. (It also seems that it doesn't need a size argument: loads into PC are only allowed for the full-word instructions.) > + switch (op1) > + { > + case 1: > + { > + switch (bits (insn1, 9, 10)) > + { > + default: /* Coprocessor instructions. */ > + /* Thumb 32bit coprocessor instructions have the same encoding > + as ARM's. */ The comment isn't really correct ... > + case 2: /* op1 = 2 */ > + if (op) /* Branch and misc control. */ > + { > + if (bit (insn2, 14) /* BLX/BL */ > + || bit (insn2, 12) /* Unconditional branch */ > + || (bits (insn1, 7, 9) != 0x7)) /* Conditional branch */ > + err = thumb2_copy_b_bl_blx (gdbarch, insn1, insn2, regs, dsc); > + else if (!bit (insn2, 12) && bits (insn1, 7, 9) != 0x7) > + /* Conditional Branch */ > + err = thumb2_copy_b_bl_blx (gdbarch, insn1, insn2, regs, dsc); The else if is now superfluous: conditional branches are covered by the first if condition. Bye, Ulrich -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE Ulrich.Weigand@de.ibm.com