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

[Yao Qi <qiyaoltc at gmail dot com>]

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 (éå)