From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19288 invoked by alias); 18 Feb 2011 06:06:12 -0000 Received: (qmail 19279 invoked by uid 22791); 18 Feb 2011 06:06:10 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL,BAYES_00,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 18 Feb 2011 06:06:06 +0000 Received: (qmail 15112 invoked from network); 18 Feb 2011 06:06:03 -0000 Received: from unknown (HELO ?192.168.0.101?) (yao@127.0.0.2) by mail.codesourcery.com with ESMTPA; 18 Feb 2011 06:06:03 -0000 Message-ID: <4D5E0C48.7050002@codesourcery.com> Date: Fri, 18 Feb 2011 06:33:00 -0000 From: Yao Qi User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.13) Gecko/20101208 Lightning/1.0b2 Thunderbird/3.1.7 MIME-Version: 1.0 To: Ulrich Weigand CC: gdb-patches@sourceware.org Subject: Re: [patch 2/3] Displaced stepping for 16-bit Thumb instructions References: <201102171922.p1HJMM9I029170@d06av02.portsmouth.uk.ibm.com> In-Reply-To: <201102171922.p1HJMM9I029170@d06av02.portsmouth.uk.ibm.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes 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/msg00445.txt.bz2 On 02/18/2011 03:22 AM, Ulrich Weigand wrote: > Yao Qi wrote: > > >> - 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. > I don't have a mixture of ARM and Thumb instructions in a single modinsn block. When displace stepping 16-bit instructions, modinsn[].insn.t is used to record 16-bit instructions and all instructions in copy area are 16-bit also. In 32-bit case, modinsn[].insn.a is used, and all instructions in copy area are 32-bit. > 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. The reason I propose a union here is to try to avoid too-many byte operations during recording instructions and copying to copy area. The union will waste some space in 16-bit instructions case, but IMO, it doesn't matter too much. I agree that we should a single flag for mode, and remove field size from struct insn. The changes in `struct displaced_step_closure' is like this, - unsigned long modinsn[DISPLACED_MODIFIED_INSNS]; + + unsigned short flag; /* indicates the mode of instructions in MODINSNS. */ + union + { + unsigned long a; + unsigned short t; + }modinsns[DISPLACED_MODIFIED_INSNS]; Do you agree on this proposed data structure? We need an agreement on this basic data structure before I start to write/change the rest of patches. > > 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. > I'll remove this chunk from my patch, and create another patch specific to this 'magic number' problem separately. > >> /* 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.) > Yes, we've already known the mode. We can use either tdep->arm_breakpoint or tdep->thumb_breakpoint directly. >> @@ -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). `if (!dsc->wrote_to_pc)' guard that we will not follow branch in this case. However, since we've known the mode, we can adjust pc directly, without bothering complicated arm_get_next_pc_raw. -- Yao (齐尧)