[PATCH/aarch64] Fix handling of hfa/hva arrays

[PATCH/aarch64] Fix handling of hfa/hva arrays

[Tristan Gingold]

Hello,

the current handling of hfa arrays is not correct: first the length comparison
is using the size instead of the length so only array of a single float could
be considered as an hfa.

Second, where used HFA were only considered as struct/union.  Incorrect
accessor (like TYPE_NFIELDS) were used on arrays.

Unfortunately, we don’t have the setup to run the gdb testsuite on that
processor.  So this patch was only manually tested (using our own
internal testsuite) on a slightly older version of gdb.

Is it ok to push ?

Tristan.

From 9a3e54726903e1c01ef79e2c4ba9e0317f3f90a6 Mon Sep 17 00:00:00 2001
From: Tristan Gingold <gingold@adacore.com>
Date: Tue, 8 Dec 2015 13:01:24 +0100
Subject: [PATCH] aarch64: fix handling of hfa/hva arrays.

gdb/
	* aarch64-tdep.c (is_hfa_or_hva): Fix length comparaison for arrays.
	(aarch64_extract_return_value): Handle arrays.
	(aarch64_push_dummy_call): Handle hfa arrays.
	(aarch64_record_load_store): Fix compiler warning.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index b5d2431..d85cf4f 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2015-12-09  Tristan Gingold  <gingold@adacore.com>
+
+	* aarch64-tdep.c (is_hfa_or_hva): Fix length comparaison for arrays.
+	(aarch64_extract_return_value): Handle arrays.
+	(aarch64_push_dummy_call): Handle hfa arrays.
+	(aarch64_record_load_store): Fix compiler warning.
+
 2015-12-08  Pierre-Marie de Rodat  <derodat@adacore.com>
 
 	* NEWS: Announce this enhancement and the corresponding new
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 2d1df03..c651872 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -932,12 +932,13 @@ is_hfa_or_hva (struct type *ty)
     {
     case TYPE_CODE_ARRAY:
       {
-	struct type *target_ty = TYPE_TARGET_TYPE (ty);
+	struct type *target_ty = check_typedef (TYPE_TARGET_TYPE (ty));
 
 	if (TYPE_VECTOR (ty))
 	  return 0;
 
-	if (TYPE_LENGTH (ty) <= 4 /* HFA or HVA has at most 4 members.  */
+	/* HFA or HVA has at most 4 members.  */
+	if (TYPE_LENGTH (ty) / TYPE_LENGTH (target_ty) <= 4
 	    && (TYPE_CODE (target_ty) == TYPE_CODE_FLT /* HFA */
 		|| (TYPE_CODE (target_ty) == TYPE_CODE_ARRAY /* HVA */
 		    && TYPE_VECTOR (target_ty))))
@@ -1313,7 +1314,16 @@ aarch64_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 	case TYPE_CODE_UNION:
 	  if (is_hfa_or_hva (arg_type))
 	    {
-	      int elements = TYPE_NFIELDS (arg_type);
+	      int elements;
+
+	      if (TYPE_CODE (arg_type) == TYPE_CODE_ARRAY)
+		{
+		  struct type *el_type;
+		  el_type = check_typedef (TYPE_TARGET_TYPE (arg_type));
+		  elements = TYPE_LENGTH (arg_type) / TYPE_LENGTH (el_type);
+		}
+	      else
+		elements = TYPE_NFIELDS (arg_type);
 
 	      /* Homogeneous Aggregates */
 	      if (info.nsrn + elements < 8)
@@ -1325,13 +1335,16 @@ aarch64_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 		      /* We know that we have sufficient registers
 			 available therefore this will never fallback
 			 to the stack.  */
-		      struct value *field =
-			value_primitive_field (arg, 0, i, arg_type);
-		      struct type *field_type =
-			check_typedef (value_type (field));
+		      struct value *val;
+
+		      if (TYPE_CODE (arg_type) == TYPE_CODE_ARRAY)
+			val = value_subscript (arg, i);
+		      else
+			val = value_primitive_field (arg, 0, i, arg_type);
 
 		      pass_in_v_or_stack (gdbarch, regcache, &info,
-					  field_type, field);
+					  check_typedef (value_type (val)),
+					  val);
 		    }
 		}
 	      else
@@ -1649,11 +1662,24 @@ aarch64_extract_return_value (struct type *type, struct regcache *regs,
     }
   else if (is_hfa_or_hva (type))
     {
-      int elements = TYPE_NFIELDS (type);
-      struct type *member_type = check_typedef (TYPE_FIELD_TYPE (type, 0));
-      int len = TYPE_LENGTH (member_type);
+      int elements;
+      struct type *el_type;
+      int len;
       int i;
 
+      if (TYPE_CODE (type) == TYPE_CODE_ARRAY)
+	{
+	  el_type = check_typedef (TYPE_TARGET_TYPE (type));
+	  len = TYPE_LENGTH (el_type);
+	  elements = TYPE_LENGTH (type) / len;
+	}
+      else
+	{
+	  elements = TYPE_NFIELDS (type);
+	  el_type = check_typedef (TYPE_FIELD_TYPE (type, 0));
+	  len = TYPE_LENGTH (el_type);
+	}
+
       for (i = 0; i < elements; i++)
 	{
 	  int regno = AARCH64_V0_REGNUM + i;
@@ -3470,7 +3496,7 @@ aarch64_record_load_store (insn_decode_record *aarch64_insn_r)
 
       if (!ld_flag)
         {
-          uint64_t reg_rm_val;
+          ULONGEST reg_rm_val;
           regcache_raw_read_unsigned (aarch64_insn_r->regcache,
                      bits (aarch64_insn_r->aarch64_insn, 16, 20), &reg_rm_val);
           if (bit (aarch64_insn_r->aarch64_insn, 12))

Re: [PATCH/aarch64] Fix handling of hfa/hva arrays

[Pedro Alves]

On 12/09/2015 11:04 AM, Tristan Gingold wrote:
> Hello,
> 
> the current handling of hfa arrays is not correct: first the length comparison
> is using the size instead of the length so only array of a single float could
> be considered as an hfa.
> 
> Second, where used HFA were only considered as struct/union.  Incorrect
> accessor (like TYPE_NFIELDS) were used on arrays.
> 
> Unfortunately, we don’t have the setup to run the gdb testsuite on that
> processor.  So this patch was only manually tested (using our own
> internal testsuite) on a slightly older version of gdb.

Note there are now 4 Aarch64 Ubuntu machines available on the gcc compile
farm: gcc113-gcc116.

Thanks,
Pedro Alves

Re: [PATCH/aarch64] Fix handling of hfa/hva arrays

[Yao Qi]

Tristan Gingold <gingold@adacore.com> writes:

> the current handling of hfa arrays is not correct: first the length comparison
> is using the size instead of the length so only array of a single float could
> be considered as an hfa.
>
> Second, where used HFA were only considered as struct/union.  Incorrect
> accessor (like TYPE_NFIELDS) were used on arrays.

Hi Tristan,
Thanks for your patch.  I spot this problem before, but I didn't fix it
because I can't write a test case in ada.  Could you add a test case to
expose these problems you described above?

>
> Unfortunately, we don’t have the setup to run the gdb testsuite on that
> processor.  So this patch was only manually tested (using our own
> internal testsuite) on a slightly older version of gdb.

Your can run testsuite on aarch64 machine on gcc compile farm, as Pedro
suggested.

> @@ -932,12 +932,13 @@ is_hfa_or_hva (struct type *ty)
>      {
>      case TYPE_CODE_ARRAY:
>        {
> -	struct type *target_ty = TYPE_TARGET_TYPE (ty);
> +	struct type *target_ty = check_typedef (TYPE_TARGET_TYPE (ty));
>  
>  	if (TYPE_VECTOR (ty))
>  	  return 0;
>  
> -	if (TYPE_LENGTH (ty) <= 4 /* HFA or HVA has at most 4 members.  */
> +	/* HFA or HVA has at most 4 members.  */
> +	if (TYPE_LENGTH (ty) / TYPE_LENGTH (target_ty) <= 4
>  	    && (TYPE_CODE (target_ty) == TYPE_CODE_FLT /* HFA */
>  		|| (TYPE_CODE (target_ty) == TYPE_CODE_ARRAY /* HVA */
>  		    && TYPE_VECTOR (target_ty))))

We can use get_array_bounds here, and take care of empty array in ada.
We can do something like this?

+       LONGEST low_bound, high_bound;
+
+       if (get_array_bounds (ty, &low_bound, &high_bound))
+         {
+           struct type *target_ty = TYPE_TARGET_TYPE (ty);
+
+           if (low_bound > high_bound)
+             {
+               /* Empty array in Ada.  */
+               return 0;
+             }
+           else if (high_bound - low_bound + 1 > 4)
+             {
+               /* There are at most 4 members in HFA.  */
+               return 0;
+             }
+           else if (TYPE_CODE (target_ty) == TYPE_CODE_FLT /* HFA */
+  		|| (TYPE_CODE (target_ty) == TYPE_CODE_ARRAY /* HVA */
+  		    && TYPE_VECTOR (target_ty))))
+             return 1;
+           else
+             return 0;
+         }
+       else
+         return 0;
+

> @@ -1313,7 +1314,16 @@ aarch64_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
>  	case TYPE_CODE_UNION:
>  	  if (is_hfa_or_hva (arg_type))
>  	    {
> -	      int elements = TYPE_NFIELDS (arg_type);
> +	      int elements;
> +
> +	      if (TYPE_CODE (arg_type) == TYPE_CODE_ARRAY)
> +		{
> +		  struct type *el_type;
> +		  el_type = check_typedef (TYPE_TARGET_TYPE (arg_type));
> +		  elements = TYPE_LENGTH (arg_type) / TYPE_LENGTH (el_type);
> +		}
> +	      else
> +		elements = TYPE_NFIELDS (arg_type);

The change is correct, but it would be nice to move them into
is_hfa_or_hva, so that it becomes,

static int
is_hfa_or_hva (struct type *ty, int *elenum)

*ELENUM is set to the number of elements or fields of *TY if *TY is a
 Homogeneous Aggregates.

What do you think?

-- 
Yao (齐尧)