[RFC][PATCH] Fix function argument and return value locations

[RFC][PATCH] Fix function argument and return value locations

[Hannes Domani]

I'm investigating some testsuite failures on Windows, here it's about
gdb.base/call-sc.exp and gdb.base/callfuncs.exp.

Also, this is not a finished patch, because some things I found out were
surprising for me.

For the function arguments, I only had these failures:
FAIL: gdb.base/callfuncs.exp: p t_float_complex_values(fc1, fc2)
FAIL: gdb.base/callfuncs.exp: p t_float_complex_many_args(fc1, fc2, fc3, fc4, fc1, fc2, fc3, fc4, fc1, fc2, fc3, fc4, fc1, fc2, fc3, fc4)

These were fixed by adding TYPE_CODE_COMPLEX to the types passed via XMM
registers.

For return values, there were a lot more issues:
- TYPE_CODE_DECFLOAT is NOT returned via XMM0.
- long double is NOT returned via XMM0.
- but __int128 IS returned via XMM0.
- the comments for TYPE_CODE_FLT state that __m128, __m128i and __m128d are
  returned by XMM0, and this is correct, but it doesn't actually check for
  them, because they are TYPE_CODE_ARRAY with TYPE_VECTOR

Could it be that this depends on the version of the used compiler?

Also, in call-sc.exp, for the "value foo returned" test, I'm wondering
if it can really ever return 90.

If any of this correct, then I will send a proper patch again (and maybe I
should split the argument part into its own patch), if not, please advise.
---
 gdb/amd64-windows-tdep.c             | 23 ++++++++++++++++++-----
 gdb/testsuite/gdb.base/call-sc.c     |  3 ++-
 gdb/testsuite/gdb.base/call-sc.exp   | 13 +++++++++++++
 gdb/testsuite/gdb.base/callfuncs.exp |  4 ++++
 4 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/gdb/amd64-windows-tdep.c b/gdb/amd64-windows-tdep.c
index 740152b7de..1f9b868562 100644
--- a/gdb/amd64-windows-tdep.c
+++ b/gdb/amd64-windows-tdep.c
@@ -77,7 +77,8 @@ static int
 amd64_windows_passed_by_xmm_register (struct type *type)
 {
   return ((TYPE_CODE (type) == TYPE_CODE_FLT
-	   || TYPE_CODE (type) == TYPE_CODE_DECFLOAT)
+	   || TYPE_CODE (type) == TYPE_CODE_DECFLOAT
+	   || TYPE_CODE (type) == TYPE_CODE_COMPLEX)
           && (TYPE_LENGTH (type) == 4 || TYPE_LENGTH (type) == 8));
 }
 
@@ -299,17 +300,29 @@ amd64_windows_return_value (struct gdbarch *gdbarch, struct value *function,
   switch (TYPE_CODE (type))
     {
       case TYPE_CODE_FLT:
-      case TYPE_CODE_DECFLOAT:
-        /* __m128, __m128i, __m128d, floats, and doubles are returned
-           via XMM0.  */
-        if (len == 4 || len == 8 || len == 16)
+        /* floats, and doubles are returned via XMM0.  */
+        if (len == 4 || len == 8)
           regnum = AMD64_XMM0_REGNUM;
         break;
+      case TYPE_CODE_ARRAY:
+        /* __m128, __m128i and __m128d are returned via XMM0.  */
+	if (TYPE_VECTOR (type) && len == 16)
+	  {
+	    enum type_code code = TYPE_CODE (TYPE_TARGET_TYPE (type));
+	    if (code == TYPE_CODE_INT || code == TYPE_CODE_FLT)
+	      {
+		regnum = AMD64_XMM0_REGNUM;
+		break;
+	      }
+	  }
+	/* fall through */
       default:
         /* All other values that are 1, 2, 4 or 8 bytes long are returned
            via RAX.  */
         if (len == 1 || len == 2 || len == 4 || len == 8)
           regnum = AMD64_RAX_REGNUM;
+	else if (len == 16 && TYPE_CODE (type) == TYPE_CODE_INT)
+	  regnum = AMD64_XMM0_REGNUM;
         break;
     }
 
diff --git a/gdb/testsuite/gdb.base/call-sc.c b/gdb/testsuite/gdb.base/call-sc.c
index ba80576ac3..c798426ec4 100644
--- a/gdb/testsuite/gdb.base/call-sc.c
+++ b/gdb/testsuite/gdb.base/call-sc.c
@@ -35,6 +35,7 @@ typedef t T;
 #endif
 
 T foo = '1', L;
+T init = '9';
 
 T fun()
 {
@@ -55,7 +56,7 @@ int main()
 {
   int i;
 
-  Fun(foo);	
+  Fun(init);
 
   /* An infinite loop that first clears all the variables and then
      calls the function.  This "hack" is to make re-testing easier -
diff --git a/gdb/testsuite/gdb.base/call-sc.exp b/gdb/testsuite/gdb.base/call-sc.exp
index 719000435a..9697c5ac24 100644
--- a/gdb/testsuite/gdb.base/call-sc.exp
+++ b/gdb/testsuite/gdb.base/call-sc.exp
@@ -291,6 +291,19 @@ proc test_scalar_returns { } {
 		fail "${test}"
 	    }
 	}
+	-re " = 57 .*${gdb_prompt} $" {
+	    if $return_value_unknown {
+		# The struct return case.
+		# The return value is stored on the stack, and since GDB
+		# didn't override it, it still has value that was stored
+		# there in the earlier Foo(init) call.
+		pass "${test}"
+	    } else {
+		# This contradicts the above claim that GDB knew
+		# the location of the return-value.
+		fail "${test}"
+	    }
+	}
 	-re ".*${gdb_prompt} $" {
 	    if $return_value_unimplemented {
 		# What a suprize.  The architecture hasn't implemented
diff --git a/gdb/testsuite/gdb.base/callfuncs.exp b/gdb/testsuite/gdb.base/callfuncs.exp
index 642fe0d7fd..8c47f72dd9 100644
--- a/gdb/testsuite/gdb.base/callfuncs.exp
+++ b/gdb/testsuite/gdb.base/callfuncs.exp
@@ -542,6 +542,10 @@ if { ![prepare_for_testing "failed to prepare" $testfile $srcfile "$compile_flag
     perform_all_tests 1
 }
 
+# On Windows, you can't replace the executable if it is still running.
+gdb_exit
+gdb_start
+
 if { ![prepare_for_testing "failed to prepare" $testfile $srcfile "$compile_flags additional_flags=-DNO_PROTOTYPES"] } {
     with_test_prefix "noproto" {
 	perform_all_tests 0
-- 
2.26.2

Re: [RFC][PATCH] Fix function argument and return value locations

[Tom Tromey]

>>>>> "Hannes" == Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> writes:

Hannes> For return values, there were a lot more issues:
Hannes> - TYPE_CODE_DECFLOAT is NOT returned via XMM0.
Hannes> - long double is NOT returned via XMM0.
Hannes> - but __int128 IS returned via XMM0.
Hannes> - the comments for TYPE_CODE_FLT state that __m128, __m128i and __m128d are
Hannes>   returned by XMM0, and this is correct, but it doesn't actually check for
Hannes>   them, because they are TYPE_CODE_ARRAY with TYPE_VECTOR

Hannes> Could it be that this depends on the version of the used compiler?

It's definitely possible, but in this case I don't know.

Normally, following the platform ABI is the best thing to do.  In some
cases, this isn't possible and you have to resort to other approaches,
like compiler sniffing or whatever.

I guess it's likely nobody ever tried these tests on Windows, or that
they did but since the failures were 'baseline', they were just ignored.

If the platform ABI doesn't cover the cases, but some common compiler
(like gcc) does, then IMO it's fine to handle what the compiler emits.
If compilers disagree, then it gets more difficult.

Hannes> Also, in call-sc.exp, for the "value foo returned" test, I'm wondering
Hannes> if it can really ever return 90.

I think the idea there is that 90 is 'Z', so this catches the case where
somehow "return" completely failed, but at the same time gdb didn't warn
about it.  Maybe it can't really happen and it's just a defensive test.
Or maybe things changed since it was written and nobody noticed to
update the test.

Hannes> If any of this correct, then I will send a proper patch again (and maybe I
Hannes> should split the argument part into its own patch), if not, please advise.

I think it looks pretty reasonable, especially if it is making failing
tests now pass :)

Hannes>  T foo = '1', L;
Hannes> +T init = '9';
 
Hannes> -  Fun(foo);	
Hannes> +  Fun(init);
 
I didn't understand the need for these parts.

Hannes> +# On Windows, you can't replace the executable if it is still running.
Hannes> +gdb_exit
Hannes> +gdb_start

This seems like something that could go in immediately.

thanks,
Tom

Re: [RFC][PATCH] Fix function argument and return value locations

[Hannes Domani]

 Am Donnerstag, 7. Mai 2020, 15:43:25 MESZ hat Tom Tromey <tom@tromey.com> Folgendes geschrieben:

> >>>>> "Hannes" == Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Hannes> For return values, there were a lot more issues:
> Hannes> - TYPE_CODE_DECFLOAT is NOT returned via XMM0.
> Hannes> - long double is NOT returned via XMM0.
> Hannes> - but __int128 IS returned via XMM0.
> Hannes> - the comments for TYPE_CODE_FLT state that __m128, __m128i and __m128d are
> Hannes>  returned by XMM0, and this is correct, but it doesn't actually check for
> Hannes>  them, because they are TYPE_CODE_ARRAY with TYPE_VECTOR
>
> Hannes> Could it be that this depends on the version of the used compiler?
>
> It's definitely possible, but in this case I don't know.
>
> Normally, following the platform ABI is the best thing to do.  In some
> cases, this isn't possible and you have to resort to other approaches,
> like compiler sniffing or whatever.
>
> I guess it's likely nobody ever tried these tests on Windows, or that
> they did but since the failures were 'baseline', they were just ignored.

I'm currently going through all failing tests in gdb.base.
Most just need some kind of path conversion between the cygwin/windows paths,
and some are just not supported on windows, and in some the output deviates
a bit from the expected output.

So far this has uncovered 5 other bugs for the windows gdb, for 4 of them
I have potential fixes (which I will submit soonish), I'm still trying to
figure out the 5th.

I have now a lot of mingw-specific checks in the testsuite, I'm not sure
what to do with them, some of them really don't look nice.
But I guess I will submit them, split by the type of changes needed
(unsupported, path conversion, output different), and let you decide what to
do with them.


> If the platform ABI doesn't cover the cases, but some common compiler
> (like gcc) does, then IMO it's fine to handle what the compiler emits.
> If compilers disagree, then it gets more difficult.

I don't really know much about platform ABI stuff, I just made a
test example with all the int and float types, and the ones mentioned in the
amd64_windows_return_value function (__m128*, _Decimal*), and adjusted its
code so it matched the actual executable compiled with gcc.

(I probably should add this example to the testsuite.)

And right now I also tested with the oldest gcc I had available (4.6.1), and
it also needed these changes I made to gdb.


> Hannes> Also, in call-sc.exp, for the "value foo returned" test, I'm wondering
> Hannes> if it can really ever return 90.
>
> I think the idea there is that 90 is 'Z', so this catches the case where
> somehow "return" completely failed, but at the same time gdb didn't warn
> about it.  Maybe it can't really happen and it's just a defensive test.
> Or maybe things changed since it was written and nobody noticed to
> update the test.
>
> Hannes> If any of this correct, then I will send a proper patch again (and maybe I
> Hannes> should split the argument part into its own patch), if not, please advise.
>
> I think it looks pretty reasonable, especially if it is making failing
> tests now pass :)

OK.


> Hannes>  T foo = '1', L;
> Hannes> +T init = '9';
>
> Hannes> -  Fun(foo);   
> Hannes> +  Fun(init);
>
> I didn't understand the need for these parts.

It corresponds to the added case in call-sc.exp:

+    -re " = 57 .*${gdb_prompt} $" {
+        if $return_value_unknown {
+        # The struct return case.
+        # The return value is stored on the stack, and since GDB
+        # didn't override it, it still has value that was stored
+        # there in the earlier Foo(init) call.
+        pass "${test}"
+        } else {
+        # This contradicts the above claim that GDB knew
+        # the location of the return-value.
+        fail "${test}"
+        }
+    }

I should have written " = 57 '9'*${gdb_prompt} $", then it would have been
move obvious why I changed init to '9'.

That's also the reason why I think that 90 can never be the result, because
it will reuse the last value that was stored on the particular stack location,
just like it happens for me here.


> Hannes> +# On Windows, you can't replace the executable if it is still running.
> Hannes> +gdb_exit
> Hannes> +gdb_start
>
> This seems like something that could go in immediately.

I have more of these cases now, and they will probably grow until I'm through
all the tests, so I will send a patch for all of them then.


Regards
Hannes