From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 47165 invoked by alias); 3 Dec 2015 13:58:34 -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 47146 invoked by uid 89); 3 Dec 2015 13:58:33 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL,BAYES_00,SPF_PASS autolearn=ham version=3.3.2 X-HELO: usplmg20.ericsson.net Received: from usplmg20.ericsson.net (HELO usplmg20.ericsson.net) (198.24.6.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Thu, 03 Dec 2015 13:58:32 +0000 Received: from EUSAAHC006.ericsson.se (Unknown_Domain [147.117.188.90]) by usplmg20.ericsson.net (Symantec Mail Security) with SMTP id C8.BF.06940.E5A40665; Thu, 3 Dec 2015 14:57:50 +0100 (CET) Received: from [142.133.110.95] (147.117.188.8) by smtp-am.internal.ericsson.com (147.117.188.92) with Microsoft SMTP Server id 14.3.248.2; Thu, 3 Dec 2015 08:58:29 -0500 Subject: Re: [PATCH v4 4/6] Support software single step on ARM in GDBServer. To: Yao Qi References: <1449062264-18565-1-git-send-email-antoine.tremblay@ericsson.com> <1449062264-18565-5-git-send-email-antoine.tremblay@ericsson.com> <86a8pru6hf.fsf@gmail.com> CC: From: Antoine Tremblay Message-ID: <56604A85.5030805@ericsson.com> Date: Thu, 03 Dec 2015 13:58:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <86a8pru6hf.fsf@gmail.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-12/txt/msg00047.txt.bz2 On 12/03/2015 06:17 AM, Yao Qi wrote: > Antoine Tremblay writes: > > Some comments on the design, > >> +/* Context for a get_next_pcs call on ARM. */ >> +struct arm_get_next_pcs >> +{ >> + /* Operations implementations. */ >> + struct arm_get_next_pcs_ops *ops; > >> + /* Byte order for data. */ >> + int byte_order; >> + /* Byte order for code. */ >> + int byte_order_for_code; >> + /* Is the pc in thumb mode. */ >> + int is_thumb; >> + /* Use 32bit or 26 bit pc. */ >> + int arm_apcs_32; >> + /* Thumb2 breakpoint instruction. */ >> + const gdb_byte *arm_thumb2_breakpoint; > > These fields are GDB specific, GDBserver doesn't need them at all. > Can we move them to arm_gdb_get_next_pcs? Field is_thumb is used in > both sides, but can't we compute it in two sides (through arm_is_thumb > and arm_is_thumb_mode) respectively, rather than having a field here? > byte_order fields seemed like a good idea at first and I liked your suggested change for read_memory_unsigned_integer. However GDB is using 2 byte orders : byte_order (for data) and byte_order_for_code to support BE8 endianness. This complicates things a bit since in common code I can't call: self->ops->read_memory_unsigned_integer (self, loc, 2) I would have no way to specify if it should read with byte_order or with byte_order_for_code. So unfortunately these need to stay in the common struct. is_thumb: OK it will add an operation in arm_get_next_pcs_ops but that's fine. arm_apcs_32: I can move the apcs32 check on GDB's side indeed. const gdb_byte *arm_thumb2_breakpoint: This one needs to stay, since while on GDB's side it could be computed through regcache/gdbarch, on GDBServer's side it's directly a variable. Still removing arm_apcs_32 and is_thumbs simplifies things thanks ! >> + struct regcache *regcache; >> +}; >> + > >> +/* get_next_pcs operations. */ >> +struct arm_get_next_pcs_ops >> +{ >> + ULONGEST (*read_memory_unsigned_integer) (CORE_ADDR memaddr, >> + int len, >> + int byte_order); > > We need argument struct arm_get_next_pcs *self, and get rid of argument > byte_order, which can be got through self. > See above answer about byte_order fields. >> + CORE_ADDR (*syscall_next_pc) (struct arm_get_next_pcs *self, CORE_ADDR pc); >> + CORE_ADDR (*addr_bits_remove) (struct arm_get_next_pcs *self, CORE_ADDR val); >> +}; > >> +/* Context for a get_next_pcs call on ARM in GDB. */ >> +struct arm_gdb_get_next_pcs >> +{ >> + /* Common context for gdb/gdbserver. */ >> + struct arm_get_next_pcs base; >> + /* Frame information. */ >> + struct frame_info *frame; > > FRAME is still used in arm_get_next_pcs_syscall_next_pc, but we should > use regcache instead of frame there. Then we can remove frame here. > I answer this in patch 3. >> + /* Architecture dependent information. */ >> + struct gdbarch *gdbarch; > > Is gdbarch used? > >> +}; > No indeed I forgot to clean that up, fixed.