Patchwork [libffi,alpha] : Use FFI_ASSERT in ffi_closure_osf_inner

login
register
mail settings
Submitter Uros Bizjak
Date July 21, 2014, 6:21 p.m.
Message ID <CAFULd4ZDJxVU+v7fLOd3jW73LUn0LEcBdwHZpdre4hcGLa6wyw@mail.gmail.com>
Download mbox | patch
Permalink /patch/372174/
State New
Headers show

Comments

Uros Bizjak - July 21, 2014, 6:21 p.m.
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.
Uros Bizjak - July 25, 2014, 9:02 a.m.
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.
Anthony Green - Sept. 20, 2014, 10:04 a.m.
[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;
Jay K - Sept. 20, 2014, 6:28 p.m.
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
Ian Taylor - Sept. 20, 2014, 10:04 p.m.
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

Patch

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;