From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 113116 invoked by alias); 28 Sep 2015 21:26:09 -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 113099 invoked by uid 89); 28 Sep 2015 21:26:07 -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; Mon, 28 Sep 2015 21:26:04 +0000 Received: from EUSAAHC008.ericsson.se (Unknown_Domain [147.117.188.96]) by usevmg21.ericsson.net (Symantec Mail Security) with SMTP id 5D.56.26730.CD449065; Mon, 28 Sep 2015 15:47:08 +0200 (CEST) Received: from [142.133.110.95] (147.117.188.8) by smtp-am.internal.ericsson.com (147.117.188.98) with Microsoft SMTP Server id 14.3.248.2; Mon, 28 Sep 2015 17:26:01 -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> CC: From: Antoine Tremblay Message-ID: <5609B069.6010200@ericsson.com> Date: Mon, 28 Sep 2015 21:26: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: <8637xy96qx.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/msg00616.txt.bz2 On 09/28/2015 06:29 AM, Yao Qi wrote: > Antoine Tremblay writes: > >> gdb/ChangeLog: >> * Makefile.in: Add arm-common.o. >> * arm-tdep.c (thumb_insn_size): Move to arm-common.h/c. > > "Move to arm-common.c". Some macros are moved, we should mention the > move here as well. Done. > >> * common/arm-common.c: New file. >> * common/arm-common.h: New file. > > arm-common.c is not a good file name to me, how about arm-insn.c? 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 ? > 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 ? > >> * configure.tgt: Add arm-common.o. >> * gdbserver/Makefile.in: Add arm-common.c/o. > > ChangeLog entries for GDBserver should be written separately. Oops my bad sorry about that. > >> * gdbserver/configure.srv: Add arm-common.o. >> * gdbserver/linux-arm-low.c (arm_breakpoint_at): Adjust for >> breakpoint types. >> (arm_breakpoint_from_pc): New function. > > arm_breakpoint_from_pc is not a new function, but arm_is_thumb_mode is. > Done. >> diff --git a/gdb/common/arm-common.c b/gdb/common/arm-common.c >> new file mode 100644 >> index 0000000..861b249 >> --- /dev/null >> +++ b/gdb/common/arm-common.c >> @@ -0,0 +1,32 @@ >> +/* Common code for generic ARM support. >> + >> + Copyright (C) 2015 Free Software Foundation, Inc. >> + > > Contents in file are moved from existing file, so we should still keep > the year range of the original file. It should be > > Copyright (C) 1988-2015 Free Software Foundation, Inc. > > OK np >> 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. > >> 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... >> >> static int >> -arm_breakpoint_at (CORE_ADDR where) >> +arm_is_thumb_mode (void) >> { >> struct regcache *regcache = get_thread_regcache (current_thread, 1); >> unsigned long cpsr; >> @@ -254,6 +265,17 @@ arm_breakpoint_at (CORE_ADDR where) >> collect_register_by_name (regcache, "cpsr", &cpsr); >> >> if (cpsr & 0x20) >> + return 1; >> + else >> + return 0; > > Indentation looks odd. Done. > >> +} >> + >> +/* Returns 1 if there is a software breakpoint at location. */ >> + >> +static int >> +arm_breakpoint_at (CORE_ADDR where) >> +{ >> + if (arm_is_thumb_mode ()) >> { >> /* Thumb mode. */ >> unsigned short insn; >> @@ -275,7 +297,7 @@ arm_breakpoint_at (CORE_ADDR where) >> unsigned long insn; >> >> (*the_target->read_memory) (where, (unsigned char *) &insn, 4); >> - if (insn == arm_breakpoint) >> + if (insn == arm_abi_breakpoint) >> return 1; >> >> if (insn == arm_eabi_breakpoint) >> @@ -285,6 +307,53 @@ arm_breakpoint_at (CORE_ADDR where) >> return 0; >> } >> >> +/* Determine the type and size of breakpoint to insert at PCPTR. Uses >> + the program counter value to determine whether a 16-bit or 32-bit >> + breakpoint should be used. It returns a pointer to a string of >> + bytes that encode a breakpoint instruction, stores the length of >> + the string to *lenptr, and adjusts the program counter (if >> + necessary) to point to the actual memory location where the >> + breakpoint should be inserted. */ >> + >> +static const unsigned char * >> +arm_breakpoint_from_pc (CORE_ADDR *pcptr, int *lenptr) >> +{ >> + /* Default if no pc is set to arm breakpoint. */ >> + if (pcptr == NULL) > > Such NULL checking is no longer needed, right? > Indeed this is gone based on comments in patch 1/5.