This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 8/8] Ptrace support for Aarch64 SVE
> On 31 May 2018, at 14:22, Simon Marchi <simon.marchi@ericsson.com> wrote:
>
> Hi Alan,
>
> Since there is a good number of macros copied from the Linux kernel, it might be
> a good idea to isolate in their own file. It would also be good to identify
> precisely which file they come from (the path relative to the root of the linux
> repo) and the git commit you used.
>
> Also, since the copied code is rather large and non-trivial, does it pose copyright
> problems? If we want to include it, maybe that separate file should not state that
> the copyright is owned by the FSF, since it's not the case? Maybe others have
> experience with this kind of things, or maybe we should get an advice from the FSF directly.
A new file makes sense (especially with regards to copyright). However, the plan was
to eventually remove the macros when "enough" time has passed - keeping everything
in aarch64-sve-linux-ptrace.h made it simpler than removing files.
Is there a minimum kernel version that gdb requires to link against in order to build?
The relevant defines are in 4.15. The kernel is now on 4.16.9.
I’m guessing this must have come up before when other targets added new kernel features?
I didn’t want to suddenly destroy everyone’s aarch64/targetall build trees without
warning.
>> diff --git a/gdb/nat/aarch64-sve-linux-ptrace.c b/gdb/nat/aarch64-sve-linux-ptrace.c
>> index 9381786fda..84c7a41f40 100644
>> --- a/gdb/nat/aarch64-sve-linux-ptrace.c
>> +++ b/gdb/nat/aarch64-sve-linux-ptrace.c
>> @@ -25,6 +25,13 @@
>> #include "aarch64-sve-linux-ptrace.h"
>> #include "arch/aarch64.h"
>>
>> +#ifndef GDBSERVER
>> +#include "defs.h"
>> +#endif
>> +#include "regcache.h"
>
> Hmm we try not add any more "#ifdef GDBSERVER" in the common code.
>
> https://sourceware.org/gdb/wiki/Common#Header_files_in_common_code_.28defs.h_vs_server.h.2C_etc..29
>
> Instead, we should try defining a common interface (probably in common/common-regcache.h?) that the
> common code will use, and that regcaches from GDB and GDBserver will implement.
I tried using common-defs.h, but gdb/regcache.h requires defines from
defs.h - RequireLongest and maybe others.
Putting defs.h at the top of gdb/regcache.h then broke in a weird way.
A lot of fiddling later, and I hadn’t found a way to make it work.
Creating common/common-regcache.h gets a bit odd because, the functions
I need for gdbserver (raw_supply, raw_collect and get_register_status)
on gdb come from:
class reg_buffer
...
enum register_status get_register_status (int regnum) const;
...
class readable_regcache : public reg_buffer
...
class detached_regcache : public readable_regcache
...
void raw_supply (int regnum, const void *buf);
...
class regcache : public detached_regcache
...
void raw_collect (int regnum, void *buf) const;
...
I don’t think that this would work:
class regcache : public detached_regcache, common_regcache
>
>> +
>> +static bool vq_change_warned = false;
>> +
>> /* Read VQ for the given tid using ptrace. If SVE is not supported then zero
>> is returned (on a system that supports SVE, then VQ cannot be zeo). */
>>
>> @@ -50,3 +57,259 @@ aarch64_sve_get_vq (int tid)
>>
>> return vq;
>> }
>> +
>> +/* Read the current SVE register set using ptrace, allocating space as
>> + required. */
>
> Put a reference to the .h here.
>
> Since this returns allocated memory, could we return an RAII object? Either
> std::vector, std::unique_ptr or gdb::unique_xmalloc_ptr.
Yes, I’ll update with one of those. Is there any documentation explaining
when to use which of these?
>
>> +
>> +gdb_byte *
>> +aarch64_sve_get_sveregs (int tid)
>> +{
>> + int ret;
>> + struct iovec iovec;
>> + struct user_sve_header header;
>> + long vq = aarch64_sve_get_vq (tid);
>> +
>> + if (vq == 0)
>> + perror_with_name (_("Unable to fetch sve register header"));
>> +
>> + /* A ptrace call with NT_ARM_SVE will return a header followed by either a
>> + dump of all the SVE and FP registers, or an fpsimd structure (identical to
>> + the one returned by NT_FPREGSET) if the kernel has not yet executed any
>> + SVE code. Make sure we allocate enough space for a full SVE dump. */
>> +
>> + iovec.iov_len = SVE_PT_SIZE (vq, SVE_PT_REGS_SVE);
>> + iovec.iov_base = xmalloc (iovec.iov_len);
>> +
>> + ret = ptrace (PTRACE_GETREGSET, tid, NT_ARM_SVE, &iovec);
>> + if (ret < 0)
>> + perror_with_name (_("Unable to fetch sve registers"));
>> +
>> + return (gdb_byte *) iovec.iov_base;
>> +}
>> +
>> +/* Put the registers from linux structure buf into regcache. */
>> +
>> +void
>> +aarch64_sve_regs_copy_to_regcache (struct regcache *regcache, const void *buf)
>> +{
>> + char *base = (char*) buf;
>> + int i;
>> + struct user_sve_header *header = (struct user_sve_header *) buf;
>> + long vq, vg_regcache;
>> +
>> + vq = sve_vq_from_vl (header->vl);
>> +
>> + /* Sanity check the data in the header. */
>> + gdb_assert (sve_vl_valid (header->vl));
>> + gdb_assert (SVE_PT_SIZE (vq, header->flags) == header->size);
>
> Again, we shouldn't use gdb_assert here, since this validates external input.
Ok.
>
>> +
>> + regcache->raw_collect (AARCH64_SVE_VG_REGNUM, &vg_regcache);
>
> When fetching registers, won't it be usually to fill a shiny new, empty regcache?
> In that case, won't it always fall into the "if" branch?
>
> In any case, should we check the status of the VG register to make sure it's REG_VALID
> before we try to collect it?
I thought the same regcache was used each time registers needed reading?
I’ll double check this.
Either way, yes, should probably check REG_VALID
>
>> + if (vg_regcache == 0)
>> + {
>> + /* VG has not been set. */
>> + vg_regcache = sve_vg_from_vl (header->vl);
>> + regcache->raw_supply (AARCH64_SVE_VG_REGNUM, &vg_regcache);
>> + }
>> + else if (vg_regcache != sve_vg_from_vl (header->vl) && !vq_change_warned)
>> + {
>> + /* Vector length on the running process has changed. GDB currently does
>> + not support this and will result in GDB showing incorrect partially
>> + incorrect data for the vector registers. Warn once and continue. We
>> + do not expect many programs to exhibit this behaviour. To fix this
>> + we need to spot the change earlier and generate a new target
>> + descriptor. */
>> + warning (_("Vector length has changed (%ld to %d). "
>> + "Vector registers may show incorrect data."),
>
> Perhaps mention "SVE vector length has changed..."? Otherwise the user may wonder
> what vectors we are talking about.
Agreed.
>
>> + vg_regcache, sve_vg_from_vl (header->vl));
>> + vq_change_warned = true;
>> + }
>> +
>> + if (HAS_SVE_STATE (*header))
>> + {
>> + /* The register dump contains a set of SVE registers. */
>> +
>> + for (i = 0; i < AARCH64_SVE_Z_REGS_NUM; i++)
>> + regcache->raw_supply (AARCH64_SVE_Z0_REGNUM + i,
>> + base + SVE_PT_SVE_ZREG_OFFSET (vq, i));
>> +
>> + for (i = 0; i < AARCH64_SVE_P_REGS_NUM; i++)
>> + regcache->raw_supply (AARCH64_SVE_P0_REGNUM + i,
>> + base + SVE_PT_SVE_PREG_OFFSET (vq, i));
>> +
>> + regcache->raw_supply (AARCH64_SVE_FFR_REGNUM,
>> + base + SVE_PT_SVE_FFR_OFFSET (vq));
>> + regcache->raw_supply (AARCH64_FPSR_REGNUM,
>> + base + SVE_PT_SVE_FPSR_OFFSET (vq));
>> + regcache->raw_supply (AARCH64_FPCR_REGNUM,
>> + base + SVE_PT_SVE_FPCR_OFFSET (vq));
>
> Align the second line with the first argument. Here's an example, though
> using spaces instead of tabs to make sure (hopefully) the mail clients display it
> correctly.
>
> regcache->raw_supply (AARCH64_SVE_Z0_REGNUM + i,
> base + SVE_PT_SVE_ZREG_OFFSET (vq, i));
>
> There are many instances throughout this file.
>
I think that’s leftover from when I changed from "raw_supply (regcache,”
to “regcache->raw_supply (“.
I’ll fix it up.
Alan.