Merge pull request #504 from andrewdotn/master

Support blocking: true for varargs calls

5 files changed, 111 insertions, 25 deletions

diff --git a/ext/ffi_c/Call.c b/ext/ffi_c/Call.c
index b028811..a738cb5 100644
--- a/ext/ffi_c/Call.c
+++ b/ext/ffi_c/Call.c
@@ -338,40 +338,27 @@ rbffi_SetupCallParams(int argc, VALUE* argv, int paramCount, Type** paramTypes,
     }
 }
 
-
-typedef struct BlockingCall_ {
-    rbffi_frame_t* frame;
-    void* function;
-    FunctionType* info;
-    void **ffiValues;
-    void* retval;
-    void* params;
-#if !(defined(HAVE_RB_THREAD_BLOCKING_REGION) || defined(HAVE_RB_THREAD_CALL_WITHOUT_GVL))
-    void* stkretval;
-#endif
-} BlockingCall;
-
 static VALUE
 call_blocking_function(void* data)
 {
-    BlockingCall* b = (BlockingCall *) data;
+    rbffi_blocking_call_t* b = (rbffi_blocking_call_t *) data;
     b->frame->has_gvl = false;
-    ffi_call(&b->info->ffi_cif, FFI_FN(b->function), b->retval, b->ffiValues);
+    ffi_call(&b->cif, FFI_FN(b->function), b->retval, b->ffiValues);
     b->frame->has_gvl = true;
 
     return Qnil;
 }
 
-static VALUE
-do_blocking_call(void *data)
+VALUE
+rbffi_do_blocking_call(void *data)
 {
     rbffi_thread_blocking_region(call_blocking_function, data, (void *) -1, NULL);
 
     return Qnil;
 }
 
-static VALUE
-save_frame_exception(void *data, VALUE exc)
+VALUE
+rbffi_save_frame_exception(void *data, VALUE exc)
 {
     rbffi_frame_t* frame = (rbffi_frame_t *) data;
     frame->exc = exc;
@@ -390,7 +377,7 @@ rbffi_CallFunction(int argc, VALUE* argv, void* function, FunctionType* fnInfo)
     retval = alloca(MAX(fnInfo->ffi_cif.rtype->size, FFI_SIZEOF_ARG));
     
     if (unlikely(fnInfo->blocking)) {
-        BlockingCall* bc;
+        rbffi_blocking_call_t* bc;
 
         /*
          * due to the way thread switching works on older ruby variants, we
@@ -399,16 +386,16 @@ rbffi_CallFunction(int argc, VALUE* argv, void* function, FunctionType* fnInfo)
 #if defined(HAVE_RB_THREAD_BLOCKING_REGION) || defined(HAVE_RB_THREAD_CALL_WITHOUT_GVL)
         ffiValues = ALLOCA_N(void *, fnInfo->parameterCount);
         params = ALLOCA_N(FFIStorage, fnInfo->parameterCount);
-        bc = ALLOCA_N(BlockingCall, 1);
+        bc = ALLOCA_N(rbffi_blocking_call_t, 1);
         bc->retval = retval;
 #else
         ffiValues = ALLOC_N(void *, fnInfo->parameterCount);
         params = ALLOC_N(FFIStorage, fnInfo->parameterCount);
-        bc = ALLOC_N(BlockingCall, 1);
+        bc = ALLOC_N(rbffi_blocking_call_t, 1);
         bc->retval = xmalloc(MAX(fnInfo->ffi_cif.rtype->size, FFI_SIZEOF_ARG));
         bc->stkretval = retval;
 #endif
-        bc->info = fnInfo;
+        bc->cif = fnInfo->ffi_cif;
         bc->function = function;
         bc->ffiValues = ffiValues;
         bc->params = params;
@@ -419,11 +406,11 @@ rbffi_CallFunction(int argc, VALUE* argv, void* function, FunctionType* fnInfo)
             fnInfo->callbackParameters, fnInfo->callbackCount, fnInfo->rbEnums);
 
         rbffi_frame_push(&frame); 
-        rb_rescue2(do_blocking_call, (VALUE) bc, save_frame_exception, (VALUE) &frame, rb_eException, (VALUE) 0);
+        rb_rescue2(rbffi_do_blocking_call, (VALUE) bc, rbffi_save_frame_exception, (VALUE) &frame, rb_eException, (VALUE) 0);
         rbffi_frame_pop(&frame);
 
 #if !(defined(HAVE_RB_THREAD_BLOCKING_REGION) || defined(HAVE_RB_THREAD_CALL_WITHOUT_GVL))
-        memcpy(bc->stkretval, bc->retval, MAX(bc->info->ffi_cif.rtype->size, FFI_SIZEOF_ARG));
+        memcpy(bc->stkretval, bc->retval, MAX(bc->cif.rtype->size, FFI_SIZEOF_ARG));
         xfree(bc->params);
         xfree(bc->ffiValues);
         xfree(bc->retval);
diff --git a/ext/ffi_c/Call.h b/ext/ffi_c/Call.h
index 0b971f2..56bdd61 100644
--- a/ext/ffi_c/Call.h
+++ b/ext/ffi_c/Call.h
@@ -33,6 +33,8 @@
 #ifndef RBFFI_CALL_H
 #define	RBFFI_CALL_H
 
+#include "Thread.h"
+
 #ifdef	__cplusplus
 extern "C" {
 #endif
@@ -85,6 +87,21 @@ Invoker rbffi_GetInvoker(struct FunctionType_* fnInfo);
 extern VALUE rbffi_GetEnumValue(VALUE enums, VALUE value);
 extern int rbffi_GetSignedIntValue(VALUE value, int type, int minValue, int maxValue, const char* typeName, VALUE enums);
 
+typedef struct rbffi_blocking_call {
+    rbffi_frame_t* frame;
+    void* function;
+    ffi_cif cif;
+    void **ffiValues;
+    void* retval;
+    void* params;
+#if !(defined(HAVE_RB_THREAD_BLOCKING_REGION) || defined(HAVE_RB_THREAD_CALL_WITHOUT_GVL))
+    void* stkretval;
+#endif
+} rbffi_blocking_call_t;
+
+VALUE rbffi_do_blocking_call(void* data);
+VALUE rbffi_save_frame_exception(void *data, VALUE exc);
+
 #ifdef	__cplusplus
 }
 #endif
diff --git a/ext/ffi_c/Variadic.c b/ext/ffi_c/Variadic.c
index 0027be2..877ffab 100644
--- a/ext/ffi_c/Variadic.c
+++ b/ext/ffi_c/Variadic.c
@@ -64,6 +64,7 @@ typedef struct VariadicInvoker_ {
     ffi_abi abi;
     void* function;
     int paramCount;
+    bool blocking;
 } VariadicInvoker;
 
 
@@ -84,6 +85,7 @@ variadic_allocate(VALUE klass)
     invoker->rbAddress = Qnil;
     invoker->rbEnums = Qnil;
     invoker->rbReturnType = Qnil;
+    invoker->blocking = false;
 
     return obj;
 }
@@ -115,6 +117,7 @@ variadic_initialize(VALUE self, VALUE rbFunction, VALUE rbParameterTypes, VALUE
     invoker->rbEnums = rb_hash_aref(options, ID2SYM(rb_intern("enums")));
     invoker->rbAddress = rbFunction;
     invoker->function = rbffi_AbstractMemory_Cast(rbFunction, rbffi_PointerClass)->address;
+    invoker->blocking = RTEST(rb_hash_aref(options, ID2SYM(rb_intern("blocking"))));
 
 #if defined(X86_WIN32)
     rbConventionStr = rb_funcall2(convention, rb_intern("to_s"), 0, NULL);
@@ -253,7 +256,28 @@ variadic_invoke(VALUE self, VALUE parameterTypes, VALUE parameterValues)
         ffiValues, NULL, 0, invoker->rbEnums);
     
     rbffi_frame_push(&frame);
+#ifdef HAVE_RB_THREAD_CALL_WITHOUT_GVL
+    /* In Call.c, blocking: true is supported on older ruby variants
+     * without rb_thread_call_without_gvl by allocating on the heap instead
+     * of the stack. Since this functionality is being added later,
+     * we’re skipping support for old rubies here. */
+    if(unlikely(invoker->blocking)) {
+        rbffi_blocking_call_t* bc;
+        bc = ALLOCA_N(rbffi_blocking_call_t, 1);
+        bc->retval = retval;
+        bc->function = invoker->function;
+        bc->ffiValues = ffiValues;
+        bc->params = params;
+        bc->frame = &frame;
+        bc->cif = cif;
+
+        rb_rescue2(rbffi_do_blocking_call, (VALUE) bc, rbffi_save_frame_exception, (VALUE) &frame, rb_eException, (VALUE) 0);
+    } else {
+        ffi_call(&cif, FFI_FN(invoker->function), retval, ffiValues);
+    }
+#else
     ffi_call(&cif, FFI_FN(invoker->function), retval, ffiValues);
+#endif
     rbffi_frame_pop(&frame);
     
     rbffi_save_errno();
diff --git a/spec/ffi/fixtures/FunctionTest.c b/spec/ffi/fixtures/FunctionTest.c
index a37373a..1dd9185 100644
--- a/spec/ffi/fixtures/FunctionTest.c
+++ b/spec/ffi/fixtures/FunctionTest.c
@@ -11,6 +11,7 @@
 #ifndef _WIN32
 #include <unistd.h>
 #include <pthread.h>
+#include <stdarg.h>
 #include <stdlib.h>
 #endif
 
@@ -61,6 +62,44 @@ void testBlockingClose(struct testBlockingData *self) {
     free(self);
 }
 
+static int sum_varargs(va_list args) {
+    char sum = 0;
+    int arg;
+    while ((arg = va_arg(args, int)) != 0) {
+        sum += arg;
+    }
+    va_end(args);
+    return sum;
+}
+
+/* Write c to pipe1 and return the value read from pipe2, or 0 if there’s
+ * an error such as a timeout, or if c does not equal the sum of the
+ * zero-terminated list of char arguments. */
+char testBlockingWRva(struct testBlockingData *self, char c, ...) {
+    va_list args;
+    va_start(args, c);
+    if (sum_varargs(args) != c) {
+        return 0;
+    }
+
+    if( pipeHelperWriteChar(self->pipe1[1], c) != 1)
+        return 0;
+    return pipeHelperReadChar(self->pipe2[0], 10);
+}
+
+char testBlockingRWva(struct testBlockingData *self, char c, ...) {
+    va_list args;
+    va_start(args, c);
+    if (sum_varargs(args) != c) {
+        return 0;
+    }
+
+    char d = pipeHelperReadChar(self->pipe1[0], 10);
+    if( pipeHelperWriteChar(self->pipe2[1], c) != 1)
+        return 0;
+    return d;
+}
+
 struct async_data {
     void (*fn)(int);
     int value;
diff --git a/spec/ffi/variadic_spec.rb b/spec/ffi/variadic_spec.rb
index 4138280..0c7292e 100644
--- a/spec/ffi/variadic_spec.rb
+++ b/spec/ffi/variadic_spec.rb
@@ -13,6 +13,11 @@ describe "Function with variadic arguments" do
     enum :enum_type2, [:c3, 42, :c4]
     attach_function :pack_varargs, [ :buffer_out, :string, :varargs ], :void
     attach_function :pack_varargs2, [ :buffer_out, :enum_type1, :string, :varargs ], :enum_type1
+
+    attach_function :testBlockingOpen, [ ], :pointer
+    attach_function :testBlockingRWva, [ :pointer, :char, :varargs ], :char, :blocking => true
+    attach_function :testBlockingWRva, [ :pointer, :char, :varargs ], :char, :blocking => true
+    attach_function :testBlockingClose, [ :pointer ], :void
   end
 
   it "takes enum arguments" do
@@ -27,6 +32,20 @@ describe "Function with variadic arguments" do
     expect(LibTest.pack_varargs2(buf, :c1, "ii", :int, :c3, :int, :c4)).to eq(:c2)
   end
 
+  it 'can wrap a blocking function with varargs' do
+    pending("not supported in 1.8") if RUBY_VERSION =~ /^1\.8\..*/
+    handle = LibTest.testBlockingOpen
+    expect(handle).not_to be_null
+    begin
+      thWR = Thread.new { LibTest.testBlockingWRva(handle, 63, :int, 40, :int, 23, :int, 0) }
+      thRW = Thread.new { LibTest.testBlockingRWva(handle, 64, :int, 40, :int, 24, :int, 0) }
+      expect(thWR.value).to eq(64)
+      expect(thRW.value).to eq(63)
+    ensure
+      LibTest.testBlockingClose(handle)
+    end
+  end
+
   [ 0, 127, -128, -1 ].each do |i|
     it "call variadic with (:char (#{i})) argument" do
       buf = FFI::Buffer.new :long_long