From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 130258 invoked by alias); 26 Nov 2015 13:35:52 -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 130231 invoked by uid 89); 26 Nov 2015 13:35:52 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.7 required=5.0 tests=AWL,BAYES_50,SPF_PASS,UNSUBSCRIBE_BODY autolearn=no version=3.3.2 X-HELO: usplmg21.ericsson.net Received: from usplmg21.ericsson.net (HELO usplmg21.ericsson.net) (198.24.6.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Thu, 26 Nov 2015 13:35:50 +0000 Received: from EUSAAHC007.ericsson.se (Unknown_Domain [147.117.188.93]) by usplmg21.ericsson.net (Symantec Mail Security) with SMTP id 77.B5.32102.2BA07565; Thu, 26 Nov 2015 14:35:46 +0100 (CET) Received: from [142.133.110.95] (147.117.188.8) by smtp-am.internal.ericsson.com (147.117.188.95) with Microsoft SMTP Server id 14.3.248.2; Thu, 26 Nov 2015 08:35:46 -0500 Subject: Re: [PATCH v3 08/10] Support software single step on ARM in GDBServer. To: Pedro Alves , References: <1448287968-12907-1-git-send-email-antoine.tremblay@ericsson.com> <1448287968-12907-9-git-send-email-antoine.tremblay@ericsson.com> <5656E3AC.1010000@redhat.com> From: Antoine Tremblay Message-ID: <56570AB2.104@ericsson.com> Date: Thu, 26 Nov 2015 13:35:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <5656E3AC.1010000@redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-11/txt/msg00557.txt.bz2 On 11/26/2015 05:49 AM, Pedro Alves wrote: > 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) > Fixed. >> >> 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? Yes I think it could, I'll see if I can reproduce that. > > I wonder whether this ends up being a regression, or whether it only trigger > on new features, like tracepoints? That was my initial assessment but I think it could be a regression indeed. 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? > It will lose control, Yao has suggested a solution, I will look into it. >> + /* 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. > Fixed. and in the other place in the file too. >> + >> + 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. > Ok. >> + >> + 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; >> } > Ok fixed other occurrences too. > >> /* 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. */ > > Done. >> +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. > Wow :( Indeed thx. > I think this shows that it'd be good to run the series against the > testsuite on native ARM. I do run that all the time but on linux, it turns out I made the misstake in arm-tdep.c but not in arm-linux-tdep.c so it did not show up in my tests. Thanks for seeing it! > > You could avoid these wrong casts by writting "&next_pcs_ctx.base" instead. > Indeed fixing like so all occurrences. >> + 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. > Fixed. >> + >> + VEC_free (CORE_ADDR, next_pcs); > > Use a cleanup instead. Fixed. > >> + >> + 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. > Ok, thanks for the example. >> + 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. > Done. Note I also used a cleanup now in install_software_single_step_breakpoint and removed the braces in the for. Thank you, Antoine