From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 34660 invoked by alias); 10 Aug 2018 19:25:26 -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 34616 invoked by uid 89); 10 Aug 2018 19:25:18 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.7 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3 autolearn=ham version=3.3.2 spammy=HX-ClientProxiedBy:404 X-HELO: sesbmg22.ericsson.net Received: from sesbmg22.ericsson.net (HELO sesbmg22.ericsson.net) (193.180.251.48) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 10 Aug 2018 19:25:16 +0000 DKIM-Signature: v=1; a=rsa-sha256; d=ericsson.com; s=mailgw201801; c=relaxed/simple; q=dns/txt; i=@ericsson.com; t=1533929114; h=From:Sender:Reply-To:Subject:Date:Message-ID:To:CC:MIME-Version:Content-Type: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:In-Reply-To:References:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=lhleXY4F3wQPo4wmPuCmsRkY4BrPAwXAMiMssUXBx9M=; b=f8quqoa9lB0w9+zU76ZZ5MNahpgcFqBj9rUTC4zGTGaBtJxsCmI+Xo5hFiwCSTle SYsi2CcyaTOD7aNRDI3/A6R7cVRjSKRsEmlubcVpxWABHd0VAm9l3GQiBStiiwTa +lqba6/+WrrneH5dNfxQXW7nGacbtBv7NGmuRmcaihw=; Received: from ESESBMB501.ericsson.se (Unknown_Domain [153.88.183.114]) by sesbmg22.ericsson.net (Symantec Mail Security) with SMTP id 03.C4.21978.A96ED6B5; Fri, 10 Aug 2018 21:25:14 +0200 (CEST) Received: from ESESSMB502.ericsson.se (153.88.183.163) by ESESBMB501.ericsson.se (153.88.183.168) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.1466.3; Fri, 10 Aug 2018 21:25:14 +0200 Received: from NAM02-CY1-obe.outbound.protection.outlook.com (153.88.183.157) by ESESSMB502.ericsson.se (153.88.183.163) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.1466.3 via Frontend Transport; Fri, 10 Aug 2018 21:25:13 +0200 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ericsson.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=HlFLtJlFR9MFWKm5FF7LcXj8ateXCZ5cgRH+fU9SilI=; b=PNN+QOcejNyKaPxO/8hQ76fVDy4FDmCOhHhi5o+iHB777TVcwJyeBl4HFmnysnXfd5jlDYZdZc2/nBJNvNu6NzwJCu8UfE8WT1GJHk6kUghuJuFmDjzz4xDEvvYFGN7JWL31MreBmJhek+9sv/0ezYCrFBo5CU57m4v6AgGFxt0= Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=simon.marchi@ericsson.com; Received: from [142.133.48.48] (192.75.88.130) by SN6PR15MB2399.namprd15.prod.outlook.com (2603:10b6:805:24::19) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1017.15; Fri, 10 Aug 2018 19:25:11 +0000 Subject: Re: [PATCH v3 3/3] Parse SVE registers in aarch64 core file reading/writing To: Alan Hayward , CC: References: <20180810160849.68985-1-alan.hayward@arm.com> <20180810160849.68985-4-alan.hayward@arm.com> From: Simon Marchi Message-ID: <353c69a3-d49c-bd10-2f5f-f86c57c9fd50@ericsson.com> Date: Fri, 10 Aug 2018 19:25:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20180810160849.68985-4-alan.hayward@arm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Return-Path: simon.marchi@ericsson.com Received-SPF: None (protection.outlook.com: ericsson.com does not designate permitted sender hosts) X-IsSubscribed: yes X-SW-Source: 2018-08/txt/msg00295.txt.bz2 Hi Alan, Just a few nits for this one. The patch is approved with these fixed. If you prefer to send a new version for review, note that I'll only be able to take a look on the 20th or after (after catching up on emails and stuff). > diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c > index 73cb9ea714..bfcee8564c 100644 > --- a/gdb/aarch64-linux-tdep.c > +++ b/gdb/aarch64-linux-tdep.c > @@ -287,6 +287,103 @@ aarch64_linux_core_read_vq (struct gdbarch *gdbarch, bfd *abfd) > return vq; > } > > +/* Supply register REGNUM from BUF to REGCACHE, using the register map > + in REGSET. If REGNUM is -1, do this for all registers in REGSET. > + If BUF is NULL, set the registers to "unavailable" status. */ > + > +static void > +aarch64_linux_supply_sve_regset (const struct regset *regset, > + struct regcache *regcache, > + int regnum, const void *buf, size_t size) > +{ > + gdb_byte *header = (gdb_byte *) buf; > + struct gdbarch *gdbarch = regcache->arch (); > + enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); > + > + if (buf == nullptr) > + return regcache->supply_regset (regset, regnum, nullptr, size); > + gdb_assert (size > SVE_HEADER_SIZE); > + > + /* BUF contains an SVE header followed by a register dump of either the > + passed in SVE regset or a NEON fpregset. */ > + > + /* Extract required fields from the header. */ > + uint64_t vl = extract_unsigned_integer (header + SVE_HEADER_VL_OFFSET, > + SVE_HEADER_VL_LENGTH, byte_order); > + uint16_t flags = extract_unsigned_integer (header + SVE_HEADER_FLAGS_OFFSET, > + SVE_HEADER_FLAGS_LENGTH, > + byte_order); > + > + if (regnum == -1 || regnum == AARCH64_SVE_VG_REGNUM) > + { > + uint64_t vg_target; > + store_integer ((gdb_byte *)&vg_target, sizeof (uint64_t), byte_order, Use gdb_byte vg_target[8]; instead of an uint64_t. That value doesn't belong to an host integer variable, putting it in an uint64_t may lead people to use it in a wrong way. > + sve_vg_from_vl (vl)); > + regcache->raw_supply (AARCH64_SVE_VG_REGNUM, &vg_target); > + } > + > + if (flags & 1) Can you replace this magic 1 with a define? > + { > + /* Register dump is a SVE structure. */ > + regcache->supply_regset (regset, regnum, > + (gdb_byte *) buf + SVE_HEADER_SIZE, > + size - SVE_HEADER_SIZE); > + } > + else > + { > + /* Register dump is a fpsimd structure. First clear the SVE > + registers. */ > + for (int i = 0; i < AARCH64_SVE_Z_REGS_NUM; i++) > + regcache->raw_supply_zeroed (AARCH64_SVE_Z0_REGNUM + i); > + for (int i = 0; i < AARCH64_SVE_P_REGS_NUM; i++) > + regcache->raw_supply_zeroed (AARCH64_SVE_P0_REGNUM + i); > + regcache->raw_supply_zeroed (AARCH64_SVE_FFR_REGNUM); > + > + /* Then supply the fpsimd registers. */ > + regcache->supply_regset (&aarch64_linux_fpregset, regnum, > + (gdb_byte *) buf + SVE_HEADER_SIZE, > + size - SVE_HEADER_SIZE); > + } > +} > + > +/* Collect register REGNUM from REGCACHE to BUF, using the register > + map in REGSET. If REGNUM is -1, do this for all registers in > + REGSET. */ > + > +static void > +aarch64_linux_collect_sve_regset (const struct regset *regset, > + const struct regcache *regcache, > + int regnum, void *buf, size_t size) > +{ > + gdb_byte *header = (gdb_byte *) buf; > + struct gdbarch *gdbarch = regcache->arch (); > + enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); > + uint64_t vq = gdbarch_tdep (gdbarch)->vq; > + > + gdb_assert (buf != NULL); > + gdb_assert (size > SVE_HEADER_SIZE); > + > + /* BUF starts with a SVE header prior to the register dump. */ > + > + store_unsigned_integer (header + SVE_HEADER_SIZE_OFFSET, > + SVE_HEADER_SIZE_LENGTH, byte_order, size); > + store_unsigned_integer (header + SVE_HEADER_MAX_SIZE_OFFSET, > + SVE_HEADER_MAX_SIZE_LENGTH, byte_order, size); > + store_unsigned_integer (header + SVE_HEADER_VL_OFFSET, SVE_HEADER_VL_LENGTH, > + byte_order, sve_vl_from_vq (vq)); > + store_unsigned_integer (header + SVE_HEADER_MAX_VL_OFFSET, > + SVE_HEADER_MAX_VL_LENGTH, byte_order, > + sve_vl_from_vq (vq)); > + store_unsigned_integer (header + SVE_HEADER_FLAGS_OFFSET, > + SVE_HEADER_FLAGS_LENGTH, byte_order, 1); I guess this 1 is the same flag as up there, so could use the same define? > diff --git a/gdb/regcache.h b/gdb/regcache.h > index ea692f38b8..ef2cc478e2 100644 > --- a/gdb/regcache.h > +++ b/gdb/regcache.h > @@ -92,6 +92,14 @@ struct regcache_map_entry > int size; > }; > > +static inline int regcache_map_entry_size (const struct regcache_map_entry *map) Please provide a comment and add a \n before the function name. Also, we might want to name it "regcache_map_size", since this function provides the size of a complete regcache_map (an array of regcache_map_entry), not the size of a single regcache_map_entry. Simon