[PATCH 1/2] Add linux_get_hwcap

[PATCH 1/2] Add linux_get_hwcap

[Alan Hayward]

Tidy up calls to read HWCAP (and HWCAP2) by adding common functions,
removing the PPC and AArch64 specific versions.

The only function difference is in aarch64_linux_core_read_description - if
the hwcap read fails it now return a valid description instead of nullptr.

Tested using AArch64, making sure PAUTH is detected correctly.

gdb/ChangeLog:

2019-03-25  Alan Hayward  <alan.hayward@arm.com>

	* aarch64-linux-nat.c (aarch64_linux_nat_target::read_description):
	Call linux_get_hwcap.
	* aarch64-linux-tdep.c (aarch64_linux_core_read_description):
	Likewise.
	(aarch64_linux_get_hwcap): Remove function.
	* aarch64-linux-tdep.h (aarch64_linux_get_hwcap): Remove
	declaration.
	* arm-linux-nat.c (arm_linux_nat_target::read_description):Call
	linux_get_hwcap.
	* arm-linux-tdep.c (arm_linux_core_read_description): Likewise.
	* linux-tdep.c (linux_get_hwcap): Add function.
	(linux_get_hwcap2): Likewise.
	* linux-tdep.h (linux_get_hwcap): Add declaration.
	(linux_get_hwcap2): Likewise.
	* ppc-linux-nat.c (ppc_linux_get_hwcap): Remove function.
	(ppc_linux_get_hwcap2): Likewise.
	(ppc_linux_nat_target::region_ok_for_hw_watchpoint): Call
	linux_get_hwcap.
	(ppc_linux_nat_target::insert_watchpoint): Likewise.
	(ppc_linux_nat_target::watchpoint_addr_within_range): Likewise.
	(ppc_linux_nat_target::read_description): Likewise.
	* ppc-linux-tdep.c (ppc_linux_core_read_description): Likewise.
	* s390-linux-nat.c: Likewise.
	* s390-linux-tdep.c (s390_core_read_description): Likewise.
---
 gdb/aarch64-linux-nat.c  |  8 ++++----
 gdb/aarch64-linux-tdep.c | 16 ++--------------
 gdb/aarch64-linux-tdep.h |  3 ---
 gdb/arm-linux-nat.c      |  7 +------
 gdb/arm-linux-tdep.c     |  5 +----
 gdb/linux-tdep.c         | 22 ++++++++++++++++++++++
 gdb/linux-tdep.h         |  6 ++++++
 gdb/ppc-linux-nat.c      | 39 +++++++--------------------------------
 gdb/ppc-linux-tdep.c     |  5 +----
 gdb/s390-linux-nat.c     |  3 +--
 gdb/s390-linux-tdep.c    |  3 +--
 11 files changed, 46 insertions(+), 71 deletions(-)

diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index 86c7e87dd5..6d43eb7070 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -42,6 +42,7 @@
 #include <asm/ptrace.h>
 
 #include "gregset.h"
+#include "linux-tdep.h"
 
 /* Defines ps_err_e, struct ps_prochandle.  */
 #include "gdb_proc_service.h"
@@ -641,11 +642,10 @@ aarch64_linux_nat_target::read_description ()
   if (ret == 0)
     return tdesc_arm_with_neon;
 
-  CORE_ADDR hwcap = 0;
-  bool pauth_p = aarch64_linux_get_hwcap (this, &hwcap)
-		 && (hwcap & AARCH64_HWCAP_PACA);
+  CORE_ADDR hwcap = linux_get_hwcap (this);
 
-  return aarch64_read_description (aarch64_sve_get_vq (tid), pauth_p);
+  return aarch64_read_description (aarch64_sve_get_vq (tid),
+				   hwcap & AARCH64_HWCAP_PACA);
 }
 
 /* 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 bcd1961744..7f2193f2fa 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -655,13 +655,10 @@ static const struct target_desc *
 aarch64_linux_core_read_description (struct gdbarch *gdbarch,
 				     struct target_ops *target, bfd *abfd)
 {
-  CORE_ADDR aarch64_hwcap = 0;
-
-  if (!aarch64_linux_get_hwcap (target, &aarch64_hwcap))
-    return nullptr;
+  CORE_ADDR hwcap = linux_get_hwcap (target);
 
   return aarch64_read_description (aarch64_linux_core_read_vq (gdbarch, abfd),
-				   aarch64_hwcap & AARCH64_HWCAP_PACA);
+				   hwcap & AARCH64_HWCAP_PACA);
 }
 
 /* Implementation of `gdbarch_stap_is_single_operand', as defined in
@@ -1439,15 +1436,6 @@ aarch64_linux_gcc_target_options (struct gdbarch *gdbarch)
   return NULL;
 }
 
-/* See aarch64-linux-tdep.h.  */
-
-bool
-aarch64_linux_get_hwcap (struct target_ops *target, CORE_ADDR *hwcap)
-{
-  *hwcap = 0;
-  return target_auxv_search (target, AT_HWCAP, hwcap) == 1;
-}
-
 static void
 aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
diff --git a/gdb/aarch64-linux-tdep.h b/gdb/aarch64-linux-tdep.h
index ec494bfc53..f075f0acd9 100644
--- a/gdb/aarch64-linux-tdep.h
+++ b/gdb/aarch64-linux-tdep.h
@@ -42,7 +42,4 @@ extern const struct regset aarch64_linux_fpregset;
 /* Matches HWCAP_PACA in kernel header arch/arm64/include/uapi/asm/hwcap.h.  */
 #define AARCH64_HWCAP_PACA (1 << 30)
 
-/* Fetch the AT_HWCAP entry from the auxv vector for the given TARGET.  */
-bool aarch64_linux_get_hwcap (struct target_ops *target, CORE_ADDR *hwcap);
-
 #endif /* AARCH64_LINUX_TDEP_H */
diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c
index 11e353e61c..b54bd5afe9 100644
--- a/gdb/arm-linux-nat.c
+++ b/gdb/arm-linux-nat.c
@@ -533,7 +533,7 @@ ps_get_thread_area (struct ps_prochandle *ph,
 const struct target_desc *
 arm_linux_nat_target::read_description ()
 {
-  CORE_ADDR arm_hwcap = 0;
+  CORE_ADDR arm_hwcap = linux_get_hwcap (this);
 
   if (have_ptrace_getregset == TRIBOOL_UNKNOWN)
     {
@@ -551,11 +551,6 @@ arm_linux_nat_target::read_description ()
 	have_ptrace_getregset = TRIBOOL_TRUE;
     }
 
-  if (target_auxv_search (this, AT_HWCAP, &arm_hwcap) != 1)
-    {
-      return this->beneath ()->read_description ();
-    }
-
   if (arm_hwcap & HWCAP_IWMMXT)
     return tdesc_arm_with_iwmmxt;
 
diff --git a/gdb/arm-linux-tdep.c b/gdb/arm-linux-tdep.c
index 11db9bae32..a5ad06434c 100644
--- a/gdb/arm-linux-tdep.c
+++ b/gdb/arm-linux-tdep.c
@@ -730,10 +730,7 @@ arm_linux_core_read_description (struct gdbarch *gdbarch,
                                  struct target_ops *target,
                                  bfd *abfd)
 {
-  CORE_ADDR arm_hwcap = 0;
-
-  if (target_auxv_search (target, AT_HWCAP, &arm_hwcap) != 1)
-    return NULL;
+  CORE_ADDR arm_hwcap = linux_get_hwcap (target);
 
   if (arm_hwcap & HWCAP_VFP)
     {
diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index 77bc714286..f6c4f7b208 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -2462,6 +2462,28 @@ linux_displaced_step_location (struct gdbarch *gdbarch)
   return addr;
 }
 
+/* See linux-tdep.h.  */
+
+CORE_ADDR
+linux_get_hwcap (struct target_ops *target)
+{
+  CORE_ADDR field;
+  if (target_auxv_search (target, AT_HWCAP, &field) != 1)
+    return 0;
+  return field;
+}
+
+/* See linux-tdep.h.  */
+
+CORE_ADDR
+linux_get_hwcap2 (struct target_ops *target)
+{
+  CORE_ADDR field;
+  if (target_auxv_search (target, AT_HWCAP2, &field) != 1)
+    return 0;
+  return field;
+}
+
 /* Display whether the gcore command is using the
    /proc/PID/coredump_filter file.  */
 
diff --git a/gdb/linux-tdep.h b/gdb/linux-tdep.h
index 824ba3afaf..ee9c2bcc90 100644
--- a/gdb/linux-tdep.h
+++ b/gdb/linux-tdep.h
@@ -61,4 +61,10 @@ extern void linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch);
 
 extern int linux_is_uclinux (void);
 
+/* Fetch the AT_HWCAP entry from the auxv vector for the given TARGET.  */
+extern CORE_ADDR linux_get_hwcap (struct target_ops *target);
+
+/* Fetch the AT_HWCAP2 entry from the auxv vector for the given TARGET.  */
+extern CORE_ADDR linux_get_hwcap2 (struct target_ops *target);
+
 #endif /* linux-tdep.h */
diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
index 11394ef36d..3a6bbf4163 100644
--- a/gdb/ppc-linux-nat.c
+++ b/gdb/ppc-linux-nat.c
@@ -1554,31 +1554,6 @@ store_ppc_registers (const struct regcache *regcache, int tid)
      function to fail most of the time, so we ignore them.  */
 }
 
-/* Fetch the AT_HWCAP entry from the aux vector.  */
-static CORE_ADDR
-ppc_linux_get_hwcap (void)
-{
-  CORE_ADDR field;
-
-  if (target_auxv_search (current_top_target (), AT_HWCAP, &field) != 1)
-    return 0;
-
-  return field;
-}
-
-/* Fetch the AT_HWCAP2 entry from the aux vector.  */
-
-static CORE_ADDR
-ppc_linux_get_hwcap2 (void)
-{
-  CORE_ADDR field;
-
-  if (target_auxv_search (current_top_target (), AT_HWCAP2, &field) != 1)
-    return 0;
-
-  return field;
-}
-
 /* The cached DABR value, to install in new threads.
    This variable is used when the PowerPC HWDEBUG ptrace
    interface is not available.  */
@@ -1735,7 +1710,7 @@ ppc_linux_nat_target::region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
          takes two hardware watchpoints though.  */
       if (len > 1
 	  && hwdebug_info.features & PPC_DEBUG_FEATURE_DATA_BP_RANGE
-	  && ppc_linux_get_hwcap () & PPC_FEATURE_BOOKE)
+	  && linux_get_hwcap (current_top_target ()) & PPC_FEATURE_BOOKE)
 	return 2;
       /* Check if the processor provides DAWR interface.  */
       if (hwdebug_info.features & PPC_DEBUG_FEATURE_DATA_BP_DAWR)
@@ -1755,7 +1730,7 @@ ppc_linux_nat_target::region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
      ptrace interface, DAC-based processors (i.e., embedded processors) will
      use addresses aligned to 4-bytes due to the way the read/write flags are
      passed in the old ptrace interface.  */
-  else if (((ppc_linux_get_hwcap () & PPC_FEATURE_BOOKE)
+  else if (((linux_get_hwcap (current_top_target ()) & PPC_FEATURE_BOOKE)
 	   && (addr + len) > (addr & ~3) + 4)
 	   || (addr + len) > (addr & ~7) + 8)
     return 0;
@@ -2294,7 +2269,7 @@ ppc_linux_nat_target::insert_watchpoint (CORE_ADDR addr, int len,
       long dabr_value;
       long read_mode, write_mode;
 
-      if (ppc_linux_get_hwcap () & PPC_FEATURE_BOOKE)
+      if (linux_get_hwcap (current_top_target ()) & PPC_FEATURE_BOOKE)
 	{
 	  /* PowerPC 440 requires only the read/write flags to be passed
 	     to the kernel.  */
@@ -2497,9 +2472,9 @@ ppc_linux_nat_target::watchpoint_addr_within_range (CORE_ADDR addr,
   int mask;
 
   if (have_ptrace_hwdebug_interface ()
-      && ppc_linux_get_hwcap () & PPC_FEATURE_BOOKE)
+      && linux_get_hwcap (current_top_target ()) & PPC_FEATURE_BOOKE)
     return start <= addr && start + length >= addr;
-  else if (ppc_linux_get_hwcap () & PPC_FEATURE_BOOKE)
+  else if (linux_get_hwcap (current_top_target ()) & PPC_FEATURE_BOOKE)
     mask = 3;
   else
     mask = 7;
@@ -2637,8 +2612,8 @@ ppc_linux_nat_target::read_description ()
 
   features.wordsize = ppc_linux_target_wordsize (tid);
 
-  CORE_ADDR hwcap = ppc_linux_get_hwcap ();
-  CORE_ADDR hwcap2 = ppc_linux_get_hwcap2 ();
+  CORE_ADDR hwcap = linux_get_hwcap (current_top_target ());
+  CORE_ADDR hwcap2 = linux_get_hwcap2 (current_top_target ());
 
   if (have_ptrace_getsetvsxregs
       && (hwcap & PPC_FEATURE_HAS_VSX))
diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c
index df44ad826e..19435602b5 100644
--- a/gdb/ppc-linux-tdep.c
+++ b/gdb/ppc-linux-tdep.c
@@ -1601,10 +1601,7 @@ ppc_linux_core_read_description (struct gdbarch *gdbarch,
   if (vsx)
     features.vsx = true;
 
-  CORE_ADDR hwcap;
-
-  if (target_auxv_search (target, AT_HWCAP, &hwcap) != 1)
-    hwcap = 0;
+  CORE_ADDR hwcap = linux_get_hwcap (target);
 
   features.isa205 = ppc_linux_has_isa205 (hwcap);
 
diff --git a/gdb/s390-linux-nat.c b/gdb/s390-linux-nat.c
index 5f6c5be8e0..3a3afae7a6 100644
--- a/gdb/s390-linux-nat.c
+++ b/gdb/s390-linux-nat.c
@@ -1016,9 +1016,8 @@ s390_linux_nat_target::read_description ()
      that mode, report s390 architecture with 64-bit GPRs.  */
 #ifdef __s390x__
   {
-    CORE_ADDR hwcap = 0;
+    CORE_ADDR hwcap = linux_get_hwcap (current_top_target ());
 
-    target_auxv_search (current_top_target (), AT_HWCAP, &hwcap);
     have_regset_tdb = (hwcap & HWCAP_S390_TE)
       && check_regset (tid, NT_S390_TDB, s390_sizeof_tdbregset);
 
diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c
index 64fe7abd80..02ae28b4ea 100644
--- a/gdb/s390-linux-tdep.c
+++ b/gdb/s390-linux-tdep.c
@@ -332,10 +332,9 @@ s390_core_read_description (struct gdbarch *gdbarch,
 			    struct target_ops *target, bfd *abfd)
 {
   asection *section = bfd_get_section_by_name (abfd, ".reg");
-  CORE_ADDR hwcap = 0;
+  CORE_ADDR hwcap = linux_get_hwcap (target);
   bool high_gprs, v1, v2, te, vx, gs;
 
-  target_auxv_search (target, AT_HWCAP, &hwcap);
   if (!section)
     return NULL;
 
-- 
2.17.2 (Apple Git-113)

[PATCH 2/2] gdbserver: Add linux_get_hwcap

[Alan Hayward]

In gdbserver, Tidy up calls to read HWCAP (and HWCAP2) by adding common
functions, removing the Arm, AArch64, PPC and S390 specific versions.

No functionality differences.

[ I wasn't sure in gdbserver when to use CORE_ADDR and when to use int/long.
  I'm assuming CORE_ADDR is fairly recent to gdbserver? ]

Tested using AArch64, making sure PAUTH is detected correctly.

gdb/gdbserver/ChangeLog:

2019-03-25  Alan Hayward  <alan.hayward@arm.com>

	* linux-aarch64-low.c (aarch64_get_hwcap): Remove function.
	(aarch64_arch_setup): Call linux_get_hwcap.
	* linux-arm-low.c (arm_get_hwcap): Remove function.
	(arm_read_description): Call linux_get_hwcap.
	* linux-low.c (linux_get_auxv): New function.
	(linux_get_hwcap): Likewise.
	(linux_get_hwcap2): Likewise.
	* linux-low.h (linux_get_hwcap): New declaration.
	(linux_get_hwcap2): Likewise.
	* linux-ppc-low.c (ppc_get_auxv): Remove function.
	(ppc_arch_setup): Call linux_get_hwcap.
	* linux-s390-low.c (s390_get_hwcap): Remove function.
	(s390_arch_setup): Call linux_get_hwcap.
---
 gdb/gdbserver/linux-aarch64-low.c | 28 ++-------------
 gdb/gdbserver/linux-arm-low.c     | 27 +-------------
 gdb/gdbserver/linux-low.c         | 58 +++++++++++++++++++++++++++++++
 gdb/gdbserver/linux-low.h         |  8 +++++
 gdb/gdbserver/linux-ppc-low.c     | 41 ++--------------------
 gdb/gdbserver/linux-s390-low.c    | 32 +----------------
 6 files changed, 72 insertions(+), 122 deletions(-)

diff --git a/gdb/gdbserver/linux-aarch64-low.c b/gdb/gdbserver/linux-aarch64-low.c
index 20c75493b0..dc4ee81d2a 100644
--- a/gdb/gdbserver/linux-aarch64-low.c
+++ b/gdb/gdbserver/linux-aarch64-low.c
@@ -505,30 +505,6 @@ aarch64_linux_new_fork (struct process_info *parent,
 /* Matches HWCAP_PACA in kernel header arch/arm64/include/uapi/asm/hwcap.h.  */
 #define AARCH64_HWCAP_PACA (1 << 30)
 
-/* Fetch the AT_HWCAP entry from the auxv vector.  */
-
-static bool
-aarch64_get_hwcap (unsigned long *valp)
-{
-  unsigned char *data = (unsigned char *) alloca (16);
-  int offset = 0;
-
-  while ((*the_target->read_auxv) (offset, data, 16) == 16)
-    {
-      unsigned long *data_p = (unsigned long *)data;
-      if (data_p[0] == AT_HWCAP)
-	{
-	  *valp = data_p[1];
-	  return true;
-	}
-
-      offset += 16;
-    }
-
-  *valp = 0;
-  return false;
-}
-
 /* Implementation of linux_target_ops method "arch_setup".  */
 
 static void
@@ -545,8 +521,8 @@ aarch64_arch_setup (void)
   if (is_elf64)
     {
       uint64_t vq = aarch64_sve_get_vq (tid);
-      unsigned long hwcap = 0;
-      bool pauth_p = aarch64_get_hwcap (&hwcap) && (hwcap & AARCH64_HWCAP_PACA);
+      unsigned long hwcap = linux_get_hwcap (8);
+      bool pauth_p = hwcap & AARCH64_HWCAP_PACA;
 
       current_process ()->tdesc = aarch64_linux_read_description (vq, pauth_p);
     }
diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
index 8cad5c5fd4..ff72a489cb 100644
--- a/gdb/gdbserver/linux-arm-low.c
+++ b/gdb/gdbserver/linux-arm-low.c
@@ -847,40 +847,15 @@ get_next_pcs_syscall_next_pc (struct arm_get_next_pcs *self)
   return next_pc;
 }
 
-static int
-arm_get_hwcap (unsigned long *valp)
-{
-  unsigned char *data = (unsigned char *) alloca (8);
-  int offset = 0;
-
-  while ((*the_target->read_auxv) (offset, data, 8) == 8)
-    {
-      unsigned int *data_p = (unsigned int *)data;
-      if (data_p[0] == AT_HWCAP)
-	{
-	  *valp = data_p[1];
-	  return 1;
-	}
-
-      offset += 8;
-    }
-
-  *valp = 0;
-  return 0;
-}
-
 static const struct target_desc *
 arm_read_description (void)
 {
   int pid = lwpid_of (current_thread);
-  unsigned long arm_hwcap = 0;
+  unsigned long arm_hwcap = linux_get_hwcap (4);
 
   /* Query hardware watchpoint/breakpoint capabilities.  */
   arm_linux_init_hwbp_cap (pid);
 
-  if (arm_get_hwcap (&arm_hwcap) == 0)
-    return tdesc_arm;
-
   if (arm_hwcap & HWCAP_IWMMXT)
     return tdesc_arm_with_iwmmxt;
 
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 6f703f589f..481919c205 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -7423,6 +7423,64 @@ linux_get_pc_64bit (struct regcache *regcache)
   return pc;
 }
 
+/* Extract the auxiliary vector entry with a_type matching MATCH, storing the
+   value in VALP and returning true.  If no entry was found, return false.  */
+
+static bool
+linux_get_auxv (int wordsize, CORE_ADDR match, CORE_ADDR *valp)
+{
+  gdb_byte *data = (gdb_byte *) alloca (2 * wordsize);
+  int offset = 0;
+
+  while ((*the_target->read_auxv) (offset, data, 2 * wordsize) == 2 * wordsize)
+    {
+      if (wordsize == 4)
+	{
+	  unsigned int *data_p = (unsigned int *)data;
+	  if (data_p[0] == match)
+	    {
+	      *valp = data_p[1];
+	      return true;
+	    }
+	}
+      else
+	{
+	  unsigned long *data_p = (unsigned long *)data;
+	  if (data_p[0] == match)
+	    {
+	      *valp = data_p[1];
+	      return true;
+	    }
+	}
+
+      offset += 2 * wordsize;
+    }
+
+  *valp = 0;
+  return false;
+}
+
+/* See linux-low.h.  */
+
+CORE_ADDR
+linux_get_hwcap (int wordsize)
+{
+  CORE_ADDR field;
+  if (!linux_get_auxv (wordsize, AT_HWCAP, &field))
+    return 0;
+  return field;
+}
+
+/* See linux-low.h.  */
+
+CORE_ADDR
+linux_get_hwcap2 (int wordsize)
+{
+  CORE_ADDR field;
+  if (!linux_get_auxv (wordsize, AT_HWCAP2, &field))
+    return 0;
+  return field;
+}
 
 static struct target_ops linux_target_ops = {
   linux_create_inferior,
diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
index 1ade35d648..711a44b665 100644
--- a/gdb/gdbserver/linux-low.h
+++ b/gdb/gdbserver/linux-low.h
@@ -435,4 +435,12 @@ bool thread_db_thread_handle (ptid_t ptid, gdb_byte **handle, int *handle_len);
 
 extern int have_ptrace_getregset;
 
+/* Fetch the AT_HWCAP entry from the auxv vector, where entries are length
+   WORDSIZE.  */
+CORE_ADDR linux_get_hwcap (int wordsize);
+
+/* Fetch the AT_HWCAP2 entry from the auxv vector, where entries are length
+   WORDSIZE.  */
+CORE_ADDR linux_get_hwcap2 (int wordsize);
+
 #endif /* GDBSERVER_LINUX_LOW_H */
diff --git a/gdb/gdbserver/linux-ppc-low.c b/gdb/gdbserver/linux-ppc-low.c
index 1b695e53fe..8deb0ce068 100644
--- a/gdb/gdbserver/linux-ppc-low.c
+++ b/gdb/gdbserver/linux-ppc-low.c
@@ -323,43 +323,6 @@ ppc_set_pc (struct regcache *regcache, CORE_ADDR pc)
     }
 }
 
-
-static int
-ppc_get_auxv (unsigned long type, unsigned long *valp)
-{
-  const struct target_desc *tdesc = current_process ()->tdesc;
-  int wordsize = register_size (tdesc, 0);
-  unsigned char *data = (unsigned char *) alloca (2 * wordsize);
-  int offset = 0;
-
-  while ((*the_target->read_auxv) (offset, data, 2 * wordsize) == 2 * wordsize)
-    {
-      if (wordsize == 4)
-	{
-	  unsigned int *data_p = (unsigned int *)data;
-	  if (data_p[0] == type)
-	    {
-	      *valp = data_p[1];
-	      return 1;
-	    }
-	}
-      else
-	{
-	  unsigned long *data_p = (unsigned long *)data;
-	  if (data_p[0] == type)
-	    {
-	      *valp = data_p[1];
-	      return 1;
-	    }
-	}
-
-      offset += 2 * wordsize;
-    }
-
-  *valp = 0;
-  return 0;
-}
-
 #ifndef __powerpc64__
 static int ppc_regmap_adjusted;
 #endif
@@ -944,8 +907,8 @@ ppc_arch_setup (void)
 
   /* The value of current_process ()->tdesc needs to be set for this
      call.  */
-  ppc_get_auxv (AT_HWCAP, &ppc_hwcap);
-  ppc_get_auxv (AT_HWCAP2, &ppc_hwcap2);
+  ppc_hwcap = linux_get_hwcap (features.wordsize);
+  ppc_hwcap2 = linux_get_hwcap2 (features.wordsize);
 
   features.isa205 = ppc_linux_has_isa205 (ppc_hwcap);
 
diff --git a/gdb/gdbserver/linux-s390-low.c b/gdb/gdbserver/linux-s390-low.c
index edbef77fe9..f65a1ec38e 100644
--- a/gdb/gdbserver/linux-s390-low.c
+++ b/gdb/gdbserver/linux-s390-low.c
@@ -467,36 +467,6 @@ s390_set_pc (struct regcache *regcache, CORE_ADDR newpc)
     }
 }
 
-/* Get HWCAP from AUXV, using the given WORDSIZE.  Return the HWCAP, or
-   zero if not found.  */
-
-static unsigned long
-s390_get_hwcap (int wordsize)
-{
-  gdb_byte *data = (gdb_byte *) alloca (2 * wordsize);
-  int offset = 0;
-
-  while ((*the_target->read_auxv) (offset, data, 2 * wordsize) == 2 * wordsize)
-    {
-      if (wordsize == 4)
-        {
-          unsigned int *data_p = (unsigned int *)data;
-          if (data_p[0] == AT_HWCAP)
-	    return data_p[1];
-        }
-      else
-        {
-          unsigned long *data_p = (unsigned long *)data;
-          if (data_p[0] == AT_HWCAP)
-	    return data_p[1];
-        }
-
-      offset += 2 * wordsize;
-    }
-
-  return 0;
-}
-
 /* Determine the word size for the given PID, in bytes.  */
 
 #ifdef __s390x__
@@ -548,7 +518,7 @@ s390_arch_setup (void)
   /* Determine word size and HWCAP.  */
   int pid = pid_of (current_thread);
   int wordsize = s390_get_wordsize (pid);
-  unsigned long hwcap = s390_get_hwcap (wordsize);
+  unsigned long hwcap = linux_get_hwcap (wordsize);
 
   /* Check whether the kernel supports extra register sets.  */
   int have_regset_last_break
-- 
2.17.2 (Apple Git-113)

Re: [PATCH 1/2] Add linux_get_hwcap

[Simon Marchi]

On 2019-03-25 8:05 a.m., Alan Hayward wrote:
> Tidy up calls to read HWCAP (and HWCAP2) by adding common functions,
> removing the PPC and AArch64 specific versions.
> 
> The only function difference is in aarch64_linux_core_read_description - if
> the hwcap read fails it now return a valid description instead of nullptr.

ARM also seems to have changed behavior, both the native target and core 
target.  It's not necessarily, a problem, as long as it's a conscious 
change.

> diff --git a/gdb/linux-tdep.h b/gdb/linux-tdep.h
> index 824ba3afaf..ee9c2bcc90 100644
> --- a/gdb/linux-tdep.h
> +++ b/gdb/linux-tdep.h
> @@ -61,4 +61,10 @@ extern void linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch);
>   
>   extern int linux_is_uclinux (void);
>   
> +/* Fetch the AT_HWCAP entry from the auxv vector for the given TARGET.  */
> +extern CORE_ADDR linux_get_hwcap (struct target_ops *target);
> +
> +/* Fetch the AT_HWCAP2 entry from the auxv vector for the given TARGET.  */
> +extern CORE_ADDR linux_get_hwcap2 (struct target_ops *target);

For these two functions, can you mention that 0 is returned if the 
search in the AUXV vector fails?

I was a bit surprised you didn't keep your version returning a bool to 
indicate whether the search succeeded or failed, but if this version is 
good enough, I am fine with it.  If we ever have a target that really 
needs to differentiate between a lookup failure and a lookup success 
that returns the value 0, we can change it back or add another overload.

This patch LGTM with the comment above updated.

Simon

Re: [PATCH 2/2] gdbserver: Add linux_get_hwcap

[Simon Marchi]

On 2019-03-25 8:05 a.m., Alan Hayward wrote:
> In gdbserver, Tidy up calls to read HWCAP (and HWCAP2) by adding common
> functions, removing the Arm, AArch64, PPC and S390 specific versions.
> 
> No functionality differences.
> 
> [ I wasn't sure in gdbserver when to use CORE_ADDR and when to use int/long.
>    I'm assuming CORE_ADDR is fairly recent to gdbserver? ]

I don't know if CORE_ADDR is a recent addition to gdbserver.  But I 
suppose CORE_ADDR was chosen as the return type for functions reading 
arbitrary AUXV values, since some of them may be pointers.  With 
CORE_ADDR, we know those values will fit in the data type.  When we 
return the HWCAP value, we know it won't be a pointer though, so 
returning a CORE_ADDR is a bit confusing, IMO.  Those functions 
returning the HWCAP value could return something else, an uint64_t 
maybe.  But then I would change it in the gdb version as well to match.

>   /* Implementation of linux_target_ops method "arch_setup".  */
>   
>   static void
> @@ -545,8 +521,8 @@ aarch64_arch_setup (void)
>     if (is_elf64)
>       {
>         uint64_t vq = aarch64_sve_get_vq (tid);
> -      unsigned long hwcap = 0;
> -      bool pauth_p = aarch64_get_hwcap (&hwcap) && (hwcap & AARCH64_HWCAP_PACA);
> +      unsigned long hwcap = linux_get_hwcap (8);
> +      bool pauth_p = hwcap & AARCH64_HWCAP_PACA;

Just wondering, can the linux-aarch64-low.c code be used to debug a process

> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index 6f703f589f..481919c205 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -7423,6 +7423,64 @@ linux_get_pc_64bit (struct regcache *regcache)
>     return pc;
>   }
>   
> +/* Extract the auxiliary vector entry with a_type matching MATCH, storing the
> +   value in VALP and returning true.  If no entry was found, return false.  */
> +
> +static bool
> +linux_get_auxv (int wordsize, CORE_ADDR match, CORE_ADDR *valp)

I think this function could return the result (CORE_ADDR) directly,
returning 0 on failure.

If 4 and 8 are the only supported wordsize values, I would suggest 
adding an assert to verify it.

> +{
> +  gdb_byte *data = (gdb_byte *) alloca (2 * wordsize);
> +  int offset = 0;
> +
> +  while ((*the_target->read_auxv) (offset, data, 2 * wordsize) == 2 * wordsize)
> +    {
> +      if (wordsize == 4)
> +	{
> +	  unsigned int *data_p = (unsigned int *)data;
> +	  if (data_p[0] == match)
> +	    {
> +	      *valp = data_p[1];
> +	      return true;
> +	    }
> +	}
> +      else
> +	{
> +	  unsigned long *data_p = (unsigned long *)data;
> +	  if (data_p[0] == match)
> +	    {
> +	      *valp = data_p[1];
> +	      return true;
> +	    }
> +	}

I am a bit worried about relying on the size of the "int" and "long" 
types in architecture-independent code.  Could we use uint32_t and 
uint64_t instead?

Simon

Re: [PATCH 1/2] Add linux_get_hwcap

[Alan Hayward]

> On 25 Mar 2019, at 15:18, Simon Marchi <simark@simark.ca> wrote:
> 
> On 2019-03-25 8:05 a.m., Alan Hayward wrote:
>> Tidy up calls to read HWCAP (and HWCAP2) by adding common functions,
>> removing the PPC and AArch64 specific versions.
>> The only function difference is in aarch64_linux_core_read_description - if
>> the hwcap read fails it now return a valid description instead of nullptr.
> 
> ARM also seems to have changed behavior, both the native target and core target.  It's not necessarily, a problem, as long as it's a conscious change.

I should have mentioned it really.  In the original it returned immediately on error.
In the new version it continues, but all the “if”s will fail and it’ll return at the
end of the function with the same result as the original early return.

> 
>> diff --git a/gdb/linux-tdep.h b/gdb/linux-tdep.h
>> index 824ba3afaf..ee9c2bcc90 100644
>> --- a/gdb/linux-tdep.h
>> +++ b/gdb/linux-tdep.h
>> @@ -61,4 +61,10 @@ extern void linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch);
>>    extern int linux_is_uclinux (void);
>>  +/* Fetch the AT_HWCAP entry from the auxv vector for the given TARGET.  */
>> +extern CORE_ADDR linux_get_hwcap (struct target_ops *target);
>> +
>> +/* Fetch the AT_HWCAP2 entry from the auxv vector for the given TARGET.  */
>> +extern CORE_ADDR linux_get_hwcap2 (struct target_ops *target);
> 
> For these two functions, can you mention that 0 is returned if the search in the AUXV vector fails?
> 

Done.

> I was a bit surprised you didn't keep your version returning a bool to indicate whether the search succeeded or failed, but if this version is good enough, I am fine with it.  If we ever have a target that really needs to differentiate between a lookup failure and a lookup success that returns the value 0, we can change it back or add another overload.
> 

Once I had changed the AArch64 code, there was nothing left that was checking the error.
So it seemed better to remove it.


> This patch LGTM with the comment above updated.
> 
> Simon

Thanks. Pushed code included below.

Alan.

diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index 86c7e87dd5..6d43eb7070 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -42,6 +42,7 @@
 #include <asm/ptrace.h>

 #include "gregset.h"
+#include "linux-tdep.h"

 /* Defines ps_err_e, struct ps_prochandle.  */
 #include "gdb_proc_service.h"
@@ -641,11 +642,10 @@ aarch64_linux_nat_target::read_description ()
   if (ret == 0)
     return tdesc_arm_with_neon;

-  CORE_ADDR hwcap = 0;
-  bool pauth_p = aarch64_linux_get_hwcap (this, &hwcap)
-                && (hwcap & AARCH64_HWCAP_PACA);
+  CORE_ADDR hwcap = linux_get_hwcap (this);

-  return aarch64_read_description (aarch64_sve_get_vq (tid), pauth_p);
+  return aarch64_read_description (aarch64_sve_get_vq (tid),
+                                  hwcap & AARCH64_HWCAP_PACA);
 }

 /* 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 bcd1961744..7f2193f2fa 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -655,13 +655,10 @@ static const struct target_desc *
 aarch64_linux_core_read_description (struct gdbarch *gdbarch,
                                     struct target_ops *target, bfd *abfd)
 {
-  CORE_ADDR aarch64_hwcap = 0;
-
-  if (!aarch64_linux_get_hwcap (target, &aarch64_hwcap))
-    return nullptr;
+  CORE_ADDR hwcap = linux_get_hwcap (target);

   return aarch64_read_description (aarch64_linux_core_read_vq (gdbarch, abfd),
-                                  aarch64_hwcap & AARCH64_HWCAP_PACA);
+                                  hwcap & AARCH64_HWCAP_PACA);
 }

 /* Implementation of `gdbarch_stap_is_single_operand', as defined in
@@ -1439,15 +1436,6 @@ aarch64_linux_gcc_target_options (struct gdbarch *gdbarch)
   return NULL;
 }

-/* See aarch64-linux-tdep.h.  */
-
-bool
-aarch64_linux_get_hwcap (struct target_ops *target, CORE_ADDR *hwcap)
-{
-  *hwcap = 0;
-  return target_auxv_search (target, AT_HWCAP, hwcap) == 1;
-}
-
 static void
 aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
diff --git a/gdb/aarch64-linux-tdep.h b/gdb/aarch64-linux-tdep.h
index ec494bfc53..f075f0acd9 100644
--- a/gdb/aarch64-linux-tdep.h
+++ b/gdb/aarch64-linux-tdep.h
@@ -42,7 +42,4 @@ extern const struct regset aarch64_linux_fpregset;
 /* Matches HWCAP_PACA in kernel header arch/arm64/include/uapi/asm/hwcap.h.  */
 #define AARCH64_HWCAP_PACA (1 << 30)

-/* Fetch the AT_HWCAP entry from the auxv vector for the given TARGET.  */
-bool aarch64_linux_get_hwcap (struct target_ops *target, CORE_ADDR *hwcap);
-
 #endif /* AARCH64_LINUX_TDEP_H */
diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c
index 11e353e61c..b54bd5afe9 100644
--- a/gdb/arm-linux-nat.c
+++ b/gdb/arm-linux-nat.c
@@ -533,7 +533,7 @@ ps_get_thread_area (struct ps_prochandle *ph,
 const struct target_desc *
 arm_linux_nat_target::read_description ()
 {
-  CORE_ADDR arm_hwcap = 0;
+  CORE_ADDR arm_hwcap = linux_get_hwcap (this);

   if (have_ptrace_getregset == TRIBOOL_UNKNOWN)
     {
@@ -551,11 +551,6 @@ arm_linux_nat_target::read_description ()
        have_ptrace_getregset = TRIBOOL_TRUE;
     }

-  if (target_auxv_search (this, AT_HWCAP, &arm_hwcap) != 1)
-    {
-      return this->beneath ()->read_description ();
-    }
-
   if (arm_hwcap & HWCAP_IWMMXT)
     return tdesc_arm_with_iwmmxt;

diff --git a/gdb/arm-linux-tdep.c b/gdb/arm-linux-tdep.c
index 11db9bae32..a5ad06434c 100644
--- a/gdb/arm-linux-tdep.c
+++ b/gdb/arm-linux-tdep.c
@@ -730,10 +730,7 @@ arm_linux_core_read_description (struct gdbarch *gdbarch,
                                  struct target_ops *target,
                                  bfd *abfd)
 {
-  CORE_ADDR arm_hwcap = 0;
-
-  if (target_auxv_search (target, AT_HWCAP, &arm_hwcap) != 1)
-    return NULL;
+  CORE_ADDR arm_hwcap = linux_get_hwcap (target);

   if (arm_hwcap & HWCAP_VFP)
     {
diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index 77bc714286..f6c4f7b208 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -2462,6 +2462,28 @@ linux_displaced_step_location (struct gdbarch *gdbarch)
   return addr;
 }

+/* See linux-tdep.h.  */
+
+CORE_ADDR
+linux_get_hwcap (struct target_ops *target)
+{
+  CORE_ADDR field;
+  if (target_auxv_search (target, AT_HWCAP, &field) != 1)
+    return 0;
+  return field;
+}
+
+/* See linux-tdep.h.  */
+
+CORE_ADDR
+linux_get_hwcap2 (struct target_ops *target)
+{
+  CORE_ADDR field;
+  if (target_auxv_search (target, AT_HWCAP2, &field) != 1)
+    return 0;
+  return field;
+}
+
 /* Display whether the gcore command is using the
    /proc/PID/coredump_filter file.  */

diff --git a/gdb/linux-tdep.h b/gdb/linux-tdep.h
index 824ba3afaf..913045a954 100644
--- a/gdb/linux-tdep.h
+++ b/gdb/linux-tdep.h
@@ -61,4 +61,12 @@ extern void linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch);

 extern int linux_is_uclinux (void);

+/* Fetch the AT_HWCAP entry from the auxv vector for the given TARGET.  On
+   error, 0 is returned.  */
+extern CORE_ADDR linux_get_hwcap (struct target_ops *target);
+
+/* Fetch the AT_HWCAP2 entry from the auxv vector for the given TARGET.  On
+   error, 0 is returned.  */
+extern CORE_ADDR linux_get_hwcap2 (struct target_ops *target);
+
 #endif /* linux-tdep.h */
diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
index 11394ef36d..3a6bbf4163 100644
--- a/gdb/ppc-linux-nat.c
+++ b/gdb/ppc-linux-nat.c
@@ -1554,31 +1554,6 @@ store_ppc_registers (const struct regcache *regcache, int tid)
      function to fail most of the time, so we ignore them.  */
 }

-/* Fetch the AT_HWCAP entry from the aux vector.  */
-static CORE_ADDR
-ppc_linux_get_hwcap (void)
-{
-  CORE_ADDR field;
-
-  if (target_auxv_search (current_top_target (), AT_HWCAP, &field) != 1)
-    return 0;
-
-  return field;
-}
-
-/* Fetch the AT_HWCAP2 entry from the aux vector.  */
-
-static CORE_ADDR
-ppc_linux_get_hwcap2 (void)
-{
-  CORE_ADDR field;
-
-  if (target_auxv_search (current_top_target (), AT_HWCAP2, &field) != 1)
-    return 0;
-
-  return field;
-}
-
 /* The cached DABR value, to install in new threads.
    This variable is used when the PowerPC HWDEBUG ptrace
    interface is not available.  */
@@ -1735,7 +1710,7 @@ ppc_linux_nat_target::region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
          takes two hardware watchpoints though.  */
       if (len > 1
          && hwdebug_info.features & PPC_DEBUG_FEATURE_DATA_BP_RANGE
-         && ppc_linux_get_hwcap () & PPC_FEATURE_BOOKE)
+         && linux_get_hwcap (current_top_target ()) & PPC_FEATURE_BOOKE)
        return 2;
       /* Check if the processor provides DAWR interface.  */
       if (hwdebug_info.features & PPC_DEBUG_FEATURE_DATA_BP_DAWR)
@@ -1755,7 +1730,7 @@ ppc_linux_nat_target::region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
      ptrace interface, DAC-based processors (i.e., embedded processors) will
      use addresses aligned to 4-bytes due to the way the read/write flags are
      passed in the old ptrace interface.  */
-  else if (((ppc_linux_get_hwcap () & PPC_FEATURE_BOOKE)
+  else if (((linux_get_hwcap (current_top_target ()) & PPC_FEATURE_BOOKE)
           && (addr + len) > (addr & ~3) + 4)
           || (addr + len) > (addr & ~7) + 8)
     return 0;
@@ -2294,7 +2269,7 @@ ppc_linux_nat_target::insert_watchpoint (CORE_ADDR addr, int len,
       long dabr_value;
       long read_mode, write_mode;

-      if (ppc_linux_get_hwcap () & PPC_FEATURE_BOOKE)
+      if (linux_get_hwcap (current_top_target ()) & PPC_FEATURE_BOOKE)
        {
          /* PowerPC 440 requires only the read/write flags to be passed
             to the kernel.  */
@@ -2497,9 +2472,9 @@ ppc_linux_nat_target::watchpoint_addr_within_range (CORE_ADDR addr,
   int mask;

   if (have_ptrace_hwdebug_interface ()
-      && ppc_linux_get_hwcap () & PPC_FEATURE_BOOKE)
+      && linux_get_hwcap (current_top_target ()) & PPC_FEATURE_BOOKE)
     return start <= addr && start + length >= addr;
-  else if (ppc_linux_get_hwcap () & PPC_FEATURE_BOOKE)
+  else if (linux_get_hwcap (current_top_target ()) & PPC_FEATURE_BOOKE)
     mask = 3;
   else
     mask = 7;
@@ -2637,8 +2612,8 @@ ppc_linux_nat_target::read_description ()

   features.wordsize = ppc_linux_target_wordsize (tid);

-  CORE_ADDR hwcap = ppc_linux_get_hwcap ();
-  CORE_ADDR hwcap2 = ppc_linux_get_hwcap2 ();
+  CORE_ADDR hwcap = linux_get_hwcap (current_top_target ());
+  CORE_ADDR hwcap2 = linux_get_hwcap2 (current_top_target ());

   if (have_ptrace_getsetvsxregs
       && (hwcap & PPC_FEATURE_HAS_VSX))
diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c
index df44ad826e..19435602b5 100644
--- a/gdb/ppc-linux-tdep.c
+++ b/gdb/ppc-linux-tdep.c
@@ -1601,10 +1601,7 @@ ppc_linux_core_read_description (struct gdbarch *gdbarch,
   if (vsx)
     features.vsx = true;

-  CORE_ADDR hwcap;
-
-  if (target_auxv_search (target, AT_HWCAP, &hwcap) != 1)
-    hwcap = 0;
+  CORE_ADDR hwcap = linux_get_hwcap (target);

   features.isa205 = ppc_linux_has_isa205 (hwcap);

diff --git a/gdb/s390-linux-nat.c b/gdb/s390-linux-nat.c
index 5f6c5be8e0..3a3afae7a6 100644
--- a/gdb/s390-linux-nat.c
+++ b/gdb/s390-linux-nat.c
@@ -1016,9 +1016,8 @@ s390_linux_nat_target::read_description ()
      that mode, report s390 architecture with 64-bit GPRs.  */
 #ifdef __s390x__
   {
-    CORE_ADDR hwcap = 0;
+    CORE_ADDR hwcap = linux_get_hwcap (current_top_target ());

-    target_auxv_search (current_top_target (), AT_HWCAP, &hwcap);
     have_regset_tdb = (hwcap & HWCAP_S390_TE)
       && check_regset (tid, NT_S390_TDB, s390_sizeof_tdbregset);

diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c
index 64fe7abd80..02ae28b4ea 100644
--- a/gdb/s390-linux-tdep.c
+++ b/gdb/s390-linux-tdep.c
@@ -332,10 +332,9 @@ s390_core_read_description (struct gdbarch *gdbarch,
                            struct target_ops *target, bfd *abfd)
 {
   asection *section = bfd_get_section_by_name (abfd, ".reg");
-  CORE_ADDR hwcap = 0;
+  CORE_ADDR hwcap = linux_get_hwcap (target);
   bool high_gprs, v1, v2, te, vx, gs;

-  target_auxv_search (target, AT_HWCAP, &hwcap);
   if (!section)
     return NULL;

Re: [PATCH 2/2] gdbserver: Add linux_get_hwcap

[Alan Hayward]

> On 25 Mar 2019, at 15:41, Simon Marchi <simark@simark.ca> wrote:
> 
> On 2019-03-25 8:05 a.m., Alan Hayward wrote:
>> In gdbserver, Tidy up calls to read HWCAP (and HWCAP2) by adding common
>> functions, removing the Arm, AArch64, PPC and S390 specific versions.
>> No functionality differences.
>> [ I wasn't sure in gdbserver when to use CORE_ADDR and when to use int/long.
>>   I'm assuming CORE_ADDR is fairly recent to gdbserver? ]
> 
> I don't know if CORE_ADDR is a recent addition to gdbserver.  But I suppose CORE_ADDR was chosen as the return type for functions reading arbitrary AUXV values, since some of them may be pointers.  With CORE_ADDR, we know those values will fit in the data type.  When we return the HWCAP value, we know it won't be a pointer though, so returning a CORE_ADDR is a bit confusing, IMO.  Those functions returning the HWCAP value could return something else, an uint64_t maybe.  But then I would change it in the gdb version as well to match.

I’ve been flipping back and too in my head to change CORE_ADDR to uint64_t.

On a 32bit build, CORE_ADDR is 32bits. So it would be making a difference. Not sure if that really matters or not?

Left this change out of the update below for now.


> 
>>  /* Implementation of linux_target_ops method "arch_setup".  */
>>    static void
>> @@ -545,8 +521,8 @@ aarch64_arch_setup (void)
>>    if (is_elf64)
>>      {
>>        uint64_t vq = aarch64_sve_get_vq (tid);
>> -      unsigned long hwcap = 0;
>> -      bool pauth_p = aarch64_get_hwcap (&hwcap) && (hwcap & AARCH64_HWCAP_PACA);
>> +      unsigned long hwcap = linux_get_hwcap (8);
>> +      bool pauth_p = hwcap & AARCH64_HWCAP_PACA;
> 
> Just wondering, can the linux-aarch64-low.c code be used to debug a process

"gdbserver :2345 a.out” works on an AArch64 box if that’s what you're asking?
The code above is used to detect point auth.

> 
>> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
>> index 6f703f589f..481919c205 100644
>> --- a/gdb/gdbserver/linux-low.c
>> +++ b/gdb/gdbserver/linux-low.c
>> @@ -7423,6 +7423,64 @@ linux_get_pc_64bit (struct regcache *regcache)
>>    return pc;
>>  }
>>  +/* Extract the auxiliary vector entry with a_type matching MATCH, storing the
>> +   value in VALP and returning true.  If no entry was found, return false.  */
>> +
>> +static bool
>> +linux_get_auxv (int wordsize, CORE_ADDR match, CORE_ADDR *valp)
> 
> I think this function could return the result (CORE_ADDR) directly,
> returning 0 on failure.

Done. Agreed that given that this function is now static, there is really no need to
keep the unused error.

With this changed, I almost changed linux_get_hwcap and linux_get_hwcap2 into inline
functions in the header. But that means adding extra includes into the header (for
the AT_HWCAP defines) and reliving the static from linux_get_auxv.

> 
> If 4 and 8 are the only supported wordsize values, I would suggest adding an assert to verify it.

Done.

> 
>> +{
>> +  gdb_byte *data = (gdb_byte *) alloca (2 * wordsize);
>> +  int offset = 0;
>> +
>> +  while ((*the_target->read_auxv) (offset, data, 2 * wordsize) == 2 * wordsize)
>> +    {
>> +      if (wordsize == 4)
>> +	{
>> +	  unsigned int *data_p = (unsigned int *)data;
>> +	  if (data_p[0] == match)
>> +	    {
>> +	      *valp = data_p[1];
>> +	      return true;
>> +	    }
>> +	}
>> +      else
>> +	{
>> +	  unsigned long *data_p = (unsigned long *)data;
>> +	  if (data_p[0] == match)
>> +	    {
>> +	      *valp = data_p[1];
>> +	      return true;
>> +	    }
>> +	}
> 
> I am a bit worried about relying on the size of the "int" and "long" types in architecture-independent code.  Could we use uint32_t and uint64_t instead?
> 

Done.

> Simon



Updated patch below. Will push if you have no more comments.

Alan.



diff --git a/gdb/gdbserver/linux-aarch64-low.c b/gdb/gdbserver/linux-aarch64-low.c
index 20c75493b0..dc4ee81d2a 100644
--- a/gdb/gdbserver/linux-aarch64-low.c
+++ b/gdb/gdbserver/linux-aarch64-low.c
@@ -505,30 +505,6 @@ aarch64_linux_new_fork (struct process_info *parent,
 /* Matches HWCAP_PACA in kernel header arch/arm64/include/uapi/asm/hwcap.h.  */
 #define AARCH64_HWCAP_PACA (1 << 30)

-/* Fetch the AT_HWCAP entry from the auxv vector.  */
-
-static bool
-aarch64_get_hwcap (unsigned long *valp)
-{
-  unsigned char *data = (unsigned char *) alloca (16);
-  int offset = 0;
-
-  while ((*the_target->read_auxv) (offset, data, 16) == 16)
-    {
-      unsigned long *data_p = (unsigned long *)data;
-      if (data_p[0] == AT_HWCAP)
-       {
-         *valp = data_p[1];
-         return true;
-       }
-
-      offset += 16;
-    }
-
-  *valp = 0;
-  return false;
-}
-
 /* Implementation of linux_target_ops method "arch_setup".  */

 static void
@@ -545,8 +521,8 @@ aarch64_arch_setup (void)
   if (is_elf64)
     {
       uint64_t vq = aarch64_sve_get_vq (tid);
-      unsigned long hwcap = 0;
-      bool pauth_p = aarch64_get_hwcap (&hwcap) && (hwcap & AARCH64_HWCAP_PACA);
+      unsigned long hwcap = linux_get_hwcap (8);
+      bool pauth_p = hwcap & AARCH64_HWCAP_PACA;

       current_process ()->tdesc = aarch64_linux_read_description (vq, pauth_p);
     }
diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
index 8cad5c5fd4..ff72a489cb 100644
--- a/gdb/gdbserver/linux-arm-low.c
+++ b/gdb/gdbserver/linux-arm-low.c
@@ -847,40 +847,15 @@ get_next_pcs_syscall_next_pc (struct arm_get_next_pcs *self)
   return next_pc;
 }

-static int
-arm_get_hwcap (unsigned long *valp)
-{
-  unsigned char *data = (unsigned char *) alloca (8);
-  int offset = 0;
-
-  while ((*the_target->read_auxv) (offset, data, 8) == 8)
-    {
-      unsigned int *data_p = (unsigned int *)data;
-      if (data_p[0] == AT_HWCAP)
-       {
-         *valp = data_p[1];
-         return 1;
-       }
-
-      offset += 8;
-    }
-
-  *valp = 0;
-  return 0;
-}
-
 static const struct target_desc *
 arm_read_description (void)
 {
   int pid = lwpid_of (current_thread);
-  unsigned long arm_hwcap = 0;
+  unsigned long arm_hwcap = linux_get_hwcap (4);

   /* Query hardware watchpoint/breakpoint capabilities.  */
   arm_linux_init_hwbp_cap (pid);

-  if (arm_get_hwcap (&arm_hwcap) == 0)
-    return tdesc_arm;
-
   if (arm_hwcap & HWCAP_IWMMXT)
     return tdesc_arm_with_iwmmxt;

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 6f703f589f..7158a6798c 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -7423,6 +7423,53 @@ linux_get_pc_64bit (struct regcache *regcache)
   return pc;
 }

+/* Fetch the entry MATCH from the auxv vector, where entries are length
+   WORDSIZE.  If no entry was found, return zero.  */
+
+static CORE_ADDR
+linux_get_auxv (int wordsize, CORE_ADDR match)
+{
+  gdb_byte *data = (gdb_byte *) alloca (2 * wordsize);
+  int offset = 0;
@@ -7423,6 +7423,53 @@ linux_get_pc_64bit (struct regcache *regcache)
   return pc;
 }

+/* Fetch the entry MATCH from the auxv vector, where entries are length
+   WORDSIZE.  If no entry was found, return zero.  */
+
+static CORE_ADDR
+linux_get_auxv (int wordsize, CORE_ADDR match)
+{
+  gdb_byte *data = (gdb_byte *) alloca (2 * wordsize);
+  int offset = 0;
+
+  gdb_assert (wordsize == 4 || wordsize == 8);
+
+  while ((*the_target->read_auxv) (offset, data, 2 * wordsize) == 2 * wordsize)
+    {
+      if (wordsize == 4)
+       {
+         uint32_t *data_p = (uint32_t *)data;
+         if (data_p[0] == match)
+           return data_p[1];
+       }
+      else
+       {
+         uint64_t *data_p = (uint64_t *)data;
+         if (data_p[0] == match)
+           return data_p[1];
+       }
+
+      offset += 2 * wordsize;
+    }
+
+  return 0;
+}
+
+/* See linux-low.h.  */
+
+CORE_ADDR
+linux_get_hwcap (int wordsize)
+{
+  return linux_get_auxv (wordsize, AT_HWCAP);
+}
+
+/* See linux-low.h.  */
+
+CORE_ADDR
+linux_get_hwcap2 (int wordsize)
+{
+  return linux_get_auxv (wordsize, AT_HWCAP2);
+}

 static struct target_ops linux_target_ops = {
   linux_create_inferior,
diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
index 1ade35d648..d825184835 100644
--- a/gdb/gdbserver/linux-low.h
+++ b/gdb/gdbserver/linux-low.h
@@ -435,4 +435,14 @@ bool thread_db_thread_handle (ptid_t ptid, gdb_byte **handle, int *handle_len);

 extern int have_ptrace_getregset;

+/* Fetch the AT_HWCAP entry from the auxv vector, where entries are length
+   WORDSIZE.  If no entry was found, return zero.  */
+
+CORE_ADDR linux_get_hwcap (int wordsize);
+
+/* Fetch the AT_HWCAP2 entry from the auxv vector, where entries are length
+   WORDSIZE.  If no entry was found, return zero.  */
+
+CORE_ADDR linux_get_hwcap2 (int wordsize);
+
 #endif /* GDBSERVER_LINUX_LOW_H */
diff --git a/gdb/gdbserver/linux-ppc-low.c b/gdb/gdbserver/linux-ppc-low.c
index 1b695e53fe..8deb0ce068 100644
--- a/gdb/gdbserver/linux-ppc-low.c
+++ b/gdb/gdbserver/linux-ppc-low.c
@@ -323,43 +323,6 @@ ppc_set_pc (struct regcache *regcache, CORE_ADDR pc)
     }
 }

-
-static int
-ppc_get_auxv (unsigned long type, unsigned long *valp)
-{
-  const struct target_desc *tdesc = current_process ()->tdesc;
-  int wordsize = register_size (tdesc, 0);
-  unsigned char *data = (unsigned char *) alloca (2 * wordsize);
-  int offset = 0;
-
-  while ((*the_target->read_auxv) (offset, data, 2 * wordsize) == 2 * wordsize)
-    {
-      if (wordsize == 4)
-       {
-         unsigned int *data_p = (unsigned int *)data;
-         if (data_p[0] == type)
-           {
-             *valp = data_p[1];
-             return 1;
-           }
-       }
-      else
-       {
-         unsigned long *data_p = (unsigned long *)data;
-         if (data_p[0] == type)
-           {
-             *valp = data_p[1];
-             return 1;
-           }
-       }
-
-      offset += 2 * wordsize;
-    }
-
-  *valp = 0;
-  return 0;
-}
-
 #ifndef __powerpc64__
 static int ppc_regmap_adjusted;
 #endif
@@ -944,8 +907,8 @@ ppc_arch_setup (void)

   /* The value of current_process ()->tdesc needs to be set for this
      call.  */
-  ppc_get_auxv (AT_HWCAP, &ppc_hwcap);
-  ppc_get_auxv (AT_HWCAP2, &ppc_hwcap2);
+  ppc_hwcap = linux_get_hwcap (features.wordsize);
+  ppc_hwcap2 = linux_get_hwcap2 (features.wordsize);

   features.isa205 = ppc_linux_has_isa205 (ppc_hwcap);

diff --git a/gdb/gdbserver/linux-s390-low.c b/gdb/gdbserver/linux-s390-low.c
index edbef77fe9..f65a1ec38e 100644
--- a/gdb/gdbserver/linux-s390-low.c
+++ b/gdb/gdbserver/linux-s390-low.c
@@ -467,36 +467,6 @@ s390_set_pc (struct regcache *regcache, CORE_ADDR newpc)
     }
 }

-/* Get HWCAP from AUXV, using the given WORDSIZE.  Return the HWCAP, or
-   zero if not found.  */
-
-static unsigned long
-s390_get_hwcap (int wordsize)
-{
-  gdb_byte *data = (gdb_byte *) alloca (2 * wordsize);
-  int offset = 0;
-
-  while ((*the_target->read_auxv) (offset, data, 2 * wordsize) == 2 * wordsize)
-    {
-      if (wordsize == 4)
-        {
-          unsigned int *data_p = (unsigned int *)data;
-          if (data_p[0] == AT_HWCAP)
-           return data_p[1];
-        }
-      else
-        {
-          unsigned long *data_p = (unsigned long *)data;
-          if (data_p[0] == AT_HWCAP)
-           return data_p[1];
-        }
-
-      offset += 2 * wordsize;
-    }
-
-  return 0;
-}
-
 /* Determine the word size for the given PID, in bytes.  */

 #ifdef __s390x__
@@ -548,7 +518,7 @@ s390_arch_setup (void)
   /* Determine word size and HWCAP.  */
   int pid = pid_of (current_thread);
   int wordsize = s390_get_wordsize (pid);
-  unsigned long hwcap = s390_get_hwcap (wordsize);
+  unsigned long hwcap = linux_get_hwcap (wordsize);

   /* Check whether the kernel supports extra register sets.  */
   int have_regset_last_break

Re: [PATCH 2/2] gdbserver: Add linux_get_hwcap

[Simon Marchi]

On 2019-03-26 9:17 a.m., Alan Hayward wrote:
>>> @@ -545,8 +521,8 @@ aarch64_arch_setup (void)
>>>     if (is_elf64)
>>>       {
>>>         uint64_t vq = aarch64_sve_get_vq (tid);
>>> -      unsigned long hwcap = 0;
>>> -      bool pauth_p = aarch64_get_hwcap (&hwcap) && (hwcap & AARCH64_HWCAP_PACA);
>>> +      unsigned long hwcap = linux_get_hwcap (8);
>>> +      bool pauth_p = hwcap & AARCH64_HWCAP_PACA;
>>
>> Just wondering, can the linux-aarch64-low.c code be used to debug a process
> 
> "gdbserver :2345 a.out” works on an AArch64 box if that’s what you're asking?
> The code above is used to detect point auth.

Oops no, half of my sentence is missing!  I meant, can this code be used 
to debug 32 bit processes, and if so, is linux_get_hwcap (8) right.

> Updated patch below. Will push if you have no more comments.

For some reason, I am not able to apply "updated" patches you send.  I 
presume that you paste it and your email client changes the formatting. 
Could you maybe send it as an attached file?  It's not ideal either, 
because it makes it difficult to reply/comment on the patch, but at 
least the email client won't change the formatting.

Simon

FW: [PATCH 2/2] gdbserver: Add linux_get_hwcap

[Alan Hayward]

[-- Attachment #1: Type: text/plain, Size: 1584 bytes --]


    (resending because mac mail forces to html format when attaching a file....)
    
    > On 26 Mar 2019, at 14:06, Simon Marchi <simark@simark.ca> wrote:
    > 
    > On 2019-03-26 9:17 a.m., Alan Hayward wrote:
    >>>> @@ -545,8 +521,8 @@ aarch64_arch_setup (void)
    >>>>    if (is_elf64)
    >>>>      {
    >>>>        uint64_t vq = aarch64_sve_get_vq (tid);
    >>>> -      unsigned long hwcap = 0;
    >>>> -      bool pauth_p = aarch64_get_hwcap (&hwcap) && (hwcap & AARCH64_HWCAP_PACA);
    >>>> +      unsigned long hwcap = linux_get_hwcap (8);
    >>>> +      bool pauth_p = hwcap & AARCH64_HWCAP_PACA;
    >>> 
    >>> Just wondering, can the linux-aarch64-low.c code be used to debug a process
    >> "gdbserver :2345 a.out” works on an AArch64 box if that’s what you're asking?
    >> The code above is used to detect point auth.
    > 
    > Oops no, half of my sentence is missing!  I meant, can this code be used to debug 32 bit processes, and if so, is linux_get_hwcap (8) right.
    > 
    
    It’s within a is_elf64 check, so should always be 8.
    
    
    >> Updated patch below. Will push if you have no more comments.
    > 
    > For some reason, I am not able to apply "updated" patches you send.  I presume that you paste it and your email client changes the formatting. Could you maybe send it as an attached file?  It's not ideal either, because it makes it difficult to reply/comment on the patch, but at least the email client won't change the formatting.
    > 
    
    Attached.
    
    
    
    


[-- Attachment #2: gdbserver_hwcap.patch --]
[-- Type: application/octet-stream, Size: 8829 bytes --]

commit b5c812d05e8bbf9fbdaeea27a5d9da459b4de0bb
Author: Alan Hayward <alan.hayward@arm.com>
Date:   28 hours ago

    gdbserver: Add linux_get_hwcap
    
    In gdbserver, Tidy up calls to read HWCAP (and HWCAP2) by adding common
    functions, removing the Arm, AArch64, PPC and S390 specific versions.
    
    No functionality differences.
    
    gdb/gdbserver/ChangeLog:
    
    2019-03-26  Alan Hayward  <alan.hayward@arm.com>
    
            * linux-aarch64-low.c (aarch64_get_hwcap): Remove function.
            (aarch64_arch_setup): Call linux_get_hwcap.
            * linux-arm-low.c (arm_get_hwcap): Remove function.
            (arm_read_description): Call linux_get_hwcap.
            * linux-low.c (linux_get_auxv): New function.
            (linux_get_hwcap): Likewise.
            (linux_get_hwcap2): Likewise.
            * linux-low.h (linux_get_hwcap): New declaration.
            (linux_get_hwcap2): Likewise.
            * linux-ppc-low.c (ppc_get_auxv): Remove function.
            (ppc_arch_setup): Call linux_get_hwcap.
            * linux-s390-low.c (s390_get_hwcap): Remove function.
            (s390_arch_setup): Call linux_get_hwcap.

diff --git a/gdb/gdbserver/linux-aarch64-low.c b/gdb/gdbserver/linux-aarch64-low.c
index 20c75493b0..dc4ee81d2a 100644
--- a/gdb/gdbserver/linux-aarch64-low.c
+++ b/gdb/gdbserver/linux-aarch64-low.c
@@ -505,30 +505,6 @@ aarch64_linux_new_fork (struct process_info *parent,
 /* Matches HWCAP_PACA in kernel header arch/arm64/include/uapi/asm/hwcap.h.  */
 #define AARCH64_HWCAP_PACA (1 << 30)
 
-/* Fetch the AT_HWCAP entry from the auxv vector.  */
-
-static bool
-aarch64_get_hwcap (unsigned long *valp)
-{
-  unsigned char *data = (unsigned char *) alloca (16);
-  int offset = 0;
-
-  while ((*the_target->read_auxv) (offset, data, 16) == 16)
-    {
-      unsigned long *data_p = (unsigned long *)data;
-      if (data_p[0] == AT_HWCAP)
-	{
-	  *valp = data_p[1];
-	  return true;
-	}
-
-      offset += 16;
-    }
-
-  *valp = 0;
-  return false;
-}
-
 /* Implementation of linux_target_ops method "arch_setup".  */
 
 static void
@@ -545,8 +521,8 @@ aarch64_arch_setup (void)
   if (is_elf64)
     {
       uint64_t vq = aarch64_sve_get_vq (tid);
-      unsigned long hwcap = 0;
-      bool pauth_p = aarch64_get_hwcap (&hwcap) && (hwcap & AARCH64_HWCAP_PACA);
+      unsigned long hwcap = linux_get_hwcap (8);
+      bool pauth_p = hwcap & AARCH64_HWCAP_PACA;
 
       current_process ()->tdesc = aarch64_linux_read_description (vq, pauth_p);
     }
diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
index 8cad5c5fd4..ff72a489cb 100644
--- a/gdb/gdbserver/linux-arm-low.c
+++ b/gdb/gdbserver/linux-arm-low.c
@@ -847,40 +847,15 @@ get_next_pcs_syscall_next_pc (struct arm_get_next_pcs *self)
   return next_pc;
 }
 
-static int
-arm_get_hwcap (unsigned long *valp)
-{
-  unsigned char *data = (unsigned char *) alloca (8);
-  int offset = 0;
-
-  while ((*the_target->read_auxv) (offset, data, 8) == 8)
-    {
-      unsigned int *data_p = (unsigned int *)data;
-      if (data_p[0] == AT_HWCAP)
-	{
-	  *valp = data_p[1];
-	  return 1;
-	}
-
-      offset += 8;
-    }
-
-  *valp = 0;
-  return 0;
-}
-
 static const struct target_desc *
 arm_read_description (void)
 {
   int pid = lwpid_of (current_thread);
-  unsigned long arm_hwcap = 0;
+  unsigned long arm_hwcap = linux_get_hwcap (4);
 
   /* Query hardware watchpoint/breakpoint capabilities.  */
   arm_linux_init_hwbp_cap (pid);
 
-  if (arm_get_hwcap (&arm_hwcap) == 0)
-    return tdesc_arm;
-
   if (arm_hwcap & HWCAP_IWMMXT)
     return tdesc_arm_with_iwmmxt;
 
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 6f703f589f..7158a6798c 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -7423,6 +7423,53 @@ linux_get_pc_64bit (struct regcache *regcache)
   return pc;
 }
 
+/* Fetch the entry MATCH from the auxv vector, where entries are length
+   WORDSIZE.  If no entry was found, return zero.  */
+
+static CORE_ADDR
+linux_get_auxv (int wordsize, CORE_ADDR match)
+{
+  gdb_byte *data = (gdb_byte *) alloca (2 * wordsize);
+  int offset = 0;
+
+  gdb_assert (wordsize == 4 || wordsize == 8);
+
+  while ((*the_target->read_auxv) (offset, data, 2 * wordsize) == 2 * wordsize)
+    {
+      if (wordsize == 4)
+	{
+	  uint32_t *data_p = (uint32_t *)data;
+	  if (data_p[0] == match)
+	    return data_p[1];
+	}
+      else
+	{
+	  uint64_t *data_p = (uint64_t *)data;
+	  if (data_p[0] == match)
+	    return data_p[1];
+	}
+
+      offset += 2 * wordsize;
+    }
+
+  return 0;
+}
+
+/* See linux-low.h.  */
+
+CORE_ADDR
+linux_get_hwcap (int wordsize)
+{
+  return linux_get_auxv (wordsize, AT_HWCAP);
+}
+
+/* See linux-low.h.  */
+
+CORE_ADDR
+linux_get_hwcap2 (int wordsize)
+{
+  return linux_get_auxv (wordsize, AT_HWCAP2);
+}
 
 static struct target_ops linux_target_ops = {
   linux_create_inferior,
diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
index 1ade35d648..d825184835 100644
--- a/gdb/gdbserver/linux-low.h
+++ b/gdb/gdbserver/linux-low.h
@@ -435,4 +435,14 @@ bool thread_db_thread_handle (ptid_t ptid, gdb_byte **handle, int *handle_len);
 
 extern int have_ptrace_getregset;
 
+/* Fetch the AT_HWCAP entry from the auxv vector, where entries are length
+   WORDSIZE.  If no entry was found, return zero.  */
+
+CORE_ADDR linux_get_hwcap (int wordsize);
+
+/* Fetch the AT_HWCAP2 entry from the auxv vector, where entries are length
+   WORDSIZE.  If no entry was found, return zero.  */
+
+CORE_ADDR linux_get_hwcap2 (int wordsize);
+
 #endif /* GDBSERVER_LINUX_LOW_H */
diff --git a/gdb/gdbserver/linux-ppc-low.c b/gdb/gdbserver/linux-ppc-low.c
index 1b695e53fe..8deb0ce068 100644
--- a/gdb/gdbserver/linux-ppc-low.c
+++ b/gdb/gdbserver/linux-ppc-low.c
@@ -323,43 +323,6 @@ ppc_set_pc (struct regcache *regcache, CORE_ADDR pc)
     }
 }
 
-
-static int
-ppc_get_auxv (unsigned long type, unsigned long *valp)
-{
-  const struct target_desc *tdesc = current_process ()->tdesc;
-  int wordsize = register_size (tdesc, 0);
-  unsigned char *data = (unsigned char *) alloca (2 * wordsize);
-  int offset = 0;
-
-  while ((*the_target->read_auxv) (offset, data, 2 * wordsize) == 2 * wordsize)
-    {
-      if (wordsize == 4)
-	{
-	  unsigned int *data_p = (unsigned int *)data;
-	  if (data_p[0] == type)
-	    {
-	      *valp = data_p[1];
-	      return 1;
-	    }
-	}
-      else
-	{
-	  unsigned long *data_p = (unsigned long *)data;
-	  if (data_p[0] == type)
-	    {
-	      *valp = data_p[1];
-	      return 1;
-	    }
-	}
-
-      offset += 2 * wordsize;
-    }
-
-  *valp = 0;
-  return 0;
-}
-
 #ifndef __powerpc64__
 static int ppc_regmap_adjusted;
 #endif
@@ -944,8 +907,8 @@ ppc_arch_setup (void)
 
   /* The value of current_process ()->tdesc needs to be set for this
      call.  */
-  ppc_get_auxv (AT_HWCAP, &ppc_hwcap);
-  ppc_get_auxv (AT_HWCAP2, &ppc_hwcap2);
+  ppc_hwcap = linux_get_hwcap (features.wordsize);
+  ppc_hwcap2 = linux_get_hwcap2 (features.wordsize);
 
   features.isa205 = ppc_linux_has_isa205 (ppc_hwcap);
 
diff --git a/gdb/gdbserver/linux-s390-low.c b/gdb/gdbserver/linux-s390-low.c
index edbef77fe9..f65a1ec38e 100644
--- a/gdb/gdbserver/linux-s390-low.c
+++ b/gdb/gdbserver/linux-s390-low.c
@@ -467,36 +467,6 @@ s390_set_pc (struct regcache *regcache, CORE_ADDR newpc)
     }
 }
 
-/* Get HWCAP from AUXV, using the given WORDSIZE.  Return the HWCAP, or
-   zero if not found.  */
-
-static unsigned long
-s390_get_hwcap (int wordsize)
-{
-  gdb_byte *data = (gdb_byte *) alloca (2 * wordsize);
-  int offset = 0;
-
-  while ((*the_target->read_auxv) (offset, data, 2 * wordsize) == 2 * wordsize)
-    {
-      if (wordsize == 4)
-        {
-          unsigned int *data_p = (unsigned int *)data;
-          if (data_p[0] == AT_HWCAP)
-	    return data_p[1];
-        }
-      else
-        {
-          unsigned long *data_p = (unsigned long *)data;
-          if (data_p[0] == AT_HWCAP)
-	    return data_p[1];
-        }
-
-      offset += 2 * wordsize;
-    }
-
-  return 0;
-}
-
 /* Determine the word size for the given PID, in bytes.  */
 
 #ifdef __s390x__
@@ -548,7 +518,7 @@ s390_arch_setup (void)
   /* Determine word size and HWCAP.  */
   int pid = pid_of (current_thread);
   int wordsize = s390_get_wordsize (pid);
-  unsigned long hwcap = s390_get_hwcap (wordsize);
+  unsigned long hwcap = linux_get_hwcap (wordsize);
 
   /* Check whether the kernel supports extra register sets.  */
   int have_regset_last_break

Re: FW: [PATCH 2/2] gdbserver: Add linux_get_hwcap

[Simon Marchi]

On 2019-03-26 10:33 a.m., Alan Hayward wrote:
> It’s within a is_elf64 check, so should always be 8.

Ah, good point.

>>> Updated patch below. Will push if you have no more comments.

It LGTM, thanks for doing this!

Simon

Re: FW: [PATCH 2/2] gdbserver: Add linux_get_hwcap

[Ulrich Weigand]

Alan Hayward wrote:

>            * linux-low.c (linux_get_auxv): New function.
>            (linux_get_hwcap): Likewise.
>            (linux_get_hwcap2): Likewise.

This still breaks the build on my Cell/B.E. dailybuild:

gdb/gdbserver/linux-low.c: In function 'CORE_ADDR linux_get_hwcap2(int)':
gdb/gdbserver/linux-low.c:7471:36: error: 'AT_HWCAP2' was not declared in this scope
   return linux_get_auxv (wordsize, AT_HWCAP2);
                                    ^~~~~~~~~

The system (RHEL5) is so old that the system headers do not
yet provide AT_HWCAP2.  The old code worked because
linux-ppc-low.c got the define via #include "elf/common.h",
which linux-low.c does not include.

I guess we should either have linux-low.c include the local
file as well (if that is possible -- there may be conflicts
with <elf.h> that is otherwise already included), or else
provide a fallback define of AT_HWCAP2 only.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH 2/2] gdbserver: Add linux_get_hwcap

[Alan Hayward]

> On 28 Mar 2019, at 09:50, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> 
> Alan Hayward wrote:
> 
>>           * linux-low.c (linux_get_auxv): New function.
>>           (linux_get_hwcap): Likewise.
>>           (linux_get_hwcap2): Likewise.
> 
> This still breaks the build on my Cell/B.E. dailybuild:
> 
> gdb/gdbserver/linux-low.c: In function 'CORE_ADDR linux_get_hwcap2(int)':
> gdb/gdbserver/linux-low.c:7471:36: error: 'AT_HWCAP2' was not declared in this scope
>   return linux_get_auxv (wordsize, AT_HWCAP2);
>                                    ^~~~~~~~~
> 
> The system (RHEL5) is so old that the system headers do not
> yet provide AT_HWCAP2.  The old code worked because
> linux-ppc-low.c got the define via #include "elf/common.h",
> which linux-low.c does not include.
> 

Gah! Another failure. Apologies.

> I guess we should either have linux-low.c include the local
> file as well (if that is possible -- there may be conflicts
> with <elf.h> that is otherwise already included), or else
> provide a fallback define of AT_HWCAP2 only.

I’ve just tried adding the include and I get conflicts.

Linux-low already has some similar fallback defines, eg:

#ifndef O_LARGEFILE
#define O_LARGEFILE 0
#endif

I’ll send out a new OBV patch with the AT_HWCAP2 added in the same way.



Alan.


> 
> Bye,
> Ulrich
> 
> -- 
>  Dr. Ulrich Weigand
>  GNU/Linux compilers and toolchain
>  Ulrich.Weigand@de.ibm.com
> 

Re: [PATCH 2/2] gdbserver: Add linux_get_hwcap

[Ulrich Weigand]

Alan Hayward wrote:

> I’ll send out a new OBV patch with the AT_HWCAP2 added in the same way.

Thanks for fixing this issue.  Now the build gets one file further and
then fails again :-)

gdb/gdbserver/linux-ppc-low.c: In function 'int is_elfv2_inferior()':
gdb/gdbserver/linux-ppc-low.c:1113:8: error: 'ppc_get_auxv' was not declared in this scope
   if (!ppc_get_auxv (AT_PHDR, &phdr))

This is because of:

            * linux-ppc-low.c (ppc_get_auxv): Remove function.

You removed two callers of this function, but one more remains ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH 2/2] gdbserver: Add linux_get_hwcap

[Peter Bergner]

On 3/25/19 7:05 AM, Alan Hayward wrote:
> 	* linux-ppc-low.c (ppc_get_auxv): Remove function.
[snip]
> diff --git a/gdb/gdbserver/linux-ppc-low.c b/gdb/gdbserver/linux-ppc-low.c
> index 1b695e53fe..8deb0ce068 100644
> --- a/gdb/gdbserver/linux-ppc-low.c
> +++ b/gdb/gdbserver/linux-ppc-low.c
> @@ -323,43 +323,6 @@ ppc_set_pc (struct regcache *regcache, CORE_ADDR pc)
>      }
>  }
>  
> -
> -static int
> -ppc_get_auxv (unsigned long type, unsigned long *valp)
> -{
> -  const struct target_desc *tdesc = current_process ()->tdesc;
> -  int wordsize = register_size (tdesc, 0);
> -  unsigned char *data = (unsigned char *) alloca (2 * wordsize);
> -  int offset = 0;
> -
> -  while ((*the_target->read_auxv) (offset, data, 2 * wordsize) == 2 * wordsize)
> -    {
> -      if (wordsize == 4)
> -	{
> -	  unsigned int *data_p = (unsigned int *)data;
> -	  if (data_p[0] == type)
> -	    {
> -	      *valp = data_p[1];
> -	      return 1;
> -	    }
> -	}
> -      else
> -	{
> -	  unsigned long *data_p = (unsigned long *)data;
> -	  if (data_p[0] == type)
> -	    {
> -	      *valp = data_p[1];
> -	      return 1;
> -	    }
> -	}
> -
> -      offset += 2 * wordsize;
> -    }
> -
> -  *valp = 0;
> -  return 0;
> -}
> -
>  #ifndef __powerpc64__
>  static int ppc_regmap_adjusted;
>  #endif
> @@ -944,8 +907,8 @@ ppc_arch_setup (void)
>  
>    /* The value of current_process ()->tdesc needs to be set for this
>       call.  */
> -  ppc_get_auxv (AT_HWCAP, &ppc_hwcap);
> -  ppc_get_auxv (AT_HWCAP2, &ppc_hwcap2);
> +  ppc_hwcap = linux_get_hwcap (features.wordsize);
> +  ppc_hwcap2 = linux_get_hwcap2 (features.wordsize);
>  
>    features.isa205 = ppc_linux_has_isa205 (ppc_hwcap);
>  

You have removed ppc_get_auxv(), but I'm still seeing uses of it that
function which are causing build errors (powerpc64le-linux):

/home/bergner/binutils/binutils-gdb/gdb/gdbserver/linux-ppc-low.c: In function ‘int is_elfv2_inferior()’:
/home/bergner/binutils/binutils-gdb/gdb/gdbserver/linux-ppc-low.c:1113:8: error: ‘ppc_get_auxv’ was not declared in this scope
   if (!ppc_get_auxv (AT_PHDR, &phdr))

Peter

Re: [PATCH 2/2] gdbserver: Add linux_get_hwcap

[Pedro Franco de Carvalho]

Hi,

I noticed that is_elfv2_inferior previously relied on ppc_get_auxv
returning whether or not the AT_PHDR key is found, besides the vector
value.  The actual value was returned in the pointer argument.  The new
linux_get_auxv returns 0 for the value if the key isn't found.  I don't
know if the value for AT_PHDR can be an actual 0.

Should linux_get_auxv be changed to return this and also take a
CORE_ADDR pointer?  It should also be made visible in the header.

Thanks!

--
Pedro Franco de Carvalho

Ulrich Weigand <uweigand@de.ibm.com> writes:

> Alan Hayward wrote:
>
>> I’ll send out a new OBV patch with the AT_HWCAP2 added in the same way.
>
> Thanks for fixing this issue.  Now the build gets one file further and
> then fails again :-)
>
> gdb/gdbserver/linux-ppc-low.c: In function 'int is_elfv2_inferior()':
> gdb/gdbserver/linux-ppc-low.c:1113:8: error: 'ppc_get_auxv' was not declared in this scope
>    if (!ppc_get_auxv (AT_PHDR, &phdr))
>
> This is because of:
>
>             * linux-ppc-low.c (ppc_get_auxv): Remove function.
>
> You removed two callers of this function, but one more remains ...
>
> Bye,
> Ulrich
>
> -- 
>   Dr. Ulrich Weigand
>   GNU/Linux compilers and toolchain
>   Ulrich.Weigand@de.ibm.com

Re: [PATCH 2/2] gdbserver: Add linux_get_hwcap

[Ulrich Weigand]

Pedro Franco de Carvalho wrote:

> I noticed that is_elfv2_inferior previously relied on ppc_get_auxv
> returning whether or not the AT_PHDR key is found, besides the vector
> value.  The actual value was returned in the pointer argument.  The new
> linux_get_auxv returns 0 for the value if the key isn't found.  I don't
> know if the value for AT_PHDR can be an actual 0.

Seems unlikely, but I guess theoretically possible on some platforms.
But in general, there is a difference between an AT_ aux vector entry
not present, or present with value zero, so the interface should be
able to represent that.
 
> Should linux_get_auxv be changed to return this and also take a
> CORE_ADDR pointer?  It should also be made visible in the header.

Sounds reasonable to me.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH 2/2] gdbserver: Add linux_get_hwcap

[Pedro Franco de Carvalho]

Hi Peter,

Peter Bergner <bergner@linux.ibm.com> writes:

> You have removed ppc_get_auxv(), but I'm still seeing uses of it that
> function which are causing build errors (powerpc64le-linux):
>
> /home/bergner/binutils/binutils-gdb/gdb/gdbserver/linux-ppc-low.c: In function ‘int is_elfv2_inferior()’:
> /home/bergner/binutils/binutils-gdb/gdb/gdbserver/linux-ppc-low.c:1113:8: error: ‘ppc_get_auxv’ was not declared in this scope
>    if (!ppc_get_auxv (AT_PHDR, &phdr))
>
> Peter

I should be able to post a patch to fix this tomorrow (there is some
discussion for this elsewhere in the thread).

--
Pedo Franco de Carvalho

Re: [PATCH 2/2] gdbserver: Add linux_get_hwcap

[Pedro Franco de Carvalho]

Hi,

I've copied the patch below.

Note that there is a difference in the interface between linux_get_auxv
and linux_get_hwcap(2), the latter still return both the status and the
entry value, but I didn't want to change all their users.

Also, contrary to the gdb client version (target_auxv_search
gdb/auxv.c), this one doesn't differentiate between an error and the
entry not being found.

Is that ok?

Thanks!

From 0af6ec5fdbe9e696e10e22b530c52e3c926c5e9c Mon Sep 17 00:00:00 2001
From: Pedro Franco de Carvalho <pedromfc@linux.ibm.com>
Date: Thu, 4 Apr 2019 14:49:37 -0300
Subject: [PATCH] Use linux_get_auxv to get AT_PHDR in the PPC stub

This patch fixes a build error due to a call to ppc_get_auxv that was
left over after linux_get_hwcap and linux_get_hwcap2 were introduced
in:

974c89e0882ddb03e294eca76a9e3d3bef90eacf gdbserver: Add
linux_get_hwcap

Because the missing call fetched AT_PHDR and not AT_HWCAP,
linux_get_auxv is now visible.

This use also required ppc_get_auxv to return a status variable
indicating that the AT_PHDR entry was not found separately from the
actual value of of the auxv entry.  Therefore, the new linux_get_auxv
function is changed to return a status variable and write the entry
value to a pointer passed as an argument.

Note that linux_get_hwcap and linux_get_hwcap2 still use the return
value as both an indicator of that the entry wasn't found and as the
actual value of the entry.

gdb/gdbserver/ChangeLog:
2019-04-DD  Pedro Franco de Carvalho  <pedromfc@linux.ibm.com>

	* linux-low.c (linux_get_auxv): Remove static.  Return auxv entry
	value in argument pointer, return 1 if the entry is found and 0
	otherwise.  Move comment.
	(linux_get_hwcap, linux_get_hwcap2): Use modified linux_get_auxv.
	* linux-low.h (linux_get_auxv): Declare.
	* linux-ppc-low.c (is_elfv2_inferior): Use linux_get_auxv.
---
 gdb/gdbserver/linux-low.c     | 29 +++++++++++++++++++----------
 gdb/gdbserver/linux-low.h     |  8 ++++++++
 gdb/gdbserver/linux-ppc-low.c |  7 +++++--
 3 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 265043f97e..65919c3262 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -7427,11 +7427,10 @@ linux_get_pc_64bit (struct regcache *regcache)
   return pc;
 }
 
-/* Fetch the entry MATCH from the auxv vector, where entries are length
-   WORDSIZE.  If no entry was found, return zero.  */
+/* See linux-low.h.  */
 
-static CORE_ADDR
-linux_get_auxv (int wordsize, CORE_ADDR match)
+int
+linux_get_auxv (int wordsize, CORE_ADDR match, CORE_ADDR *valp)
 {
   gdb_byte *data = (gdb_byte *) alloca (2 * wordsize);
   int offset = 0;
@@ -7442,15 +7441,21 @@ linux_get_auxv (int wordsize, CORE_ADDR match)
     {
       if (wordsize == 4)
 	{
-	  uint32_t *data_p = (uint32_t *)data;
+	  uint32_t *data_p = (uint32_t *) data;
 	  if (data_p[0] == match)
-	    return data_p[1];
+	    {
+	      *valp = data_p[1];
+	      return 1;
+	    }
 	}
       else
 	{
-	  uint64_t *data_p = (uint64_t *)data;
+	  uint64_t *data_p = (uint64_t *) data;
 	  if (data_p[0] == match)
-	    return data_p[1];
+	    {
+	      *valp = data_p[1];
+	      return 1;
+	    }
 	}
 
       offset += 2 * wordsize;
@@ -7464,7 +7469,9 @@ linux_get_auxv (int wordsize, CORE_ADDR match)
 CORE_ADDR
 linux_get_hwcap (int wordsize)
 {
-  return linux_get_auxv (wordsize, AT_HWCAP);
+  CORE_ADDR hwcap = 0;
+  linux_get_auxv (wordsize, AT_HWCAP, &hwcap);
+  return hwcap;
 }
 
 /* See linux-low.h.  */
@@ -7472,7 +7479,9 @@ linux_get_hwcap (int wordsize)
 CORE_ADDR
 linux_get_hwcap2 (int wordsize)
 {
-  return linux_get_auxv (wordsize, AT_HWCAP2);
+  CORE_ADDR hwcap2 = 0;
+  linux_get_auxv (wordsize, AT_HWCAP2, &hwcap2);
+  return hwcap2;
 }
 
 static struct target_ops linux_target_ops = {
diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
index d825184835..d5d074efc5 100644
--- a/gdb/gdbserver/linux-low.h
+++ b/gdb/gdbserver/linux-low.h
@@ -435,6 +435,14 @@ bool thread_db_thread_handle (ptid_t ptid, gdb_byte **handle, int *handle_len);
 
 extern int have_ptrace_getregset;
 
+/* Search for the value with type MATCH in the auxv vector with
+   entries of length WORDSIZE bytes.  If found, store the value in
+   *VALP and return 1.  If not found or if there is an error, return
+   0.  */
+
+int linux_get_auxv (int wordsize, CORE_ADDR match,
+		    CORE_ADDR *valp);
+
 /* Fetch the AT_HWCAP entry from the auxv vector, where entries are length
    WORDSIZE.  If no entry was found, return zero.  */
 
diff --git a/gdb/gdbserver/linux-ppc-low.c b/gdb/gdbserver/linux-ppc-low.c
index 8deb0ce068..f17f05a0a3 100644
--- a/gdb/gdbserver/linux-ppc-low.c
+++ b/gdb/gdbserver/linux-ppc-low.c
@@ -1107,10 +1107,13 @@ is_elfv2_inferior (void)
 #else
   const int def_res = 0;
 #endif
-  unsigned long phdr;
+  CORE_ADDR phdr;
   Elf64_Ehdr ehdr;
 
-  if (!ppc_get_auxv (AT_PHDR, &phdr))
+  const struct target_desc *tdesc = current_process ()->tdesc;
+  int wordsize = register_size (tdesc, 0);
+
+  if (!linux_get_auxv (wordsize, AT_PHDR, &phdr))
     return def_res;
 
   /* Assume ELF header is at the beginning of the page where program headers
-- 
2.13.6

Re: [PATCH 2/2] gdbserver: Add linux_get_hwcap

[Ulrich Weigand]

Pedro Franco de Carvalho wrote: 

> Note that there is a difference in the interface between linux_get_auxv
> and linux_get_hwcap(2), the latter still return both the status and the
> entry value, but I didn't want to change all their users.

Actually, I think this is fine -- for hwcap, 0 is a natural default
value to use if the entry is not present.
 
> Also, contrary to the gdb client version (target_auxv_search
> gdb/auxv.c), this one doesn't differentiate between an error and the
> entry not being found.

That also seems reasonable.  If anybody requires more specific error
handling, that can always be added later.

> gdb/gdbserver/ChangeLog:
> 2019-04-DD  Pedro Franco de Carvalho  <pedromfc@linux.ibm.com>
> 
> 	* linux-low.c (linux_get_auxv): Remove static.  Return auxv entry
> 	value in argument pointer, return 1 if the entry is found and 0
> 	otherwise.  Move comment.
> 	(linux_get_hwcap, linux_get_hwcap2): Use modified linux_get_auxv.
> 	* linux-low.h (linux_get_auxv): Declare.
> 	* linux-ppc-low.c (is_elfv2_inferior): Use linux_get_auxv.

This is OK.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH 2/2] gdbserver: Add linux_get_hwcap

[Pedro Franco de Carvalho]

Ulrich Weigand <uweigand@de.ibm.com> writes:

> This is OK.
>
> Thanks,
> Ulrich

Thanks! I checked this in.

--
Pedro Franco de Carvalho

Re: [PATCH 2/2] gdbserver: Add linux_get_hwcap

[Alan Hayward]

Apologies, I’ve been away for the last week, only just saw this whole thread.
Thanks for fixing this without me.

Pedro: Some comments/nits below.  Given this is pushed, I’m happy if you don’t
raise a whole new patch for it.


> On 5 Apr 2019, at 17:39, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> 
> Pedro Franco de Carvalho wrote: 
> 
>> Note that there is a difference in the interface between linux_get_auxv
>> and linux_get_hwcap(2), the latter still return both the status and the
>> entry value, but I didn't want to change all their users.
> 
> Actually, I think this is fine -- for hwcap, 0 is a natural default
> value to use if the entry is not present.

Yes, all the targets in GDB treated an error condition the same as a hwcap
value of 0.  Which is sensible given it’s for checking features present in
the hardware/cpu.

> 
>> Also, contrary to the gdb client version (target_auxv_search
>> gdb/auxv.c), this one doesn't differentiate between an error and the
>> entry not being found.
> 
> That also seems reasonable.  If anybody requires more specific error
> handling, that can always be added later.


Originally I was going to keep gdbserver linux_get_auxv with the same returns
as the gdb version.  However, given it is a static function and the hwap
functions were not checking the error value, I removed it.

> 
>> gdb/gdbserver/ChangeLog:
>> 2019-04-DD  Pedro Franco de Carvalho  <pedromfc@linux.ibm.com>
>> 
>> 	* linux-low.c (linux_get_auxv): Remove static.  Return auxv entry
>> 	value in argument pointer, return 1 if the entry is found and 0
>> 	otherwise.  Move comment.

Returning true/false would have been better than 1/0. The build requires C++ support.

>> 	(linux_get_hwcap, linux_get_hwcap2): Use modified linux_get_auxv.
>> 	* linux-low.h (linux_get_auxv): Declare.
>> 	* linux-ppc-low.c (is_elfv2_inferior): Use linux_get_auxv.
> 


> index d825184835..d5d074efc5 100644
> --- a/gdb/gdbserver/linux-low.h
> +++ b/gdb/gdbserver/linux-low.h
> @@ -435,6 +435,14 @@ bool thread_db_thread_handle (ptid_t ptid, gdb_byte **handle, int *handle_len);
> 
> extern int have_ptrace_getregset;
> 
> +/* Search for the value with type MATCH in the auxv vector with
> +   entries of length WORDSIZE bytes.  If found, store the value in
> +   *VALP and return 1.  If not found or if there is an error, return
> +   0.  */

Comment should state that VALP is not checked for nullptr.

> +
> +int linux_get_auxv (int wordsize, CORE_ADDR match,
> +		    CORE_ADDR *valp);
> +



> 
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index 265043f97e..65919c3262 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -7427,11 +7427,10 @@ linux_get_pc_64bit (struct regcache *regcache)
>   return pc;
> }
> 
> -/* Fetch the entry MATCH from the auxv vector, where entries are length
> -   WORDSIZE.  If no entry was found, return zero.  */
> +/* See linux-low.h.  */
> 
> -static CORE_ADDR
> -linux_get_auxv (int wordsize, CORE_ADDR match)
> +int
> +linux_get_auxv (int wordsize, CORE_ADDR match, CORE_ADDR *valp)
> {
>   gdb_byte *data = (gdb_byte *) alloca (2 * wordsize);
>   int offset = 0;

Would be useful to set valp to zero here. (Not sure if the GDB version
does this either).

> @@ -7442,15 +7441,21 @@ linux_get_auxv (int wordsize, CORE_ADDR match)
>     {
>       if (wordsize == 4)
> 	{
> -	  uint32_t *data_p = (uint32_t *)data;
> +	  uint32_t *data_p = (uint32_t *) data;
> 	  if (data_p[0] == match)
> -	    return data_p[1];
> +	    {
> +	      *valp = data_p[1];
> +	      return 1;
> +	    }
> 	}
>       else
> 	{
> -	  uint64_t *data_p = (uint64_t *)data;
> +	  uint64_t *data_p = (uint64_t *) data;
> 	  if (data_p[0] == match)
> -	    return data_p[1];
> +	    {
> +	      *valp = data_p[1];
> +	      return 1;
> +	    }
> 	}



> diff --git a/gdb/gdbserver/linux-ppc-low.c b/gdb/gdbserver/linux-ppc-low.c
> index 8deb0ce068..f17f05a0a3 100644
> --- a/gdb/gdbserver/linux-ppc-low.c
> +++ b/gdb/gdbserver/linux-ppc-low.c
> @@ -1107,10 +1107,13 @@ is_elfv2_inferior (void)
> #else
>   const int def_res = 0;
> #endif
> -  unsigned long phdr;
> +  CORE_ADDR phdr;
>   Elf64_Ehdr ehdr;
> 
> -  if (!ppc_get_auxv (AT_PHDR, &phdr))
> +  const struct target_desc *tdesc = current_process ()->tdesc;
> +  int wordsize = register_size (tdesc, 0);
> +
> +  if (!linux_get_auxv (wordsize, AT_PHDR, &phdr))
>     return def_res;

I don’t think there is a requirement here for the error to be return separately
to the phdr. With the my version of linux_get_auxv, on error you would get 0
for phdr. Given that it is an address, 0 should never be a valid value.

With the code pre my patch and this patch, I’m not sure what will happen if the
PHDR value is 0 - will read_inferior_memory then the memcmp deal with that? (To
be fair, I suspect there are bigger issues to deal with if phdr is 0).

Therefore I’d suggest it’d be better to have:

CORE_ADDR phdr = linux_get_auxv (wordsize, AT_PHDR);
if (phdr == nullptr)
  return def_res;



Thanks,
Alan.

Re: [PATCH 2/2] gdbserver: Add linux_get_hwcap

[Pedro Franco de Carvalho]

Hi Alan,

Thank you for the comments.

Alan Hayward <Alan.Hayward@arm.com> writes:

>> -static CORE_ADDR
>> -linux_get_auxv (int wordsize, CORE_ADDR match)
>> +int
>> +linux_get_auxv (int wordsize, CORE_ADDR match, CORE_ADDR *valp)
>> {
>>   gdb_byte *data = (gdb_byte *) alloca (2 * wordsize);
>>   int offset = 0;
>
> Would be useful to set valp to zero here. (Not sure if the GDB version
> does this either).

The client version doesn't do this (target_auxv_search). Should I be
consistent with that or change it here?

> I don’t think there is a requirement here for the error to be return separately
> to the phdr. With the my version of linux_get_auxv, on error you would get 0
> for phdr. Given that it is an address, 0 should never be a valid value.
>
> With the code pre my patch and this patch, I’m not sure what will happen if the
> PHDR value is 0 - will read_inferior_memory then the memcmp deal with that? (To
> be fair, I suspect there are bigger issues to deal with if phdr is 0).

I'm probably nitpicking, but I tried using memory at 0x0 with mmap and
it seems to work, even if it is unusual, and gdb can read from that
address, so apparently ptrace can read memory at 0x0, which should mean
that read_inferior_memory can do that too.

Still, I don't think the program headers could really be at 0x0, since
there would be no room for the ELF header before that.  However,
wouldn't it make sense to return in error separately, so that the next
person who reads that function doesn't have to go through this
reasoning?

Besides, other auxv values that might be retrieved in the future with
linux_get_auxv could have a real meaning for 0.

Other than that, I agree with your other comments.

> Thanks,
> Alan.

Thanks!

--
Pedro Franco de Carvalho