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

  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.
  

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
  

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?