From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 100027 invoked by alias); 2 Jul 2018 21:22: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 100018 invoked by uid 89); 2 Jul 2018 21:22:28 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy=gdb's, gdbs, GDBs, GDB's X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 02 Jul 2018 21:22:25 +0000 Received: from [10.0.0.11] (unknown [192.222.164.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id BC0831E529; Mon, 2 Jul 2018 17:22:23 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=simark.ca; s=mail; t=1530566543; bh=qZiczvnYZOsDBBQd+yEp8rnOHvy4IWXX7J2U12mFKXU=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=WtIkswTWzuDqOUEwtMdrKo7jtdFN8oiYgGFJQsJ4lbJ2PmRmygSFIJK/d2k2a5Rdh nAxqOAkZUPVaK8POfoN/DXfnQZHO30DPZs1FCLvV0J/JuQhKZQkzeTUdYDYToggvk9 06lnlvQ3k9ZF5O1EIHeE7SYuSLjTjpXQoz1wA26A= Subject: Re: [RFC] Core file support for Aaarch64 SVE To: Alan Hayward , gdb-patches@sourceware.org Cc: nd@arm.com References: <20180625151002.35415-1-alan.hayward@arm.com> From: Simon Marchi Message-ID: Date: Mon, 02 Jul 2018 21:22:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <20180625151002.35415-1-alan.hayward@arm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-07/txt/msg00034.txt.bz2 On 2018-06-25 11:10 AM, Alan Hayward wrote: > The following patch fails to compile for an all targets build. > This is due to aarch64-linux-tdep.c requiring functions from > nat/aarch64-sve-linux-ptrace.c. This is used because the SVE > section of a aarch64 core file starts with a header structure > (the same as in a ptrace read), which is defined in the kernel. > > I considered putting the nat/ file into an all targets build, but > this seemed wrong (in addition it would require additional .deps > dir adding for nat/). Indeed, this can't be done. Suppose you are building a GDB with target=aarch64 and host=x86_64, then the structure definition wouldn't be found and it wouldn't compile. > What is the correct solution here? Should I move the required nat/ > functions into elsewhere? The arch/ still doesn't feel right. arch/ is for arch-specific things, ideally shareable between GDB and GDBserver, but that are host-agnostic. Since the required structure definition is only available when building AArch64 programs but you want to be able to parse cores with a GDB that is itself a non-AArch64 program, then that structure can't be used. You also can't just copy the structure in GDB's source tree and use it, because you can't rely on the type sizes, padding and byte order being the same on other architectures. So I think the only safe thing to do is to read the fields "by hand", using extract_unsigned_integer and friends. > Other than the above issue, the patch is ready and working. I thought > it best to ask here rather than guess the correct solution. > > Thanks, > Alan > > Support both the reading and writing of core files on aarch64 SVE. > > SVE core files are doumented here: > https://github.com/torvalds/linux/blob/master/Documentation/arm64/sve.txt > * A NT_ARM_SVE note will be added to each coredump for each thread of the > dumped process. The contents will be equivalent to the data that would have > been read if a PTRACE_GETREGSET of NT_ARM_SVE were executed for each thread > when the coredump was generated. > > When creating a core files target description, read the SVE section to find > the vector length. > > When reading/writing core files, use the existing nat/ functions to parse > the structure. Given that the function is only called through the one call > chain, there is no need to support copying a single register or invalidating > the set. > > Dependant on binutils patch "[PATCH] Add BFD core support for Aarch64 SVE" > https://sourceware.org/ml/binutils/2018-06/msg00314.html > > Checked with make check on x86 and aarch64. > Generated core files on SVE emulator both from gdb "generate-core-file" > command and a segfault. Loaded these back into gdb. > > 2018-06-25 Alan Hayward > > gdb/ > * aarch64-linux-tdep.c (aarch64_linux_core_read_vq): New function. > (aarch64_linux_supply_sve_regset): Likewise. > (aarch64_linux_collect_sve_regset): Likewise. > (aarch64_linux_iterate_over_regset_sections): Check for SVE. > (aarch64_linux_core_read_description): Likewise. > * nat/aarch64-sve-linux-sigcontext.h (struct _aarch64_ctx): Add for > build compatibility. > (struct user_fpsimd_state): Likewise. > --- > gdb/aarch64-linux-tdep.c | 114 ++++++++++++++++++++++++++++++++- > gdb/nat/aarch64-sve-linux-sigcontext.h | 14 ++++ > 2 files changed, 125 insertions(+), 3 deletions(-) > > diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c > index 93b6d416a3..919873b816 100644 > --- a/gdb/aarch64-linux-tdep.c > +++ b/gdb/aarch64-linux-tdep.c > @@ -47,6 +47,7 @@ > #include "linux-record.h" > #include "auxv.h" > #include "elf/common.h" > +#include "nat/aarch64-sve-linux-ptrace.h" > > > /* Signal frame handling. > > @@ -169,6 +170,100 @@ aarch64_linux_sigframe_init (const struct tramp_frame *self, > trad_frame_set_id (this_cache, frame_id_build (sp, func)); > } > > + > +/* Get VG value from SVE section in the core dump. */ VG or VQ? > + > +static uint64_t > +aarch64_linux_core_read_vq (bfd *abfd) > +{ > + struct user_sve_header header; > + asection *sve_section = bfd_get_section_by_name (abfd, ".reg-aarch-sve"); > + > + if (!sve_section) > + { > + /* No SVE state. */ > + return 0; > + } > + > + size_t size = bfd_section_size (abfd, sve_section); > + > + /* Check extended state size. */ > + if (size < sizeof (struct user_sve_header)) > + { > + warning (_("'.reg-aarch-sve' section in core file too small.")); > + return 0; > + } > + > + if (! bfd_get_section_contents (abfd, sve_section, &header, 0, > + sizeof (struct user_sve_header))) As mentioned earlier, even if this could compile on non-AArch64 systems, it might give bad results. For example, if the host is big-endian, the values will all be read with the wrong endianness. > + { > + warning (_("Couldn't read sve header from " > + "'.reg-aarch-sve' section in core file.")); > + return 0; > + } > + > + if (!sve_vl_valid (header.vl)) > + { > + warning (_("sve header invalid in" > + "'.reg-aarch-sve' section in core file.")); > + return 0; > + } > + > + return sve_vq_from_vl (header.vl); > +} > + > + > +/* Supply register REGNUM from BUF to REGCACHE, ignoring the register map > + in REGSET. */ > + > +static void > +aarch64_linux_supply_sve_regset (const struct regset *regset, > + struct regcache *regcache, > + int regnum, const void *buf, size_t size) > +{ > + /* Only called via aarch64_linux_iterate_over_regset_sections loop, so no > + need to support anything other than a full regset, and no need to support > + null buffers. */ > + gdb_assert (buf != NULL); > + gdb_assert (regnum == -1); > + > + aarch64_sve_regs_copy_to_reg_buf (regcache, buf); > +} > + > +/* Collect register REGNUM from REGCACHE to BUF, ignoring 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) > +{ > + /* Only called via aarch64_linux_iterate_over_regset_sections loop, so no > + need to support anything other than a full regset, and no need to support > + null buffers. */ > + gdb_assert (buf != NULL); > + gdb_assert (regnum == -1); > + > + /* Read vector size from regcache. */ > + uint64_t vg; > + regcache->raw_collect_integer (AARCH64_SVE_VG_REGNUM, (gdb_byte *)&vg, > + sizeof (uint64_t), false); > + uint64_t vq = sve_vq_from_vg (vg); > + > + /* Initialize a valid SVE header. */ > + struct user_sve_header *header = (struct user_sve_header *)buf; > + header->flags = SVE_PT_REGS_SVE; > + header->size = SVE_PT_SIZE (vq, header->flags); > + header->max_size = size; > + header->vl = sve_vl_from_vg (vg); > + header->max_vl = SVE_VL_MAX; > + > + gdb_assert (size >= header->size); > + > + aarch64_sve_regs_copy_from_reg_buf (regcache, buf); > +} > + > static const struct tramp_frame aarch64_linux_rt_sigframe = > { > SIGTRAMP_FRAME, > @@ -219,6 +314,12 @@ const struct regset aarch64_linux_fpregset = > regcache_supply_regset, regcache_collect_regset > }; > > +const struct regset aarch64_linux_sve_regset = > + { > + NULL, aarch64_linux_supply_sve_regset, aarch64_linux_collect_sve_regset, > + REGSET_VARIABLE_SIZE > + }; > + > /* Implement the "regset_from_core_section" gdbarch method. */ > > static void > @@ -227,10 +328,17 @@ aarch64_linux_iterate_over_regset_sections (struct gdbarch *gdbarch, > void *cb_data, > const struct regcache *regcache) > { > + struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); > + > cb (".reg", AARCH64_LINUX_SIZEOF_GREGSET, &aarch64_linux_gregset, > NULL, cb_data); > - cb (".reg2", AARCH64_LINUX_SIZEOF_FPREGSET, &aarch64_linux_fpregset, > - NULL, cb_data); > + > + if (tdep->has_sve ()) > + cb (".reg-aarch-sve", SVE_PT_SIZE (tdep->vq, 0), > + &aarch64_linux_sve_regset, "SVE registers", cb_data); > + else > + cb (".reg2", AARCH64_LINUX_SIZEOF_FPREGSET, &aarch64_linux_fpregset, NULL, > + cb_data); > } > > /* Implement the "core_read_description" gdbarch method. SVE not yet > @@ -245,7 +353,7 @@ aarch64_linux_core_read_description (struct gdbarch *gdbarch, > if (target_auxv_search (target, AT_HWCAP, &aarch64_hwcap) != 1) > return NULL; > > - return aarch64_read_description (0); > + return aarch64_read_description (aarch64_linux_core_read_vq (abfd)); > } > > /* Implementation of `gdbarch_stap_is_single_operand', as defined in > diff --git a/gdb/nat/aarch64-sve-linux-sigcontext.h b/gdb/nat/aarch64-sve-linux-sigcontext.h > index bdece8e17d..ad6e111e71 100644 > --- a/gdb/nat/aarch64-sve-linux-sigcontext.h > +++ b/gdb/nat/aarch64-sve-linux-sigcontext.h > @@ -19,6 +19,20 @@ > #ifndef AARCH64_SVE_LINUX_SIGCONTEXT_H > #define AARCH64_SVE_LINUX_SIGCONTEXT_H > > +#ifndef _aarch64_ctx > +struct _aarch64_ctx { > + __u32 magic; > + __u32 size; > +}; > + > +struct user_fpsimd_state { > + __uint128_t vregs[32]; > + __u32 fpsr; > + __u32 fpcr; > + __u32 __reserved[2]; > +}; > +#endif Does this ifndef really work? AFAIK, you can't use ifdefs to check whether a struct type has been defined. The ifdefs are processed by the preprocessor and the struct types are processed by the compiler. An alternative would be an autoconf test. Simon