[PATCH 0/8] Add SVE support for Aarch64 GDB

classic Classic list List threaded Threaded
47 messages Options
123
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 8/8] Ptrace support for Aarch64 SVE

Alan Hayward

>>>> 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
>
> I did some quick hacking and it seems to work to have this in common/common-regcache.h
>
> struct reg_buffer_common
> {
>  virtual ~reg_buffer_common () = default;
>  virtual void raw_supply (int regnum, const void *buf) = 0;
>  virtual void raw_collect (int regnum, void *buf) const = 0;
>  virtual bool raw_compare (int regnum, const void *buf, int offset) const = 0;
>  virtual register_status get_register_status (int regnum) const = 0;
> };
>
> and make your code in nat/ take a pointer to reg_buffer_common.  gdb's reg_buffer
> and gdbserver's regcache should inherit from reg_buffer_common.  I  built
> it on other regcache refactor patches I have in the pipeline, so maybe some other
> changes in there are needed, maybe not.  I pushed the result on this branch, it's
> a bit raw but it should be enough to show the idea.
>
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=shortlog;h=refs/heads/users/simark/regcache-for-alan

Thanks for trying this out, however I tried adding your changes
into my build, but that results in the following:

../../src/binutils-gdb/gdb/linux-fork.c: In function ‘void fork_save_infrun_state(fork_info*, int)’:
../../src/binutils-gdb/gdb/linux-fork.c:298:75: error: invalid new-expression of abstract class type ‘readonly_detached_regcache’
   fp->savedregs = new readonly_detached_regcache (*get_current_regcache ());
                                                                           ^
In file included from ../../src/binutils-gdb/gdb/gdbarch.h:70:0,
                 from ../../src/binutils-gdb/gdb/defs.h:531,
                 from ../../src/binutils-gdb/gdb/linux-fork.c:20:
../../src/binutils-gdb/gdb/regcache.h:367:7: note:   because the following virtual functions are pure within ‘readonly_detached_regcache’:
 class readonly_detached_regcache : public readable_regcache
       ^
In file included from ../../src/binutils-gdb/gdb/regcache.h:23:0,
                 from ../../src/binutils-gdb/gdb/gdbarch.h:70,
                 from ../../src/binutils-gdb/gdb/defs.h:531,
                 from ../../src/binutils-gdb/gdb/linux-fork.c:20:
../../src/binutils-gdb/gdb/common/common-regcache.h:68:16: note: virtual void reg_buffer_common::raw_supply(int, const void*)
   virtual void raw_supply (int regnum, const void *buf) = 0;
                ^
../../src/binutils-gdb/gdb/common/common-regcache.h:69:16: note: virtual void reg_buffer_common::raw_collect(int, void*) const
   virtual void raw_collect (int regnum, void *buf) const = 0;


And then similar errors trying to create a register_dump_reg_buffer.


I could then add the following to readonly_detached_regcache:

  void raw_supply (int regnum, const void *buf) override
  { gdb_assert(false); }

  void raw_collect (int regnum, void *buf) const override
  { gdb_assert(false); }

...Or even make the methods in reg_buffer_common have a
{ gdb_assert(false); } body.

But that feels wrong, as it’s breaking the nice regcache abstractions.

Any thoughts?

Alan.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/8] Function for reading the Aarch64 SVE vector length.

Sergio Durigan Junior
In reply to this post by Alan Hayward
On Friday, May 11 2018, Alan Hayward wrote:

> Add a method for reading the SVE vector length using ptrace. This returns
> 0 for systems without SVE support.
>
> Note the defines taken from Linux kernel headers in aarch64-sve-linux-ptrace.h.
> See the covering email for details about this.
>
> There are multiple ways of expressing the vector length. Thankfully these are
> all wll defined. I've added convertors for going from one to the other.
> Hopefully this will help to prevent future confusion.

Hi Alan,

I'm trying to build GDB on Fedora Rawhide Aarch64, and I'm seeing the
following breakage:

    CXX    aarch64-linux-nat.o
  In file included from ../../gdb/aarch64-tdep.h:25,
                   from ../../gdb/aarch64-linux-nat.c:30:
  ../../gdb/arch/aarch64.h:79: error: "sve_vq_from_vl" redefined [-Werror]
   #define sve_vq_from_vl(vl) ((vl) / 0x10)

  In file included from /usr/include/bits/sigcontext.h:30,
                   from /usr/include/signal.h:291,
                   from build-gnulib/import/signal.h:52,
                   from ../../gdb/linux-nat.h:23,
                   from ../../gdb/aarch64-linux-nat.c:26:
  /usr/include/asm/sigcontext.h:154: note: this is the location of the previous definition
   #define sve_vq_from_vl(vl) ((vl) / SVE_VQ_BYTES)

  In file included from ../../gdb/aarch64-tdep.h:25,
                   from ../../gdb/aarch64-linux-nat.c:30:
  ../../gdb/arch/aarch64.h:80: error: "sve_vl_from_vq" redefined [-Werror]
   #define sve_vl_from_vq(vq) ((vq) * 0x10)

  In file included from /usr/include/bits/sigcontext.h:30,
                   from /usr/include/signal.h:291,
                   from build-gnulib/import/signal.h:52,
                   from ../../gdb/linux-nat.h:23,
                   from ../../gdb/aarch64-linux-nat.c:26:
  /usr/include/asm/sigcontext.h:155: note: this is the location of the previous definition
   #define sve_vl_from_vq(vq) ((vq) * SVE_VQ_BYTES)

    CXX    aarch32-linux-nat.o
    CXX    aarch64-linux-hw-point.o
    CXX    aarch64-linux.o
    CXX    aarch64-sve-linux-ptrace.o
    CXX    cli/cli-cmds.o
  In file included from ../../gdb/nat/aarch64-sve-linux-ptrace.c:26:
  ../../gdb/arch/aarch64.h:79: error: "sve_vq_from_vl" redefined [-Werror]
   #define sve_vq_from_vl(vl) ((vl) / 0x10)

  In file included from ../../gdb/nat/aarch64-sve-linux-ptrace.h:27,
                   from ../../gdb/nat/aarch64-sve-linux-ptrace.c:25:
  /usr/include/asm/sigcontext.h:154: note: this is the location of the previous definition
   #define sve_vq_from_vl(vl) ((vl) / SVE_VQ_BYTES)

  In file included from ../../gdb/nat/aarch64-sve-linux-ptrace.c:26:
  ../../gdb/arch/aarch64.h:80: error: "sve_vl_from_vq" redefined [-Werror]
   #define sve_vl_from_vq(vq) ((vq) * 0x10)

  In file included from ../../gdb/nat/aarch64-sve-linux-ptrace.h:27,
                   from ../../gdb/nat/aarch64-sve-linux-ptrace.c:25:
  /usr/include/asm/sigcontext.h:155: note: this is the location of the previous definition
   #define sve_vl_from_vq(vq) ((vq) * SVE_VQ_BYTES)


I think it may be a good idea to guard these #define's with #ifndef's.
I'm trying to get ahold of a machine to test this (the build failure
happened on a machine which I don't have direct control over).

Thanks,

> 2018-05-11  Alan Hayward  <[hidden email]>
>
> gdb/
> * Makefile.in: Add new header.
> * gdb/arch/aarch64.h (sve_vg_from_vl): New macro.
> (sve_vl_from_vg): Likewise.
> (sve_vq_from_vl): Likewise.
> (sve_vl_from_vq): Likewise.
> (sve_vq_from_vg): Likewise.
> (sve_vg_from_vq): Likewise.
> * configure.nat: Add new c file.
> * nat/aarch64-sve-linux-ptrace.c: New file.
> * nat/aarch64-sve-linux-ptrace.h: New file.
>
> gdbserver/
> * configure.srv: Add new c/h file.
> ---
>  gdb/Makefile.in                    |  1 +
>  gdb/arch/aarch64.h                 | 17 +++++++++
>  gdb/configure.nat                  |  2 +-
>  gdb/gdbserver/configure.srv        |  1 +
>  gdb/nat/aarch64-sve-linux-ptrace.c | 52 +++++++++++++++++++++++++++
>  gdb/nat/aarch64-sve-linux-ptrace.h | 73 ++++++++++++++++++++++++++++++++++++++
>  6 files changed, 145 insertions(+), 1 deletion(-)
>  create mode 100644 gdb/nat/aarch64-sve-linux-ptrace.c
>  create mode 100644 gdb/nat/aarch64-sve-linux-ptrace.h
>
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 87d74a7703..64042d1bd1 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -1478,6 +1478,7 @@ HFILES_NO_SRCDIR = \
>   mi/mi-parse.h \
>   nat/aarch64-linux.h \
>   nat/aarch64-linux-hw-point.h \
> + nat/aarch64-sve-linux-ptrace.h \
>   nat/amd64-linux-siginfo.h \
>   nat/gdb_ptrace.h \
>   nat/gdb_thread_db.h \
> diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
> index 1846e04163..af0b157c51 100644
> --- a/gdb/arch/aarch64.h
> +++ b/gdb/arch/aarch64.h
> @@ -48,6 +48,23 @@ enum aarch64_regnum
>  #define AARCH64_V_REGS_NUM 32
>  #define AARCH64_NUM_REGS AARCH64_FPCR_REGNUM + 1
>  
> +/* There are a number of ways of expressing the current SVE vector size:
> +
> +   VL : Vector Length.
> + The number of bytes in an SVE Z register.
> +   VQ : Vector Quotient.
> + The number of 128bit chunks in an SVE Z register.
> +   VG : Vector Gradient.
> + The number of 64bit chunks in an SVE Z register.  */
> +
> +#define sve_vg_from_vl(vl) ((vl) / 8)
> +#define sve_vl_from_vg(vg) ((vg) * 8)
> +#define sve_vq_from_vl(vl) ((vl) / 0x10)
> +#define sve_vl_from_vq(vq) ((vq) * 0x10)
> +#define sve_vq_from_vg(vg) (sve_vq_from_vl (sve_vl_from_vg (vg)))
> +#define sve_vg_from_vq(vq) (sve_vg_from_vl (sve_vl_from_vq (vq)))
> +
> +
>  /* Maximum supported VQ value.  Increase if required.  */
>  #define AARCH64_MAX_SVE_VQ  16
>  
> diff --git a/gdb/configure.nat b/gdb/configure.nat
> index 6b0f44fede..d7d79adaca 100644
> --- a/gdb/configure.nat
> +++ b/gdb/configure.nat
> @@ -228,7 +228,7 @@ case ${gdb_host} in
>      aarch64)
>   #  Host: AArch64 based machine running GNU/Linux
>   NATDEPFILES="${NATDEPFILES} aarch64-linux-nat.o \
> - aarch32-linux-nat.o aarch64-linux-hw-point.o aarch64-linux.o"
> + aarch32-linux-nat.o aarch64-linux-hw-point.o aarch64-linux.o aarch64-sve-linux-ptrace.o"
>   ;;
>      arm)
>   # Host: ARM based machine running GNU/Linux
> diff --git a/gdb/gdbserver/configure.srv b/gdb/gdbserver/configure.srv
> index ffeefb9b92..8bf0dcc650 100644
> --- a/gdb/gdbserver/configure.srv
> +++ b/gdb/gdbserver/configure.srv
> @@ -54,6 +54,7 @@ case "${target}" in
>   srv_tgtobj="$srv_tgtobj arch/aarch64-insn.o"
>   srv_tgtobj="$srv_tgtobj arch/aarch64.o"
>   srv_tgtobj="$srv_tgtobj linux-aarch64-tdesc.o"
> + srv_tgtobj="$srv_tgtobj aarch64-sve-linux-ptrace.o"
>   srv_tgtobj="${srv_tgtobj} $srv_linux_obj"
>   srv_linux_regsets=yes
>   srv_linux_thread_db=yes
> diff --git a/gdb/nat/aarch64-sve-linux-ptrace.c b/gdb/nat/aarch64-sve-linux-ptrace.c
> new file mode 100644
> index 0000000000..9381786fda
> --- /dev/null
> +++ b/gdb/nat/aarch64-sve-linux-ptrace.c
> @@ -0,0 +1,52 @@
> +/* Common target dependent for AArch64 systems.
> +
> +   Copyright (C) 2017 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include <sys/utsname.h>
> +#include <sys/uio.h>
> +#include "common-defs.h"
> +#include "elf/external.h"
> +#include "elf/common.h"
> +#include "aarch64-sve-linux-ptrace.h"
> +#include "arch/aarch64.h"
> +
> +/* 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).  */
> +
> +unsigned long
> +aarch64_sve_get_vq (int tid)
> +{
> +  struct iovec iovec;
> +  struct user_sve_header header;
> +
> +  iovec.iov_len = sizeof (header);
> +  iovec.iov_base = &header;
> +
> +  /* Ptrace gives the vector length in bytes.  Convert it to VQ, the number of
> +     128bit chunks in a Z register.  We use VQ because 128bits is the minimum
> +     a Z register can increase in size.  */
> +
> +  if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_SVE, &iovec) < 0)
> +    /* SVE is not supported.  */
> +    return 0;
> +
> +  long vq = sve_vq_from_vl (header.vl);
> +  gdb_assert (sve_vl_valid (header.vl));
> +
> +  return vq;
> +}
> diff --git a/gdb/nat/aarch64-sve-linux-ptrace.h b/gdb/nat/aarch64-sve-linux-ptrace.h
> new file mode 100644
> index 0000000000..b318150ac1
> --- /dev/null
> +++ b/gdb/nat/aarch64-sve-linux-ptrace.h
> @@ -0,0 +1,73 @@
> +/* Common target dependent for AArch64 systems.
> +
> +   Copyright (C) 2017 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef AARCH64_SVE_LINUX_PTRACE_H
> +#define AARCH64_SVE_LINUX_PTRACE_H
> +
> +/* Where indicated, this file contains defines and macros lifted directly from
> +   the Linux kernel headers, with no modification.
> +   Refer to Linux kernel documentation for details.  */
> +
> +#include <asm/sigcontext.h>
> +#include <sys/utsname.h>
> +#include <sys/ptrace.h>
> +#include <asm/ptrace.h>
> +
> +/* 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).  */
> +
> +extern unsigned long aarch64_sve_get_vq (int tid);
> +
> +/* Structures and defines taken from sigcontext.h.  */
> +
> +#ifndef SVE_SIG_ZREGS_SIZE
> +
> +#define SVE_VQ_BYTES 16 /* number of bytes per quadword */
> +
> +#define SVE_VQ_MIN 1
> +#define SVE_VQ_MAX 512
> +
> +#define SVE_VL_MIN (SVE_VQ_MIN * SVE_VQ_BYTES)
> +#define SVE_VL_MAX (SVE_VQ_MAX * SVE_VQ_BYTES)
> +
> +#define SVE_NUM_ZREGS 32
> +#define SVE_NUM_PREGS 16
> +
> +#define sve_vl_valid(vl) \
> + ((vl) % SVE_VQ_BYTES == 0 && (vl) >= SVE_VL_MIN && (vl) <= SVE_VL_MAX)
> +
> +#endif /* SVE_SIG_ZREGS_SIZE.  */
> +
> +
> +/* Structures and defines taken from ptrace.h.  */
> +
> +#ifndef SVE_PT_SVE_ZREG_SIZE
> +
> +struct user_sve_header {
> + __u32 size; /* total meaningful regset content in bytes */
> + __u32 max_size; /* maxmium possible size for this thread */
> + __u16 vl; /* current vector length */
> + __u16 max_vl; /* maximum possible vector length */
> + __u16 flags;
> + __u16 __reserved;
> +};
> +
> +#endif /* SVE_PT_SVE_ZREG_SIZE.  */
> +
> +#endif /* aarch64-sve-linux-ptrace.h */
> --
> 2.15.1 (Apple Git-101)

--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
Reply | Threaded
Open this post in threaded view
|

[PATCH] Guard declarations of 'sve_*_from_*' macros on Aarch64 (and unbreak build)

Sergio Durigan Junior
Commit 122394f1476b1c925a281b15399119500c8231c1 ("Function for reading
the Aarch64 SVE vector length") has added macros to manipulate SVE
vector sizes based on Linux kernel sources, but did not guard them
with #ifndef's, which breaks the build when the system headers already
have these macros:

    CXX    aarch64-linux-nat.o
  In file included from ../../gdb/aarch64-tdep.h:25,
                   from ../../gdb/aarch64-linux-nat.c:30:
  ../../gdb/arch/aarch64.h:79: error: "sve_vq_from_vl" redefined [-Werror]
   #define sve_vq_from_vl(vl) ((vl) / 0x10)

  In file included from /usr/include/bits/sigcontext.h:30,
                   from /usr/include/signal.h:291,
                   from build-gnulib/import/signal.h:52,
                   from ../../gdb/linux-nat.h:23,
                   from ../../gdb/aarch64-linux-nat.c:26:
  /usr/include/asm/sigcontext.h:154: note: this is the location of the previous definition
   #define sve_vq_from_vl(vl) ((vl) / SVE_VQ_BYTES)

  In file included from ../../gdb/aarch64-tdep.h:25,
                   from ../../gdb/aarch64-linux-nat.c:30:
  ../../gdb/arch/aarch64.h:80: error: "sve_vl_from_vq" redefined [-Werror]
   #define sve_vl_from_vq(vq) ((vq) * 0x10)

  In file included from /usr/include/bits/sigcontext.h:30,
                   from /usr/include/signal.h:291,
                   from build-gnulib/import/signal.h:52,
                   from ../../gdb/linux-nat.h:23,
                   from ../../gdb/aarch64-linux-nat.c:26:
  /usr/include/asm/sigcontext.h:155: note: this is the location of the previous definition
   #define sve_vl_from_vq(vq) ((vq) * SVE_VQ_BYTES)

In order to fix this breakage, this commit guards the declaration of
the macros using #ifndef's.  It is important to mention that the
system headers use a value to multiply/divide the vector
length (SVE_VQ_BYTES, defined as 16 on the system I'm using) which is
different than the values being used by the macros defined in GDB.  I
wasn't sure how to consolidate that, so I chose to ignore this
discrepancy.

Tested by making sure GDB compiles fine again.  Since the system I'm
using doesn't have support for SVE, there's no way I can really test
these changes.

gdb/ChangeLog:
2018-06-05  Sergio Durigan Junior  <[hidden email]>

        * arch/aarch64.h (sve_vg_from_vl): Guard with #ifndef.
        (sve_vl_from_vg): Likewise.
        (sve_vq_from_vl): Likewise.
        (sve_vl_from_vq): Likewise.
        (sve_vq_from_vg): Likewise.
        (sve_vg_from_vq): Likewise.
---
 gdb/arch/aarch64.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
index 9040d8d4c8..c378a4b239 100644
--- a/gdb/arch/aarch64.h
+++ b/gdb/arch/aarch64.h
@@ -74,12 +74,24 @@ enum aarch64_regnum
    VG : Vector Gradient.
  The number of 64bit chunks in an SVE Z register.  */
 
+#ifndef sve_vg_from_vl
 #define sve_vg_from_vl(vl) ((vl) / 8)
+#endif
+#ifndef sve_vl_from_vg
 #define sve_vl_from_vg(vg) ((vg) * 8)
+#endif
+#ifndef sve_vq_from_vl
 #define sve_vq_from_vl(vl) ((vl) / 0x10)
+#endif
+#ifndef sve_vl_from_vq
 #define sve_vl_from_vq(vq) ((vq) * 0x10)
+#endif
+#ifndef sve_vq_from_vg
 #define sve_vq_from_vg(vg) (sve_vq_from_vl (sve_vl_from_vg (vg)))
+#endif
+#ifndef sve_vg_from_vq
 #define sve_vg_from_vq(vq) (sve_vg_from_vl (sve_vl_from_vq (vq)))
+#endif
 
 
 /* Maximum supported VQ value.  Increase if required.  */
--
2.14.3

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Guard declarations of 'sve_*_from_*' macros on Aarch64 (and unbreak build)

Sergio Durigan Junior
On Tuesday, June 05 2018, I wrote:

> In order to fix this breakage, this commit guards the declaration of
> the macros using #ifndef's.  It is important to mention that the
> system headers use a value to multiply/divide the vector
> length (SVE_VQ_BYTES, defined as 16 on the system I'm using) which is
> different than the values being used by the macros defined in GDB.  I
> wasn't sure how to consolidate that, so I chose to ignore this
> discrepancy.

Actually, I just saw that there's no discrepancy.  The system header
defines the following macros:

  #define SVE_VQ_BYTES            16      /* number of bytes per quadword */                    
  ...
  #define sve_vq_from_vl(vl)      ((vl) / SVE_VQ_BYTES)                                          
  #define sve_vl_from_vq(vq)      ((vq) * SVE_VQ_BYTES)                                          

and GDB does this:

  #ifndef sve_vq_from_vl
  #define sve_vq_from_vl(vl) ((vl) / 0x10)
  #endif
  #ifndef sve_vl_from_vq
  #define sve_vl_from_vq(vq) ((vq) * 0x10)
  #endif

which is fine.  I'll remove this comment from the commit log.

Thanks,

--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Guard declarations of 'sve_*_from_*' macros on Aarch64 (and unbreak build)

Alan Hayward
In reply to this post by Sergio Durigan Junior


> On 5 Jun 2018, at 23:05, Sergio Durigan Junior <[hidden email]> wrote:
>
> Commit 122394f1476b1c925a281b15399119500c8231c1 ("Function for reading
> the Aarch64 SVE vector length") has added macros to manipulate SVE
> vector sizes based on Linux kernel sources, but did not guard them
> with #ifndef's, which breaks the build when the system headers already
> have these macros:
>
>    CXX    aarch64-linux-nat.o
>  In file included from ../../gdb/aarch64-tdep.h:25,
>                   from ../../gdb/aarch64-linux-nat.c:30:
>  ../../gdb/arch/aarch64.h:79: error: "sve_vq_from_vl" redefined [-Werror]
>   #define sve_vq_from_vl(vl) ((vl) / 0x10)
>
>  In file included from /usr/include/bits/sigcontext.h:30,
>                   from /usr/include/signal.h:291,
>                   from build-gnulib/import/signal.h:52,
>                   from ../../gdb/linux-nat.h:23,
>                   from ../../gdb/aarch64-linux-nat.c:26:
>  /usr/include/asm/sigcontext.h:154: note: this is the location of the previous definition
>   #define sve_vq_from_vl(vl) ((vl) / SVE_VQ_BYTES)
>
>  In file included from ../../gdb/aarch64-tdep.h:25,
>                   from ../../gdb/aarch64-linux-nat.c:30:
>  ../../gdb/arch/aarch64.h:80: error: "sve_vl_from_vq" redefined [-Werror]
>   #define sve_vl_from_vq(vq) ((vq) * 0x10)
>
>  In file included from /usr/include/bits/sigcontext.h:30,
>                   from /usr/include/signal.h:291,
>                   from build-gnulib/import/signal.h:52,
>                   from ../../gdb/linux-nat.h:23,
>                   from ../../gdb/aarch64-linux-nat.c:26:
>  /usr/include/asm/sigcontext.h:155: note: this is the location of the previous definition
>   #define sve_vl_from_vq(vq) ((vq) * SVE_VQ_BYTES)
>
> In order to fix this breakage, this commit guards the declaration of
> the macros using #ifndef’s.

Apologies for breaking this.
I originally had them in nat/aarch64-sve-linux-ptrace.h, within the
SVE_SIG_ZREGS_SIZE block, which does guard appropriately.

Thanks for fixing so quickly.

>  It is important to mention that the
> system headers use a value to multiply/divide the vector
> length (SVE_VQ_BYTES, defined as 16 on the system I'm using) which is
> different than the values being used by the macros defined in GDB.  I
> wasn't sure how to consolidate that, so I chose to ignore this
> discrepancy.
>

Yes, (as you pointed out in the next email), they compile down to the same.
When I moved them I changed to explicit values to remove the dependency.


> Tested by making sure GDB compiles fine again.  Since the system I'm
> using doesn't have support for SVE, there's no way I can really test
> these changes.
>

Changes work fine for me on my SVE builds.


> gdb/ChangeLog:
> 2018-06-05  Sergio Durigan Junior  <[hidden email]>
>
> * arch/aarch64.h (sve_vg_from_vl): Guard with #ifndef.
> (sve_vl_from_vg): Likewise.
> (sve_vq_from_vl): Likewise.
> (sve_vl_from_vq): Likewise.
> (sve_vq_from_vg): Likewise.
> (sve_vg_from_vq): Likewise.
> ---
> gdb/arch/aarch64.h | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
> index 9040d8d4c8..c378a4b239 100644
> --- a/gdb/arch/aarch64.h
> +++ b/gdb/arch/aarch64.h
> @@ -74,12 +74,24 @@ enum aarch64_regnum
>    VG : Vector Gradient.
> The number of 64bit chunks in an SVE Z register.  */
>
> +#ifndef sve_vg_from_vl
> #define sve_vg_from_vl(vl) ((vl) / 8)
> +#endif
> +#ifndef sve_vl_from_vg
> #define sve_vl_from_vg(vg) ((vg) * 8)
> +#endif

The guards around these first two aren’t needed. The kernel only
defines the VQ to/from VL macros - as those are the only values the
kernel cares about. Only GDB cares about VG because that is needed
for dwarf.


> +#ifndef sve_vq_from_vl
> #define sve_vq_from_vl(vl) ((vl) / 0x10)
> +#endif
> +#ifndef sve_vl_from_vq
> #define sve_vl_from_vq(vq) ((vq) * 0x10)
> +#endif

These two are fine!

> +#ifndef sve_vq_from_vg
> #define sve_vq_from_vg(vg) (sve_vq_from_vl (sve_vl_from_vg (vg)))
> +#endif
> +#ifndef sve_vg_from_vq
> #define sve_vg_from_vq(vq) (sve_vg_from_vl (sve_vl_from_vq (vq)))
> +#endif

Again these last two aren’t needed.

FYI, either today or tomorrow I’ll be posting a new set of SVE patches.
As part of that series, due to review comments, I’ll be moving the all
the linux kernel defines to two new files which contain only kernel
defines (From ptrace.h and sigcontext.h). I’ll double check this works
with new header files. Regardless of that, I think your patch should
still go in to unbreak the build until then.


Changes are good to me if the VG guards are removed (but I’m not a
maintainer so cannot officially approve it).



Alan.


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Guard declarations of 'sve_*_from_*' macros on Aarch64 (and unbreak build)

Simon Marchi-2
On 2018-06-06 03:34 AM, Alan Hayward wrote:

> FYI, either today or tomorrow I’ll be posting a new set of SVE patches.
> As part of that series, due to review comments, I’ll be moving the all
> the linux kernel defines to two new files which contain only kernel
> defines (From ptrace.h and sigcontext.h). I’ll double check this works
> with new header files. Regardless of that, I think your patch should
> still go in to unbreak the build until then.
>
>
> Changes are good to me if the VG guards are removed (but I’m not a
> maintainer so cannot officially approve it).

LGTM with Alan's proposed changes.

Simon
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Guard declarations of 'sve_*_from_*' macros on Aarch64 (and unbreak build)

Sergio Durigan Junior
In reply to this post by Alan Hayward
On Wednesday, June 06 2018, Alan Hayward wrote:

>> On 5 Jun 2018, at 23:05, Sergio Durigan Junior <[hidden email]> wrote:
>>
>> Commit 122394f1476b1c925a281b15399119500c8231c1 ("Function for reading
>> the Aarch64 SVE vector length") has added macros to manipulate SVE
>> vector sizes based on Linux kernel sources, but did not guard them
>> with #ifndef's, which breaks the build when the system headers already
>> have these macros:
>>
>>    CXX    aarch64-linux-nat.o
>>  In file included from ../../gdb/aarch64-tdep.h:25,
>>                   from ../../gdb/aarch64-linux-nat.c:30:
>>  ../../gdb/arch/aarch64.h:79: error: "sve_vq_from_vl" redefined [-Werror]
>>   #define sve_vq_from_vl(vl) ((vl) / 0x10)
>>
>>  In file included from /usr/include/bits/sigcontext.h:30,
>>                   from /usr/include/signal.h:291,
>>                   from build-gnulib/import/signal.h:52,
>>                   from ../../gdb/linux-nat.h:23,
>>                   from ../../gdb/aarch64-linux-nat.c:26:
>>  /usr/include/asm/sigcontext.h:154: note: this is the location of the previous definition
>>   #define sve_vq_from_vl(vl) ((vl) / SVE_VQ_BYTES)
>>
>>  In file included from ../../gdb/aarch64-tdep.h:25,
>>                   from ../../gdb/aarch64-linux-nat.c:30:
>>  ../../gdb/arch/aarch64.h:80: error: "sve_vl_from_vq" redefined [-Werror]
>>   #define sve_vl_from_vq(vq) ((vq) * 0x10)
>>
>>  In file included from /usr/include/bits/sigcontext.h:30,
>>                   from /usr/include/signal.h:291,
>>                   from build-gnulib/import/signal.h:52,
>>                   from ../../gdb/linux-nat.h:23,
>>                   from ../../gdb/aarch64-linux-nat.c:26:
>>  /usr/include/asm/sigcontext.h:155: note: this is the location of the previous definition
>>   #define sve_vl_from_vq(vq) ((vq) * SVE_VQ_BYTES)
>>
>> In order to fix this breakage, this commit guards the declaration of
>> the macros using #ifndef’s.
>
> Apologies for breaking this.
> I originally had them in nat/aarch64-sve-linux-ptrace.h, within the
> SVE_SIG_ZREGS_SIZE block, which does guard appropriately.
>
> Thanks for fixing so quickly.

No problem.

>> Tested by making sure GDB compiles fine again.  Since the system I'm
>> using doesn't have support for SVE, there's no way I can really test
>> these changes.
>>
>
> Changes work fine for me on my SVE builds.

Thanks for testing!

>> gdb/ChangeLog:
>> 2018-06-05  Sergio Durigan Junior  <[hidden email]>
>>
>> * arch/aarch64.h (sve_vg_from_vl): Guard with #ifndef.
>> (sve_vl_from_vg): Likewise.
>> (sve_vq_from_vl): Likewise.
>> (sve_vl_from_vq): Likewise.
>> (sve_vq_from_vg): Likewise.
>> (sve_vg_from_vq): Likewise.
>> ---
>> gdb/arch/aarch64.h | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
>> index 9040d8d4c8..c378a4b239 100644
>> --- a/gdb/arch/aarch64.h
>> +++ b/gdb/arch/aarch64.h
>> @@ -74,12 +74,24 @@ enum aarch64_regnum
>>    VG : Vector Gradient.
>> The number of 64bit chunks in an SVE Z register.  */
>>
>> +#ifndef sve_vg_from_vl
>> #define sve_vg_from_vl(vl) ((vl) / 8)
>> +#endif
>> +#ifndef sve_vl_from_vg
>> #define sve_vl_from_vg(vg) ((vg) * 8)
>> +#endif
>
> The guards around these first two aren’t needed. The kernel only
> defines the VQ to/from VL macros - as those are the only values the
> kernel cares about. Only GDB cares about VG because that is needed
> for dwarf.

Ah, fair enough.  Removed.

>> +#ifndef sve_vq_from_vl
>> #define sve_vq_from_vl(vl) ((vl) / 0x10)
>> +#endif
>> +#ifndef sve_vl_from_vq
>> #define sve_vl_from_vq(vq) ((vq) * 0x10)
>> +#endif
>
> These two are fine!
>
>> +#ifndef sve_vq_from_vg
>> #define sve_vq_from_vg(vg) (sve_vq_from_vl (sve_vl_from_vg (vg)))
>> +#endif
>> +#ifndef sve_vg_from_vq
>> #define sve_vg_from_vq(vq) (sve_vg_from_vl (sve_vl_from_vq (vq)))
>> +#endif
>
> Again these last two aren’t needed.

Removed.

On Wednesday, June 06 2018, Simon Marchi wrote:

> On 2018-06-06 03:34 AM, Alan Hayward wrote:
>> FYI, either today or tomorrow I’ll be posting a new set of SVE patches.
>> As part of that series, due to review comments, I’ll be moving the all
>> the linux kernel defines to two new files which contain only kernel
>> defines (From ptrace.h and sigcontext.h). I’ll double check this works
>> with new header files. Regardless of that, I think your patch should
>> still go in to unbreak the build until then.
>>
>>
>> Changes are good to me if the VG guards are removed (but I’m not a
>> maintainer so cannot officially approve it).
>
> LGTM with Alan's proposed changes.

Thanks, pushed:

e5a77256e8961294b3ea7d483124834311ca363b

--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
123