From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 111838 invoked by alias); 26 Nov 2015 15:12:00 -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 111826 invoked by uid 89); 26 Nov 2015 15:12:00 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL,BAYES_00,SPF_PASS autolearn=ham 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 15:11:58 +0000 Received: from EUSAAHC005.ericsson.se (Unknown_Domain [147.117.188.87]) by usplmg21.ericsson.net (Symantec Mail Security) with SMTP id FC.5B.32102.B3127565; Thu, 26 Nov 2015 16:11:55 +0100 (CET) Received: from [142.133.110.95] (147.117.188.8) by smtp-am.internal.ericsson.com (147.117.188.89) with Microsoft SMTP Server id 14.3.248.2; Thu, 26 Nov 2015 10:11:55 -0500 Subject: Re: [PATCH v3 08/10] Support software single step on ARM in GDBServer. To: Yao Qi References: <1448287968-12907-1-git-send-email-antoine.tremblay@ericsson.com> <1448287968-12907-9-git-send-email-antoine.tremblay@ericsson.com> <86ziy1vsdr.fsf@gmail.com> CC: From: Antoine Tremblay Message-ID: <5657213B.30504@ericsson.com> Date: Thu, 26 Nov 2015 15:12: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: <86ziy1vsdr.fsf@gmail.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-11/txt/msg00561.txt.bz2 On 11/26/2015 07:48 AM, Yao Qi wrote: > Antoine Tremblay writes: > >> 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. > > They are using frame not DWARF to identify the PC, however, it is not > necessary. When GDB or GDBserver is doing software single step, it must > be in the inner-most frame, so we can get the caller PC by examining the > stack at a certain offset from SP (from regcache), like what are doing in > arm_linux_sigreturn_init and arm_linux_rt_sigreturn_init, rather than > replying frame unwinding. See my comments to arm_get_next_pcs_syscall_next_pc. > Humm and frame can use the dwarf unwinder can it not ? frame is still very confusing to me. But I'm glad it's not the only option, I'll examine the stack and fix it. >> + >> +/* Wrapper over syscall_next_pc for use in get_next_pcs. */ >> + >> +CORE_ADDR >> +arm_get_next_pcs_syscall_next_pc (struct arm_get_next_pcs *self, CORE_ADDR pc) >> +{ >> + struct arm_gdb_get_next_pcs *next_pcs = (struct arm_gdb_get_next_pcs *) self; >> + struct gdbarch_tdep *tdep; >> + >> + tdep = gdbarch_tdep (next_pcs->gdbarch); >> + if (tdep->syscall_next_pc != NULL) >> + return tdep->syscall_next_pc (next_pcs->frame); >> + >> + return 0; >> +} >> + > > >> +/* When PC is at a syscall instruction, return the PC of the next >> + instruction to be executed. */ >> +static CORE_ADDR >> +get_next_pcs_syscall_next_pc (struct arm_get_next_pcs *self, CORE_ADDR pc) >> +{ >> + CORE_ADDR return_addr = 0; >> + int is_thumb = self->is_thumb; >> + ULONGEST svc_number = 0; >> + >> + if (is_thumb) >> + { >> + svc_number = self->ops->collect_register_unsigned (self, 7); >> + return_addr = pc + 2; >> + } >> + else >> + { >> + unsigned long this_instr = self->ops->read_memory_unsigned_integer >> + ((CORE_ADDR) pc, 4, self->byte_order_for_code); >> + >> + unsigned long svc_operand = (0x00ffffff & this_instr); >> + if (svc_operand) /* OABI. */ >> + { >> + svc_number = svc_operand - 0x900000; >> + } >> + else /* EABI. */ >> + { >> + svc_number = self->ops->collect_register_unsigned (self, 7); >> + } >> + >> + return_addr = pc + 4; >> + } >> + >> + /* Is this a sigreturn or rt_sigreturn syscall? >> + If so it is currently not handeled. */ >> + if (svc_number == 119 || svc_number == 173) >> + { >> + if (debug_threads) >> + debug_printf ("Unhandled sigreturn or rt_sigreturn syscall\n"); > > We can still compute the caller pc by examining stack. > >> + } >> + >> + /* Addresses for calling Thumb functions have the bit 0 set. */ >> + if (is_thumb) >> + return_addr = MAKE_THUMB_ADDR (return_addr); >> + >> + return return_addr; >> +} >> > > This leads me thinking more about current software single step code in > GDB. In GDB side, we are using frame to access registers, while in > GDBserver, we are using regcache. Why don't we use regcache in both > sides? so that the "struct arm_get_next_pcs_ops" can be simplified, for > example, field collect_register_unsigned may be no longer necessary. > > In fact, software_single_step used to have regcache argument, added by > https://sourceware.org/ml/gdb-patches/2007-04/msg00218.html but we > changed to argument frame > https://sourceware.org/ml/gdb-patches/2007-04/msg00218.html This is the same link as the previous one... IMO, it is > better to use regcache than frame. We have two options, > > #1, switch from frame apis to regcache apis to access registers in arm > software single step. We can get regcache by get_current_regcache (). > #2, change argument of gdbarch method software_single_step from frame > to regcache, which means all its implementations need update, and > switch to regcache apis to access registers. > > #2 is the right way to go in long term, and we really need to improve > software_single_step. Let me what do you think. > Looking at the impacts of #2, I do not feel comfortable including these changes in this patch set. I feel they would require a patch set of their own. However #1 seems like something possible I would start by this option if that's fine with you ? Also, I can still do the refactoring before this patch but it will require more work since I'll have to diff the functions moved etc.. do you feel it's required to do so or the refactoring could be done after this patch ? Thank you, Antoine