Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libffi on SPARC V9 have problem calling functions with floating point number arguments #778

Open
psumbera opened this issue May 2, 2023 · 11 comments

Comments

@psumbera
Copy link
Contributor

psumbera commented May 2, 2023

I see the problem on Solaris SPARC V9 with /usr/bin/gjs when it calls

clutter_actor_set_size (ClutterActor *self, gfloat width, gfloat height);

https://github.com/GNOME/mutter/blob/d643eb5c6fe50e7f1afffda0e8747a87f668a799/clutter/clutter/clutter-actor.c#L9473

Note the floating point number arguments. When I add some debug printfs there I see that it gets just zeros.

This is regression. Old version 3.2.1 worked well. The first bad commit was: 2b27890
(but I know it was major rewrite up and there were other follow up SPARC releated commits)

@psumbera
Copy link
Contributor Author

psumbera commented May 3, 2023

The resolution for this particual issue seems to be add SPARC_FLAG_FP_ARGS into flags:

flags |= SPARC_FLAG_FP_ARGS;

for FFI_TYPE_FLOAT and FFI_TYPE_DOUBLE

case FFI_TYPE_FLOAT:

But there seems to be some other SPARC problems too... (gnome-shell has still some issues even it's now able to start).

@psumbera
Copy link
Contributor Author

psumbera commented May 4, 2023

I see that ffi_prep_cif_machdep_core does set SPARC_FLAG_FP_ARGS apropriatery. And libffi tests seems to pass (even with floating point arguments).

On the other hand gjs 1.70.2 doesn't seem to call ffi_prep_cif for clutter_actor_set at all.

@thesamesam
Copy link

thesamesam commented May 6, 2023

Thanks for reporting this, I think I've seen this as well but I wasn't sure enough yet if it was a libffi bug (https://bugs.gentoo.org/882071) but I think it must be the same.

@thesamesam
Copy link

thesamesam commented Jun 9, 2023

cc @rth7680 (commit author), although not sure if you're involved with libffi nowadays.

@psumbera
Copy link
Contributor Author

I have looked at this again and now with my initial finding in #778 (comment) I don't see any other obvious problems (I believe originally there was problem with Gnome unlocking for example).

So, maybe following patch is enough?

--- libffi-3.4.4/src/sparc/ffi64.c
+++ libffi-3.4.4/src/sparc/ffi64.c
@@ -383,12 +383,16 @@
          break;
        case FFI_TYPE_UINT32:
        case FFI_TYPE_FLOAT:
+         flags |= SPARC_FLAG_FP_ARGS;
          *argp++ = *(UINT32 *)a;
          break;
+       case FFI_TYPE_DOUBLE:
+         flags |= SPARC_FLAG_FP_ARGS;
+         *argp++ = *(UINT64 *)a;
+         break;
        case FFI_TYPE_SINT64:
        case FFI_TYPE_UINT64:
        case FFI_TYPE_POINTER:
-       case FFI_TYPE_DOUBLE:
          *argp++ = *(UINT64 *)a;
          break;

@thesamesam can you please test whether it solves your problems?

@rth7680
Copy link
Contributor

rth7680 commented Oct 18, 2023

Setting FP_ARGS for FFI_TYPE_UINT32 is obviously wrong.
Need to separate the FFI_TYPE_FLOAT case.

@psumbera
Copy link
Contributor Author

Ok, it sounds right. I have updated the patch. But with my test case of Gnome (gjs) it doesn't seem to make any difference...

--- libffi-3.4.4/src/sparc/ffi64.c
+++ libffi-3.4.4/src/sparc/ffi64.c
@@ -382,13 +382,19 @@
          *argp++ = *(SINT32 *)a;
          break;
        case FFI_TYPE_UINT32:
-       case FFI_TYPE_FLOAT:
          *argp++ = *(UINT32 *)a;
          break;
        case FFI_TYPE_SINT64:
        case FFI_TYPE_UINT64:
        case FFI_TYPE_POINTER:
+         *argp++ = *(UINT64 *)a;
+         break;
+       case FFI_TYPE_FLOAT:
+         flags |= SPARC_FLAG_FP_ARGS;
+         *argp++ = *(UINT32 *)a;
+         break;
        case FFI_TYPE_DOUBLE:
+         flags |= SPARC_FLAG_FP_ARGS;
          *argp++ = *(UINT64 *)a;
          break;

@thesamesam
Copy link

Thanks, I'll try the second patch first. Wrt no difference - you mean both patches fix your issue, or you mean the second patch doesn't fix your gjs problem? (Hopefully the former..)

@psumbera
Copy link
Contributor Author

Thanks, I'll try the second patch first. Wrt no difference - you mean both patches fix your issue, or you mean the second patch doesn't fix your gjs problem? (Hopefully the former..)

Yes, the seond patch works for me too. I wasn't able to run gjs tests (which is probably your test case) as there is some unrelated Python problem (I think).

@thesamesam
Copy link

thesamesam commented Oct 19, 2023

gjs tests before, with the failing GIMarshalling test:

 1/71 gjs:JS / GIMarshalling                       ERROR           7.93s   killed by signal 6 SIGABRT
>>> G_DEBUG=fatal-warnings,fatal-criticals TSAN_OPTIONS=history_size=5,force_seq_cst_atomics=1,suppressions=/var/tmp/portage/dev-libs/gjs-1.78.0/work/gjs-1.78.0/installed-tests/extra/tsan.supp GSETTINGS_SCHEM
A_DIR=/var/tmp/portage/dev-libs/gjs-1.78.0/work/gjs-1.78.0-build/installed-tests/js GJS_DEBUG_TOPICS='' LD_LIBRARY_PATH=/var/tmp/portage/dev-libs/gjs-1.78.0/work/gjs-1.78.0-build/:/var/tmp/portage/dev-libs/gj
s-1.78.0/work/gjs-1.78.0-build:/var/tmp/portage/dev-libs/gjs-1.78.0/work/gjs-1.78.0-build/installed-tests/js:/var/tmp/portage/dev-libs/gjs-1.78.0/work/gjs-1.78.0-build/installed-tests/js/libgjstesttools ENABL
E_GTK=yes LSAN_OPTIONS=fast_unwind_on_malloc=0,exitcode=23,suppressions=/var/tmp/portage/dev-libs/gjs-1.78.0/work/gjs-1.78.0/installed-tests/extra/lsan.supp G_FILENAME_ENCODING=latin1 LC_ALL=C.utf8 TOP_BUILDD
IR=/var/tmp/portage/dev-libs/gjs-1.78.0/work/gjs-1.78.0-build DYLD_FALLBACK_LIBRARY_PATH=/var/tmp/portage/dev-libs/gjs-1.78.0/work/gjs-1.78.0-build:/var/tmp/portage/dev-libs/gjs-1.78.0/work/gjs-1.78.0-build/i
nstalled-tests/js:/var/tmp/portage/dev-libs/gjs-1.78.0/work/gjs-1.78.0-build/installed-tests/js/libgjstesttools GJS_USE_UNINSTALLED_FILES=1 G_SLICE=always-malloc GI_TYPELIB_PATH=/var/tmp/portage/dev-libs/gjs-
1.78.0/work/gjs-1.78.0-build:/var/tmp/portage/dev-libs/gjs-1.78.0/work/gjs-1.78.0-build/installed-tests/js:/var/tmp/portage/dev-libs/gjs-1.78.0/work/gjs-1.78.0-build/installed-tests/js/libgjstesttools GJS_PAT
H='' GSETTINGS_BACKEND=memory GJS_DEBUG_OUTPUT=stderr NO_AT_BRIDGE=1 MALLOC_PERTURB_=62 ASAN_OPTIONS=intercept_tls_get_addr=0 /var/tmp/portage/dev-libs/gjs-1.78.0/work/gjs-1.78.0-build/installed-tests/js/mini
jasmine /var/tmp/portage/dev-libs/gjs-1.78.0/work/gjs-1.78.0-build/../gjs-1.78.0/installed-tests/js/testGIMarshalling.js

stderr:
**
ERROR:/usr/share/gobject-introspection-1.0/tests/gimarshallingtests.c:904:gi_marshalling_tests_float_in: assertion failed (v == G_MAXFLOAT): (0 == 3.40282347e+38)

(test program exited with status code -6)


71/71 gjs:Scripts / CommandLine                    OK             16.65s   46 subtests passed

Summary of Failures:

 2/71 gjs:JS / GObjectValue                FAIL            2.79s   134/137 subtests passed
 3/71 gjs:JS / Regress                     FAIL            2.37s   275/276 subtests passed
 1/71 gjs:JS / GIMarshalling               ERROR           7.93s   killed by signal 6 SIGABRT

Ok:                 68
Expected Fail:      0
Fail:               3
Unexpected Pass:    0
Skipped:            0
Timeout:            0

After, all tests pass:

Ok:                 71
Expected Fail:      0
Fail:               0
Unexpected Pass:    0
Skipped:            0
Timeout:            0

Full log written to /var/tmp/portage/dev-libs/gjs-1.78.0/work/gjs-1.78.0-build/meson-logs/testlog.txt
>>> Completed testing dev-libs/gjs-1.78.0

Thanks a bunch! Would you mind making a PR for it?

gentoo-bot pushed a commit to gentoo/gentoo that referenced this issue Oct 19, 2023
This fixes the gjs test suite on sparc. Tests continue to pass on libffi
and gjs tests now completely pass (previously GIMarshalling failed). The fix
is obvious so I don't see much of a need to wait until it's merged upstream,
and it only affects sparc.

Bug: libffi/libffi#778
Closes: https://bugs.gentoo.org/882071
Signed-off-by: Sam James <sam@gentoo.org>
@thesamesam
Copy link

Fixed by #802.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants