From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24724 invoked by alias); 29 Sep 2015 11:38:29 -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 24708 invoked by uid 89); 29 Sep 2015 11:38:28 -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: usevmg21.ericsson.net Received: from usevmg21.ericsson.net (HELO usevmg21.ericsson.net) (198.24.6.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Tue, 29 Sep 2015 11:38:26 +0000 Received: from EUSAAHC006.ericsson.se (Unknown_Domain [147.117.188.90]) by usevmg21.ericsson.net (Symantec Mail Security) with SMTP id 3A.B6.26730.A9C0A065; Tue, 29 Sep 2015 05:59:22 +0200 (CEST) Received: from [142.133.110.95] (147.117.188.8) by smtp-am.internal.ericsson.com (147.117.188.92) with Microsoft SMTP Server id 14.3.248.2; Tue, 29 Sep 2015 07:38:23 -0400 Subject: Re: [PATCH 3/5] Add support for ARM breakpoint types in GDBServer. To: Yao Qi References: <1442577749-6650-1-git-send-email-antoine.tremblay@ericsson.com> <1442577749-6650-4-git-send-email-antoine.tremblay@ericsson.com> <8637xy96qx.fsf@gmail.com> <5609B069.6010200@ericsson.com> <86pp117hio.fsf@gmail.com> CC: From: Antoine Tremblay Message-ID: <560A782F.4010303@ericsson.com> Date: Tue, 29 Sep 2015 11:38:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <86pp117hio.fsf@gmail.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-09/txt/msg00627.txt.bz2 On 09/29/2015 04:32 AM, Yao Qi wrote: > Antoine Tremblay writes: > >> This will contain more stuff as I post the next patch sets for single >> stepping etc.. I would like to keep a more general name. >> >> It's basically all coming from arm-tdep.c but I don't want to name it >> common/arm-tdep.c to avoid confusion and Makefile problems. >> >> arm-ctdep.c ? >> > > How about arm.c? > There's already arm.h in there I think I can actually merge what is in there with what I have, and create arm.c. >>> Please move this file to arch/ directory rather than common/ >> >> It seems weird to me to have common objects somewhere else then in >> common/ , if common becomes more a lib we don't want it all over the >> place no ? >> >> Would creating a common/arch be acceptable ? I guess we would have to >> move things like x86-xstate.h etc.. there too ? > > common/ was created in order to share code between GDB and GDBserver, see > https://sourceware.org/gdb/wiki/Common After that, we find that we'll > end up with moving most of gdb files into common/ and we'd better move > different common things into different common directory. Nowadays, we > have nat/ common/ and arch/ directories for common things. arm.c should > be put in arch/ directory. Ok I understand thanks for explaining. > >>>> diff --git a/gdb/configure.tgt b/gdb/configure.tgt >>>> index c42b4df..e831f59 100644 >>>> --- a/gdb/configure.tgt >>>> +++ b/gdb/configure.tgt >>>> @@ -89,7 +89,7 @@ arm*-wince-pe | arm*-*-mingw32ce*) >>>> ;; >>>> arm*-*-linux*) >>>> # Target: ARM based machine running GNU/Linux >>>> - gdb_target_obs="arm-tdep.o arm-linux-tdep.o glibc-tdep.o \ >>>> + gdb_target_obs="arm-common.o arm-tdep.o arm-linux-tdep.o glibc-tdep.o \ >>>> solib-svr4.o symfile-mem.o linux-tdep.o linux-record.o" >>>> build_gdbserver=yes >>> >>> since arm-common.o is moved out of arm-tdep.o, so it should be added to >>> every target which uses arm-tdep.o, such as aarch64*-*-linux*, >>> arm*-wince-pe, etc. >> >> Even if they don't use it ? But ok. >> > > No, they use it. This patch moves thumb_insn_size to arm-common.o, but > thumb_insn_size is used by arm-tdep.c. If we don't add arm-common.o to > every target which uses arm-tdep.o, the build will fail on these targets > > build/gdb/aarch64/gdb/../../../../binutils-gdb/gdb/arm-tdep.c:846: undefined reference to `thumb_insn_size' > build/gdb/aarch64/gdb/../../../../binutils-gdb/gdb/arm-tdep.c:846: undefined reference to `thumb_insn_size' > arm-tdep.o: In function `arm_adjust_breakpoint_address': > build/gdb/aarch64/gdb/../../../../binutils-gdb/gdb/arm-tdep.c:5417: undefined reference to `thumb_insn_size' > build/gdb/aarch64/gdb/../../../../binutils-gdb/gdb/arm-tdep.c:5467: undefined reference to `thumb_insn_size' > arm-tdep.o: In function `arm_breakpoint_from_pc': > build/gdb/aarch64/gdb/../../../../binutils-gdb/gdb/arm-tdep.c:8858: undefined reference to `thumb_insn_size' > Indeed, thanks >>> >>>> diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c >>>> index 367c704..15ecb70 100644 >>>> --- a/gdb/gdbserver/linux-arm-low.c >>>> +++ b/gdb/gdbserver/linux-arm-low.c >>>> @@ -30,6 +30,8 @@ >>>> #include "nat/gdb_ptrace.h" >>>> #include >>>> >>>> +#include "common/arm-common.h" >>>> + >>>> /* Defined in auto-generated files. */ >>>> void init_registers_arm (void); >>>> extern const struct target_desc *tdesc_arm; >>>> @@ -234,19 +236,28 @@ arm_set_pc (struct regcache *regcache, CORE_ADDR pc) >>>> } >>>> >>>> /* Correct in either endianness. */ >>>> -static const unsigned long arm_breakpoint = 0xef9f0001; >>>> -#define arm_breakpoint_len 4 >>>> -static const unsigned short thumb_breakpoint = 0xde01; >>>> -static const unsigned short thumb2_breakpoint[] = { 0xf7f0, 0xa000 }; >>>> +#define arm_abi_breakpoint 0xef9f0001UL >>>> >>>> /* For new EABI binaries. We recognize it regardless of which ABI >>>> is used for gdbserver, so single threaded debugging should work >>>> OK, but for multi-threaded debugging we only insert the current >>>> ABI's breakpoint instruction. For now at least. */ >>>> -static const unsigned long arm_eabi_breakpoint = 0xe7f001f0; >>>> +#define arm_eabi_breakpoint 0xe7f001f0UL >>>> + >>>> +#ifndef __ARM_EABI__ >>>> +static const unsigned long arm_breakpoint = arm_abi_breakpoint; >>>> +#else >>>> +static const unsigned long arm_breakpoint = arm_eabi_breakpoint; >>>> +#endif >>>> + >>>> +#define arm_breakpoint_len 4 >>>> +static const unsigned short thumb_breakpoint = 0xde01; >>>> +#define thumb_breakpoint_len 2 >>>> +static const unsigned short thumb2_breakpoint[] = { 0xf7f0, 0xa000 }; >>>> +#define thumb2_breakpoint_len 4 >>> >>> I am confused by your changes here. Why do you change them? >>> >> >> Before arm_breakpoint_from_pc would be : >> >> #ifndef __ARM_EABI__ >> return (const unsigned char *) &arm_breakpoint; >> #else >> - return (const unsigned char *) &arm_eabi_breakpoint; >> #endif >> >> And arm_breakpoint_at would check for the arm_breakpoint || >> arm_eabi_breakpoint. >> >> Thus the selected arm_breakpoint would be what that function returned >> and arm_breakpoint was arm_abi_breakpoint. >> >> It felt more clear to me to name those for what they are : 2 separate >> entities arm_abi_breakpoint and arm_eabi_breakpoint and set the >> current used one as arm_breakpoint. >> >> This allows a cleaner breakpoint_from_pc as it just returns >> arm_breakpoint rather than having the #ifdef in that function. >> >> Any other reference to the arm_breakpoint would also be clear of #ifdefs... > > Please don't combine movement and refactoring. This patch is about > movement, so let us do movement only. Refactor can be done separately. > Ok , would you find it acceptable still to include the refactoring in the series as the next patch or should I create it out of series ?