From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30636 invoked by alias); 25 Feb 2011 19:22:20 -0000 Received: (qmail 30611 invoked by uid 22791); 25 Feb 2011 19:22:18 -0000 X-SWARE-Spam-Status: No, hits=0.0 required=5.0 tests=AWL,BAYES_50,MSGID_FROM_MTA_HEADER,SPF_SOFTFAIL,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mtagate4.uk.ibm.com (HELO mtagate4.uk.ibm.com) (194.196.100.164) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 25 Feb 2011 19:22:12 +0000 Received: from d06nrmr1707.portsmouth.uk.ibm.com (d06nrmr1707.portsmouth.uk.ibm.com [9.149.39.225]) by mtagate4.uk.ibm.com (8.13.1/8.13.1) with ESMTP id p1PJM8XN001695 for ; Fri, 25 Feb 2011 19:22:08 GMT Received: from d06av02.portsmouth.uk.ibm.com (d06av02.portsmouth.uk.ibm.com [9.149.37.228]) by d06nrmr1707.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p1PJMIC01814636 for ; Fri, 25 Feb 2011 19:22:18 GMT 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 p1PJM82U003610 for ; Fri, 25 Feb 2011 12:22:08 -0700 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 p1PJM7bw003601; Fri, 25 Feb 2011 12:22:07 -0700 Message-Id: <201102251922.p1PJM7bw003601@d06av02.portsmouth.uk.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Fri, 25 Feb 2011 20:22:07 +0100 Subject: Re: [patch 2/3] Displaced stepping for 16-bit Thumb instructions To: yao@codesourcery.com (Yao Qi) Date: Fri, 25 Feb 2011 20:17:00 -0000 From: "Ulrich Weigand" Cc: gdb-patches@sourceware.org In-Reply-To: <4D67E90B.70409@codesourcery.com> from "Yao Qi" at Feb 26, 2011 01:38:19 AM 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-02/txt/msg00778.txt.bz2 Yao Qi wrote: > This new patch implements what we discussed above. There is a minor > difference on rule #3. "Thumb 32-bit instruction occupies *two* slots > with flag `Thumb-2'", because we have to choose breakpoint type (thumb > breakpoint or thumb-2 breakpoint) according to this flag. Actually, there's no need to get complicated w.r.t. breakpoints. The only reason for using a Thumb-2 breakpoint is if we *replace* an existing 32-bit instruction and don't want to mess up instruction stream parsing. (E.g. if the breakpoint is under an IT block and happens to be skipped, instruction execution would continue with the "second half" of the replaced instruction if we had used just a regular Thumb breakpoint.) However, with displaced stepping, we construct a full instruction sequence from scratch. In this case, we can just always use a 16-bit Thumb breakpoint instruction. (In fact, throughout the instruction sequence we construct, we can freely intermix 16-bit and 32-bit instructions. We just cannot intermix ARM and Thumb, of course.) > + /* Compute pipeline offset: > + - When executing an ARM instruction, PC reads as the address of the > + current instruction plus 8. > + - When executing a Thumb instruction, PC reads as the address of the > + current instruction plus 4. */ > + > + if (displaced_in_arm_mode (regs)) > + from += 8; > + else > + from += 4; > + > if (debug_displaced) > fprintf_unfiltered (gdb_stdlog, "displaced: read pc value %.8lx\n", > - (unsigned long) from + 8); > - return (ULONGEST) from + 8; /* Pipeline offset. */ > + (unsigned long) from); > + return (ULONGEST) from; /* Pipeline offset. */ Just remove the comment from that last line here; the offset is now handled above. > dsc->u.ldst.restore_r4 = 1; > - dsc->modinsn[0] = 0xe92d8000; /* push {pc} */ > - dsc->modinsn[1] = 0xe8bd0010; /* pop {r4} */ > - dsc->modinsn[2] = 0xe044400f; /* sub r4, r4, pc. */ > - dsc->modinsn[3] = 0xe2844008; /* add r4, r4, #8. */ > - dsc->modinsn[4] = 0xe0800004; /* add r0, r0, r4. */ > + RECORD_ARM_MODE_INSN (0, 0xe92d8000); /* push {pc} */ > + RECORD_ARM_MODE_INSN (1, 0xe8bd0010); /* pop {r4} */ > + RECORD_ARM_MODE_INSN (2, 0xe044400f); /* sub r4, r4, pc. */ > + RECORD_ARM_MODE_INSN (3, 0xe2844008); /* add r4, r4, #8. */ > + RECORD_ARM_MODE_INSN (4, 0xe0800004); /* add r0, r0, r4. */ > > /* As above. */ > if (immed) > - dsc->modinsn[5] = (insn & 0xfff00fff) | 0x20000; > + RECORD_ARM_MODE_INSN (5, ((insn & 0xfff00fff) | 0x20000)); > else > - dsc->modinsn[5] = (insn & 0xfff00ff0) | 0x20003; > - > - dsc->modinsn[6] = 0x0; /* breakpoint location. */ > - dsc->modinsn[7] = 0x0; /* scratch space. */ > + RECORD_ARM_MODE_INSN (5, ((insn & 0xfff00ff0) | 0x20003)); > > + RECORD_ARM_MODE_INSN (6, 0x00); /* breakpoint location. */ > + RECORD_ARM_MODE_INSN (7, 0x00); /* scratch space. */ This reminds me: after your latest patch in that area, we do not actually use any scratch space in the instruction stream any more, so this could be removed ... > + { > + CORE_ADDR next_pc; > + if (dsc->insn_mode == ARM) > + next_pc = dsc->insn_addr + 4; > + else if (dsc->insn_mode == THUMB) > + next_pc = dsc->insn_addr + 2; > + else > + { > + struct frame_info *fi = get_current_frame (); > + enum bfd_endian byte_order_for_code > + = gdbarch_byte_order_for_code (gdbarch); > + unsigned short inst1 > + = read_memory_unsigned_integer (dsc->insn_addr, 2, > + byte_order_for_code); > + > + next_pc = dsc->insn_addr + thumb_insn_size (inst1); > + } Huh? Shouldn't we know this already? See below ... [*] > +enum INSN_MODE {ARM, THUMB, THUMB_2}; > + > /* Structures used for displaced stepping. */ > > +/* Record an ARM mode instruction in one slot. */ > +#define RECORD_ARM_MODE_INSN(INDEX, INSN) do \ > +{\ > + dsc->modinsn[INDEX] = INSN;\ > + dsc->insn_mode = ARM;\ > + } while (0) This doesn't really make sense to me; note how the insn_mode flag gets overwritten every time one of the macros is used. Rather, the insn_mode flag should be set *once*, at very top of arm_process_displaced_insn, and it should indicate the mode of the *original* instruction we're replacing (whether it is ARM, 16-bit Thumb, or 32-bit Thumb). If we do that, we don't have to re-examine the original instruction as above at [*]. [ In fact, it might be even easier to replace insn_mode with *two* separate fields: * insn_size holds the size (4 or 2) of the *original* insn * is_thumb is true if the original insn (and thus all replacement insns) are Thumb instead of ARM ] > + /* The mode of instructions copied in array MODINSN. */ > + enum INSN_MODE insn_mode; > + > + /* The slots in the array is used in this way below, > + - ARM instruction occupies one slot with flag `ARM', > + - Thumb 16 bit instruction occupies one slot with flag `Thumb' > + - Thumb 32-bit instruction occupies *two* slots with flag `Thumb'. */ > unsigned long modinsn[DISPLACED_MODIFIED_INSNS]; So this should rather say: "insn_mode" is the mode of the original instruction. If insn_mode is ARM, then each entry of modinsn holds 4 bytes corresponding to one ARM instruction; if insn_mode is a Thumb mode, then each modinsn slot holds 2 bytes corresponding to either a 16-bit Thumb instruction or one half of a 32-bit Thumb instruction. Bye, Ulrich -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE Ulrich.Weigand@de.ibm.com