[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