From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 59030 invoked by alias); 13 Dec 2016 12:24:36 -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 58716 invoked by uid 89); 13 Dec 2016 12:24:35 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-5.0 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: foss.arm.com Received: from foss.arm.com (HELO foss.arm.com) (217.140.101.70) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 13 Dec 2016 12:24:24 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 9A1D4F; Tue, 13 Dec 2016 04:24:23 -0800 (PST) Received: from [10.45.32.207] (e105284-mac.manchester.arm.com [10.45.32.207]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id 841C33F220; Tue, 13 Dec 2016 04:24:22 -0800 (PST) User-Agent: Microsoft-MacOutlook/14.7.0.161029 Date: Tue, 13 Dec 2016 12:24:00 -0000 Subject: Re: [PATCH 2/8] AARCH64 SVE: Add gdbarch methods From: Alan Hayward To: Yao Qi CC: "gdb-patches@sourceware.org" Message-ID: References: <20161212204717.gaw2b77noo3i43h6@localhost> In-Reply-To: <20161212204717.gaw2b77noo3i43h6@localhost> Mime-version: 1.0 Content-type: text/plain; charset="UTF-8" Content-transfer-encoding: quoted-printable X-SW-Source: 2016-12/txt/msg00286.txt.bz2 On 12/12/2016 20:47, "Yao Qi" wrote: >On 16-12-05 12:26:43, Alan Hayward wrote: >> This is part of a series adding AARCH64 SVE support to gdb and >>gdbserver. >>=20 >> This patch simply adds two gdbarch methods: >> target_description_changed_p () >> target_get_tdep_info () > >We need more rationale on why do we add these two new gdbarch methods. > >>=20 >> In a later patch, the aarch64 version of these methods will need to >>access >> regcache as a VEC(cached_reg_t). >>=20 >> These method will remain unused until a later patch in the series. > >Could you add the code using these gdbarch methods in the same patch? >It is odd to add a function in one patch, and use it in another. Agreed that on it=E2=80=99s own this patch looks a little odd. My other option is to combine this and patch 8 together into one patch. I originally tried that but I felt it made the patch too cluttered. The idea of this patch was to contain all the uninteresting boilerplate code that will support the interesting code in the later patches. This same point also applies to patch 3/8 and 4/8. I=E2=80=99ll update the explanations/comments for this patch and 3/8, 4/8. If required I can combine them into the later patches, but I=E2=80=99d rath= er leave them separate. > >>=20 >> Tested on x86 and aarch64. >> Ok to commit? >>=20 >> Alan. >>=20 >>=20 >>=20 >> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c >> index=20 >>=20 >>6b95d7c8654521bb58d3af32d898b23380320915..aba7a8525529102d01e3c7cd282a9ee >>79 >> 3d643fe 100644 >> --- a/gdb/aarch64-tdep.c >> +++ b/gdb/aarch64-tdep.c >> @@ -44,23 +44,17 @@ >> #include "infcall.h" >> #include "ax.h" >> #include "ax-gdb.h" >> - >> #include "aarch64-tdep.h" >> - >> #include "elf-bfd.h" >> #include "elf/aarch64.h" >> - >> #include "vec.h" >> - >> #include "record.h" >> #include "record-full.h" >> - >> #include "features/aarch64.c" >> - >> #include "arch/aarch64-insn.h" >> - > >Is it intended to remove these blank lines? Yes - I was trying to make the file look a little cleaner. I=E2=80=99ll rem= ove them. > >> #include "opcode/aarch64.h" >> #include >> +#include "remote.h" >>=20 > > > >> #define submask(x) ((1L << ((x) + 1)) - 1) >> #define bit(obj,st) (((obj) >> (st)) & 1) >> @@ -2643,6 +2637,24 @@ aarch64_displaced_step_hw_singlestep (struct >> gdbarch *gdbarch, >> return 1; >> } >>=20 >> +/* Implement the "target_description_changed_p" gdbarch method. */ > >A blank line is needed. Ok. > >> +static int >> +aarch64_target_description_changed_p (struct gdbarch *gdbarch, >> + ptid_t ptid, >> + void *registers) >> +{ >> + VEC(cached_reg_t) *registerlist =3D (VEC(cached_reg_t)*)registers; >> + return 0; >> +} >> + >> +/* Implement the "target_get_tdep_info" gdbarch method. */ >> +void * >> +aarch64_target_get_tdep_info (void *registers) >> +{ > >Likewise. Ok. > >> + VEC(cached_reg_t) *regcache =3D (VEC(cached_reg_t)*)registers; >> + return NULL; >> +} >> + >> diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh >> index=20 >>=20 >>ba57008300d2e463f67bab6561f76908f0933dfb..1eaea537a985ddda6208199feabd8d1 >>4e >> 91e837c 100755 >> --- a/gdb/gdbarch.sh >> +++ b/gdb/gdbarch.sh >> @@ -1163,6 +1163,12 @@ m:const char >> *:gnu_triplet_regexp:void:::default_gnu_triplet_regexp::0 >> # each address in memory. >>=20=20 >>m:int:addressable_memory_unit_size:void:::default_addressable_memory_unit >>_ >> size::0 >>=20 >> +# Given a list of registers, check if the target description has >>changed. >> +m:int:target_description_changed_p:ptid_t ptid, void *registers:ptid, >> registers::default_target_description_changed_p::0 >> + >> +# Given a list of registers, return a tdep info. >> +f:void*:target_get_tdep_info:void >> *registers:registers::default_target_get_tdep_info::0 > >More documentation on these two methods are needed. Ok. > >> + >> EOF >> } >>=20 >> diff --git a/gdb/remote.h b/gdb/remote.h >> index=20 >>=20 >>75e7e670ea1cde00897ef2a3c402161b76b9b564..ffd1758efc5e3b7e5244b599b288fa8 >>d0 >> 1c804fe 100644 >> --- a/gdb/remote.h >> +++ b/gdb/remote.h >> @@ -23,6 +23,15 @@ >>=20 >> struct target_desc; >>=20 >> +typedef struct cached_reg >> +{ >> + int num; >> + gdb_byte data[MAX_REGISTER_SIZE]; >> +} cached_reg_t; >> + >> +DEF_VEC_O(cached_reg_t); >> + >> + > >We may need to move this structure to regcache.h, and rename it with >"reg_info" for example. Then, "struct reg_info" in python/py-unwind.c >can be removed. A good side-effect of this change is that we remove one >usage of MAX_REGISTER_SIZE. Ok. > >--=20 >Yao=20