diff mbox

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

Message ID 1541632206-143680-1-git-send-email-brobecker@adacore.com
State New, archived
Headers

Commit Message

Joel Brobecker Nov. 7, 2018, 11:10 p.m. UTC 
  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.

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

OK to commit?

Thank you,

Comments

Alan Hayward Nov. 8, 2018, 10:18 a.m. UTC | #1 
> 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

>
Joel Brobecker Nov. 8, 2018, 3:34 p.m. UTC | #2 
> Apologies, this was my goof.  Fix looks good to me.

Thank you. Considering that I really like what you did to the code
in that respect, please do not apologize! ;-)

> > 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

Pushed to master.

> I had to install gnat to test this (which is probably why I never
> spotted the fail before).

I don't think it was passing before either. AdaCore had a local patch
to handle this that Tristan tried to submit. But, at the time, all
we could do to validate it was run it through AdaCore's testsuite,
and the maintainer at the time asked that it be run through the official
testsuite, which would have required a bit of up-front work that we
never got around to doing.

This time around, one of my coworkers had a compiler handy for me,
so it was less effort :).

> Ideally, this case should have been tested
> as part of gdb.base/infcall-nested-structs.exp.

I thought about that and got started on writing a C testcase for it.
Except that C doesn't allow return values that are arrays. But thinking
about it more, with AArch64, we have the advantage that the calling
convention for return values are the same as for arguments, so we could
exercise the same scenario using an inferior function call with one
argument being an array of floats!
diff mbox

Patch

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;
 	  }
       }