diff mbox

[2/2] gdbserver: Add linux_get_hwcap

Message ID 20190325120542.92123-2-alan.hayward@arm.com
State New
Headers show

Commit Message

Alan Hayward March 25, 2019, 12:05 p.m. UTC
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(-)

Comments

Simon Marchi March 25, 2019, 3:41 p.m. UTC | #1
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
Peter Bergner April 2, 2019, 10 p.m. UTC | #2
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
Pedro Franco de Carvalho April 4, 2019, 9:22 p.m. UTC | #3
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
diff mbox

Patch

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