From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 102735 invoked by alias); 26 Nov 2015 10:49:22 -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 102717 invoked by uid 89); 26 Nov 2015 10:49:21 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,UNSUBSCRIBE_BODY autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 26 Nov 2015 10:49:19 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id 88514461C4; Thu, 26 Nov 2015 10:49:18 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id tAQAnGRb009393; Thu, 26 Nov 2015 05:49:17 -0500 Message-ID: <5656E3AC.1010000@redhat.com> Date: Thu, 26 Nov 2015 10:49:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Antoine Tremblay , gdb-patches@sourceware.org Subject: Re: [PATCH v3 08/10] Support software single step on ARM in GDBServer. References: <1448287968-12907-1-git-send-email-antoine.tremblay@ericsson.com> <1448287968-12907-9-git-send-email-antoine.tremblay@ericsson.com> In-Reply-To: <1448287968-12907-9-git-send-email-antoine.tremblay@ericsson.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-11/txt/msg00544.txt.bz2 On 11/23/2015 02:12 PM, Antoine Tremblay wrote: > In this v3: > > * The common arm_get_next_pcs call has been refactored the same way like > so : VEC (CORE_ADDR) *arm_get_next_pcs (struct arm_get_next_pcs *ctx, > CORE_ADDR pc); > > * Use ctor functions to construct gdb|gdbserver_get_next_pcs context. > > * Some style fixes. > > --- > > This patch teaches GDBserver how to software single step on ARM > linux by sharing code with GDB. > > The arm_get_next_pcs function in GDB is now shared with GDBServer. So that > GDBServer can use the function to return the possible addresses of the next PC. > > A proper shared context was also needed so that we could share the code, this > context is described as this data structure (expressed as a class > hierarchy): > > struct arch_get_next_pcs > struct arch_(gdb|gdbserver)_get_next_pcs > > Where arch can by replaced by arm for this patch. This structure should be > flexible enough to accomodate any arch that would need a get_next_pcs > context. (accommodate) > > Limitations : > > GDBServer can't step over a sigreturn or rt_sigreturn syscall since this would > require the DWARF information to identify the caller PC. This situation > only prints a warning for the moment. I wonder whether this ends up being a regression? E.g., if you put a breakpoint with a condition that evals false, on top of such an instruction? I wonder whether this ends up being a regression, or whether it only trigger on new features, like tracepoints? I'm thinking that it may be a regression for the case of a conditional breakpoint with a conditional evaluated on the target? Other than the warning, what happens? Does gdbserver lose control of the inferior? > + /* Insert a breakpoint right after the end of the atomic sequence. */ > + breaks[0] = loc; > + > + /* Check for duplicated breakpoints. Check also for a breakpoint > + placed (branch instruction's destination) anywhere in sequence. */ > + if (last_breakpoint > + && (breaks[1] == breaks[0] > + || (breaks[1] >= pc && breaks[1] < loc))) > + last_breakpoint = 0; > + > + /* Adds the breakpoints to the list to be inserted. */ > + for (index = 0; index <= last_breakpoint; index++) > + { > + VEC_safe_push (CORE_ADDR, *next_pcs, MAKE_THUMB_ADDR (breaks[index])); > + } No braces. > + > + return 1; > +} > + > + else if (itstate & 0x0f) > + { > + /* We are in a conditional block. Check the condition. */ > + int cond = itstate >> 4; > + > + if (! condition_true (cond, status)) > + /* Advance to the next instruction. All the 32-bit > + instructions share a common prefix. */ > + VEC_safe_push (CORE_ADDR, *next_pcs, > + MAKE_THUMB_ADDR (pc + thumb_insn_size (inst1))); Here there should be braces around the comment and the VEC call. > + > + return *next_pcs; > + > + /* Otherwise, handle the instruction normally. */ > + } > @@ -912,27 +922,44 @@ arm_linux_software_single_step (struct frame_info *frame) > { > struct gdbarch *gdbarch = get_frame_arch (frame); > struct address_space *aspace = get_frame_address_space (frame); > - CORE_ADDR next_pc; > - > - if (arm_deal_with_atomic_sequence (frame)) > - return 1; > + struct arm_gdb_get_next_pcs next_pcs_ctx; > + CORE_ADDR pc; > + int i; > + VEC (CORE_ADDR) *next_pcs = NULL; > > /* If the target does have hardware single step, GDB doesn't have > to bother software single step. */ > if (target_can_do_single_step () == 1) > return 0; > > - next_pc = arm_get_next_pc (frame, get_frame_pc (frame)); > + arm_gdb_get_next_pcs_ctor (&next_pcs_ctx, > + &arm_linux_get_next_pcs_ops, > + gdbarch_byte_order (gdbarch), > + gdbarch_byte_order_for_code (gdbarch), > + arm_frame_is_thumb (frame), > + arm_apcs_32, > + gdbarch_tdep (gdbarch)->thumb2_breakpoint, > + frame, > + gdbarch); > > - /* The Linux kernel offers some user-mode helpers in a high page. We can > - not read this page (as of 2.6.23), and even if we could then we couldn't > - set breakpoints in it, and even if we could then the atomic operations > - would fail when interrupted. They are all called as functions and return > - to the address in LR, so step to there instead. */ > - if (next_pc > 0xffff0000) > - next_pc = get_frame_register_unsigned (frame, ARM_LR_REGNUM); > + next_pcs = arm_get_next_pcs ((struct arm_get_next_pcs *) &next_pcs_ctx, > + get_frame_pc (frame)); > + > + for (i = 0; VEC_iterate (CORE_ADDR, next_pcs, i, pc); i++) > + { > + /* The Linux kernel offers some user-mode helpers in a high page. We can > + not read this page (as of 2.6.23), and even if we could then we > + couldn't set breakpoints in it, and even if we could then the atomic > + operations would fail when interrupted. They are all called as > + functions and return to the address in LR, so step to there > + instead. */ > + if (pc > 0xffff0000) > + pc = get_frame_register_unsigned (frame, ARM_LR_REGNUM); > + > + arm_insert_single_step_breakpoint (gdbarch, aspace, pc); > + } > > - arm_insert_single_step_breakpoint (gdbarch, aspace, next_pc); > + VEC_free (CORE_ADDR, next_pcs); This would be better freed with a cleanup: VEC (CORE_ADDR) *next_pcs = NULL; old_chain = make_cleanup (VEC_cleanup (CORE_ADDR), next_pcs); ... do_cleanups (old_chain); > > return 1; > } > /* There's a lot of code before this instruction. Start with an > optimistic search; it's easy to recognize halfwords that can > @@ -7291,6 +6106,116 @@ thumb2_copy_block_xfer (struct gdbarch *gdbarch, uint16_t insn1, uint16_t insn2, > return 0; > } > > +/* Initialize arm_gdb_get_next_pcs_stor. */ /* Initialize arm_gdb_get_next_pcs. */ > +void > +arm_gdb_get_next_pcs_ctor (struct arm_gdb_get_next_pcs *self, > + struct arm_get_next_pcs_ops *ops, > + int byte_order, > +/* single_step() is called just before we want to resume the inferior, > + if we want to single-step it but there is no hardware or kernel > + single-step support. We find the target of the coming instructions > + and breakpoint them. */ > + > +int > +arm_software_single_step (struct frame_info *frame) > +{ > + struct gdbarch *gdbarch = get_frame_arch (frame); > + struct address_space *aspace = get_frame_address_space (frame); > + struct arm_gdb_get_next_pcs next_pcs_ctx; > + CORE_ADDR pc; > + int i; > + VEC (CORE_ADDR) *next_pcs = NULL; > + > + arm_gdb_get_next_pcs_ctor (&next_pcs_ctx, > + &arm_get_next_pcs_ops, > + gdbarch_byte_order (gdbarch), > + gdbarch_byte_order_for_code (gdbarch), > + arm_frame_is_thumb (frame), > + arm_apcs_32, > + gdbarch_tdep (gdbarch)->thumb2_breakpoint, > + frame, > + gdbarch); > + > + next_pcs = arm_get_next_pcs ((struct arm_get_next_pcs *) &next_pcs, This is wrong. Should be "(struct arm_get_next_pcs *) &next_pcs_ctx" instead. I think this shows that it'd be good to run the series against the testsuite on native ARM. You could avoid these wrong casts by writting "&next_pcs_ctx.base" instead. > + get_frame_pc (frame)); > + > + for (i = 0; VEC_iterate (CORE_ADDR, next_pcs, i, pc); i++) > + { > + arm_insert_single_step_breakpoint (gdbarch, aspace, pc); > + } No braces. > + > + VEC_free (CORE_ADDR, next_pcs); Use a cleanup instead. > + > + return 1; > +} > + > /* Cleanup/copy SVC (SWI) instructions. These two functions are overridden > for Linux, where some SVC instructions must be treated specially. */ > > diff --git a/gdb/arm-tdep.h b/gdb/arm-tdep.h > index 9b8447b..f3a13a4 100644 > --- a/gdb/arm-tdep.h > +++ b/gdb/arm-tdep.h > @@ -23,6 +23,9 @@ > struct gdbarch; > struct regset; > struct address_space; > +struct get_next_pcs; > +struct arm_get_next_pcs; > +struct gdb_get_next_pcs; > > #include "arch/arm.h" > > @@ -221,6 +224,17 @@ struct displaced_step_closure > struct displaced_step_closure *); > }; > > +/* 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; > + /* Architecture dependent information. */ > + struct gdbarch *gdbarch; > +}; > + > /* Values for the WRITE_PC argument to displaced_write_reg. If the register > write may write to the PC, specifies the way the CPSR T bit, etc. is > modified by the instruction. */ > @@ -250,11 +264,37 @@ extern void > ULONGEST val, enum pc_write_style write_pc); > > CORE_ADDR arm_skip_stub (struct frame_info *, CORE_ADDR); > -CORE_ADDR arm_get_next_pc (struct frame_info *, CORE_ADDR); > + > +ULONGEST arm_get_next_pcs_read_memory_unsigned_integer (CORE_ADDR memaddr, > + int len, > + int byte_order); > + > +void arm_gdb_get_next_pcs_ctor (struct arm_gdb_get_next_pcs *self, > + struct arm_get_next_pcs_ops *ops, > + int byte_order, > + int byte_order_for_code, > + int is_thumb, > + int arm_apcs_32, > + const gdb_byte *arm_thumb2_breakpoint, > + struct frame_info *frame, > + struct gdbarch *gdbarch); > + > +CORE_ADDR > +arm_get_next_pcs_addr_bits_remove (struct arm_get_next_pcs *self, See extension.h for how to indent these long declarations. The function name should not be at column 0. > + CORE_ADDR val); > + > +ULONGEST > +arm_get_next_pcs_collect_register_unsigned (struct arm_get_next_pcs *self, > + int n); > + > +CORE_ADDR > +arm_get_next_pcs_syscall_next_pc (struct arm_get_next_pcs *self, CORE_ADDR pc); > + > +int arm_software_single_step (struct frame_info *frame); > + > + > /* These are in in current kernels. */ > #define HWCAP_VFP 64 > #define HWCAP_IWMMXT 512 > @@ -146,6 +156,29 @@ static int arm_regmap[] = { > 64 > }; > > +/* Forward declarations needed for get_next_pcs ops. */ > +static ULONGEST > +get_next_pcs_read_memory_unsigned_integer (CORE_ADDR memaddr, > + int len, > + int byte_order); > + > +static ULONGEST > +get_next_pcs_collect_register_unsigned (struct arm_get_next_pcs *self, int n); > + > +static CORE_ADDR > +get_next_pcs_addr_bits_remove (struct arm_get_next_pcs *self, CORE_ADDR val); > + > +static CORE_ADDR > +get_next_pcs_syscall_next_pc (struct arm_get_next_pcs *self, CORE_ADDR pc); > + Ditto. Thanks, Pedro Alves