Re: [PATCH v6 5/7] Support software single step on ARM in GDBServer.

Antoine Tremblay
2015-12-07T15:58:25Z

On 12/07/2015 10:17 AM, Yao Qi wrote:
> Antoine Tremblay <antoine.tremblay@ericsson.com> 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) <arm_gdbserver_get_next_pcs>:
>> 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 <http://www.gnu.org/licenses/>. */
>> +
>> +#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.