Re: [RFA] (AArch64) wrong value returned by "finish" for HFA

[Alan Hayward <Alan.Hayward@arm.com>]

> On 7 Nov 2018, at 23:10, Joel Brobecker <brobecker@adacore.com> wrote:
> 
> Hello,
> 
> Consider the gdb.ada/array_return.exp testcase, and in particular,
> consider the following code...
> 
>   type Small_Float_Vector is array (1 .. 2) of Float;
> 
>   function Create_Small_Float_Vector return Small_Float_Vector is
>   begin
>      return (others => 4.25);
>   end Create_Small_Float_Vector;
> 
> ... which declares a type which is an array with 2 floats in it
> (floats are 4 bytes on AArch64), trying to get GDB to print
> the return value from that function does not work:
> 
>    (gdb) fin
>    Run till exit from #0  pck.create_small_float_vector () at /[...]/pck.adb:15
>    0x000000000000062c in p () at /[...]/p.adb:11
>    11         Vector := Create_Small_Float_Vector;
>    Value returned is $1 = (4.25, 0.0)
>                                  ^^^
>                                  |||
> 
> We expected the value shown to be:
> 
>    (gdb) fin
>    Run till exit from #0  pck.create_small_float_vector () at /[...]/pck.adb:15
>    0x000000000000062c in p () at /[...]/p.adb:11
>    11         Vector := Create_Small_Float_Vector;
>    Value returned is $1 = (4.25, 4.25)
> 
> Because the return type is an HFA, it is returned via the first two
> SIMD registers. However, what happens is that the current implementation
> fails to realize that this is an HFA, and therefore fetches the return
> value from the wrong location. And the reason why it fails to realize
> this is because it thinks that our array has 8 elements (HFAs have
> a maximum of 4). Looking at aapcs_is_vfp_call_or_return_candidate_1,
> where this is determined, we can easily see why (looks like a thinko):
> 
>        | case TYPE_CODE_ARRAY:
>        | [...]
>        |         struct type *target_type = TYPE_TARGET_TYPE (type);
>        |         int count = aapcs_is_vfp_call_or_return_candidate_1
>        |                       (target_type, fundamental_type);
>        |
>        |         if (count == -1)
>        |           return count;
>        |
>  !! -> |         count *= TYPE_LENGTH (type);
>        |           return count;
> 
> Here, we first determine the count for one element of our array,
> and so we should then be multiplying that count by the number
> of elements in our array (2 in our case). But instead, we multiply it
> by the total size (8). As a result, we do not classify the return
> type as an HFA, and thus pick the wrong location for fetching
> the return value.
> 

Apologies, this was my goof.  Fix looks good to me.

> gdb/ChangeLog:
> 
>        * aarch64-tdep.c (aapcs_is_vfp_call_or_return_candidate_1):
>        return the correct count for potential HFAs.
> 
> Tested on aarch64-linux, fixes:
> 
>    array_return.exp: value printed by finish of Create_Small_Float_Vector


I had to install gnat to test this (which is probably why I never spotted the
fail before). Ideally, this case should have been tested as part of
gdb.base/infcall-nested-structs.exp.


As a side note, I’ve also got a work in progress fix for this area of code
to fix gdb.dwarf2/dw2-cp-infcall-ref-static.exp which uses a recursive type
written in dwarf.


> 
> OK to commit?
> 
> Thank you,
> -- 
> Joel
> 
> ---
> gdb/aarch64-tdep.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 0211930..51f1632 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -1212,7 +1212,7 @@ aapcs_is_vfp_call_or_return_candidate_1 (struct type *type,
> 	    if (count == -1)
> 	      return count;
> 
> -	    count *= TYPE_LENGTH (type);
> +	    count *= (TYPE_LENGTH (type) / TYPE_LENGTH (target_type));
> 	      return count;
> 	  }
>       }
> -- 
> 2.1.4
>