| Message ID | CAFULd4ZDJxVU+v7fLOd3jW73LUn0LEcBdwHZpdre4hcGLa6wyw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Jul 21, 2014 at 8:21 PM, Uros Bizjak <ubizjak@gmail.com> wrote: > Attached patch fixes libgo reflect test failure with libffi closures. > The gccgo compiler started to use FFI closures recently; the compiler > passes ffi_type_void for structures with zero members. > > ffi_call form src/alpha/ffi.c allows FFI_TYPE_VOID arguments in > non-debug mode through the default: case, but ffi_closure_osf_inner > aborts with this type of argument. > > The patch changes the default case in ffi_closure_osf_inner from abort > to FFI_ASSERT, an this way synchronizes argument handling in both > cases. > > 2014-07-21 Uros Bizjak <ubizjak@gmail.com> > > * src/alpha/ffi.c: Do not include stdlib.h. > (ffi_closure_osf_inner) <default>: Use FFI_ASSERT instead of abort. > > Patch was tested with libffi testsuite on alphaev6-linux-gnu. > Additionally, the patch fixed reflect test from the libgo testsuite > and go.test/test/recover.go test from the gcc testsuite. I have installed the patch in gcc mainline SVN as r213049. Uros.
[replying to an ancient post here..] Uros Bizjak <ubizjak@gmail.com> writes: > Hello! > > Attached patch fixes libgo reflect test failure with libffi closures. > The gccgo compiler started to use FFI closures recently; the compiler > passes ffi_type_void for structures with zero members. Why not just pass an FFI_TYPE_STRUCT with zero members? > ffi_call form src/alpha/ffi.c allows FFI_TYPE_VOID arguments in > non-debug mode through the default: case, but ffi_closure_osf_inner > aborts with this type of argument. > > The patch changes the default case in ffi_closure_osf_inner from abort > to FFI_ASSERT, an this way synchronizes argument handling in both > cases. > > 2014-07-21 Uros Bizjak <ubizjak@gmail.com> > > * src/alpha/ffi.c: Do not include stdlib.h. > (ffi_closure_osf_inner) <default>: Use FFI_ASSERT instead of abort. > > Patch was tested with libffi testsuite on alphaev6-linux-gnu. > Additionally, the patch fixed reflect test from the libgo testsuite > and go.test/test/recover.go test from the gcc testsuite. Why not add an FFI_TYPE_VOID case so it doesn't ever abort if that's expected behaviour? The default case is there to catch unexpected values. AG > > Uros. > > Index: src/alpha/ffi.c > =================================================================== > --- src/alpha/ffi.c (revision 212882) > +++ src/alpha/ffi.c (working copy) > @@ -27,7 +27,6 @@ > > #include <ffi.h> > #include <ffi_common.h> > -#include <stdlib.h> > > /* Force FFI_TYPE_LONGDOUBLE to be different than FFI_TYPE_DOUBLE; > all further uses in this file will refer to the 128-bit type. */ > @@ -273,7 +272,7 @@ ffi_closure_osf_inner(ffi_closure *closure, void * > break; > > default: > - abort (); > + FFI_ASSERT (0); > } > > argn += ALIGN(size, FFI_SIZEOF_ARG) / FFI_SIZEOF_ARG;
On Sep 20, 2014, at 3:04 AM, Anthony Green <green@moxielogic.com> wrote: > > Why not just pass an FFI_TYPE_STRUCT with zero members? My information may be old or irrelevant but I have used structs with no members with gcc backend, but with nonzero size and alignment, and ran into backend problems, particularly on sparc64, passing them as parameters. Is that what is being used here? Maybe best to add some members to achieve equivalent size/alignment? - Jay
On Sat, Sep 20, 2014 at 3:04 AM, Anthony Green <green@moxielogic.com> wrote: > >> Attached patch fixes libgo reflect test failure with libffi closures. >> The gccgo compiler started to use FFI closures recently; the compiler >> passes ffi_type_void for structures with zero members. > > Why not just pass an FFI_TYPE_STRUCT with zero members? Because when an empty struct is used as a return type libffi returns a failure from ffi_prep_cif_core: /* Initialize the return type if necessary */ if ((cif->rtype->size == 0) && (initialize_aggregate(cif->rtype) != FFI_OK)) return FFI_BAD_TYPEDEF; This is because initialize_aggregate fails: if (UNLIKELY(arg == NULL || arg->elements == NULL)) return FFI_BAD_TYPEDEF; I haven't looked farther. Ian
On Sat, Sep 20, 2014 at 12:04 PM, Anthony Green <green@moxielogic.com> wrote: > > [replying to an ancient post here..] > > Uros Bizjak <ubizjak@gmail.com> writes: > >> Hello! >> >> Attached patch fixes libgo reflect test failure with libffi closures. >> The gccgo compiler started to use FFI closures recently; the compiler >> passes ffi_type_void for structures with zero members. > > Why not just pass an FFI_TYPE_STRUCT with zero members? > >> ffi_call form src/alpha/ffi.c allows FFI_TYPE_VOID arguments in >> non-debug mode through the default: case, but ffi_closure_osf_inner >> aborts with this type of argument. >> >> The patch changes the default case in ffi_closure_osf_inner from abort >> to FFI_ASSERT, an this way synchronizes argument handling in both >> cases. >> >> 2014-07-21 Uros Bizjak <ubizjak@gmail.com> >> >> * src/alpha/ffi.c: Do not include stdlib.h. >> (ffi_closure_osf_inner) <default>: Use FFI_ASSERT instead of abort. >> >> Patch was tested with libffi testsuite on alphaev6-linux-gnu. >> Additionally, the patch fixed reflect test from the libgo testsuite >> and go.test/test/recover.go test from the gcc testsuite. > > Why not add an FFI_TYPE_VOID case so it doesn't ever abort if that's > expected behaviour? The default case is there to catch unexpected > values. The patch just equalizes handling of unknown arguments to ffi_call. Also, the approach is the same as for x86_64 and i386 - these targets doesn't abort for unknown arguments. There is no problem in adding handling of FFI_TYPE_VOID to both functions, if this is the preferred solution. Uros.
Index: src/alpha/ffi.c =================================================================== --- src/alpha/ffi.c (revision 212882) +++ src/alpha/ffi.c (working copy) @@ -27,7 +27,6 @@ #include <ffi.h> #include <ffi_common.h> -#include <stdlib.h> /* Force FFI_TYPE_LONGDOUBLE to be different than FFI_TYPE_DOUBLE; all further uses in this file will refer to the 128-bit type. */ @@ -273,7 +272,7 @@ ffi_closure_osf_inner(ffi_closure *closure, void * break; default: - abort (); + FFI_ASSERT (0); } argn += ALIGN(size, FFI_SIZEOF_ARG) / FFI_SIZEOF_ARG;
Hello! Attached patch fixes libgo reflect test failure with libffi closures. The gccgo compiler started to use FFI closures recently; the compiler passes ffi_type_void for structures with zero members. ffi_call form src/alpha/ffi.c allows FFI_TYPE_VOID arguments in non-debug mode through the default: case, but ffi_closure_osf_inner aborts with this type of argument. The patch changes the default case in ffi_closure_osf_inner from abort to FFI_ASSERT, an this way synchronizes argument handling in both cases. 2014-07-21 Uros Bizjak <ubizjak@gmail.com> * src/alpha/ffi.c: Do not include stdlib.h. (ffi_closure_osf_inner) <default>: Use FFI_ASSERT instead of abort. Patch was tested with libffi testsuite on alphaev6-linux-gnu. Additionally, the patch fixed reflect test from the libgo testsuite and go.test/test/recover.go test from the gcc testsuite. Uros.