public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] Use aarch64_features to describe register features in target descriptions.
@ 2022-05-05 18:46 John Baldwin
  2022-05-18 13:59 ` John Baldwin
  0 siblings, 1 reply; 6+ messages in thread
From: John Baldwin @ 2022-05-05 18:46 UTC (permalink / raw)
  To: gdb-patches

Replace the sve bool member of aarch64_features with a vq member that
holds the vector quotient.  It is zero if SVE is not present.

Add std::hash<> specialization and operator== so that aarch64_features
can be used as a key with std::unordered_map<>.

Change the various functions that create or lookup aarch64 target
descriptions to accept a const aarch64_features object rather than a
growing number of arguments.

Replace the multi-dimension tdesc_aarch64_list arrays used to cache
target descriptions with unordered_maps indexed by aarch64_feature.
---
 gdb/aarch64-fbsd-nat.c           |  5 +++--
 gdb/aarch64-fbsd-tdep.c          |  5 ++++-
 gdb/aarch64-linux-nat.c          | 10 +++++----
 gdb/aarch64-linux-tdep.c         | 11 +++++----
 gdb/aarch64-tdep.c               | 21 +++++++++++-------
 gdb/aarch64-tdep.h               |  3 +--
 gdb/arch/aarch64.c               | 13 +++++------
 gdb/arch/aarch64.h               | 38 ++++++++++++++++++++++++--------
 gdbserver/linux-aarch64-ipa.cc   |  4 ++--
 gdbserver/linux-aarch64-low.cc   | 11 ++++-----
 gdbserver/linux-aarch64-tdesc.cc | 18 +++++++--------
 gdbserver/linux-aarch64-tdesc.h  |  6 +++--
 gdbserver/netbsd-aarch64-low.cc  |  2 +-
 13 files changed, 89 insertions(+), 58 deletions(-)

diff --git a/gdb/aarch64-fbsd-nat.c b/gdb/aarch64-fbsd-nat.c
index 910bf5bb190..fb7a29b5afb 100644
--- a/gdb/aarch64-fbsd-nat.c
+++ b/gdb/aarch64-fbsd-nat.c
@@ -149,8 +149,9 @@ aarch64_fbsd_nat_target::store_registers (struct regcache *regcache,
 const struct target_desc *
 aarch64_fbsd_nat_target::read_description ()
 {
-  bool tls = have_regset (inferior_ptid, NT_ARM_TLS) != 0;
-  return aarch64_read_description (0, false, false, tls);
+  aarch64_features features;
+  features.tls = have_regset (inferior_ptid, NT_ARM_TLS) != 0;
+  return aarch64_read_description (features);
 }
 
 #ifdef HAVE_DBREG
diff --git a/gdb/aarch64-fbsd-tdep.c b/gdb/aarch64-fbsd-tdep.c
index fdf0795b9bf..891546b3c64 100644
--- a/gdb/aarch64-fbsd-tdep.c
+++ b/gdb/aarch64-fbsd-tdep.c
@@ -178,7 +178,10 @@ aarch64_fbsd_core_read_description (struct gdbarch *gdbarch,
 {
   asection *tls = bfd_get_section_by_name (abfd, ".reg-aarch-tls");
 
-  return aarch64_read_description (0, false, false, tls != nullptr);
+  aarch64_features features;
+  features.tls = tls != nullptr;
+
+  return aarch64_read_description (features);
 }
 
 /* Implement the get_thread_local_address gdbarch method.  */
diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index 10b0ca10984..9cde72b247b 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -709,11 +709,13 @@ aarch64_linux_nat_target::read_description ()
   CORE_ADDR hwcap = linux_get_hwcap (this);
   CORE_ADDR hwcap2 = linux_get_hwcap2 (this);
 
-  bool pauth_p = hwcap & AARCH64_HWCAP_PACA;
-  bool mte_p = hwcap2 & HWCAP2_MTE;
+  aarch64_features features;
+  features.vq = aarch64_sve_get_vq (tid);
+  features.pauth = hwcap & AARCH64_HWCAP_PACA;
+  features.mte = hwcap2 & HWCAP2_MTE;
+  features.tls = true;
 
-  return aarch64_read_description (aarch64_sve_get_vq (tid), pauth_p, mte_p,
-				   true);
+  return aarch64_read_description (features);
 }
 
 /* Convert a native/host siginfo object, into/from the siginfo in the
diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index dbebcd4f0e0..453692df2f4 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -779,10 +779,13 @@ aarch64_linux_core_read_description (struct gdbarch *gdbarch,
   CORE_ADDR hwcap = linux_get_hwcap (target);
   CORE_ADDR hwcap2 = linux_get_hwcap2 (target);
 
-  bool pauth_p = hwcap & AARCH64_HWCAP_PACA;
-  bool mte_p = hwcap2 & HWCAP2_MTE;
-  return aarch64_read_description (aarch64_linux_core_read_vq (gdbarch, abfd),
-				   pauth_p, mte_p, tls != nullptr);
+  aarch64_features features;
+  features.vq = aarch64_linux_core_read_vq (gdbarch, abfd);
+  features.pauth = hwcap & AARCH64_HWCAP_PACA;
+  features.mte = hwcap2 & HWCAP2_MTE;
+  features.tls = tls != nullptr;
+
+  return aarch64_read_description (features);
 }
 
 /* Implementation of `gdbarch_stap_is_single_operand', as defined in
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 9d06ebfe27c..09fe8ae2fdb 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -52,13 +52,14 @@
 
 #include "opcode/aarch64.h"
 #include <algorithm>
+#include <unordered_map>
 
 /* A Homogeneous Floating-Point or Short-Vector Aggregate may have at most
    four members.  */
 #define HA_MAX_NUM_FLDS		4
 
 /* All possible aarch64 target descriptors.  */
-static target_desc *tdesc_aarch64_list[AARCH64_MAX_SVE_VQ + 1][2/*pauth*/][2 /* mte */][2 /* tls */];
+static std::unordered_map <aarch64_features, target_desc *> tdesc_aarch64_map;
 
 /* The standard register names, and all the valid aliases for them.  */
 static const struct
@@ -3332,18 +3333,18 @@ aarch64_displaced_step_hw_singlestep (struct gdbarch *gdbarch)
    TLS_P indicates the presence of the Thread Local Storage feature.  */
 
 const target_desc *
-aarch64_read_description (uint64_t vq, bool pauth_p, bool mte_p, bool tls_p)
+aarch64_read_description (const aarch64_features &features)
 {
-  if (vq > AARCH64_MAX_SVE_VQ)
-    error (_("VQ is %" PRIu64 ", maximum supported value is %d"), vq,
+  if (features.vq > AARCH64_MAX_SVE_VQ)
+    error (_("VQ is %" PRIu64 ", maximum supported value is %d"), features.vq,
 	   AARCH64_MAX_SVE_VQ);
 
-  struct target_desc *tdesc = tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p];
+  struct target_desc *tdesc = tdesc_aarch64_map[features];
 
   if (tdesc == NULL)
     {
-      tdesc = aarch64_create_target_description (vq, pauth_p, mte_p, tls_p);
-      tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p] = tdesc;
+      tdesc = aarch64_create_target_description (features);
+      tdesc_aarch64_map[features] = tdesc;
     }
 
   return tdesc;
@@ -3450,7 +3451,11 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
      value.  */
   const struct target_desc *tdesc = info.target_desc;
   if (!tdesc_has_registers (tdesc) || vq != aarch64_get_tdesc_vq (tdesc))
-    tdesc = aarch64_read_description (vq, false, false, false);
+    {
+      aarch64_features features;
+      features.vq = vq;
+      tdesc = aarch64_read_description (features);
+    }
   gdb_assert (tdesc);
 
   feature_core = tdesc_find_feature (tdesc,"org.gnu.gdb.aarch64.core");
diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
index e4cdebb6311..3b9f67d6344 100644
--- a/gdb/aarch64-tdep.h
+++ b/gdb/aarch64-tdep.h
@@ -121,8 +121,7 @@ struct aarch64_gdbarch_tdep : gdbarch_tdep
   }
 };
 
-const target_desc *aarch64_read_description (uint64_t vq, bool pauth_p,
-					     bool mte_p, bool tls_p);
+const target_desc *aarch64_read_description (const aarch64_features &features);
 
 extern int aarch64_process_record (struct gdbarch *gdbarch,
 			       struct regcache *regcache, CORE_ADDR addr);
diff --git a/gdb/arch/aarch64.c b/gdb/arch/aarch64.c
index 733a3fd6d2a..0f73286f145 100644
--- a/gdb/arch/aarch64.c
+++ b/gdb/arch/aarch64.c
@@ -29,8 +29,7 @@
 /* See arch/aarch64.h.  */
 
 target_desc *
-aarch64_create_target_description (uint64_t vq, bool pauth_p, bool mte_p,
-				   bool tls_p)
+aarch64_create_target_description (const aarch64_features &features)
 {
   target_desc_up tdesc = allocate_target_description ();
 
@@ -42,19 +41,19 @@ aarch64_create_target_description (uint64_t vq, bool pauth_p, bool mte_p,
 
   regnum = create_feature_aarch64_core (tdesc.get (), regnum);
 
-  if (vq == 0)
+  if (features.vq == 0)
     regnum = create_feature_aarch64_fpu (tdesc.get (), regnum);
   else
-    regnum = create_feature_aarch64_sve (tdesc.get (), regnum, vq);
+    regnum = create_feature_aarch64_sve (tdesc.get (), regnum, features.vq);
 
-  if (pauth_p)
+  if (features.pauth)
     regnum = create_feature_aarch64_pauth (tdesc.get (), regnum);
 
   /* Memory tagging extension registers.  */
-  if (mte_p)
+  if (features.mte)
     regnum = create_feature_aarch64_mte (tdesc.get (), regnum);
 
-  if (tls_p)
+  if (features.tls)
     regnum = create_feature_aarch64_tls (tdesc.get (), regnum);
 
   return tdesc.release ();
diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
index 8496a0341f7..72ec4193eba 100644
--- a/gdb/arch/aarch64.h
+++ b/gdb/arch/aarch64.h
@@ -26,23 +26,43 @@
    used to select register sets.  */
 struct aarch64_features
 {
-  bool sve = false;
+  /* A non zero VQ value indicates both the presence of SVE and the
+     Vector Quotient - the number of 128bit chunks in an SVE Z
+     register.  */
+  uint64_t vq = 0;
+
   bool pauth = false;
   bool mte = false;
   bool tls = false;
 };
 
-/* Create the aarch64 target description.  A non zero VQ value indicates both
-   the presence of SVE and the Vector Quotient - the number of 128bit chunks in
-   an SVE Z register.  HAS_PAUTH_P indicates the presence of the PAUTH
-   feature.
+inline bool operator==(const aarch64_features &lhs, const aarch64_features &rhs)
+{
+  return lhs.vq == rhs.vq
+    && lhs.pauth == rhs.pauth
+    && lhs.mte == rhs.mte
+    && lhs.tls == rhs.tls;
+}
 
-   MTE_P indicates the presence of the Memory Tagging Extension feature.
+template<>
+struct std::hash<aarch64_features>
+{
+  std::size_t operator()(const aarch64_features &features) const noexcept
+  {
+    std::size_t h;
 
-   TLS_P indicates the presence of the Thread Local Storage feature.  */
+    h = features.vq;
+    h = h << 1 | features.pauth;
+    h = h << 1 | features.mte;
+    h = h << 1 | features.tls;
+    return h;
+  }
+};
 
-target_desc *aarch64_create_target_description (uint64_t vq, bool has_pauth_p,
-						bool mte_p, bool tls_p);
+/* Create the aarch64 target description.  */
+
+target_desc *
+  aarch64_create_target_description (const aarch64_features &features);
 
 /* Register numbers of various important registers.
    Note that on SVE, the Z registers reuse the V register numbers and the V
diff --git a/gdbserver/linux-aarch64-ipa.cc b/gdbserver/linux-aarch64-ipa.cc
index dc907d3ff88..918f85f6ea9 100644
--- a/gdbserver/linux-aarch64-ipa.cc
+++ b/gdbserver/linux-aarch64-ipa.cc
@@ -152,7 +152,7 @@ get_raw_reg (const unsigned char *raw_regs, int regnum)
 const struct target_desc *
 get_ipa_tdesc (int idx)
 {
-  return aarch64_linux_read_description (0, false, false, false);
+  return aarch64_linux_read_description ({});
 }
 
 /* Allocate buffer for the jump pads.  The branch instruction has a reach
@@ -205,5 +205,5 @@ void
 initialize_low_tracepoint (void)
 {
   /* SVE, pauth, MTE and TLS not yet supported.  */
-  aarch64_linux_read_description (0, false, false, false);
+  aarch64_linux_read_description ({});
 }
diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
index ba0a810e0f9..db508696261 100644
--- a/gdbserver/linux-aarch64-low.cc
+++ b/gdbserver/linux-aarch64-low.cc
@@ -779,11 +779,11 @@ aarch64_adjust_register_sets (const struct aarch64_features &features)
 	  break;
 	case NT_FPREGSET:
 	  /* This is unavailable when SVE is present.  */
-	  if (!features.sve)
+	  if (features.vq == 0)
 	    regset->size = sizeof (struct user_fpsimd_state);
 	  break;
 	case NT_ARM_SVE:
-	  if (features.sve)
+	  if (features.vq > 0)
 	    regset->size = SVE_PT_SIZE (AARCH64_MAX_SVE_VQ, SVE_PT_REGS_SVE);
 	  break;
 	case NT_ARM_PAC_MASK:
@@ -824,17 +824,14 @@ aarch64_target::low_arch_setup ()
     {
       struct aarch64_features features;
 
-      uint64_t vq = aarch64_sve_get_vq (tid);
-      features.sve = (vq > 0);
+      features.vq = aarch64_sve_get_vq (tid);
       /* A-profile PAC is 64-bit only.  */
       features.pauth = linux_get_hwcap (8) & AARCH64_HWCAP_PACA;
       /* A-profile MTE is 64-bit only.  */
       features.mte = linux_get_hwcap2 (8) & HWCAP2_MTE;
       features.tls = true;
 
-      current_process ()->tdesc
-	= aarch64_linux_read_description (vq, features.pauth, features.mte,
-					  features.tls);
+      current_process ()->tdesc = aarch64_linux_read_description (features);
 
       /* Adjust the register sets we should use for this particular set of
 	 features.  */
diff --git a/gdbserver/linux-aarch64-tdesc.cc b/gdbserver/linux-aarch64-tdesc.cc
index be96612d571..93f2bedde0d 100644
--- a/gdbserver/linux-aarch64-tdesc.cc
+++ b/gdbserver/linux-aarch64-tdesc.cc
@@ -25,36 +25,36 @@
 #include "arch/aarch64.h"
 #include "linux-aarch32-low.h"
 #include <inttypes.h>
+#include <unordered_map>
 
 /* All possible aarch64 target descriptors.  */
-struct target_desc *tdesc_aarch64_list[AARCH64_MAX_SVE_VQ + 1][2/*pauth*/][2 /* mte */][2 /* tls */];
+static std::unordered_map<aarch64_features, target_desc *> tdesc_aarch64_map;
 
 /* Create the aarch64 target description.  */
 
 const target_desc *
-aarch64_linux_read_description (uint64_t vq, bool pauth_p, bool mte_p,
-				bool tls_p)
+aarch64_linux_read_description (const aarch64_features &features)
 {
-  if (vq > AARCH64_MAX_SVE_VQ)
-    error (_("VQ is %" PRIu64 ", maximum supported value is %d"), vq,
+  if (features.vq > AARCH64_MAX_SVE_VQ)
+    error (_("VQ is %" PRIu64 ", maximum supported value is %d"), features.vq,
 	   AARCH64_MAX_SVE_VQ);
 
-  struct target_desc *tdesc = tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p];
+  struct target_desc *tdesc = tdesc_aarch64_map[features];
 
   if (tdesc == NULL)
     {
-      tdesc = aarch64_create_target_description (vq, pauth_p, mte_p, tls_p);
+      tdesc = aarch64_create_target_description (features);
 
       static const char *expedite_regs_aarch64[] = { "x29", "sp", "pc", NULL };
       static const char *expedite_regs_aarch64_sve[] = { "x29", "sp", "pc",
 							 "vg", NULL };
 
-      if (vq == 0)
+      if (features.vq == 0)
 	init_target_desc (tdesc, expedite_regs_aarch64);
       else
 	init_target_desc (tdesc, expedite_regs_aarch64_sve);
 
-      tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p] = tdesc;
+      tdesc_aarch64_map[features] = tdesc;
     }
 
   return tdesc;
diff --git a/gdbserver/linux-aarch64-tdesc.h b/gdbserver/linux-aarch64-tdesc.h
index 4ab658447a2..30bcd24d13d 100644
--- a/gdbserver/linux-aarch64-tdesc.h
+++ b/gdbserver/linux-aarch64-tdesc.h
@@ -20,7 +20,9 @@
 #ifndef GDBSERVER_LINUX_AARCH64_TDESC_H
 #define GDBSERVER_LINUX_AARCH64_TDESC_H
 
-const target_desc * aarch64_linux_read_description (uint64_t vq, bool pauth_p,
-						    bool mte_p, bool tls_p);
+#include "arch/aarch64.h"
+
+const target_desc *
+  aarch64_linux_read_description (const aarch64_features &features);
 
 #endif /* GDBSERVER_LINUX_AARCH64_TDESC_H */
diff --git a/gdbserver/netbsd-aarch64-low.cc b/gdbserver/netbsd-aarch64-low.cc
index b371e599232..f8447b0d1ee 100644
--- a/gdbserver/netbsd-aarch64-low.cc
+++ b/gdbserver/netbsd-aarch64-low.cc
@@ -96,7 +96,7 @@ void
 netbsd_aarch64_target::low_arch_setup ()
 {
   target_desc *tdesc
-    = aarch64_create_target_description (0, false, false, false);
+    = aarch64_create_target_description ({});
 
   static const char *expedite_regs_aarch64[] = { "x29", "sp", "pc", NULL };
   init_target_desc (tdesc, expedite_regs_aarch64);
-- 
2.34.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Use aarch64_features to describe register features in target descriptions.
  2022-05-05 18:46 [PATCH] Use aarch64_features to describe register features in target descriptions John Baldwin
@ 2022-05-18 13:59 ` John Baldwin
  2022-05-18 14:23   ` Luis Machado
  2022-05-20 11:52   ` Luis Machado
  0 siblings, 2 replies; 6+ messages in thread
From: John Baldwin @ 2022-05-18 13:59 UTC (permalink / raw)
  To: gdb-patches

On 5/5/22 11:46 AM, John Baldwin wrote:
> Replace the sve bool member of aarch64_features with a vq member that
> holds the vector quotient.  It is zero if SVE is not present.
> 
> Add std::hash<> specialization and operator== so that aarch64_features
> can be used as a key with std::unordered_map<>.
> 
> Change the various functions that create or lookup aarch64 target
> descriptions to accept a const aarch64_features object rather than a
> growing number of arguments.
> 
> Replace the multi-dimension tdesc_aarch64_list arrays used to cache
> target descriptions with unordered_maps indexed by aarch64_feature.

Ping?  (Sorry, I thought I had cc'd Luis on this when I sent it, but it seems
like it was only sent to the list)

> ---
>   gdb/aarch64-fbsd-nat.c           |  5 +++--
>   gdb/aarch64-fbsd-tdep.c          |  5 ++++-
>   gdb/aarch64-linux-nat.c          | 10 +++++----
>   gdb/aarch64-linux-tdep.c         | 11 +++++----
>   gdb/aarch64-tdep.c               | 21 +++++++++++-------
>   gdb/aarch64-tdep.h               |  3 +--
>   gdb/arch/aarch64.c               | 13 +++++------
>   gdb/arch/aarch64.h               | 38 ++++++++++++++++++++++++--------
>   gdbserver/linux-aarch64-ipa.cc   |  4 ++--
>   gdbserver/linux-aarch64-low.cc   | 11 ++++-----
>   gdbserver/linux-aarch64-tdesc.cc | 18 +++++++--------
>   gdbserver/linux-aarch64-tdesc.h  |  6 +++--
>   gdbserver/netbsd-aarch64-low.cc  |  2 +-
>   13 files changed, 89 insertions(+), 58 deletions(-)
> 
> diff --git a/gdb/aarch64-fbsd-nat.c b/gdb/aarch64-fbsd-nat.c
> index 910bf5bb190..fb7a29b5afb 100644
> --- a/gdb/aarch64-fbsd-nat.c
> +++ b/gdb/aarch64-fbsd-nat.c
> @@ -149,8 +149,9 @@ aarch64_fbsd_nat_target::store_registers (struct regcache *regcache,
>   const struct target_desc *
>   aarch64_fbsd_nat_target::read_description ()
>   {
> -  bool tls = have_regset (inferior_ptid, NT_ARM_TLS) != 0;
> -  return aarch64_read_description (0, false, false, tls);
> +  aarch64_features features;
> +  features.tls = have_regset (inferior_ptid, NT_ARM_TLS) != 0;
> +  return aarch64_read_description (features);
>   }
>   
>   #ifdef HAVE_DBREG
> diff --git a/gdb/aarch64-fbsd-tdep.c b/gdb/aarch64-fbsd-tdep.c
> index fdf0795b9bf..891546b3c64 100644
> --- a/gdb/aarch64-fbsd-tdep.c
> +++ b/gdb/aarch64-fbsd-tdep.c
> @@ -178,7 +178,10 @@ aarch64_fbsd_core_read_description (struct gdbarch *gdbarch,
>   {
>     asection *tls = bfd_get_section_by_name (abfd, ".reg-aarch-tls");
>   
> -  return aarch64_read_description (0, false, false, tls != nullptr);
> +  aarch64_features features;
> +  features.tls = tls != nullptr;
> +
> +  return aarch64_read_description (features);
>   }
>   
>   /* Implement the get_thread_local_address gdbarch method.  */
> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index 10b0ca10984..9cde72b247b 100644
> --- a/gdb/aarch64-linux-nat.c
> +++ b/gdb/aarch64-linux-nat.c
> @@ -709,11 +709,13 @@ aarch64_linux_nat_target::read_description ()
>     CORE_ADDR hwcap = linux_get_hwcap (this);
>     CORE_ADDR hwcap2 = linux_get_hwcap2 (this);
>   
> -  bool pauth_p = hwcap & AARCH64_HWCAP_PACA;
> -  bool mte_p = hwcap2 & HWCAP2_MTE;
> +  aarch64_features features;
> +  features.vq = aarch64_sve_get_vq (tid);
> +  features.pauth = hwcap & AARCH64_HWCAP_PACA;
> +  features.mte = hwcap2 & HWCAP2_MTE;
> +  features.tls = true;
>   
> -  return aarch64_read_description (aarch64_sve_get_vq (tid), pauth_p, mte_p,
> -				   true);
> +  return aarch64_read_description (features);
>   }
>   
>   /* Convert a native/host siginfo object, into/from the siginfo in the
> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
> index dbebcd4f0e0..453692df2f4 100644
> --- a/gdb/aarch64-linux-tdep.c
> +++ b/gdb/aarch64-linux-tdep.c
> @@ -779,10 +779,13 @@ aarch64_linux_core_read_description (struct gdbarch *gdbarch,
>     CORE_ADDR hwcap = linux_get_hwcap (target);
>     CORE_ADDR hwcap2 = linux_get_hwcap2 (target);
>   
> -  bool pauth_p = hwcap & AARCH64_HWCAP_PACA;
> -  bool mte_p = hwcap2 & HWCAP2_MTE;
> -  return aarch64_read_description (aarch64_linux_core_read_vq (gdbarch, abfd),
> -				   pauth_p, mte_p, tls != nullptr);
> +  aarch64_features features;
> +  features.vq = aarch64_linux_core_read_vq (gdbarch, abfd);
> +  features.pauth = hwcap & AARCH64_HWCAP_PACA;
> +  features.mte = hwcap2 & HWCAP2_MTE;
> +  features.tls = tls != nullptr;
> +
> +  return aarch64_read_description (features);
>   }
>   
>   /* Implementation of `gdbarch_stap_is_single_operand', as defined in
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 9d06ebfe27c..09fe8ae2fdb 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -52,13 +52,14 @@
>   
>   #include "opcode/aarch64.h"
>   #include <algorithm>
> +#include <unordered_map>
>   
>   /* A Homogeneous Floating-Point or Short-Vector Aggregate may have at most
>      four members.  */
>   #define HA_MAX_NUM_FLDS		4
>   
>   /* All possible aarch64 target descriptors.  */
> -static target_desc *tdesc_aarch64_list[AARCH64_MAX_SVE_VQ + 1][2/*pauth*/][2 /* mte */][2 /* tls */];
> +static std::unordered_map <aarch64_features, target_desc *> tdesc_aarch64_map;
>   
>   /* The standard register names, and all the valid aliases for them.  */
>   static const struct
> @@ -3332,18 +3333,18 @@ aarch64_displaced_step_hw_singlestep (struct gdbarch *gdbarch)
>      TLS_P indicates the presence of the Thread Local Storage feature.  */
>   
>   const target_desc *
> -aarch64_read_description (uint64_t vq, bool pauth_p, bool mte_p, bool tls_p)
> +aarch64_read_description (const aarch64_features &features)
>   {
> -  if (vq > AARCH64_MAX_SVE_VQ)
> -    error (_("VQ is %" PRIu64 ", maximum supported value is %d"), vq,
> +  if (features.vq > AARCH64_MAX_SVE_VQ)
> +    error (_("VQ is %" PRIu64 ", maximum supported value is %d"), features.vq,
>   	   AARCH64_MAX_SVE_VQ);
>   
> -  struct target_desc *tdesc = tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p];
> +  struct target_desc *tdesc = tdesc_aarch64_map[features];
>   
>     if (tdesc == NULL)
>       {
> -      tdesc = aarch64_create_target_description (vq, pauth_p, mte_p, tls_p);
> -      tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p] = tdesc;
> +      tdesc = aarch64_create_target_description (features);
> +      tdesc_aarch64_map[features] = tdesc;
>       }
>   
>     return tdesc;
> @@ -3450,7 +3451,11 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>        value.  */
>     const struct target_desc *tdesc = info.target_desc;
>     if (!tdesc_has_registers (tdesc) || vq != aarch64_get_tdesc_vq (tdesc))
> -    tdesc = aarch64_read_description (vq, false, false, false);
> +    {
> +      aarch64_features features;
> +      features.vq = vq;
> +      tdesc = aarch64_read_description (features);
> +    }
>     gdb_assert (tdesc);
>   
>     feature_core = tdesc_find_feature (tdesc,"org.gnu.gdb.aarch64.core");
> diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
> index e4cdebb6311..3b9f67d6344 100644
> --- a/gdb/aarch64-tdep.h
> +++ b/gdb/aarch64-tdep.h
> @@ -121,8 +121,7 @@ struct aarch64_gdbarch_tdep : gdbarch_tdep
>     }
>   };
>   
> -const target_desc *aarch64_read_description (uint64_t vq, bool pauth_p,
> -					     bool mte_p, bool tls_p);
> +const target_desc *aarch64_read_description (const aarch64_features &features);
>   
>   extern int aarch64_process_record (struct gdbarch *gdbarch,
>   			       struct regcache *regcache, CORE_ADDR addr);
> diff --git a/gdb/arch/aarch64.c b/gdb/arch/aarch64.c
> index 733a3fd6d2a..0f73286f145 100644
> --- a/gdb/arch/aarch64.c
> +++ b/gdb/arch/aarch64.c
> @@ -29,8 +29,7 @@
>   /* See arch/aarch64.h.  */
>   
>   target_desc *
> -aarch64_create_target_description (uint64_t vq, bool pauth_p, bool mte_p,
> -				   bool tls_p)
> +aarch64_create_target_description (const aarch64_features &features)
>   {
>     target_desc_up tdesc = allocate_target_description ();
>   
> @@ -42,19 +41,19 @@ aarch64_create_target_description (uint64_t vq, bool pauth_p, bool mte_p,
>   
>     regnum = create_feature_aarch64_core (tdesc.get (), regnum);
>   
> -  if (vq == 0)
> +  if (features.vq == 0)
>       regnum = create_feature_aarch64_fpu (tdesc.get (), regnum);
>     else
> -    regnum = create_feature_aarch64_sve (tdesc.get (), regnum, vq);
> +    regnum = create_feature_aarch64_sve (tdesc.get (), regnum, features.vq);
>   
> -  if (pauth_p)
> +  if (features.pauth)
>       regnum = create_feature_aarch64_pauth (tdesc.get (), regnum);
>   
>     /* Memory tagging extension registers.  */
> -  if (mte_p)
> +  if (features.mte)
>       regnum = create_feature_aarch64_mte (tdesc.get (), regnum);
>   
> -  if (tls_p)
> +  if (features.tls)
>       regnum = create_feature_aarch64_tls (tdesc.get (), regnum);
>   
>     return tdesc.release ();
> diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
> index 8496a0341f7..72ec4193eba 100644
> --- a/gdb/arch/aarch64.h
> +++ b/gdb/arch/aarch64.h
> @@ -26,23 +26,43 @@
>      used to select register sets.  */
>   struct aarch64_features
>   {
> -  bool sve = false;
> +  /* A non zero VQ value indicates both the presence of SVE and the
> +     Vector Quotient - the number of 128bit chunks in an SVE Z
> +     register.  */
> +  uint64_t vq = 0;
> +
>     bool pauth = false;
>     bool mte = false;
>     bool tls = false;
>   };
>   
> -/* Create the aarch64 target description.  A non zero VQ value indicates both
> -   the presence of SVE and the Vector Quotient - the number of 128bit chunks in
> -   an SVE Z register.  HAS_PAUTH_P indicates the presence of the PAUTH
> -   feature.
> +inline bool operator==(const aarch64_features &lhs, const aarch64_features &rhs)
> +{
> +  return lhs.vq == rhs.vq
> +    && lhs.pauth == rhs.pauth
> +    && lhs.mte == rhs.mte
> +    && lhs.tls == rhs.tls;
> +}
>   
> -   MTE_P indicates the presence of the Memory Tagging Extension feature.
> +template<>
> +struct std::hash<aarch64_features>
> +{
> +  std::size_t operator()(const aarch64_features &features) const noexcept
> +  {
> +    std::size_t h;
>   
> -   TLS_P indicates the presence of the Thread Local Storage feature.  */
> +    h = features.vq;
> +    h = h << 1 | features.pauth;
> +    h = h << 1 | features.mte;
> +    h = h << 1 | features.tls;
> +    return h;
> +  }
> +};
>   
> -target_desc *aarch64_create_target_description (uint64_t vq, bool has_pauth_p,
> -						bool mte_p, bool tls_p);
> +/* Create the aarch64 target description.  */
> +
> +target_desc *
> +  aarch64_create_target_description (const aarch64_features &features);
>   
>   /* Register numbers of various important registers.
>      Note that on SVE, the Z registers reuse the V register numbers and the V
> diff --git a/gdbserver/linux-aarch64-ipa.cc b/gdbserver/linux-aarch64-ipa.cc
> index dc907d3ff88..918f85f6ea9 100644
> --- a/gdbserver/linux-aarch64-ipa.cc
> +++ b/gdbserver/linux-aarch64-ipa.cc
> @@ -152,7 +152,7 @@ get_raw_reg (const unsigned char *raw_regs, int regnum)
>   const struct target_desc *
>   get_ipa_tdesc (int idx)
>   {
> -  return aarch64_linux_read_description (0, false, false, false);
> +  return aarch64_linux_read_description ({});
>   }
>   
>   /* Allocate buffer for the jump pads.  The branch instruction has a reach
> @@ -205,5 +205,5 @@ void
>   initialize_low_tracepoint (void)
>   {
>     /* SVE, pauth, MTE and TLS not yet supported.  */
> -  aarch64_linux_read_description (0, false, false, false);
> +  aarch64_linux_read_description ({});
>   }
> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
> index ba0a810e0f9..db508696261 100644
> --- a/gdbserver/linux-aarch64-low.cc
> +++ b/gdbserver/linux-aarch64-low.cc
> @@ -779,11 +779,11 @@ aarch64_adjust_register_sets (const struct aarch64_features &features)
>   	  break;
>   	case NT_FPREGSET:
>   	  /* This is unavailable when SVE is present.  */
> -	  if (!features.sve)
> +	  if (features.vq == 0)
>   	    regset->size = sizeof (struct user_fpsimd_state);
>   	  break;
>   	case NT_ARM_SVE:
> -	  if (features.sve)
> +	  if (features.vq > 0)
>   	    regset->size = SVE_PT_SIZE (AARCH64_MAX_SVE_VQ, SVE_PT_REGS_SVE);
>   	  break;
>   	case NT_ARM_PAC_MASK:
> @@ -824,17 +824,14 @@ aarch64_target::low_arch_setup ()
>       {
>         struct aarch64_features features;
>   
> -      uint64_t vq = aarch64_sve_get_vq (tid);
> -      features.sve = (vq > 0);
> +      features.vq = aarch64_sve_get_vq (tid);
>         /* A-profile PAC is 64-bit only.  */
>         features.pauth = linux_get_hwcap (8) & AARCH64_HWCAP_PACA;
>         /* A-profile MTE is 64-bit only.  */
>         features.mte = linux_get_hwcap2 (8) & HWCAP2_MTE;
>         features.tls = true;
>   
> -      current_process ()->tdesc
> -	= aarch64_linux_read_description (vq, features.pauth, features.mte,
> -					  features.tls);
> +      current_process ()->tdesc = aarch64_linux_read_description (features);
>   
>         /* Adjust the register sets we should use for this particular set of
>   	 features.  */
> diff --git a/gdbserver/linux-aarch64-tdesc.cc b/gdbserver/linux-aarch64-tdesc.cc
> index be96612d571..93f2bedde0d 100644
> --- a/gdbserver/linux-aarch64-tdesc.cc
> +++ b/gdbserver/linux-aarch64-tdesc.cc
> @@ -25,36 +25,36 @@
>   #include "arch/aarch64.h"
>   #include "linux-aarch32-low.h"
>   #include <inttypes.h>
> +#include <unordered_map>
>   
>   /* All possible aarch64 target descriptors.  */
> -struct target_desc *tdesc_aarch64_list[AARCH64_MAX_SVE_VQ + 1][2/*pauth*/][2 /* mte */][2 /* tls */];
> +static std::unordered_map<aarch64_features, target_desc *> tdesc_aarch64_map;
>   
>   /* Create the aarch64 target description.  */
>   
>   const target_desc *
> -aarch64_linux_read_description (uint64_t vq, bool pauth_p, bool mte_p,
> -				bool tls_p)
> +aarch64_linux_read_description (const aarch64_features &features)
>   {
> -  if (vq > AARCH64_MAX_SVE_VQ)
> -    error (_("VQ is %" PRIu64 ", maximum supported value is %d"), vq,
> +  if (features.vq > AARCH64_MAX_SVE_VQ)
> +    error (_("VQ is %" PRIu64 ", maximum supported value is %d"), features.vq,
>   	   AARCH64_MAX_SVE_VQ);
>   
> -  struct target_desc *tdesc = tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p];
> +  struct target_desc *tdesc = tdesc_aarch64_map[features];
>   
>     if (tdesc == NULL)
>       {
> -      tdesc = aarch64_create_target_description (vq, pauth_p, mte_p, tls_p);
> +      tdesc = aarch64_create_target_description (features);
>   
>         static const char *expedite_regs_aarch64[] = { "x29", "sp", "pc", NULL };
>         static const char *expedite_regs_aarch64_sve[] = { "x29", "sp", "pc",
>   							 "vg", NULL };
>   
> -      if (vq == 0)
> +      if (features.vq == 0)
>   	init_target_desc (tdesc, expedite_regs_aarch64);
>         else
>   	init_target_desc (tdesc, expedite_regs_aarch64_sve);
>   
> -      tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p] = tdesc;
> +      tdesc_aarch64_map[features] = tdesc;
>       }
>   
>     return tdesc;
> diff --git a/gdbserver/linux-aarch64-tdesc.h b/gdbserver/linux-aarch64-tdesc.h
> index 4ab658447a2..30bcd24d13d 100644
> --- a/gdbserver/linux-aarch64-tdesc.h
> +++ b/gdbserver/linux-aarch64-tdesc.h
> @@ -20,7 +20,9 @@
>   #ifndef GDBSERVER_LINUX_AARCH64_TDESC_H
>   #define GDBSERVER_LINUX_AARCH64_TDESC_H
>   
> -const target_desc * aarch64_linux_read_description (uint64_t vq, bool pauth_p,
> -						    bool mte_p, bool tls_p);
> +#include "arch/aarch64.h"
> +
> +const target_desc *
> +  aarch64_linux_read_description (const aarch64_features &features);
>   
>   #endif /* GDBSERVER_LINUX_AARCH64_TDESC_H */
> diff --git a/gdbserver/netbsd-aarch64-low.cc b/gdbserver/netbsd-aarch64-low.cc
> index b371e599232..f8447b0d1ee 100644
> --- a/gdbserver/netbsd-aarch64-low.cc
> +++ b/gdbserver/netbsd-aarch64-low.cc
> @@ -96,7 +96,7 @@ void
>   netbsd_aarch64_target::low_arch_setup ()
>   {
>     target_desc *tdesc
> -    = aarch64_create_target_description (0, false, false, false);
> +    = aarch64_create_target_description ({});
>   
>     static const char *expedite_regs_aarch64[] = { "x29", "sp", "pc", NULL };
>     init_target_desc (tdesc, expedite_regs_aarch64);


-- 
John Baldwin

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Use aarch64_features to describe register features in target descriptions.
  2022-05-18 13:59 ` John Baldwin
@ 2022-05-18 14:23   ` Luis Machado
  2022-05-20 11:52   ` Luis Machado
  1 sibling, 0 replies; 6+ messages in thread
From: Luis Machado @ 2022-05-18 14:23 UTC (permalink / raw)
  To: John Baldwin, gdb-patches

Hi!

On 5/18/22 14:59, John Baldwin wrote:
> On 5/5/22 11:46 AM, John Baldwin wrote:
>> Replace the sve bool member of aarch64_features with a vq member that
>> holds the vector quotient.  It is zero if SVE is not present.
>>
>> Add std::hash<> specialization and operator== so that aarch64_features
>> can be used as a key with std::unordered_map<>.
>>
>> Change the various functions that create or lookup aarch64 target
>> descriptions to accept a const aarch64_features object rather than a
>> growing number of arguments.
>>
>> Replace the multi-dimension tdesc_aarch64_list arrays used to cache
>> target descriptions with unordered_maps indexed by aarch64_feature.
> 
> Ping?  (Sorry, I thought I had cc'd Luis on this when I sent it, but it seems
> like it was only sent to the list)

Sorry I didn't notice this earlier. Sometimes the inbound server drops my copy of the e-mail and it
only makes its way to the list.

> 
>> ---
>>   gdb/aarch64-fbsd-nat.c           |  5 +++--
>>   gdb/aarch64-fbsd-tdep.c          |  5 ++++-
>>   gdb/aarch64-linux-nat.c          | 10 +++++----
>>   gdb/aarch64-linux-tdep.c         | 11 +++++----
>>   gdb/aarch64-tdep.c               | 21 +++++++++++-------
>>   gdb/aarch64-tdep.h               |  3 +--
>>   gdb/arch/aarch64.c               | 13 +++++------
>>   gdb/arch/aarch64.h               | 38 ++++++++++++++++++++++++--------
>>   gdbserver/linux-aarch64-ipa.cc   |  4 ++--
>>   gdbserver/linux-aarch64-low.cc   | 11 ++++-----
>>   gdbserver/linux-aarch64-tdesc.cc | 18 +++++++--------
>>   gdbserver/linux-aarch64-tdesc.h  |  6 +++--
>>   gdbserver/netbsd-aarch64-low.cc  |  2 +-
>>   13 files changed, 89 insertions(+), 58 deletions(-)
>>
>> diff --git a/gdb/aarch64-fbsd-nat.c b/gdb/aarch64-fbsd-nat.c
>> index 910bf5bb190..fb7a29b5afb 100644
>> --- a/gdb/aarch64-fbsd-nat.c
>> +++ b/gdb/aarch64-fbsd-nat.c
>> @@ -149,8 +149,9 @@ aarch64_fbsd_nat_target::store_registers (struct regcache *regcache,
>>   const struct target_desc *
>>   aarch64_fbsd_nat_target::read_description ()
>>   {
>> -  bool tls = have_regset (inferior_ptid, NT_ARM_TLS) != 0;
>> -  return aarch64_read_description (0, false, false, tls);
>> +  aarch64_features features;
>> +  features.tls = have_regset (inferior_ptid, NT_ARM_TLS) != 0;
>> +  return aarch64_read_description (features);
>>   }
>>   #ifdef HAVE_DBREG
>> diff --git a/gdb/aarch64-fbsd-tdep.c b/gdb/aarch64-fbsd-tdep.c
>> index fdf0795b9bf..891546b3c64 100644
>> --- a/gdb/aarch64-fbsd-tdep.c
>> +++ b/gdb/aarch64-fbsd-tdep.c
>> @@ -178,7 +178,10 @@ aarch64_fbsd_core_read_description (struct gdbarch *gdbarch,
>>   {
>>     asection *tls = bfd_get_section_by_name (abfd, ".reg-aarch-tls");
>> -  return aarch64_read_description (0, false, false, tls != nullptr);
>> +  aarch64_features features;
>> +  features.tls = tls != nullptr;
>> +
>> +  return aarch64_read_description (features);
>>   }
>>   /* Implement the get_thread_local_address gdbarch method.  */
>> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
>> index 10b0ca10984..9cde72b247b 100644
>> --- a/gdb/aarch64-linux-nat.c
>> +++ b/gdb/aarch64-linux-nat.c
>> @@ -709,11 +709,13 @@ aarch64_linux_nat_target::read_description ()
>>     CORE_ADDR hwcap = linux_get_hwcap (this);
>>     CORE_ADDR hwcap2 = linux_get_hwcap2 (this);
>> -  bool pauth_p = hwcap & AARCH64_HWCAP_PACA;
>> -  bool mte_p = hwcap2 & HWCAP2_MTE;
>> +  aarch64_features features;
>> +  features.vq = aarch64_sve_get_vq (tid);
>> +  features.pauth = hwcap & AARCH64_HWCAP_PACA;
>> +  features.mte = hwcap2 & HWCAP2_MTE;
>> +  features.tls = true;
>> -  return aarch64_read_description (aarch64_sve_get_vq (tid), pauth_p, mte_p,
>> -                   true);
>> +  return aarch64_read_description (features);
>>   }
>>   /* Convert a native/host siginfo object, into/from the siginfo in the
>> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
>> index dbebcd4f0e0..453692df2f4 100644
>> --- a/gdb/aarch64-linux-tdep.c
>> +++ b/gdb/aarch64-linux-tdep.c
>> @@ -779,10 +779,13 @@ aarch64_linux_core_read_description (struct gdbarch *gdbarch,
>>     CORE_ADDR hwcap = linux_get_hwcap (target);
>>     CORE_ADDR hwcap2 = linux_get_hwcap2 (target);
>> -  bool pauth_p = hwcap & AARCH64_HWCAP_PACA;
>> -  bool mte_p = hwcap2 & HWCAP2_MTE;
>> -  return aarch64_read_description (aarch64_linux_core_read_vq (gdbarch, abfd),
>> -                   pauth_p, mte_p, tls != nullptr);
>> +  aarch64_features features;
>> +  features.vq = aarch64_linux_core_read_vq (gdbarch, abfd);
>> +  features.pauth = hwcap & AARCH64_HWCAP_PACA;
>> +  features.mte = hwcap2 & HWCAP2_MTE;
>> +  features.tls = tls != nullptr;
>> +
>> +  return aarch64_read_description (features);
>>   }
>>   /* Implementation of `gdbarch_stap_is_single_operand', as defined in
>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>> index 9d06ebfe27c..09fe8ae2fdb 100644
>> --- a/gdb/aarch64-tdep.c
>> +++ b/gdb/aarch64-tdep.c
>> @@ -52,13 +52,14 @@
>>   #include "opcode/aarch64.h"
>>   #include <algorithm>
>> +#include <unordered_map>
>>   /* A Homogeneous Floating-Point or Short-Vector Aggregate may have at most
>>      four members.  */
>>   #define HA_MAX_NUM_FLDS        4
>>   /* All possible aarch64 target descriptors.  */
>> -static target_desc *tdesc_aarch64_list[AARCH64_MAX_SVE_VQ + 1][2/*pauth*/][2 /* mte */][2 /* tls */];
>> +static std::unordered_map <aarch64_features, target_desc *> tdesc_aarch64_map;
>>   /* The standard register names, and all the valid aliases for them.  */
>>   static const struct
>> @@ -3332,18 +3333,18 @@ aarch64_displaced_step_hw_singlestep (struct gdbarch *gdbarch)
>>      TLS_P indicates the presence of the Thread Local Storage feature.  */
>>   const target_desc *
>> -aarch64_read_description (uint64_t vq, bool pauth_p, bool mte_p, bool tls_p)
>> +aarch64_read_description (const aarch64_features &features)
>>   {
>> -  if (vq > AARCH64_MAX_SVE_VQ)
>> -    error (_("VQ is %" PRIu64 ", maximum supported value is %d"), vq,
>> +  if (features.vq > AARCH64_MAX_SVE_VQ)
>> +    error (_("VQ is %" PRIu64 ", maximum supported value is %d"), features.vq,
>>          AARCH64_MAX_SVE_VQ);
>> -  struct target_desc *tdesc = tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p];
>> +  struct target_desc *tdesc = tdesc_aarch64_map[features];
>>     if (tdesc == NULL)
>>       {
>> -      tdesc = aarch64_create_target_description (vq, pauth_p, mte_p, tls_p);
>> -      tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p] = tdesc;
>> +      tdesc = aarch64_create_target_description (features);
>> +      tdesc_aarch64_map[features] = tdesc;
>>       }
>>     return tdesc;
>> @@ -3450,7 +3451,11 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>>        value.  */
>>     const struct target_desc *tdesc = info.target_desc;
>>     if (!tdesc_has_registers (tdesc) || vq != aarch64_get_tdesc_vq (tdesc))
>> -    tdesc = aarch64_read_description (vq, false, false, false);
>> +    {
>> +      aarch64_features features;
>> +      features.vq = vq;
>> +      tdesc = aarch64_read_description (features);
>> +    }
>>     gdb_assert (tdesc);
>>     feature_core = tdesc_find_feature (tdesc,"org.gnu.gdb.aarch64.core");
>> diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
>> index e4cdebb6311..3b9f67d6344 100644
>> --- a/gdb/aarch64-tdep.h
>> +++ b/gdb/aarch64-tdep.h
>> @@ -121,8 +121,7 @@ struct aarch64_gdbarch_tdep : gdbarch_tdep
>>     }
>>   };
>> -const target_desc *aarch64_read_description (uint64_t vq, bool pauth_p,
>> -                         bool mte_p, bool tls_p);
>> +const target_desc *aarch64_read_description (const aarch64_features &features);
>>   extern int aarch64_process_record (struct gdbarch *gdbarch,
>>                      struct regcache *regcache, CORE_ADDR addr);
>> diff --git a/gdb/arch/aarch64.c b/gdb/arch/aarch64.c
>> index 733a3fd6d2a..0f73286f145 100644
>> --- a/gdb/arch/aarch64.c
>> +++ b/gdb/arch/aarch64.c
>> @@ -29,8 +29,7 @@
>>   /* See arch/aarch64.h.  */
>>   target_desc *
>> -aarch64_create_target_description (uint64_t vq, bool pauth_p, bool mte_p,
>> -                   bool tls_p)
>> +aarch64_create_target_description (const aarch64_features &features)
>>   {
>>     target_desc_up tdesc = allocate_target_description ();
>> @@ -42,19 +41,19 @@ aarch64_create_target_description (uint64_t vq, bool pauth_p, bool mte_p,
>>     regnum = create_feature_aarch64_core (tdesc.get (), regnum);
>> -  if (vq == 0)
>> +  if (features.vq == 0)
>>       regnum = create_feature_aarch64_fpu (tdesc.get (), regnum);
>>     else
>> -    regnum = create_feature_aarch64_sve (tdesc.get (), regnum, vq);
>> +    regnum = create_feature_aarch64_sve (tdesc.get (), regnum, features.vq);
>> -  if (pauth_p)
>> +  if (features.pauth)
>>       regnum = create_feature_aarch64_pauth (tdesc.get (), regnum);
>>     /* Memory tagging extension registers.  */
>> -  if (mte_p)
>> +  if (features.mte)
>>       regnum = create_feature_aarch64_mte (tdesc.get (), regnum);
>> -  if (tls_p)
>> +  if (features.tls)
>>       regnum = create_feature_aarch64_tls (tdesc.get (), regnum);
>>     return tdesc.release ();
>> diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
>> index 8496a0341f7..72ec4193eba 100644
>> --- a/gdb/arch/aarch64.h
>> +++ b/gdb/arch/aarch64.h
>> @@ -26,23 +26,43 @@
>>      used to select register sets.  */
>>   struct aarch64_features
>>   {
>> -  bool sve = false;
>> +  /* A non zero VQ value indicates both the presence of SVE and the
>> +     Vector Quotient - the number of 128bit chunks in an SVE Z
>> +     register.  */
>> +  uint64_t vq = 0;
>> +
>>     bool pauth = false;
>>     bool mte = false;
>>     bool tls = false;
>>   };
>> -/* Create the aarch64 target description.  A non zero VQ value indicates both
>> -   the presence of SVE and the Vector Quotient - the number of 128bit chunks in
>> -   an SVE Z register.  HAS_PAUTH_P indicates the presence of the PAUTH
>> -   feature.
>> +inline bool operator==(const aarch64_features &lhs, const aarch64_features &rhs)
>> +{
>> +  return lhs.vq == rhs.vq
>> +    && lhs.pauth == rhs.pauth
>> +    && lhs.mte == rhs.mte
>> +    && lhs.tls == rhs.tls;
>> +}
>> -   MTE_P indicates the presence of the Memory Tagging Extension feature.
>> +template<>
>> +struct std::hash<aarch64_features>
>> +{
>> +  std::size_t operator()(const aarch64_features &features) const noexcept
>> +  {
>> +    std::size_t h;
>> -   TLS_P indicates the presence of the Thread Local Storage feature.  */
>> +    h = features.vq;
>> +    h = h << 1 | features.pauth;
>> +    h = h << 1 | features.mte;
>> +    h = h << 1 | features.tls;
>> +    return h;
>> +  }
>> +};
>> -target_desc *aarch64_create_target_description (uint64_t vq, bool has_pauth_p,
>> -                        bool mte_p, bool tls_p);
>> +/* Create the aarch64 target description.  */
>> +
>> +target_desc *
>> +  aarch64_create_target_description (const aarch64_features &features);
>>   /* Register numbers of various important registers.
>>      Note that on SVE, the Z registers reuse the V register numbers and the V
>> diff --git a/gdbserver/linux-aarch64-ipa.cc b/gdbserver/linux-aarch64-ipa.cc
>> index dc907d3ff88..918f85f6ea9 100644
>> --- a/gdbserver/linux-aarch64-ipa.cc
>> +++ b/gdbserver/linux-aarch64-ipa.cc
>> @@ -152,7 +152,7 @@ get_raw_reg (const unsigned char *raw_regs, int regnum)
>>   const struct target_desc *
>>   get_ipa_tdesc (int idx)
>>   {
>> -  return aarch64_linux_read_description (0, false, false, false);
>> +  return aarch64_linux_read_description ({});
>>   }
>>   /* Allocate buffer for the jump pads.  The branch instruction has a reach
>> @@ -205,5 +205,5 @@ void
>>   initialize_low_tracepoint (void)
>>   {
>>     /* SVE, pauth, MTE and TLS not yet supported.  */
>> -  aarch64_linux_read_description (0, false, false, false);
>> +  aarch64_linux_read_description ({});
>>   }
>> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
>> index ba0a810e0f9..db508696261 100644
>> --- a/gdbserver/linux-aarch64-low.cc
>> +++ b/gdbserver/linux-aarch64-low.cc
>> @@ -779,11 +779,11 @@ aarch64_adjust_register_sets (const struct aarch64_features &features)
>>         break;
>>       case NT_FPREGSET:
>>         /* This is unavailable when SVE is present.  */
>> -      if (!features.sve)
>> +      if (features.vq == 0)
>>           regset->size = sizeof (struct user_fpsimd_state);
>>         break;
>>       case NT_ARM_SVE:
>> -      if (features.sve)
>> +      if (features.vq > 0)
>>           regset->size = SVE_PT_SIZE (AARCH64_MAX_SVE_VQ, SVE_PT_REGS_SVE);
>>         break;
>>       case NT_ARM_PAC_MASK:
>> @@ -824,17 +824,14 @@ aarch64_target::low_arch_setup ()
>>       {
>>         struct aarch64_features features;
>> -      uint64_t vq = aarch64_sve_get_vq (tid);
>> -      features.sve = (vq > 0);
>> +      features.vq = aarch64_sve_get_vq (tid);
>>         /* A-profile PAC is 64-bit only.  */
>>         features.pauth = linux_get_hwcap (8) & AARCH64_HWCAP_PACA;
>>         /* A-profile MTE is 64-bit only.  */
>>         features.mte = linux_get_hwcap2 (8) & HWCAP2_MTE;
>>         features.tls = true;
>> -      current_process ()->tdesc
>> -    = aarch64_linux_read_description (vq, features.pauth, features.mte,
>> -                      features.tls);
>> +      current_process ()->tdesc = aarch64_linux_read_description (features);
>>         /* Adjust the register sets we should use for this particular set of
>>        features.  */
>> diff --git a/gdbserver/linux-aarch64-tdesc.cc b/gdbserver/linux-aarch64-tdesc.cc
>> index be96612d571..93f2bedde0d 100644
>> --- a/gdbserver/linux-aarch64-tdesc.cc
>> +++ b/gdbserver/linux-aarch64-tdesc.cc
>> @@ -25,36 +25,36 @@
>>   #include "arch/aarch64.h"
>>   #include "linux-aarch32-low.h"
>>   #include <inttypes.h>
>> +#include <unordered_map>
>>   /* All possible aarch64 target descriptors.  */
>> -struct target_desc *tdesc_aarch64_list[AARCH64_MAX_SVE_VQ + 1][2/*pauth*/][2 /* mte */][2 /* tls */];
>> +static std::unordered_map<aarch64_features, target_desc *> tdesc_aarch64_map;
>>   /* Create the aarch64 target description.  */
>>   const target_desc *
>> -aarch64_linux_read_description (uint64_t vq, bool pauth_p, bool mte_p,
>> -                bool tls_p)
>> +aarch64_linux_read_description (const aarch64_features &features)
>>   {
>> -  if (vq > AARCH64_MAX_SVE_VQ)
>> -    error (_("VQ is %" PRIu64 ", maximum supported value is %d"), vq,
>> +  if (features.vq > AARCH64_MAX_SVE_VQ)
>> +    error (_("VQ is %" PRIu64 ", maximum supported value is %d"), features.vq,
>>          AARCH64_MAX_SVE_VQ);
>> -  struct target_desc *tdesc = tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p];
>> +  struct target_desc *tdesc = tdesc_aarch64_map[features];
>>     if (tdesc == NULL)
>>       {
>> -      tdesc = aarch64_create_target_description (vq, pauth_p, mte_p, tls_p);
>> +      tdesc = aarch64_create_target_description (features);
>>         static const char *expedite_regs_aarch64[] = { "x29", "sp", "pc", NULL };
>>         static const char *expedite_regs_aarch64_sve[] = { "x29", "sp", "pc",
>>                                "vg", NULL };
>> -      if (vq == 0)
>> +      if (features.vq == 0)
>>       init_target_desc (tdesc, expedite_regs_aarch64);
>>         else
>>       init_target_desc (tdesc, expedite_regs_aarch64_sve);
>> -      tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p] = tdesc;
>> +      tdesc_aarch64_map[features] = tdesc;
>>       }
>>     return tdesc;
>> diff --git a/gdbserver/linux-aarch64-tdesc.h b/gdbserver/linux-aarch64-tdesc.h
>> index 4ab658447a2..30bcd24d13d 100644
>> --- a/gdbserver/linux-aarch64-tdesc.h
>> +++ b/gdbserver/linux-aarch64-tdesc.h
>> @@ -20,7 +20,9 @@
>>   #ifndef GDBSERVER_LINUX_AARCH64_TDESC_H
>>   #define GDBSERVER_LINUX_AARCH64_TDESC_H
>> -const target_desc * aarch64_linux_read_description (uint64_t vq, bool pauth_p,
>> -                            bool mte_p, bool tls_p);
>> +#include "arch/aarch64.h"
>> +
>> +const target_desc *
>> +  aarch64_linux_read_description (const aarch64_features &features);
>>   #endif /* GDBSERVER_LINUX_AARCH64_TDESC_H */
>> diff --git a/gdbserver/netbsd-aarch64-low.cc b/gdbserver/netbsd-aarch64-low.cc
>> index b371e599232..f8447b0d1ee 100644
>> --- a/gdbserver/netbsd-aarch64-low.cc
>> +++ b/gdbserver/netbsd-aarch64-low.cc
>> @@ -96,7 +96,7 @@ void
>>   netbsd_aarch64_target::low_arch_setup ()
>>   {
>>     target_desc *tdesc
>> -    = aarch64_create_target_description (0, false, false, false);
>> +    = aarch64_create_target_description ({});
>>     static const char *expedite_regs_aarch64[] = { "x29", "sp", "pc", NULL };
>>     init_target_desc (tdesc, expedite_regs_aarch64);
> 
> 

I was considering something less C++ish, but this patch looks great to me. I think it cleans things up quite nicely.

Thanks for addressing this.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Use aarch64_features to describe register features in target descriptions.
  2022-05-18 13:59 ` John Baldwin
  2022-05-18 14:23   ` Luis Machado
@ 2022-05-20 11:52   ` Luis Machado
  2022-05-20 16:39     ` John Baldwin
  1 sibling, 1 reply; 6+ messages in thread
From: Luis Machado @ 2022-05-20 11:52 UTC (permalink / raw)
  To: John Baldwin, gdb-patches

On 5/18/22 14:59, John Baldwin wrote:
> On 5/5/22 11:46 AM, John Baldwin wrote:
>> Replace the sve bool member of aarch64_features with a vq member that
>> holds the vector quotient.  It is zero if SVE is not present.
>>
>> Add std::hash<> specialization and operator== so that aarch64_features
>> can be used as a key with std::unordered_map<>.
>>
>> Change the various functions that create or lookup aarch64 target
>> descriptions to accept a const aarch64_features object rather than a
>> growing number of arguments.
>>
>> Replace the multi-dimension tdesc_aarch64_list arrays used to cache
>> target descriptions with unordered_maps indexed by aarch64_feature.
> 
> Ping?  (Sorry, I thought I had cc'd Luis on this when I sent it, but it seems
> like it was only sent to the list)
> 
>> ---
>>   gdb/aarch64-fbsd-nat.c           |  5 +++--
>>   gdb/aarch64-fbsd-tdep.c          |  5 ++++-
>>   gdb/aarch64-linux-nat.c          | 10 +++++----
>>   gdb/aarch64-linux-tdep.c         | 11 +++++----
>>   gdb/aarch64-tdep.c               | 21 +++++++++++-------
>>   gdb/aarch64-tdep.h               |  3 +--
>>   gdb/arch/aarch64.c               | 13 +++++------
>>   gdb/arch/aarch64.h               | 38 ++++++++++++++++++++++++--------
>>   gdbserver/linux-aarch64-ipa.cc   |  4 ++--
>>   gdbserver/linux-aarch64-low.cc   | 11 ++++-----
>>   gdbserver/linux-aarch64-tdesc.cc | 18 +++++++--------
>>   gdbserver/linux-aarch64-tdesc.h  |  6 +++--
>>   gdbserver/netbsd-aarch64-low.cc  |  2 +-
>>   13 files changed, 89 insertions(+), 58 deletions(-)
>>
>> diff --git a/gdb/aarch64-fbsd-nat.c b/gdb/aarch64-fbsd-nat.c
>> index 910bf5bb190..fb7a29b5afb 100644
>> --- a/gdb/aarch64-fbsd-nat.c
>> +++ b/gdb/aarch64-fbsd-nat.c
>> @@ -149,8 +149,9 @@ aarch64_fbsd_nat_target::store_registers (struct regcache *regcache,
>>   const struct target_desc *
>>   aarch64_fbsd_nat_target::read_description ()
>>   {
>> -  bool tls = have_regset (inferior_ptid, NT_ARM_TLS) != 0;
>> -  return aarch64_read_description (0, false, false, tls);
>> +  aarch64_features features;
>> +  features.tls = have_regset (inferior_ptid, NT_ARM_TLS) != 0;
>> +  return aarch64_read_description (features);
>>   }
>>   #ifdef HAVE_DBREG
>> diff --git a/gdb/aarch64-fbsd-tdep.c b/gdb/aarch64-fbsd-tdep.c
>> index fdf0795b9bf..891546b3c64 100644
>> --- a/gdb/aarch64-fbsd-tdep.c
>> +++ b/gdb/aarch64-fbsd-tdep.c
>> @@ -178,7 +178,10 @@ aarch64_fbsd_core_read_description (struct gdbarch *gdbarch,
>>   {
>>     asection *tls = bfd_get_section_by_name (abfd, ".reg-aarch-tls");
>> -  return aarch64_read_description (0, false, false, tls != nullptr);
>> +  aarch64_features features;
>> +  features.tls = tls != nullptr;
>> +
>> +  return aarch64_read_description (features);
>>   }
>>   /* Implement the get_thread_local_address gdbarch method.  */
>> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
>> index 10b0ca10984..9cde72b247b 100644
>> --- a/gdb/aarch64-linux-nat.c
>> +++ b/gdb/aarch64-linux-nat.c
>> @@ -709,11 +709,13 @@ aarch64_linux_nat_target::read_description ()
>>     CORE_ADDR hwcap = linux_get_hwcap (this);
>>     CORE_ADDR hwcap2 = linux_get_hwcap2 (this);
>> -  bool pauth_p = hwcap & AARCH64_HWCAP_PACA;
>> -  bool mte_p = hwcap2 & HWCAP2_MTE;
>> +  aarch64_features features;
>> +  features.vq = aarch64_sve_get_vq (tid);
>> +  features.pauth = hwcap & AARCH64_HWCAP_PACA;
>> +  features.mte = hwcap2 & HWCAP2_MTE;
>> +  features.tls = true;
>> -  return aarch64_read_description (aarch64_sve_get_vq (tid), pauth_p, mte_p,
>> -                   true);
>> +  return aarch64_read_description (features);
>>   }
>>   /* Convert a native/host siginfo object, into/from the siginfo in the
>> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
>> index dbebcd4f0e0..453692df2f4 100644
>> --- a/gdb/aarch64-linux-tdep.c
>> +++ b/gdb/aarch64-linux-tdep.c
>> @@ -779,10 +779,13 @@ aarch64_linux_core_read_description (struct gdbarch *gdbarch,
>>     CORE_ADDR hwcap = linux_get_hwcap (target);
>>     CORE_ADDR hwcap2 = linux_get_hwcap2 (target);
>> -  bool pauth_p = hwcap & AARCH64_HWCAP_PACA;
>> -  bool mte_p = hwcap2 & HWCAP2_MTE;
>> -  return aarch64_read_description (aarch64_linux_core_read_vq (gdbarch, abfd),
>> -                   pauth_p, mte_p, tls != nullptr);
>> +  aarch64_features features;
>> +  features.vq = aarch64_linux_core_read_vq (gdbarch, abfd);
>> +  features.pauth = hwcap & AARCH64_HWCAP_PACA;
>> +  features.mte = hwcap2 & HWCAP2_MTE;
>> +  features.tls = tls != nullptr;
>> +
>> +  return aarch64_read_description (features);
>>   }
>>   /* Implementation of `gdbarch_stap_is_single_operand', as defined in
>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>> index 9d06ebfe27c..09fe8ae2fdb 100644
>> --- a/gdb/aarch64-tdep.c
>> +++ b/gdb/aarch64-tdep.c
>> @@ -52,13 +52,14 @@
>>   #include "opcode/aarch64.h"
>>   #include <algorithm>
>> +#include <unordered_map>
>>   /* A Homogeneous Floating-Point or Short-Vector Aggregate may have at most
>>      four members.  */
>>   #define HA_MAX_NUM_FLDS        4
>>   /* All possible aarch64 target descriptors.  */
>> -static target_desc *tdesc_aarch64_list[AARCH64_MAX_SVE_VQ + 1][2/*pauth*/][2 /* mte */][2 /* tls */];
>> +static std::unordered_map <aarch64_features, target_desc *> tdesc_aarch64_map;
>>   /* The standard register names, and all the valid aliases for them.  */
>>   static const struct
>> @@ -3332,18 +3333,18 @@ aarch64_displaced_step_hw_singlestep (struct gdbarch *gdbarch)
>>      TLS_P indicates the presence of the Thread Local Storage feature.  */
>>   const target_desc *
>> -aarch64_read_description (uint64_t vq, bool pauth_p, bool mte_p, bool tls_p)
>> +aarch64_read_description (const aarch64_features &features)
>>   {
>> -  if (vq > AARCH64_MAX_SVE_VQ)
>> -    error (_("VQ is %" PRIu64 ", maximum supported value is %d"), vq,
>> +  if (features.vq > AARCH64_MAX_SVE_VQ)
>> +    error (_("VQ is %" PRIu64 ", maximum supported value is %d"), features.vq,
>>          AARCH64_MAX_SVE_VQ);
>> -  struct target_desc *tdesc = tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p];
>> +  struct target_desc *tdesc = tdesc_aarch64_map[features];
>>     if (tdesc == NULL)
>>       {
>> -      tdesc = aarch64_create_target_description (vq, pauth_p, mte_p, tls_p);
>> -      tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p] = tdesc;
>> +      tdesc = aarch64_create_target_description (features);
>> +      tdesc_aarch64_map[features] = tdesc;
>>       }
>>     return tdesc;
>> @@ -3450,7 +3451,11 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>>        value.  */
>>     const struct target_desc *tdesc = info.target_desc;
>>     if (!tdesc_has_registers (tdesc) || vq != aarch64_get_tdesc_vq (tdesc))
>> -    tdesc = aarch64_read_description (vq, false, false, false);
>> +    {
>> +      aarch64_features features;
>> +      features.vq = vq;
>> +      tdesc = aarch64_read_description (features);
>> +    }
>>     gdb_assert (tdesc);
>>     feature_core = tdesc_find_feature (tdesc,"org.gnu.gdb.aarch64.core");
>> diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
>> index e4cdebb6311..3b9f67d6344 100644
>> --- a/gdb/aarch64-tdep.h
>> +++ b/gdb/aarch64-tdep.h
>> @@ -121,8 +121,7 @@ struct aarch64_gdbarch_tdep : gdbarch_tdep
>>     }
>>   };
>> -const target_desc *aarch64_read_description (uint64_t vq, bool pauth_p,
>> -                         bool mte_p, bool tls_p);
>> +const target_desc *aarch64_read_description (const aarch64_features &features);
>>   extern int aarch64_process_record (struct gdbarch *gdbarch,
>>                      struct regcache *regcache, CORE_ADDR addr);
>> diff --git a/gdb/arch/aarch64.c b/gdb/arch/aarch64.c
>> index 733a3fd6d2a..0f73286f145 100644
>> --- a/gdb/arch/aarch64.c
>> +++ b/gdb/arch/aarch64.c
>> @@ -29,8 +29,7 @@
>>   /* See arch/aarch64.h.  */
>>   target_desc *
>> -aarch64_create_target_description (uint64_t vq, bool pauth_p, bool mte_p,
>> -                   bool tls_p)
>> +aarch64_create_target_description (const aarch64_features &features)
>>   {
>>     target_desc_up tdesc = allocate_target_description ();
>> @@ -42,19 +41,19 @@ aarch64_create_target_description (uint64_t vq, bool pauth_p, bool mte_p,
>>     regnum = create_feature_aarch64_core (tdesc.get (), regnum);
>> -  if (vq == 0)
>> +  if (features.vq == 0)
>>       regnum = create_feature_aarch64_fpu (tdesc.get (), regnum);
>>     else
>> -    regnum = create_feature_aarch64_sve (tdesc.get (), regnum, vq);
>> +    regnum = create_feature_aarch64_sve (tdesc.get (), regnum, features.vq);
>> -  if (pauth_p)
>> +  if (features.pauth)
>>       regnum = create_feature_aarch64_pauth (tdesc.get (), regnum);
>>     /* Memory tagging extension registers.  */
>> -  if (mte_p)
>> +  if (features.mte)
>>       regnum = create_feature_aarch64_mte (tdesc.get (), regnum);
>> -  if (tls_p)
>> +  if (features.tls)
>>       regnum = create_feature_aarch64_tls (tdesc.get (), regnum);
>>     return tdesc.release ();
>> diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
>> index 8496a0341f7..72ec4193eba 100644
>> --- a/gdb/arch/aarch64.h
>> +++ b/gdb/arch/aarch64.h
>> @@ -26,23 +26,43 @@
>>      used to select register sets.  */
>>   struct aarch64_features
>>   {
>> -  bool sve = false;
>> +  /* A non zero VQ value indicates both the presence of SVE and the
>> +     Vector Quotient - the number of 128bit chunks in an SVE Z
>> +     register.  */
>> +  uint64_t vq = 0;
>> +
>>     bool pauth = false;
>>     bool mte = false;
>>     bool tls = false;
>>   };
>> -/* Create the aarch64 target description.  A non zero VQ value indicates both
>> -   the presence of SVE and the Vector Quotient - the number of 128bit chunks in
>> -   an SVE Z register.  HAS_PAUTH_P indicates the presence of the PAUTH
>> -   feature.
>> +inline bool operator==(const aarch64_features &lhs, const aarch64_features &rhs)
>> +{
>> +  return lhs.vq == rhs.vq
>> +    && lhs.pauth == rhs.pauth
>> +    && lhs.mte == rhs.mte
>> +    && lhs.tls == rhs.tls;
>> +}
>> -   MTE_P indicates the presence of the Memory Tagging Extension feature.
>> +template<>
>> +struct std::hash<aarch64_features>
>> +{
>> +  std::size_t operator()(const aarch64_features &features) const noexcept
>> +  {
>> +    std::size_t h;
>> -   TLS_P indicates the presence of the Thread Local Storage feature.  */
>> +    h = features.vq;
>> +    h = h << 1 | features.pauth;
>> +    h = h << 1 | features.mte;
>> +    h = h << 1 | features.tls;
>> +    return h;
>> +  }
>> +};
>> -target_desc *aarch64_create_target_description (uint64_t vq, bool has_pauth_p,
>> -                        bool mte_p, bool tls_p);
>> +/* Create the aarch64 target description.  */
>> +
>> +target_desc *
>> +  aarch64_create_target_description (const aarch64_features &features);
>>   /* Register numbers of various important registers.
>>      Note that on SVE, the Z registers reuse the V register numbers and the V
>> diff --git a/gdbserver/linux-aarch64-ipa.cc b/gdbserver/linux-aarch64-ipa.cc
>> index dc907d3ff88..918f85f6ea9 100644
>> --- a/gdbserver/linux-aarch64-ipa.cc
>> +++ b/gdbserver/linux-aarch64-ipa.cc
>> @@ -152,7 +152,7 @@ get_raw_reg (const unsigned char *raw_regs, int regnum)
>>   const struct target_desc *
>>   get_ipa_tdesc (int idx)
>>   {
>> -  return aarch64_linux_read_description (0, false, false, false);
>> +  return aarch64_linux_read_description ({});
>>   }
>>   /* Allocate buffer for the jump pads.  The branch instruction has a reach
>> @@ -205,5 +205,5 @@ void
>>   initialize_low_tracepoint (void)
>>   {
>>     /* SVE, pauth, MTE and TLS not yet supported.  */
>> -  aarch64_linux_read_description (0, false, false, false);
>> +  aarch64_linux_read_description ({});
>>   }
>> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
>> index ba0a810e0f9..db508696261 100644
>> --- a/gdbserver/linux-aarch64-low.cc
>> +++ b/gdbserver/linux-aarch64-low.cc
>> @@ -779,11 +779,11 @@ aarch64_adjust_register_sets (const struct aarch64_features &features)
>>         break;
>>       case NT_FPREGSET:
>>         /* This is unavailable when SVE is present.  */
>> -      if (!features.sve)
>> +      if (features.vq == 0)
>>           regset->size = sizeof (struct user_fpsimd_state);
>>         break;
>>       case NT_ARM_SVE:
>> -      if (features.sve)
>> +      if (features.vq > 0)
>>           regset->size = SVE_PT_SIZE (AARCH64_MAX_SVE_VQ, SVE_PT_REGS_SVE);
>>         break;
>>       case NT_ARM_PAC_MASK:
>> @@ -824,17 +824,14 @@ aarch64_target::low_arch_setup ()
>>       {
>>         struct aarch64_features features;
>> -      uint64_t vq = aarch64_sve_get_vq (tid);
>> -      features.sve = (vq > 0);
>> +      features.vq = aarch64_sve_get_vq (tid);
>>         /* A-profile PAC is 64-bit only.  */
>>         features.pauth = linux_get_hwcap (8) & AARCH64_HWCAP_PACA;
>>         /* A-profile MTE is 64-bit only.  */
>>         features.mte = linux_get_hwcap2 (8) & HWCAP2_MTE;
>>         features.tls = true;
>> -      current_process ()->tdesc
>> -    = aarch64_linux_read_description (vq, features.pauth, features.mte,
>> -                      features.tls);
>> +      current_process ()->tdesc = aarch64_linux_read_description (features);
>>         /* Adjust the register sets we should use for this particular set of
>>        features.  */
>> diff --git a/gdbserver/linux-aarch64-tdesc.cc b/gdbserver/linux-aarch64-tdesc.cc
>> index be96612d571..93f2bedde0d 100644
>> --- a/gdbserver/linux-aarch64-tdesc.cc
>> +++ b/gdbserver/linux-aarch64-tdesc.cc
>> @@ -25,36 +25,36 @@
>>   #include "arch/aarch64.h"
>>   #include "linux-aarch32-low.h"
>>   #include <inttypes.h>
>> +#include <unordered_map>
>>   /* All possible aarch64 target descriptors.  */
>> -struct target_desc *tdesc_aarch64_list[AARCH64_MAX_SVE_VQ + 1][2/*pauth*/][2 /* mte */][2 /* tls */];
>> +static std::unordered_map<aarch64_features, target_desc *> tdesc_aarch64_map;
>>   /* Create the aarch64 target description.  */
>>   const target_desc *
>> -aarch64_linux_read_description (uint64_t vq, bool pauth_p, bool mte_p,
>> -                bool tls_p)
>> +aarch64_linux_read_description (const aarch64_features &features)
>>   {
>> -  if (vq > AARCH64_MAX_SVE_VQ)
>> -    error (_("VQ is %" PRIu64 ", maximum supported value is %d"), vq,
>> +  if (features.vq > AARCH64_MAX_SVE_VQ)
>> +    error (_("VQ is %" PRIu64 ", maximum supported value is %d"), features.vq,
>>          AARCH64_MAX_SVE_VQ);
>> -  struct target_desc *tdesc = tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p];
>> +  struct target_desc *tdesc = tdesc_aarch64_map[features];
>>     if (tdesc == NULL)
>>       {
>> -      tdesc = aarch64_create_target_description (vq, pauth_p, mte_p, tls_p);
>> +      tdesc = aarch64_create_target_description (features);
>>         static const char *expedite_regs_aarch64[] = { "x29", "sp", "pc", NULL };
>>         static const char *expedite_regs_aarch64_sve[] = { "x29", "sp", "pc",
>>                                "vg", NULL };
>> -      if (vq == 0)
>> +      if (features.vq == 0)
>>       init_target_desc (tdesc, expedite_regs_aarch64);
>>         else
>>       init_target_desc (tdesc, expedite_regs_aarch64_sve);
>> -      tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p] = tdesc;
>> +      tdesc_aarch64_map[features] = tdesc;
>>       }
>>     return tdesc;
>> diff --git a/gdbserver/linux-aarch64-tdesc.h b/gdbserver/linux-aarch64-tdesc.h
>> index 4ab658447a2..30bcd24d13d 100644
>> --- a/gdbserver/linux-aarch64-tdesc.h
>> +++ b/gdbserver/linux-aarch64-tdesc.h
>> @@ -20,7 +20,9 @@
>>   #ifndef GDBSERVER_LINUX_AARCH64_TDESC_H
>>   #define GDBSERVER_LINUX_AARCH64_TDESC_H
>> -const target_desc * aarch64_linux_read_description (uint64_t vq, bool pauth_p,
>> -                            bool mte_p, bool tls_p);
>> +#include "arch/aarch64.h"
>> +
>> +const target_desc *
>> +  aarch64_linux_read_description (const aarch64_features &features);
>>   #endif /* GDBSERVER_LINUX_AARCH64_TDESC_H */
>> diff --git a/gdbserver/netbsd-aarch64-low.cc b/gdbserver/netbsd-aarch64-low.cc
>> index b371e599232..f8447b0d1ee 100644
>> --- a/gdbserver/netbsd-aarch64-low.cc
>> +++ b/gdbserver/netbsd-aarch64-low.cc
>> @@ -96,7 +96,7 @@ void
>>   netbsd_aarch64_target::low_arch_setup ()
>>   {
>>     target_desc *tdesc
>> -    = aarch64_create_target_description (0, false, false, false);
>> +    = aarch64_create_target_description ({});
>>     static const char *expedite_regs_aarch64[] = { "x29", "sp", "pc", NULL };
>>     init_target_desc (tdesc, expedite_regs_aarch64);
> 
> 

It seems this is running into the following for older compilers:

/binutils-gdb--gdb/gdb/arch/aarch64.h:48:13: error: specialization of 'template<class _Tp> struct std::hash' in different namespace [-fpermissive]

  struct std::hash<aarch64_features>

This is GCC 6.4.1. This feels like a bug in the compiler according to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56480. But can we make it so the compiler is happy with it?

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Use aarch64_features to describe register features in target descriptions.
  2022-05-20 11:52   ` Luis Machado
@ 2022-05-20 16:39     ` John Baldwin
  2022-05-23  9:29       ` Luis Machado
  0 siblings, 1 reply; 6+ messages in thread
From: John Baldwin @ 2022-05-20 16:39 UTC (permalink / raw)
  To: Luis Machado, gdb-patches

On 5/20/22 4:52 AM, Luis Machado wrote:
> On 5/18/22 14:59, John Baldwin wrote:
>> On 5/5/22 11:46 AM, John Baldwin wrote:
>>> Replace the sve bool member of aarch64_features with a vq member that
>>> holds the vector quotient.  It is zero if SVE is not present.
>>>
>>> Add std::hash<> specialization and operator== so that aarch64_features
>>> can be used as a key with std::unordered_map<>.
>>>
>>> Change the various functions that create or lookup aarch64 target
>>> descriptions to accept a const aarch64_features object rather than a
>>> growing number of arguments.
>>>
>>> Replace the multi-dimension tdesc_aarch64_list arrays used to cache
>>> target descriptions with unordered_maps indexed by aarch64_feature.
>>
>> Ping?  (Sorry, I thought I had cc'd Luis on this when I sent it, but it seems
>> like it was only sent to the list)
>>
>>> ---
>>>    gdb/aarch64-fbsd-nat.c           |  5 +++--
>>>    gdb/aarch64-fbsd-tdep.c          |  5 ++++-
>>>    gdb/aarch64-linux-nat.c          | 10 +++++----
>>>    gdb/aarch64-linux-tdep.c         | 11 +++++----
>>>    gdb/aarch64-tdep.c               | 21 +++++++++++-------
>>>    gdb/aarch64-tdep.h               |  3 +--
>>>    gdb/arch/aarch64.c               | 13 +++++------
>>>    gdb/arch/aarch64.h               | 38 ++++++++++++++++++++++++--------
>>>    gdbserver/linux-aarch64-ipa.cc   |  4 ++--
>>>    gdbserver/linux-aarch64-low.cc   | 11 ++++-----
>>>    gdbserver/linux-aarch64-tdesc.cc | 18 +++++++--------
>>>    gdbserver/linux-aarch64-tdesc.h  |  6 +++--
>>>    gdbserver/netbsd-aarch64-low.cc  |  2 +-
>>>    13 files changed, 89 insertions(+), 58 deletions(-)
>>>
>>> diff --git a/gdb/aarch64-fbsd-nat.c b/gdb/aarch64-fbsd-nat.c
>>> index 910bf5bb190..fb7a29b5afb 100644
>>> --- a/gdb/aarch64-fbsd-nat.c
>>> +++ b/gdb/aarch64-fbsd-nat.c
>>> @@ -149,8 +149,9 @@ aarch64_fbsd_nat_target::store_registers (struct regcache *regcache,
>>>    const struct target_desc *
>>>    aarch64_fbsd_nat_target::read_description ()
>>>    {
>>> -  bool tls = have_regset (inferior_ptid, NT_ARM_TLS) != 0;
>>> -  return aarch64_read_description (0, false, false, tls);
>>> +  aarch64_features features;
>>> +  features.tls = have_regset (inferior_ptid, NT_ARM_TLS) != 0;
>>> +  return aarch64_read_description (features);
>>>    }
>>>    #ifdef HAVE_DBREG
>>> diff --git a/gdb/aarch64-fbsd-tdep.c b/gdb/aarch64-fbsd-tdep.c
>>> index fdf0795b9bf..891546b3c64 100644
>>> --- a/gdb/aarch64-fbsd-tdep.c
>>> +++ b/gdb/aarch64-fbsd-tdep.c
>>> @@ -178,7 +178,10 @@ aarch64_fbsd_core_read_description (struct gdbarch *gdbarch,
>>>    {
>>>      asection *tls = bfd_get_section_by_name (abfd, ".reg-aarch-tls");
>>> -  return aarch64_read_description (0, false, false, tls != nullptr);
>>> +  aarch64_features features;
>>> +  features.tls = tls != nullptr;
>>> +
>>> +  return aarch64_read_description (features);
>>>    }
>>>    /* Implement the get_thread_local_address gdbarch method.  */
>>> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
>>> index 10b0ca10984..9cde72b247b 100644
>>> --- a/gdb/aarch64-linux-nat.c
>>> +++ b/gdb/aarch64-linux-nat.c
>>> @@ -709,11 +709,13 @@ aarch64_linux_nat_target::read_description ()
>>>      CORE_ADDR hwcap = linux_get_hwcap (this);
>>>      CORE_ADDR hwcap2 = linux_get_hwcap2 (this);
>>> -  bool pauth_p = hwcap & AARCH64_HWCAP_PACA;
>>> -  bool mte_p = hwcap2 & HWCAP2_MTE;
>>> +  aarch64_features features;
>>> +  features.vq = aarch64_sve_get_vq (tid);
>>> +  features.pauth = hwcap & AARCH64_HWCAP_PACA;
>>> +  features.mte = hwcap2 & HWCAP2_MTE;
>>> +  features.tls = true;
>>> -  return aarch64_read_description (aarch64_sve_get_vq (tid), pauth_p, mte_p,
>>> -                   true);
>>> +  return aarch64_read_description (features);
>>>    }
>>>    /* Convert a native/host siginfo object, into/from the siginfo in the
>>> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
>>> index dbebcd4f0e0..453692df2f4 100644
>>> --- a/gdb/aarch64-linux-tdep.c
>>> +++ b/gdb/aarch64-linux-tdep.c
>>> @@ -779,10 +779,13 @@ aarch64_linux_core_read_description (struct gdbarch *gdbarch,
>>>      CORE_ADDR hwcap = linux_get_hwcap (target);
>>>      CORE_ADDR hwcap2 = linux_get_hwcap2 (target);
>>> -  bool pauth_p = hwcap & AARCH64_HWCAP_PACA;
>>> -  bool mte_p = hwcap2 & HWCAP2_MTE;
>>> -  return aarch64_read_description (aarch64_linux_core_read_vq (gdbarch, abfd),
>>> -                   pauth_p, mte_p, tls != nullptr);
>>> +  aarch64_features features;
>>> +  features.vq = aarch64_linux_core_read_vq (gdbarch, abfd);
>>> +  features.pauth = hwcap & AARCH64_HWCAP_PACA;
>>> +  features.mte = hwcap2 & HWCAP2_MTE;
>>> +  features.tls = tls != nullptr;
>>> +
>>> +  return aarch64_read_description (features);
>>>    }
>>>    /* Implementation of `gdbarch_stap_is_single_operand', as defined in
>>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>>> index 9d06ebfe27c..09fe8ae2fdb 100644
>>> --- a/gdb/aarch64-tdep.c
>>> +++ b/gdb/aarch64-tdep.c
>>> @@ -52,13 +52,14 @@
>>>    #include "opcode/aarch64.h"
>>>    #include <algorithm>
>>> +#include <unordered_map>
>>>    /* A Homogeneous Floating-Point or Short-Vector Aggregate may have at most
>>>       four members.  */
>>>    #define HA_MAX_NUM_FLDS        4
>>>    /* All possible aarch64 target descriptors.  */
>>> -static target_desc *tdesc_aarch64_list[AARCH64_MAX_SVE_VQ + 1][2/*pauth*/][2 /* mte */][2 /* tls */];
>>> +static std::unordered_map <aarch64_features, target_desc *> tdesc_aarch64_map;
>>>    /* The standard register names, and all the valid aliases for them.  */
>>>    static const struct
>>> @@ -3332,18 +3333,18 @@ aarch64_displaced_step_hw_singlestep (struct gdbarch *gdbarch)
>>>       TLS_P indicates the presence of the Thread Local Storage feature.  */
>>>    const target_desc *
>>> -aarch64_read_description (uint64_t vq, bool pauth_p, bool mte_p, bool tls_p)
>>> +aarch64_read_description (const aarch64_features &features)
>>>    {
>>> -  if (vq > AARCH64_MAX_SVE_VQ)
>>> -    error (_("VQ is %" PRIu64 ", maximum supported value is %d"), vq,
>>> +  if (features.vq > AARCH64_MAX_SVE_VQ)
>>> +    error (_("VQ is %" PRIu64 ", maximum supported value is %d"), features.vq,
>>>           AARCH64_MAX_SVE_VQ);
>>> -  struct target_desc *tdesc = tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p];
>>> +  struct target_desc *tdesc = tdesc_aarch64_map[features];
>>>      if (tdesc == NULL)
>>>        {
>>> -      tdesc = aarch64_create_target_description (vq, pauth_p, mte_p, tls_p);
>>> -      tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p] = tdesc;
>>> +      tdesc = aarch64_create_target_description (features);
>>> +      tdesc_aarch64_map[features] = tdesc;
>>>        }
>>>      return tdesc;
>>> @@ -3450,7 +3451,11 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>>>         value.  */
>>>      const struct target_desc *tdesc = info.target_desc;
>>>      if (!tdesc_has_registers (tdesc) || vq != aarch64_get_tdesc_vq (tdesc))
>>> -    tdesc = aarch64_read_description (vq, false, false, false);
>>> +    {
>>> +      aarch64_features features;
>>> +      features.vq = vq;
>>> +      tdesc = aarch64_read_description (features);
>>> +    }
>>>      gdb_assert (tdesc);
>>>      feature_core = tdesc_find_feature (tdesc,"org.gnu.gdb.aarch64.core");
>>> diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
>>> index e4cdebb6311..3b9f67d6344 100644
>>> --- a/gdb/aarch64-tdep.h
>>> +++ b/gdb/aarch64-tdep.h
>>> @@ -121,8 +121,7 @@ struct aarch64_gdbarch_tdep : gdbarch_tdep
>>>      }
>>>    };
>>> -const target_desc *aarch64_read_description (uint64_t vq, bool pauth_p,
>>> -                         bool mte_p, bool tls_p);
>>> +const target_desc *aarch64_read_description (const aarch64_features &features);
>>>    extern int aarch64_process_record (struct gdbarch *gdbarch,
>>>                       struct regcache *regcache, CORE_ADDR addr);
>>> diff --git a/gdb/arch/aarch64.c b/gdb/arch/aarch64.c
>>> index 733a3fd6d2a..0f73286f145 100644
>>> --- a/gdb/arch/aarch64.c
>>> +++ b/gdb/arch/aarch64.c
>>> @@ -29,8 +29,7 @@
>>>    /* See arch/aarch64.h.  */
>>>    target_desc *
>>> -aarch64_create_target_description (uint64_t vq, bool pauth_p, bool mte_p,
>>> -                   bool tls_p)
>>> +aarch64_create_target_description (const aarch64_features &features)
>>>    {
>>>      target_desc_up tdesc = allocate_target_description ();
>>> @@ -42,19 +41,19 @@ aarch64_create_target_description (uint64_t vq, bool pauth_p, bool mte_p,
>>>      regnum = create_feature_aarch64_core (tdesc.get (), regnum);
>>> -  if (vq == 0)
>>> +  if (features.vq == 0)
>>>        regnum = create_feature_aarch64_fpu (tdesc.get (), regnum);
>>>      else
>>> -    regnum = create_feature_aarch64_sve (tdesc.get (), regnum, vq);
>>> +    regnum = create_feature_aarch64_sve (tdesc.get (), regnum, features.vq);
>>> -  if (pauth_p)
>>> +  if (features.pauth)
>>>        regnum = create_feature_aarch64_pauth (tdesc.get (), regnum);
>>>      /* Memory tagging extension registers.  */
>>> -  if (mte_p)
>>> +  if (features.mte)
>>>        regnum = create_feature_aarch64_mte (tdesc.get (), regnum);
>>> -  if (tls_p)
>>> +  if (features.tls)
>>>        regnum = create_feature_aarch64_tls (tdesc.get (), regnum);
>>>      return tdesc.release ();
>>> diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
>>> index 8496a0341f7..72ec4193eba 100644
>>> --- a/gdb/arch/aarch64.h
>>> +++ b/gdb/arch/aarch64.h
>>> @@ -26,23 +26,43 @@
>>>       used to select register sets.  */
>>>    struct aarch64_features
>>>    {
>>> -  bool sve = false;
>>> +  /* A non zero VQ value indicates both the presence of SVE and the
>>> +     Vector Quotient - the number of 128bit chunks in an SVE Z
>>> +     register.  */
>>> +  uint64_t vq = 0;
>>> +
>>>      bool pauth = false;
>>>      bool mte = false;
>>>      bool tls = false;
>>>    };
>>> -/* Create the aarch64 target description.  A non zero VQ value indicates both
>>> -   the presence of SVE and the Vector Quotient - the number of 128bit chunks in
>>> -   an SVE Z register.  HAS_PAUTH_P indicates the presence of the PAUTH
>>> -   feature.
>>> +inline bool operator==(const aarch64_features &lhs, const aarch64_features &rhs)
>>> +{
>>> +  return lhs.vq == rhs.vq
>>> +    && lhs.pauth == rhs.pauth
>>> +    && lhs.mte == rhs.mte
>>> +    && lhs.tls == rhs.tls;
>>> +}
>>> -   MTE_P indicates the presence of the Memory Tagging Extension feature.
>>> +template<>
>>> +struct std::hash<aarch64_features>
>>> +{
>>> +  std::size_t operator()(const aarch64_features &features) const noexcept
>>> +  {
>>> +    std::size_t h;
>>> -   TLS_P indicates the presence of the Thread Local Storage feature.  */
>>> +    h = features.vq;
>>> +    h = h << 1 | features.pauth;
>>> +    h = h << 1 | features.mte;
>>> +    h = h << 1 | features.tls;
>>> +    return h;
>>> +  }
>>> +};
>>> -target_desc *aarch64_create_target_description (uint64_t vq, bool has_pauth_p,
>>> -                        bool mte_p, bool tls_p);
>>> +/* Create the aarch64 target description.  */
>>> +
>>> +target_desc *
>>> +  aarch64_create_target_description (const aarch64_features &features);
>>>    /* Register numbers of various important registers.
>>>       Note that on SVE, the Z registers reuse the V register numbers and the V
>>> diff --git a/gdbserver/linux-aarch64-ipa.cc b/gdbserver/linux-aarch64-ipa.cc
>>> index dc907d3ff88..918f85f6ea9 100644
>>> --- a/gdbserver/linux-aarch64-ipa.cc
>>> +++ b/gdbserver/linux-aarch64-ipa.cc
>>> @@ -152,7 +152,7 @@ get_raw_reg (const unsigned char *raw_regs, int regnum)
>>>    const struct target_desc *
>>>    get_ipa_tdesc (int idx)
>>>    {
>>> -  return aarch64_linux_read_description (0, false, false, false);
>>> +  return aarch64_linux_read_description ({});
>>>    }
>>>    /* Allocate buffer for the jump pads.  The branch instruction has a reach
>>> @@ -205,5 +205,5 @@ void
>>>    initialize_low_tracepoint (void)
>>>    {
>>>      /* SVE, pauth, MTE and TLS not yet supported.  */
>>> -  aarch64_linux_read_description (0, false, false, false);
>>> +  aarch64_linux_read_description ({});
>>>    }
>>> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
>>> index ba0a810e0f9..db508696261 100644
>>> --- a/gdbserver/linux-aarch64-low.cc
>>> +++ b/gdbserver/linux-aarch64-low.cc
>>> @@ -779,11 +779,11 @@ aarch64_adjust_register_sets (const struct aarch64_features &features)
>>>          break;
>>>        case NT_FPREGSET:
>>>          /* This is unavailable when SVE is present.  */
>>> -      if (!features.sve)
>>> +      if (features.vq == 0)
>>>            regset->size = sizeof (struct user_fpsimd_state);
>>>          break;
>>>        case NT_ARM_SVE:
>>> -      if (features.sve)
>>> +      if (features.vq > 0)
>>>            regset->size = SVE_PT_SIZE (AARCH64_MAX_SVE_VQ, SVE_PT_REGS_SVE);
>>>          break;
>>>        case NT_ARM_PAC_MASK:
>>> @@ -824,17 +824,14 @@ aarch64_target::low_arch_setup ()
>>>        {
>>>          struct aarch64_features features;
>>> -      uint64_t vq = aarch64_sve_get_vq (tid);
>>> -      features.sve = (vq > 0);
>>> +      features.vq = aarch64_sve_get_vq (tid);
>>>          /* A-profile PAC is 64-bit only.  */
>>>          features.pauth = linux_get_hwcap (8) & AARCH64_HWCAP_PACA;
>>>          /* A-profile MTE is 64-bit only.  */
>>>          features.mte = linux_get_hwcap2 (8) & HWCAP2_MTE;
>>>          features.tls = true;
>>> -      current_process ()->tdesc
>>> -    = aarch64_linux_read_description (vq, features.pauth, features.mte,
>>> -                      features.tls);
>>> +      current_process ()->tdesc = aarch64_linux_read_description (features);
>>>          /* Adjust the register sets we should use for this particular set of
>>>         features.  */
>>> diff --git a/gdbserver/linux-aarch64-tdesc.cc b/gdbserver/linux-aarch64-tdesc.cc
>>> index be96612d571..93f2bedde0d 100644
>>> --- a/gdbserver/linux-aarch64-tdesc.cc
>>> +++ b/gdbserver/linux-aarch64-tdesc.cc
>>> @@ -25,36 +25,36 @@
>>>    #include "arch/aarch64.h"
>>>    #include "linux-aarch32-low.h"
>>>    #include <inttypes.h>
>>> +#include <unordered_map>
>>>    /* All possible aarch64 target descriptors.  */
>>> -struct target_desc *tdesc_aarch64_list[AARCH64_MAX_SVE_VQ + 1][2/*pauth*/][2 /* mte */][2 /* tls */];
>>> +static std::unordered_map<aarch64_features, target_desc *> tdesc_aarch64_map;
>>>    /* Create the aarch64 target description.  */
>>>    const target_desc *
>>> -aarch64_linux_read_description (uint64_t vq, bool pauth_p, bool mte_p,
>>> -                bool tls_p)
>>> +aarch64_linux_read_description (const aarch64_features &features)
>>>    {
>>> -  if (vq > AARCH64_MAX_SVE_VQ)
>>> -    error (_("VQ is %" PRIu64 ", maximum supported value is %d"), vq,
>>> +  if (features.vq > AARCH64_MAX_SVE_VQ)
>>> +    error (_("VQ is %" PRIu64 ", maximum supported value is %d"), features.vq,
>>>           AARCH64_MAX_SVE_VQ);
>>> -  struct target_desc *tdesc = tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p];
>>> +  struct target_desc *tdesc = tdesc_aarch64_map[features];
>>>      if (tdesc == NULL)
>>>        {
>>> -      tdesc = aarch64_create_target_description (vq, pauth_p, mte_p, tls_p);
>>> +      tdesc = aarch64_create_target_description (features);
>>>          static const char *expedite_regs_aarch64[] = { "x29", "sp", "pc", NULL };
>>>          static const char *expedite_regs_aarch64_sve[] = { "x29", "sp", "pc",
>>>                                 "vg", NULL };
>>> -      if (vq == 0)
>>> +      if (features.vq == 0)
>>>        init_target_desc (tdesc, expedite_regs_aarch64);
>>>          else
>>>        init_target_desc (tdesc, expedite_regs_aarch64_sve);
>>> -      tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p] = tdesc;
>>> +      tdesc_aarch64_map[features] = tdesc;
>>>        }
>>>      return tdesc;
>>> diff --git a/gdbserver/linux-aarch64-tdesc.h b/gdbserver/linux-aarch64-tdesc.h
>>> index 4ab658447a2..30bcd24d13d 100644
>>> --- a/gdbserver/linux-aarch64-tdesc.h
>>> +++ b/gdbserver/linux-aarch64-tdesc.h
>>> @@ -20,7 +20,9 @@
>>>    #ifndef GDBSERVER_LINUX_AARCH64_TDESC_H
>>>    #define GDBSERVER_LINUX_AARCH64_TDESC_H
>>> -const target_desc * aarch64_linux_read_description (uint64_t vq, bool pauth_p,
>>> -                            bool mte_p, bool tls_p);
>>> +#include "arch/aarch64.h"
>>> +
>>> +const target_desc *
>>> +  aarch64_linux_read_description (const aarch64_features &features);
>>>    #endif /* GDBSERVER_LINUX_AARCH64_TDESC_H */
>>> diff --git a/gdbserver/netbsd-aarch64-low.cc b/gdbserver/netbsd-aarch64-low.cc
>>> index b371e599232..f8447b0d1ee 100644
>>> --- a/gdbserver/netbsd-aarch64-low.cc
>>> +++ b/gdbserver/netbsd-aarch64-low.cc
>>> @@ -96,7 +96,7 @@ void
>>>    netbsd_aarch64_target::low_arch_setup ()
>>>    {
>>>      target_desc *tdesc
>>> -    = aarch64_create_target_description (0, false, false, false);
>>> +    = aarch64_create_target_description ({});
>>>      static const char *expedite_regs_aarch64[] = { "x29", "sp", "pc", NULL };
>>>      init_target_desc (tdesc, expedite_regs_aarch64);
>>
>>
> 
> It seems this is running into the following for older compilers:
> 
> /binutils-gdb--gdb/gdb/arch/aarch64.h:48:13: error: specialization of 'template<class _Tp> struct std::hash' in different namespace [-fpermissive]
> 
>    struct std::hash<aarch64_features>
> 
> This is GCC 6.4.1. This feels like a bug in the compiler according to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56480. But can we make it so the compiler is happy with it?
Hmm, I don't have an old version of GCC easily accessible, but I think
this might fix it:

diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
index 72ec4193eba..8e3fd36726a 100644
--- a/gdb/arch/aarch64.h
+++ b/gdb/arch/aarch64.h
@@ -44,20 +44,23 @@ inline bool operator==(const aarch64_features &lhs, const aarch64_features &rhs)
      && lhs.tls == rhs.tls;
  }
  
-template<>
-struct std::hash<aarch64_features>
+namespace std
  {
-  std::size_t operator()(const aarch64_features &features) const noexcept
+  template<>
+  struct hash<aarch64_features>
    {
-    std::size_t h;
-
-    h = features.vq;
-    h = h << 1 | features.pauth;
-    h = h << 1 | features.mte;
-    h = h << 1 | features.tls;
-    return h;
-  }
-};
+    std::size_t operator()(const aarch64_features &features) const noexcept
+    {
+      std::size_t h;
+
+      h = features.vq;
+      h = h << 1 | features.pauth;
+      h = h << 1 | features.mte;
+      h = h << 1 | features.tls;
+      return h;
+    }
+  };
+}


-- 
John Baldwin

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Use aarch64_features to describe register features in target descriptions.
  2022-05-20 16:39     ` John Baldwin
@ 2022-05-23  9:29       ` Luis Machado
  0 siblings, 0 replies; 6+ messages in thread
From: Luis Machado @ 2022-05-23  9:29 UTC (permalink / raw)
  To: John Baldwin, gdb-patches

On 5/20/22 17:39, John Baldwin wrote:
> On 5/20/22 4:52 AM, Luis Machado wrote:
>> On 5/18/22 14:59, John Baldwin wrote:
>>> On 5/5/22 11:46 AM, John Baldwin wrote:
>>>> Replace the sve bool member of aarch64_features with a vq member that
>>>> holds the vector quotient.  It is zero if SVE is not present.
>>>>
>>>> Add std::hash<> specialization and operator== so that aarch64_features
>>>> can be used as a key with std::unordered_map<>.
>>>>
>>>> Change the various functions that create or lookup aarch64 target
>>>> descriptions to accept a const aarch64_features object rather than a
>>>> growing number of arguments.
>>>>
>>>> Replace the multi-dimension tdesc_aarch64_list arrays used to cache
>>>> target descriptions with unordered_maps indexed by aarch64_feature.
>>>
>>> Ping?  (Sorry, I thought I had cc'd Luis on this when I sent it, but it seems
>>> like it was only sent to the list)
>>>
>>>> ---
>>>>    gdb/aarch64-fbsd-nat.c           |  5 +++--
>>>>    gdb/aarch64-fbsd-tdep.c          |  5 ++++-
>>>>    gdb/aarch64-linux-nat.c          | 10 +++++----
>>>>    gdb/aarch64-linux-tdep.c         | 11 +++++----
>>>>    gdb/aarch64-tdep.c               | 21 +++++++++++-------
>>>>    gdb/aarch64-tdep.h               |  3 +--
>>>>    gdb/arch/aarch64.c               | 13 +++++------
>>>>    gdb/arch/aarch64.h               | 38 ++++++++++++++++++++++++--------
>>>>    gdbserver/linux-aarch64-ipa.cc   |  4 ++--
>>>>    gdbserver/linux-aarch64-low.cc   | 11 ++++-----
>>>>    gdbserver/linux-aarch64-tdesc.cc | 18 +++++++--------
>>>>    gdbserver/linux-aarch64-tdesc.h  |  6 +++--
>>>>    gdbserver/netbsd-aarch64-low.cc  |  2 +-
>>>>    13 files changed, 89 insertions(+), 58 deletions(-)
>>>>
>>>> diff --git a/gdb/aarch64-fbsd-nat.c b/gdb/aarch64-fbsd-nat.c
>>>> index 910bf5bb190..fb7a29b5afb 100644
>>>> --- a/gdb/aarch64-fbsd-nat.c
>>>> +++ b/gdb/aarch64-fbsd-nat.c
>>>> @@ -149,8 +149,9 @@ aarch64_fbsd_nat_target::store_registers (struct regcache *regcache,
>>>>    const struct target_desc *
>>>>    aarch64_fbsd_nat_target::read_description ()
>>>>    {
>>>> -  bool tls = have_regset (inferior_ptid, NT_ARM_TLS) != 0;
>>>> -  return aarch64_read_description (0, false, false, tls);
>>>> +  aarch64_features features;
>>>> +  features.tls = have_regset (inferior_ptid, NT_ARM_TLS) != 0;
>>>> +  return aarch64_read_description (features);
>>>>    }
>>>>    #ifdef HAVE_DBREG
>>>> diff --git a/gdb/aarch64-fbsd-tdep.c b/gdb/aarch64-fbsd-tdep.c
>>>> index fdf0795b9bf..891546b3c64 100644
>>>> --- a/gdb/aarch64-fbsd-tdep.c
>>>> +++ b/gdb/aarch64-fbsd-tdep.c
>>>> @@ -178,7 +178,10 @@ aarch64_fbsd_core_read_description (struct gdbarch *gdbarch,
>>>>    {
>>>>      asection *tls = bfd_get_section_by_name (abfd, ".reg-aarch-tls");
>>>> -  return aarch64_read_description (0, false, false, tls != nullptr);
>>>> +  aarch64_features features;
>>>> +  features.tls = tls != nullptr;
>>>> +
>>>> +  return aarch64_read_description (features);
>>>>    }
>>>>    /* Implement the get_thread_local_address gdbarch method.  */
>>>> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
>>>> index 10b0ca10984..9cde72b247b 100644
>>>> --- a/gdb/aarch64-linux-nat.c
>>>> +++ b/gdb/aarch64-linux-nat.c
>>>> @@ -709,11 +709,13 @@ aarch64_linux_nat_target::read_description ()
>>>>      CORE_ADDR hwcap = linux_get_hwcap (this);
>>>>      CORE_ADDR hwcap2 = linux_get_hwcap2 (this);
>>>> -  bool pauth_p = hwcap & AARCH64_HWCAP_PACA;
>>>> -  bool mte_p = hwcap2 & HWCAP2_MTE;
>>>> +  aarch64_features features;
>>>> +  features.vq = aarch64_sve_get_vq (tid);
>>>> +  features.pauth = hwcap & AARCH64_HWCAP_PACA;
>>>> +  features.mte = hwcap2 & HWCAP2_MTE;
>>>> +  features.tls = true;
>>>> -  return aarch64_read_description (aarch64_sve_get_vq (tid), pauth_p, mte_p,
>>>> -                   true);
>>>> +  return aarch64_read_description (features);
>>>>    }
>>>>    /* Convert a native/host siginfo object, into/from the siginfo in the
>>>> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
>>>> index dbebcd4f0e0..453692df2f4 100644
>>>> --- a/gdb/aarch64-linux-tdep.c
>>>> +++ b/gdb/aarch64-linux-tdep.c
>>>> @@ -779,10 +779,13 @@ aarch64_linux_core_read_description (struct gdbarch *gdbarch,
>>>>      CORE_ADDR hwcap = linux_get_hwcap (target);
>>>>      CORE_ADDR hwcap2 = linux_get_hwcap2 (target);
>>>> -  bool pauth_p = hwcap & AARCH64_HWCAP_PACA;
>>>> -  bool mte_p = hwcap2 & HWCAP2_MTE;
>>>> -  return aarch64_read_description (aarch64_linux_core_read_vq (gdbarch, abfd),
>>>> -                   pauth_p, mte_p, tls != nullptr);
>>>> +  aarch64_features features;
>>>> +  features.vq = aarch64_linux_core_read_vq (gdbarch, abfd);
>>>> +  features.pauth = hwcap & AARCH64_HWCAP_PACA;
>>>> +  features.mte = hwcap2 & HWCAP2_MTE;
>>>> +  features.tls = tls != nullptr;
>>>> +
>>>> +  return aarch64_read_description (features);
>>>>    }
>>>>    /* Implementation of `gdbarch_stap_is_single_operand', as defined in
>>>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>>>> index 9d06ebfe27c..09fe8ae2fdb 100644
>>>> --- a/gdb/aarch64-tdep.c
>>>> +++ b/gdb/aarch64-tdep.c
>>>> @@ -52,13 +52,14 @@
>>>>    #include "opcode/aarch64.h"
>>>>    #include <algorithm>
>>>> +#include <unordered_map>
>>>>    /* A Homogeneous Floating-Point or Short-Vector Aggregate may have at most
>>>>       four members.  */
>>>>    #define HA_MAX_NUM_FLDS        4
>>>>    /* All possible aarch64 target descriptors.  */
>>>> -static target_desc *tdesc_aarch64_list[AARCH64_MAX_SVE_VQ + 1][2/*pauth*/][2 /* mte */][2 /* tls */];
>>>> +static std::unordered_map <aarch64_features, target_desc *> tdesc_aarch64_map;
>>>>    /* The standard register names, and all the valid aliases for them.  */
>>>>    static const struct
>>>> @@ -3332,18 +3333,18 @@ aarch64_displaced_step_hw_singlestep (struct gdbarch *gdbarch)
>>>>       TLS_P indicates the presence of the Thread Local Storage feature.  */
>>>>    const target_desc *
>>>> -aarch64_read_description (uint64_t vq, bool pauth_p, bool mte_p, bool tls_p)
>>>> +aarch64_read_description (const aarch64_features &features)
>>>>    {
>>>> -  if (vq > AARCH64_MAX_SVE_VQ)
>>>> -    error (_("VQ is %" PRIu64 ", maximum supported value is %d"), vq,
>>>> +  if (features.vq > AARCH64_MAX_SVE_VQ)
>>>> +    error (_("VQ is %" PRIu64 ", maximum supported value is %d"), features.vq,
>>>>           AARCH64_MAX_SVE_VQ);
>>>> -  struct target_desc *tdesc = tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p];
>>>> +  struct target_desc *tdesc = tdesc_aarch64_map[features];
>>>>      if (tdesc == NULL)
>>>>        {
>>>> -      tdesc = aarch64_create_target_description (vq, pauth_p, mte_p, tls_p);
>>>> -      tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p] = tdesc;
>>>> +      tdesc = aarch64_create_target_description (features);
>>>> +      tdesc_aarch64_map[features] = tdesc;
>>>>        }
>>>>      return tdesc;
>>>> @@ -3450,7 +3451,11 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>>>>         value.  */
>>>>      const struct target_desc *tdesc = info.target_desc;
>>>>      if (!tdesc_has_registers (tdesc) || vq != aarch64_get_tdesc_vq (tdesc))
>>>> -    tdesc = aarch64_read_description (vq, false, false, false);
>>>> +    {
>>>> +      aarch64_features features;
>>>> +      features.vq = vq;
>>>> +      tdesc = aarch64_read_description (features);
>>>> +    }
>>>>      gdb_assert (tdesc);
>>>>      feature_core = tdesc_find_feature (tdesc,"org.gnu.gdb.aarch64.core");
>>>> diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
>>>> index e4cdebb6311..3b9f67d6344 100644
>>>> --- a/gdb/aarch64-tdep.h
>>>> +++ b/gdb/aarch64-tdep.h
>>>> @@ -121,8 +121,7 @@ struct aarch64_gdbarch_tdep : gdbarch_tdep
>>>>      }
>>>>    };
>>>> -const target_desc *aarch64_read_description (uint64_t vq, bool pauth_p,
>>>> -                         bool mte_p, bool tls_p);
>>>> +const target_desc *aarch64_read_description (const aarch64_features &features);
>>>>    extern int aarch64_process_record (struct gdbarch *gdbarch,
>>>>                       struct regcache *regcache, CORE_ADDR addr);
>>>> diff --git a/gdb/arch/aarch64.c b/gdb/arch/aarch64.c
>>>> index 733a3fd6d2a..0f73286f145 100644
>>>> --- a/gdb/arch/aarch64.c
>>>> +++ b/gdb/arch/aarch64.c
>>>> @@ -29,8 +29,7 @@
>>>>    /* See arch/aarch64.h.  */
>>>>    target_desc *
>>>> -aarch64_create_target_description (uint64_t vq, bool pauth_p, bool mte_p,
>>>> -                   bool tls_p)
>>>> +aarch64_create_target_description (const aarch64_features &features)
>>>>    {
>>>>      target_desc_up tdesc = allocate_target_description ();
>>>> @@ -42,19 +41,19 @@ aarch64_create_target_description (uint64_t vq, bool pauth_p, bool mte_p,
>>>>      regnum = create_feature_aarch64_core (tdesc.get (), regnum);
>>>> -  if (vq == 0)
>>>> +  if (features.vq == 0)
>>>>        regnum = create_feature_aarch64_fpu (tdesc.get (), regnum);
>>>>      else
>>>> -    regnum = create_feature_aarch64_sve (tdesc.get (), regnum, vq);
>>>> +    regnum = create_feature_aarch64_sve (tdesc.get (), regnum, features.vq);
>>>> -  if (pauth_p)
>>>> +  if (features.pauth)
>>>>        regnum = create_feature_aarch64_pauth (tdesc.get (), regnum);
>>>>      /* Memory tagging extension registers.  */
>>>> -  if (mte_p)
>>>> +  if (features.mte)
>>>>        regnum = create_feature_aarch64_mte (tdesc.get (), regnum);
>>>> -  if (tls_p)
>>>> +  if (features.tls)
>>>>        regnum = create_feature_aarch64_tls (tdesc.get (), regnum);
>>>>      return tdesc.release ();
>>>> diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
>>>> index 8496a0341f7..72ec4193eba 100644
>>>> --- a/gdb/arch/aarch64.h
>>>> +++ b/gdb/arch/aarch64.h
>>>> @@ -26,23 +26,43 @@
>>>>       used to select register sets.  */
>>>>    struct aarch64_features
>>>>    {
>>>> -  bool sve = false;
>>>> +  /* A non zero VQ value indicates both the presence of SVE and the
>>>> +     Vector Quotient - the number of 128bit chunks in an SVE Z
>>>> +     register.  */
>>>> +  uint64_t vq = 0;
>>>> +
>>>>      bool pauth = false;
>>>>      bool mte = false;
>>>>      bool tls = false;
>>>>    };
>>>> -/* Create the aarch64 target description.  A non zero VQ value indicates both
>>>> -   the presence of SVE and the Vector Quotient - the number of 128bit chunks in
>>>> -   an SVE Z register.  HAS_PAUTH_P indicates the presence of the PAUTH
>>>> -   feature.
>>>> +inline bool operator==(const aarch64_features &lhs, const aarch64_features &rhs)
>>>> +{
>>>> +  return lhs.vq == rhs.vq
>>>> +    && lhs.pauth == rhs.pauth
>>>> +    && lhs.mte == rhs.mte
>>>> +    && lhs.tls == rhs.tls;
>>>> +}
>>>> -   MTE_P indicates the presence of the Memory Tagging Extension feature.
>>>> +template<>
>>>> +struct std::hash<aarch64_features>
>>>> +{
>>>> +  std::size_t operator()(const aarch64_features &features) const noexcept
>>>> +  {
>>>> +    std::size_t h;
>>>> -   TLS_P indicates the presence of the Thread Local Storage feature.  */
>>>> +    h = features.vq;
>>>> +    h = h << 1 | features.pauth;
>>>> +    h = h << 1 | features.mte;
>>>> +    h = h << 1 | features.tls;
>>>> +    return h;
>>>> +  }
>>>> +};
>>>> -target_desc *aarch64_create_target_description (uint64_t vq, bool has_pauth_p,
>>>> -                        bool mte_p, bool tls_p);
>>>> +/* Create the aarch64 target description.  */
>>>> +
>>>> +target_desc *
>>>> +  aarch64_create_target_description (const aarch64_features &features);
>>>>    /* Register numbers of various important registers.
>>>>       Note that on SVE, the Z registers reuse the V register numbers and the V
>>>> diff --git a/gdbserver/linux-aarch64-ipa.cc b/gdbserver/linux-aarch64-ipa.cc
>>>> index dc907d3ff88..918f85f6ea9 100644
>>>> --- a/gdbserver/linux-aarch64-ipa.cc
>>>> +++ b/gdbserver/linux-aarch64-ipa.cc
>>>> @@ -152,7 +152,7 @@ get_raw_reg (const unsigned char *raw_regs, int regnum)
>>>>    const struct target_desc *
>>>>    get_ipa_tdesc (int idx)
>>>>    {
>>>> -  return aarch64_linux_read_description (0, false, false, false);
>>>> +  return aarch64_linux_read_description ({});
>>>>    }
>>>>    /* Allocate buffer for the jump pads.  The branch instruction has a reach
>>>> @@ -205,5 +205,5 @@ void
>>>>    initialize_low_tracepoint (void)
>>>>    {
>>>>      /* SVE, pauth, MTE and TLS not yet supported.  */
>>>> -  aarch64_linux_read_description (0, false, false, false);
>>>> +  aarch64_linux_read_description ({});
>>>>    }
>>>> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
>>>> index ba0a810e0f9..db508696261 100644
>>>> --- a/gdbserver/linux-aarch64-low.cc
>>>> +++ b/gdbserver/linux-aarch64-low.cc
>>>> @@ -779,11 +779,11 @@ aarch64_adjust_register_sets (const struct aarch64_features &features)
>>>>          break;
>>>>        case NT_FPREGSET:
>>>>          /* This is unavailable when SVE is present.  */
>>>> -      if (!features.sve)
>>>> +      if (features.vq == 0)
>>>>            regset->size = sizeof (struct user_fpsimd_state);
>>>>          break;
>>>>        case NT_ARM_SVE:
>>>> -      if (features.sve)
>>>> +      if (features.vq > 0)
>>>>            regset->size = SVE_PT_SIZE (AARCH64_MAX_SVE_VQ, SVE_PT_REGS_SVE);
>>>>          break;
>>>>        case NT_ARM_PAC_MASK:
>>>> @@ -824,17 +824,14 @@ aarch64_target::low_arch_setup ()
>>>>        {
>>>>          struct aarch64_features features;
>>>> -      uint64_t vq = aarch64_sve_get_vq (tid);
>>>> -      features.sve = (vq > 0);
>>>> +      features.vq = aarch64_sve_get_vq (tid);
>>>>          /* A-profile PAC is 64-bit only.  */
>>>>          features.pauth = linux_get_hwcap (8) & AARCH64_HWCAP_PACA;
>>>>          /* A-profile MTE is 64-bit only.  */
>>>>          features.mte = linux_get_hwcap2 (8) & HWCAP2_MTE;
>>>>          features.tls = true;
>>>> -      current_process ()->tdesc
>>>> -    = aarch64_linux_read_description (vq, features.pauth, features.mte,
>>>> -                      features.tls);
>>>> +      current_process ()->tdesc = aarch64_linux_read_description (features);
>>>>          /* Adjust the register sets we should use for this particular set of
>>>>         features.  */
>>>> diff --git a/gdbserver/linux-aarch64-tdesc.cc b/gdbserver/linux-aarch64-tdesc.cc
>>>> index be96612d571..93f2bedde0d 100644
>>>> --- a/gdbserver/linux-aarch64-tdesc.cc
>>>> +++ b/gdbserver/linux-aarch64-tdesc.cc
>>>> @@ -25,36 +25,36 @@
>>>>    #include "arch/aarch64.h"
>>>>    #include "linux-aarch32-low.h"
>>>>    #include <inttypes.h>
>>>> +#include <unordered_map>
>>>>    /* All possible aarch64 target descriptors.  */
>>>> -struct target_desc *tdesc_aarch64_list[AARCH64_MAX_SVE_VQ + 1][2/*pauth*/][2 /* mte */][2 /* tls */];
>>>> +static std::unordered_map<aarch64_features, target_desc *> tdesc_aarch64_map;
>>>>    /* Create the aarch64 target description.  */
>>>>    const target_desc *
>>>> -aarch64_linux_read_description (uint64_t vq, bool pauth_p, bool mte_p,
>>>> -                bool tls_p)
>>>> +aarch64_linux_read_description (const aarch64_features &features)
>>>>    {
>>>> -  if (vq > AARCH64_MAX_SVE_VQ)
>>>> -    error (_("VQ is %" PRIu64 ", maximum supported value is %d"), vq,
>>>> +  if (features.vq > AARCH64_MAX_SVE_VQ)
>>>> +    error (_("VQ is %" PRIu64 ", maximum supported value is %d"), features.vq,
>>>>           AARCH64_MAX_SVE_VQ);
>>>> -  struct target_desc *tdesc = tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p];
>>>> +  struct target_desc *tdesc = tdesc_aarch64_map[features];
>>>>      if (tdesc == NULL)
>>>>        {
>>>> -      tdesc = aarch64_create_target_description (vq, pauth_p, mte_p, tls_p);
>>>> +      tdesc = aarch64_create_target_description (features);
>>>>          static const char *expedite_regs_aarch64[] = { "x29", "sp", "pc", NULL };
>>>>          static const char *expedite_regs_aarch64_sve[] = { "x29", "sp", "pc",
>>>>                                 "vg", NULL };
>>>> -      if (vq == 0)
>>>> +      if (features.vq == 0)
>>>>        init_target_desc (tdesc, expedite_regs_aarch64);
>>>>          else
>>>>        init_target_desc (tdesc, expedite_regs_aarch64_sve);
>>>> -      tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p] = tdesc;
>>>> +      tdesc_aarch64_map[features] = tdesc;
>>>>        }
>>>>      return tdesc;
>>>> diff --git a/gdbserver/linux-aarch64-tdesc.h b/gdbserver/linux-aarch64-tdesc.h
>>>> index 4ab658447a2..30bcd24d13d 100644
>>>> --- a/gdbserver/linux-aarch64-tdesc.h
>>>> +++ b/gdbserver/linux-aarch64-tdesc.h
>>>> @@ -20,7 +20,9 @@
>>>>    #ifndef GDBSERVER_LINUX_AARCH64_TDESC_H
>>>>    #define GDBSERVER_LINUX_AARCH64_TDESC_H
>>>> -const target_desc * aarch64_linux_read_description (uint64_t vq, bool pauth_p,
>>>> -                            bool mte_p, bool tls_p);
>>>> +#include "arch/aarch64.h"
>>>> +
>>>> +const target_desc *
>>>> +  aarch64_linux_read_description (const aarch64_features &features);
>>>>    #endif /* GDBSERVER_LINUX_AARCH64_TDESC_H */
>>>> diff --git a/gdbserver/netbsd-aarch64-low.cc b/gdbserver/netbsd-aarch64-low.cc
>>>> index b371e599232..f8447b0d1ee 100644
>>>> --- a/gdbserver/netbsd-aarch64-low.cc
>>>> +++ b/gdbserver/netbsd-aarch64-low.cc
>>>> @@ -96,7 +96,7 @@ void
>>>>    netbsd_aarch64_target::low_arch_setup ()
>>>>    {
>>>>      target_desc *tdesc
>>>> -    = aarch64_create_target_description (0, false, false, false);
>>>> +    = aarch64_create_target_description ({});
>>>>      static const char *expedite_regs_aarch64[] = { "x29", "sp", "pc", NULL };
>>>>      init_target_desc (tdesc, expedite_regs_aarch64);
>>>
>>>
>>
>> It seems this is running into the following for older compilers:
>>
>> /binutils-gdb--gdb/gdb/arch/aarch64.h:48:13: error: specialization of 'template<class _Tp> struct std::hash' in different namespace [-fpermissive]
>>
>>    struct std::hash<aarch64_features>
>>
>> This is GCC 6.4.1. This feels like a bug in the compiler according to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56480. But can we make it so the compiler is happy with it?
> Hmm, I don't have an old version of GCC easily accessible, but I think
> this might fix it:

It does. I tested this with both the old a new compilers. Would you please push it?

Thanks!

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-05-23  9:29 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-05 18:46 [PATCH] Use aarch64_features to describe register features in target descriptions John Baldwin
2022-05-18 13:59 ` John Baldwin
2022-05-18 14:23   ` Luis Machado
2022-05-20 11:52   ` Luis Machado
2022-05-20 16:39     ` John Baldwin
2022-05-23  9:29       ` Luis Machado

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).