diff mbox

[2/2] gdbserver: Add linux_get_hwcap

Message ID E6E2F099-C14C-4048-B9CB-0515B65CF78C@arm.com
State New
Headers show

Commit Message

Alan Hayward March 26, 2019, 1:17 p.m. UTC
> 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.

Comments

Simon Marchi March 26, 2019, 2:06 p.m. UTC | #1
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
Alan Hayward March 26, 2019, 2:34 p.m. UTC | #2
(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.
Simon Marchi March 26, 2019, 2:56 p.m. UTC | #3
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
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..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