From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 38798 invoked by alias); 29 Sep 2015 08:32: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 38786 invoked by uid 89); 29 Sep 2015 08:32:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-pa0-f49.google.com Received: from mail-pa0-f49.google.com (HELO mail-pa0-f49.google.com) (209.85.220.49) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Tue, 29 Sep 2015 08:32:27 +0000 Received: by padhy16 with SMTP id hy16so200316543pad.1 for ; Tue, 29 Sep 2015 01:32:25 -0700 (PDT) X-Received: by 10.68.191.200 with SMTP id ha8mr31800485pbc.72.1443515545242; Tue, 29 Sep 2015 01:32:25 -0700 (PDT) Received: from E107787-LIN (power-aix.osuosl.org. [140.211.15.154]) by smtp.gmail.com with ESMTPSA id u1sm23932698pbz.56.2015.09.29.01.32.21 (version=TLS1_2 cipher=AES128-SHA256 bits=128/128); Tue, 29 Sep 2015 01:32:24 -0700 (PDT) From: Yao Qi To: Antoine Tremblay Cc: Yao Qi , Subject: Re: [PATCH 3/5] Add support for ARM breakpoint types in GDBServer. 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> Date: Tue, 29 Sep 2015 08:32:00 -0000 In-Reply-To: <5609B069.6010200@ericsson.com> (Antoine Tremblay's message of "Mon, 28 Sep 2015 17:26:01 -0400") Message-ID: <86pp117hio.fsf@gmail.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2015-09/txt/msg00622.txt.bz2 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? >> 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. >>> 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=3D"arm-tdep.o arm-linux-tdep.o glibc-tdep.o \ >>> + gdb_target_obs=3D"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=3Dyes >> >> 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: undefine= d reference to `thumb_insn_size' build/gdb/aarch64/gdb/../../../../binutils-gdb/gdb/arm-tdep.c:846: undefine= d 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: undefin= ed reference to `thumb_insn_size' build/gdb/aarch64/gdb/../../../../binutils-gdb/gdb/arm-tdep.c:5467: undefin= ed 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: undefin= ed reference to `thumb_insn_size' >> >>> diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-lo= w.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 =3D 0xef9f0001; >>> -#define arm_breakpoint_len 4 >>> -static const unsigned short thumb_breakpoint =3D 0xde01; >>> -static const unsigned short thumb2_breakpoint[] =3D { 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 =3D 0xe7f001f0; >>> +#define arm_eabi_breakpoint 0xe7f001f0UL >>> + >>> +#ifndef __ARM_EABI__ >>> +static const unsigned long arm_breakpoint =3D arm_abi_breakpoint; >>> +#else >>> +static const unsigned long arm_breakpoint =3D arm_eabi_breakpoint; >>> +#endif >>> + >>> +#define arm_breakpoint_len 4 >>> +static const unsigned short thumb_breakpoint =3D 0xde01; >>> +#define thumb_breakpoint_len 2 >>> +static const unsigned short thumb2_breakpoint[] =3D { 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. --=20 Yao (=E9=BD=90=E5=B0=A7)