From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30593 invoked by alias); 17 Feb 2011 19:22:33 -0000 Received: (qmail 30546 invoked by uid 22791); 17 Feb 2011 19:22:32 -0000 X-SWARE-Spam-Status: No, hits=-1.3 required=5.0 tests=AWL,BAYES_00,MSGID_FROM_MTA_HEADER,SPF_SOFTFAIL,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mtagate6.uk.ibm.com (HELO mtagate6.uk.ibm.com) (194.196.100.166) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 17 Feb 2011 19:22:26 +0000 Received: from d06nrmr1707.portsmouth.uk.ibm.com (d06nrmr1707.portsmouth.uk.ibm.com [9.149.39.225]) by mtagate6.uk.ibm.com (8.13.1/8.13.1) with ESMTP id p1HJMNFV005951 for ; Thu, 17 Feb 2011 19:22:24 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 p1HJMU1C1933480 for ; Thu, 17 Feb 2011 19:22:30 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 p1HJMNb1029195 for ; Thu, 17 Feb 2011 12:22:23 -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 p1HJMM9I029170; Thu, 17 Feb 2011 12:22:22 -0700 Message-Id: <201102171922.p1HJMM9I029170@d06av02.portsmouth.uk.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Thu, 17 Feb 2011 20:22:22 +0100 Subject: Re: [patch 2/3] Displaced stepping for 16-bit Thumb instructions To: yao@codesourcery.com (Yao Qi) Date: Thu, 17 Feb 2011 19:46:00 -0000 From: "Ulrich Weigand" Cc: gdb-patches@sourceware.org In-Reply-To: <4D15FCD1.7070706@codesourcery.com> from "Yao Qi" at Dec 25, 2010 10:16:49 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-02/txt/msg00434.txt.bz2 Yao Qi wrote: > Current implementation of displaced stepping in ARM assumes instruction > size is fixed 32-bit. Patch 2 is to rewrite some infrastructure code to > be ready to handle non-32-bit instructions. This patch doesn't change > any GDB functionality either. Thanks for working on this! > * gdb/arm-tdep.h (struct displaced_step_closure): Add new field > modinsns. > Remove field modinsn. > (RECORD_MOD_32BIT_INSN): New macro. > (RECORD_MOD_16BIT_INSN): New macro. > (RECORD_MOD_INSN): New macro. > - unsigned long modinsn[DISPLACED_MODIFIED_INSNS]; > + > + struct insn > + { > + union > + { > + unsigned long a; > + unsigned short t; > + }insn; > + unsigned short size; > + }modinsns[DISPLACED_MODIFIED_INSNS]; > + I don't think this is the right way to go. You cannot have a mixture of ARM and Thumb instructions in a single modinsn block, and if you have Thumb instructions, they all need to be transfered in 16-bit chunks, even the 32-bit Thumb2 instructions, to get the endian conversion right. So I think you should rather keep a single modinsn array of unsigned long. When filling it in, ARM instructions are handled as today, 16-bit Thumb instructions are likewise just filled into one modinsn slot, and 32-bit Thumb instructions are filled into two modinsn slots. When copying the modinsn array out to the target, each slot is transfered as 4 bytes in ARM mode, and as 2 bytes in Thumb mode. To know in which mode you are, it is probably best to have a single flag in the struct displaced_step_closure that indicated whether it is ARM or Thumb; this flag would be set once at the start of arm_process_displaced_insn, and used throughout the code whereever we need to know the mode. This approach would make most of the changes in this patch obsolete. > (cleanup_branch): Replace magic number by macros. > - ULONGEST pc = displaced_read_reg (regs, from, 15); > - displaced_write_reg (regs, dsc, 14, pc - 4, CANNOT_WRITE_PC); > + ULONGEST pc = displaced_read_reg (regs, from, ARM_PC_REGNUM); > + displaced_write_reg (regs, dsc, ARM_LR_REGNUM, pc - 4, CANNOT_WRITE_PC); I'm not sure about this change -- other callers just pass in plain register numbers as well ... Either those should all be changed, or none of them. In any case, this is really an unrelated change, and should be done -if at all- in a separate patch. > @@ -5908,23 +5928,44 @@ arm_displaced_init_closure (struct gdbarch *gdbarch, CORE_ADDR from, > CORE_ADDR to, struct displaced_step_closure *dsc) > { > struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); > - unsigned int i; > + unsigned int i, len, offset; > enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch); > > + offset = 0; > /* Poke modified instruction(s). */ > for (i = 0; i < dsc->numinsns; i++) > { > + unsigned long insn; > + if (dsc->modinsns[i].size == 4) > + insn = dsc->modinsns[i].insn.a; > + else if (dsc->modinsns[i].size == 2) > + insn = dsc->modinsns[i].insn.t; > + else > + gdb_assert (0); > + > if (debug_displaced) > - fprintf_unfiltered (gdb_stdlog, "displaced: writing insn %.8lx at " > - "%.8lx\n", (unsigned long) dsc->modinsn[i], > - (unsigned long) to + i * 4); > - write_memory_unsigned_integer (to + i * 4, 4, byte_order_for_code, > - dsc->modinsn[i]); > + { > + fprintf_unfiltered (gdb_stdlog, "displaced: writing insn "); > + if (dsc->modinsns[i].size == 4) > + fprintf_unfiltered (gdb_stdlog, "%.8lx", > + dsc->modinsns[i].insn.a); > + else if (dsc->modinsns[i].size == 2) > + fprintf_unfiltered (gdb_stdlog, "%.4x", > + dsc->modinsns[i].insn.t); > + > + fprintf_unfiltered (gdb_stdlog, " at %.8lx\n", > + (unsigned long) to + offset); > + } > + write_memory_unsigned_integer (to + offset, dsc->modinsns[i].size, > + byte_order_for_code, > + insn); > + offset += dsc->modinsns[i].size; > } As indicated above, this should just copy dsc->numinsns chunks of size 4 in ARM mode, and of size 2 in Thumb mode. > /* Put breakpoint afterwards. */ > - write_memory (to + dsc->numinsns * 4, tdep->arm_breakpoint, > - tdep->arm_breakpoint_size); > + write_memory (to + arm_displaced_step_breakpoint_offset (dsc), > + arm_breakpoint_from_pc (gdbarch, &from, &len), > + len); Calling arm_breakpoint_from_pc is not a good idea, since this calls arm_pc_is_thumb, which may end up getting a wrong result. Since we already know whether we're in ARM or Thumb mode, you should just emit either tdep->arm_breakpoint or tdep->thumb_breakpoint. (Since we're not *replacing* any instruction here, there is never a need to use the Thumb-2 breakpoint.) > @@ -5960,7 +6001,11 @@ arm_displaced_step_fixup (struct gdbarch *gdbarch, > dsc->cleanup (gdbarch, regs, dsc); > > if (!dsc->wrote_to_pc) > - regcache_cooked_write_unsigned (regs, ARM_PC_REGNUM, dsc->insn_addr + 4); > + { > + struct frame_info *fi = get_current_frame (); > + regcache_cooked_write_unsigned (regs, ARM_PC_REGNUM, > + arm_get_next_pc_raw(fi, dsc->insn_addr, 0)); > + } Hmm, arm_get_next_pc_raw tries to follow branches etc, which is probably not what we want here. Again, I'd rather just check ARM vs. Thumb state (in Thumb mode we could then check the instruction to see whether it is a 16-bit or 32-bit instruction --- or even better, the original decoding step could have just set a flag in dsc). Bye, Ulrich -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE Ulrich.Weigand@de.ibm.com