From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 91992 invoked by alias); 7 Dec 2015 15:58:25 -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 91691 invoked by uid 89); 7 Dec 2015 15:58:24 -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; Mon, 07 Dec 2015 15:58:17 +0000 Received: from EUSAAHC002.ericsson.se (Unknown_Domain [147.117.188.78]) by usplmg20.ericsson.net (Symantec Mail Security) with SMTP id 86.90.06940.C3CA5665; Mon, 7 Dec 2015 16:56:44 +0100 (CET) Received: from [142.133.110.95] (147.117.188.8) by smtp-am.internal.ericsson.com (147.117.188.80) with Microsoft SMTP Server id 14.3.248.2; Mon, 7 Dec 2015 10:58:14 -0500 Subject: Re: [PATCH v6 5/7] Support software single step on ARM in GDBServer. To: Yao Qi References: <1449254773-19019-1-git-send-email-antoine.tremblay@ericsson.com> <1449254773-19019-6-git-send-email-antoine.tremblay@ericsson.com> <8637ves2xp.fsf@gmail.com> CC: From: Antoine Tremblay Message-ID: <5665AC96.1090808@ericsson.com> Date: Mon, 07 Dec 2015 15: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: <8637ves2xp.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/msg00139.txt.bz2 On 12/07/2015 10:17 AM, Yao Qi wrote: > Antoine Tremblay writes: > >> gdb/ChangeLog: >> * Makefile.in: Add arm-get-next-pcs.c/o, arm-linux.c/o. >> to all arm targets. > > Please describe how Makefile.in is changed, as I suggested in previous patch. > OK. >> * aarch32-linux-nat.c: Include arch/arm-get-next-pcs.h. >> * arch/arm-get-next-pcs.c: New file. >> * arch/arm-get-next-pcs.h: New file. >> * arch/arm-linux.h: New file. >> * arch/arm-linux.c: New file. >> * arm-linux-nat.c: Include arch/arm.h, arch/arm-get-next-pcs.h >> arch/arm-linux.h. >> * arm-linux-tdep.c: Include arch/arm.h,arch/arm-get-next-pcs.h >> arch/arm-linux.h. >> (arm_linux_get_next_pcs_ops): New struct. >> (ARM_SIGCONTEXT_R0, ARM_UCONTEXT_SIGCONTEXT, >> ARM_OLD_RT_SIGFRAME_SIGINFO, ARM_OLD_RT_SIGFRAME_UCONTEXT, >> ARM_NEW_RT_SIGFRAME_UCONTEXT, ARM_NEW_SIGFRAME_MAGIC): Move stack >> layout defines to arch/arm-linux.h. >> (arm_linux_sigreturn_next_pc_offset): Move to arch/arm-linux.c. >> (arm_linux_software_single_step): Adjust for >> arm_get_next_pcs implementation. >> * arm-tdep.c: Include arch/arm-get-next-pcs.h. >> (arm_get_next_pcs_ops): New struct. >> (submask): Move macro to arm-get-next-pcs.h. >> (bit): Likewise. >> (bits): Likewise. >> (sbits): Likewise. >> (BranchDest): Likewise. > >> (thumb_instruction_changes_pc): Move to arm-get-next-pcs.c >> (thumb2_instruction_changes_pc): Likewise. >> (arm_instruction_changes_pc): Likewise. > > I feel they should be moved to arch/arm.c, because they are not about > getting the next pcs. > OK. I went at first with the idea that if they were only used in get_next_pc to leave them with it as the criteria since I was not sure where to draw the line. But these do feel more generic. >> (shifted_reg_val): Likewise. >> (thumb_advance_itstate): Likewise. > > and this one. > OK. >> (thumb_get_next_pc_raw): Likewise. >> (arm_get_next_pc_raw): Likewise. >> (arm_get_next_pc): Likewise. >> (thumb_deal_with_atomic_sequence_raw): Likewise. >> (arm_deal_with_atomic_sequence_raw): Likewise. >> (arm_deal_with_atomic_sequence): Likewise. >> (arm_get_next_pcs_read_memory_unsigned_integer): New function. >> (arm_get_next_pcs_addr_bits_remove): Likewise. >> (arm_get_next_pcs_syscall_next_pc): Likewise. >> (arm_get_next_pcs_is_thumb): Likewise. >> (arm_software_single_step): Adjust for arm_get_next_pcs >> implementation. >> * arm-tdep.h: (arm_get_next_pc): Remove declaration. >> (arm_get_next_pcs_read_memory_unsigned_integer): >> New declaration. >> (arm_get_next_pcs_addr_bits_remove): Likewise. >> (arm_get_next_pcs_syscall_next_pc): Likewise. >> (arm_software_single_step): Likewise. >> (arm_deal_with_atomic_sequence: Remove declaration. >> (arm_get_next_pcs_is_thumb): New delcaration. >> * arm-wince-tdep.c: Include arch/arm-get-next-pcs.h. >> * armbsd-tdep.c: Likewise. >> * armnbsd-tdep.c: Likewise. >> * armobsd-tdep.c: Likewise. > > Why do we need to include arm-get-next-pcs.h in these *-tdep.c files? > We don't now, some defines were needed before, I forgot to clean that up sorry. >> * common/gdb_vecs.h: Add CORE_ADDR vector definition. >> * configure.tgt: Add arm-get-next-pcs.o, arm-linux.o to all >> relevent arm targets. >> * symtab.h: Move CORE_ADDR vector definition to gdb_vecs.h. >> >> gdb/gdbserver/ChangeLog: >> * Makefile.in : Add arm-get-next-pcs.c/o, arm-linux.c/o. > > Please describe what are changed. > OK. >> * configure.srv: Add arm-get-next-pcs.o, arm-linux.o >> to needed arm targets. >> * linux-arm-low.c: Include arch/arm-linux.h, >> aarch/arm-get-next-pcs.h. >> (get_next_pcs_ops): New struct. >> (get_next_pcs_addr_bits_remove): New function. >> (get_next_pcs_is_thumb): New function. >> (get_next_pcs_read_memory_unsigned_integer): Likewise. >> (arm_sigreturn_next_pc): Likewise. >> (get_next_pcs_syscall_next_pc): Likewise. >> (arm_gdbserver_get_next_pcs): Likewise. >> (struct linux_target_ops) : >> Initialize. >> * linux-low.h: Move CORE_ADDR vector definition to gdb_vecs.h. >> * server.h: Include gdb_vecs.h. >> --- >> gdb/Makefile.in | 16 +- >> gdb/aarch32-linux-nat.c | 1 + >> gdb/arch/arm-get-next-pcs.c | 1216 +++++++++++++++++++++++++++++++++ >> gdb/arch/arm-get-next-pcs.h | 95 +++ >> gdb/arch/arm-linux.c | 59 ++ >> gdb/arch/arm-linux.h | 73 ++ >> gdb/arm-linux-nat.c | 2 + >> gdb/arm-linux-tdep.c | 136 ++-- >> gdb/arm-tdep.c | 1495 +++++------------------------------------ >> gdb/arm-tdep.h | 18 +- >> gdb/arm-wince-tdep.c | 1 + >> gdb/armbsd-tdep.c | 1 + >> gdb/armnbsd-tdep.c | 1 + >> gdb/armobsd-tdep.c | 1 + >> gdb/common/gdb_vecs.h | 2 + >> gdb/configure.tgt | 20 +- >> gdb/gdbserver/Makefile.in | 9 +- >> gdb/gdbserver/configure.srv | 2 + >> gdb/gdbserver/linux-arm-low.c | 144 +++- >> gdb/gdbserver/linux-low.h | 2 - >> gdb/gdbserver/server.h | 1 + >> gdb/symtab.h | 2 - >> 22 files changed, 1871 insertions(+), 1426 deletions(-) >> create mode 100644 gdb/arch/arm-get-next-pcs.c >> create mode 100644 gdb/arch/arm-get-next-pcs.h >> create mode 100644 gdb/arch/arm-linux.c >> create mode 100644 gdb/arch/arm-linux.h >> > > > >> + >> +/* Checks for an atomic sequence of instructions beginning with a LDREX{,B,H,D} >> + instruction and ending with a STREX{,B,H,D} instruction. If such a sequence >> + is found, attempt to step through it. The end of the sequence address is >> + added to the next_pcs list. */ >> + >> +static int >> +thumb_deal_with_atomic_sequence_raw (struct arm_get_next_pcs *self, >> + CORE_ADDR pc, >> + VEC (CORE_ADDR) **next_pcs) > > We can return VEC (CORE_ADDR) * here, if it is NULL, the sequence is not > found, otherwise, the end of the sequence addresses are returned in the vector. > OK. >> + >> +/* Checks for an atomic sequence of instructions beginning with a LDREX{,B,H,D} >> + instruction and ending with a STREX{,B,H,D} instruction. If such a sequence >> + is found, attempt to step through it. The end of the sequence address is >> + added to the next_pcs list. */ >> + >> +static int >> +arm_deal_with_atomic_sequence_raw (struct arm_get_next_pcs *self, >> + CORE_ADDR pc, >> + VEC (CORE_ADDR) **next_pcs) > > Likewise. > >> + >> +/* See arm-get-next-pcs.h. */ >> + >> +VEC (CORE_ADDR) * >> +arm_get_next_pcs (struct arm_get_next_pcs *self, CORE_ADDR pc) >> +{ >> + VEC (CORE_ADDR) *next_pcs = NULL; >> + >> + if (self->ops->is_thumb (self)) >> + { >> + if (!thumb_deal_with_atomic_sequence_raw (self, pc, &next_pcs)) >> + return thumb_get_next_pcs_raw (self, pc, &next_pcs); > > Why do we need to pass next_pcs to thumb_get_next_pcs_raw? Isn't > simpler to write it like this, > > next_pcs = thumb_deal_with_atomic_sequence_raw (self, pc); > if (next_pcs == NULL) > next_pcs = thumb_get_next_pcs_raw (self, pc); > Indeed, I was focused on not losing the VEC allocation/resize and forgot thumb_deal_with_atomic_sequence_raw and thumb_get_next_pcs_raw will not combine their vectors. Thanks. >> + } >> + else >> + { >> + if (!arm_deal_with_atomic_sequence_raw (self, pc, &next_pcs)) >> + return arm_get_next_pcs_raw (self, pc, &next_pcs); >> + } >> + >> + return next_pcs; >> +} > >> diff --git a/gdb/arch/arm-get-next-pcs.h b/gdb/arch/arm-get-next-pcs.h >> new file mode 100644 >> index 0000000..39921e0 >> --- /dev/null >> +++ b/gdb/arch/arm-get-next-pcs.h >> @@ -0,0 +1,95 @@ >> +/* Common code for ARM software single stepping support. >> + >> + Copyright (C) 1988-2015 Free Software Foundation, Inc. >> + >> + This file is part of GDB. >> + >> + This program is free software; you can redistribute it and/or modify >> + it under the terms of the GNU General Public License as published by >> + the Free Software Foundation; either version 3 of the License, or >> + (at your option) any later version. >> + >> + This program is distributed in the hope that it will be useful, >> + but WITHOUT ANY WARRANTY; without even the implied warranty of >> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + GNU General Public License for more details. >> + >> + You should have received a copy of the GNU General Public License >> + along with this program. If not, see . */ >> + >> +#ifndef ARM_GET_NEXT_PCS_H >> +#define ARM_GET_NEXT_PCS_H 1 >> + >> +/* Support routines for instruction parsing. */ >> +#define submask(x) ((1L << ((x) + 1)) - 1) >> +#define bits(obj,st,fn) (((obj) >> (st)) & submask ((fn) - (st))) >> +#define bit(obj,st) (((obj) >> (st)) & 1) >> +#define sbits(obj,st,fn) \ >> + ((long) (bits(obj,st,fn) | ((long) bit(obj,fn) * ~ submask (fn - st)))) >> +#define BranchDest(addr,instr) \ >> + ((CORE_ADDR) (((unsigned long) (addr)) + 8 + (sbits (instr, 0, 23) << 2))) >> + > > These macros should be moved to arch/arm.h. > OK.