Merge pull request #1176 from kmod/rearrange_and_call

Make rearrangeArgs take a callback instead passing back values

Makefile

@@ -776,7 +776,7 @@ check$1 test$1: $(PYTHON_EXE_DEPS) pyston$1
 	@# we pass -I to cpython tests and skip failing ones because they are sloooow otherwise
 	$(PYTHON) $(TOOLS_DIR)/tester.py -R pyston$1 -j$(TEST_THREADS) -a=-S -k --exit-code-only --skip-failing -t50 $(TEST_DIR)/cpython $(ARGS)
 	$(PYTHON) $(TOOLS_DIR)/tester.py -R pyston$1 -j$(TEST_THREADS) -k -a=-S --exit-code-only --skip-failing -t600 $(TEST_DIR)/integration $(ARGS)
-	$(PYTHON) $(TOOLS_DIR)/tester.py -a=-X -R pyston$1 -j$(TEST_THREADS) -a=-n -a=-S -k $(TESTS_DIR) $(ARGS)
+	$(PYTHON) $(TOOLS_DIR)/tester.py -a=-X -R pyston$1 -j$(TEST_THREADS) -a=-n -a=-S -t50 -k $(TESTS_DIR) $(ARGS)
 	$(PYTHON) $(TOOLS_DIR)/tester.py -R pyston$1 -j$(TEST_THREADS) -a=-O -a=-S -k $(TESTS_DIR) $(ARGS)
 
 .PHONY: run$1 dbg$1

src/capi/typeobject.cpp

@@ -3411,51 +3411,40 @@ static Box* tppProxyToTpCall(Box* self, CallRewriteArgs* rewrite_args, ArgPassSp
         paramspec.takes_kwargs = false;
     }
 
-    bool rewrite_success = false;
-    Box** oargs = NULL;
-    try {
-        rearrangeArguments(paramspec, NULL, "", NULL, rewrite_args, rewrite_success, argspec, arg1, arg2, arg3, args,
-                           oargs, keyword_names, NULL);
-    } catch (ExcInfo e) {
-        if (S == CAPI) {
-            setCAPIException(e);
-            return NULL;
-        } else
-            throw e;
-    }
+    auto continuation = [=](CallRewriteArgs* rewrite_args, Box* arg1, Box* arg2, Box* arg3, Box** args) {
+        if (!paramspec.takes_kwargs)
+            arg2 = NULL;
 
-    AUTO_DECREF(arg1);
-    if (!paramspec.takes_kwargs)
-        arg2 = NULL;
-    AUTO_XDECREF(arg2);
+        if (rewrite_args) {
+            if (!paramspec.takes_kwargs)
+                rewrite_args->arg2 = rewrite_args->rewriter->loadConst(0, Location::forArg(2));
+
+            // Currently, guard that the value of tp_call didn't change, and then
+            // emit a call to the current function address.
+            // It might be better to just load the current value of tp_call and call it
+            // (after guarding it's not null), or maybe not.  But the rewriter doesn't currently
+            // support calling a RewriterVar (can only call fixed function addresses).
+            RewriterVar* r_cls = rewrite_args->obj->getAttr(offsetof(Box, cls));
+            r_cls->addAttrGuard(offsetof(BoxedClass, tp_call), (intptr_t)self->cls->tp_call);
+
+            rewrite_args->out_rtn = rewrite_args->rewriter->call(true, (void*)self->cls->tp_call, rewrite_args->obj,
+                                                                 rewrite_args->arg1, rewrite_args->arg2);
+            rewrite_args->out_rtn->setType(RefType::OWNED);
+            if (S == CXX)
+                rewrite_args->rewriter->checkAndThrowCAPIException(rewrite_args->out_rtn);
+            rewrite_args->out_success = true;
+        }
 
-    if (!rewrite_success)
-        rewrite_args = NULL;
+        Box* r = self->cls->tp_call(self, arg1, arg2);
+        if (S == CXX && !r)
+            throwCAPIException();
+        return r;
+    };
 
-    if (rewrite_args) {
-        if (!paramspec.takes_kwargs)
-            rewrite_args->arg2 = rewrite_args->rewriter->loadConst(0, Location::forArg(2));
-
-        // Currently, guard that the value of tp_call didn't change, and then
-        // emit a call to the current function address.
-        // It might be better to just load the current value of tp_call and call it
-        // (after guarding it's not null), or maybe not.  But the rewriter doesn't currently
-        // support calling a RewriterVar (can only call fixed function addresses).
-        RewriterVar* r_cls = rewrite_args->obj->getAttr(offsetof(Box, cls));
-        r_cls->addAttrGuard(offsetof(BoxedClass, tp_call), (intptr_t)self->cls->tp_call);
-
-        rewrite_args->out_rtn = rewrite_args->rewriter->call(true, (void*)self->cls->tp_call, rewrite_args->obj,
-                                                             rewrite_args->arg1, rewrite_args->arg2);
-        rewrite_args->out_rtn->setType(RefType::OWNED);
-        if (S == CXX)
-            rewrite_args->rewriter->checkAndThrowCAPIException(rewrite_args->out_rtn);
-        rewrite_args->out_success = true;
-    }
-
-    Box* r = self->cls->tp_call(self, arg1, arg2);
-    if (!r && S == CXX)
-        throwCAPIException();
-    return r;
+    return callCXXFromStyle<S>([&]() {
+        return rearrangeArgumentsAndCall(paramspec, NULL, "", NULL, rewrite_args, argspec, arg1, arg2, arg3, args,
+                                         keyword_names, continuation);
+    });
 }
 
 extern "C" void PyType_RequestHcAttrs(PyTypeObject* cls, int offset) noexcept {

src/core/types.h

@@ -262,15 +262,16 @@ struct ParamReceiveSpec {
         assert(num_defaults <= MAX_DEFAULTS);
     }
 
-    bool operator==(ParamReceiveSpec rhs) {
+    bool operator==(ParamReceiveSpec rhs) const {
         return takes_varargs == rhs.takes_varargs && takes_kwargs == rhs.takes_kwargs
                && num_defaults == rhs.num_defaults && num_args == rhs.num_args;
     }
 
-    bool operator!=(ParamReceiveSpec rhs) { return !(*this == rhs); }
+    bool operator!=(ParamReceiveSpec rhs) const { return !(*this == rhs); }
 
-    int totalReceived() { return num_args + (takes_varargs ? 1 : 0) + (takes_kwargs ? 1 : 0); }
-    int kwargsIndex() { return num_args + (takes_varargs ? 1 : 0); }
+    int totalReceived() const { return num_args + (takes_varargs ? 1 : 0) + (takes_kwargs ? 1 : 0); }
+    int varargsIndex() const { return num_args; }
+    int kwargsIndex() const { return num_args + (takes_varargs ? 1 : 0); }
 };
 
 // Inline-caches contain fastpath code, and need to know that their fastpath is valid for a particular set

src/runtime/capi.cpp

@@ -1797,85 +1797,82 @@ Box* BoxedCApiFunction::tppCall(Box* _self, CallRewriteArgs* rewrite_args, ArgPa
         // so we could just rearrangeArguments to the form that it wants and then call tp_new directly.
     }
 
-    bool rewrite_success = false;
-    rearrangeArguments(paramspec, NULL, self->method_def->ml_name, defaults, rewrite_args, rewrite_success, argspec,
-                       arg1, arg2, arg3, args, oargs, keyword_names, NULL);
-
-    AUTO_DECREF_ARGS(paramspec, arg1, arg2, arg3, oargs);
-
-    if (!rewrite_success)
-        rewrite_args = NULL;
-
-    RewriterVar* r_passthrough = NULL;
-    if (rewrite_args)
-        r_passthrough = rewrite_args->rewriter->loadConst((intptr_t)self->passthrough, Location::forArg(0));
-
-    Box* rtn;
-    if (flags == METH_VARARGS) {
-        rtn = (Box*)func(self->passthrough, arg1);
-        if (rewrite_args)
-            rewrite_args->out_rtn = rewrite_args->rewriter->call(true, (void*)func, r_passthrough, rewrite_args->arg1)
-                                        ->setType(RefType::OWNED);
-    } else if (flags == (METH_VARARGS | METH_KEYWORDS)) {
-        rtn = (Box*)((PyCFunctionWithKeywords)func)(self->passthrough, arg1, arg2);
-        if (rewrite_args)
-            rewrite_args->out_rtn = rewrite_args->rewriter->call(true, (void*)func, r_passthrough, rewrite_args->arg1,
-                                                                 rewrite_args->arg2)->setType(RefType::OWNED);
-    } else if (flags == METH_NOARGS) {
-        rtn = (Box*)func(self->passthrough, NULL);
-        if (rewrite_args)
-            rewrite_args->out_rtn
-                = rewrite_args->rewriter->call(true, (void*)func, r_passthrough,
-                                               rewrite_args->rewriter->loadConst(0, Location::forArg(1)))
-                      ->setType(RefType::OWNED);
-    } else if (flags == METH_O) {
-        rtn = (Box*)func(self->passthrough, arg1);
+    auto continuation = [=](CallRewriteArgs* rewrite_args, Box* arg1, Box* arg2, Box* arg3, Box** args) {
+        RewriterVar* r_passthrough = NULL;
         if (rewrite_args)
-            rewrite_args->out_rtn = rewrite_args->rewriter->call(true, (void*)func, r_passthrough, rewrite_args->arg1)
-                                        ->setType(RefType::OWNED);
-    } else if ((flags & ~(METH_O3 | METH_D3)) == 0) {
-        assert(paramspec.totalReceived() <= 3); // would need to pass through oargs
-        rtn = ((Box * (*)(Box*, Box*, Box*, Box*))func)(self->passthrough, arg1, arg2, arg3);
-        if (rewrite_args) {
-            if (paramspec.totalReceived() == 1)
+            r_passthrough = rewrite_args->rewriter->loadConst((intptr_t)self->passthrough, Location::forArg(0));
+
+        Box* rtn;
+        if (flags == METH_VARARGS) {
+            rtn = (Box*)func(self->passthrough, arg1);
+            if (rewrite_args)
                 rewrite_args->out_rtn = rewrite_args->rewriter->call(true, (void*)func, r_passthrough,
                                                                      rewrite_args->arg1)->setType(RefType::OWNED);
-            else if (paramspec.totalReceived() == 2)
+        } else if (flags == (METH_VARARGS | METH_KEYWORDS)) {
+            rtn = (Box*)((PyCFunctionWithKeywords)func)(self->passthrough, arg1, arg2);
+            if (rewrite_args)
                 rewrite_args->out_rtn
                     = rewrite_args->rewriter->call(true, (void*)func, r_passthrough, rewrite_args->arg1,
                                                    rewrite_args->arg2)->setType(RefType::OWNED);
-            else if (paramspec.totalReceived() == 3)
+        } else if (flags == METH_NOARGS) {
+            rtn = (Box*)func(self->passthrough, NULL);
+            if (rewrite_args)
                 rewrite_args->out_rtn
-                    = rewrite_args->rewriter->call(true, (void*)func, r_passthrough, rewrite_args->arg1,
-                                                   rewrite_args->arg2, rewrite_args->arg3)->setType(RefType::OWNED);
-            else
-                abort();
+                    = rewrite_args->rewriter->call(true, (void*)func, r_passthrough,
+                                                   rewrite_args->rewriter->loadConst(0, Location::forArg(1)))
+                          ->setType(RefType::OWNED);
+        } else if (flags == METH_O) {
+            rtn = (Box*)func(self->passthrough, arg1);
+            if (rewrite_args)
+                rewrite_args->out_rtn = rewrite_args->rewriter->call(true, (void*)func, r_passthrough,
+                                                                     rewrite_args->arg1)->setType(RefType::OWNED);
+        } else if ((flags & ~(METH_O3 | METH_D3)) == 0) {
+            assert(paramspec.totalReceived() <= 3); // would need to pass through oargs
+            rtn = ((Box * (*)(Box*, Box*, Box*, Box*))func)(self->passthrough, arg1, arg2, arg3);
+            if (rewrite_args) {
+                if (paramspec.totalReceived() == 1)
+                    rewrite_args->out_rtn = rewrite_args->rewriter->call(true, (void*)func, r_passthrough,
+                                                                         rewrite_args->arg1)->setType(RefType::OWNED);
+                else if (paramspec.totalReceived() == 2)
+                    rewrite_args->out_rtn
+                        = rewrite_args->rewriter->call(true, (void*)func, r_passthrough, rewrite_args->arg1,
+                                                       rewrite_args->arg2)->setType(RefType::OWNED);
+                else if (paramspec.totalReceived() == 3)
+                    rewrite_args->out_rtn
+                        = rewrite_args->rewriter->call(true, (void*)func, r_passthrough, rewrite_args->arg1,
+                                                       rewrite_args->arg2, rewrite_args->arg3)->setType(RefType::OWNED);
+                else
+                    abort();
+            }
+        } else if (flags == METH_OLDARGS) {
+            /* the really old style */
+
+            rewrite_args = NULL;
+
+            int size = PyTuple_GET_SIZE(arg1);
+            Box* arg = arg1;
+            if (size == 1)
+                arg = PyTuple_GET_ITEM(arg1, 0);
+            else if (size == 0)
+                arg = NULL;
+            rtn = func(self->passthrough, arg);
+        } else {
+            RELEASE_ASSERT(0, "0x%x", flags);
         }
-    } else if (flags == METH_OLDARGS) {
-        /* the really old style */
 
-        rewrite_args = NULL;
-
-        int size = PyTuple_GET_SIZE(arg1);
-        Box* arg = arg1;
-        if (size == 1)
-            arg = PyTuple_GET_ITEM(arg1, 0);
-        else if (size == 0)
-            arg = NULL;
-        rtn = func(self->passthrough, arg);
-    } else {
-        RELEASE_ASSERT(0, "0x%x", flags);
-    }
+        if (rewrite_args) {
+            rewrite_args->rewriter->checkAndThrowCAPIException(rewrite_args->out_rtn);
+            rewrite_args->out_success = true;
+        }
 
-    if (rewrite_args) {
-        rewrite_args->rewriter->checkAndThrowCAPIException(rewrite_args->out_rtn);
-        rewrite_args->out_success = true;
-    }
+        if (S == CXX && !rtn)
+            throwCAPIException();
+        assert(rtn && "should have set + thrown an exception!");
+        return rtn;
+    };
 
-    if (!rtn)
-        throwCAPIException();
-    assert(rtn && "should have set + thrown an exception!");
-    return rtn;
+    return rearrangeArgumentsAndCall(paramspec, NULL, self->method_def->ml_name, defaults, rewrite_args, argspec, arg1,
+                                     arg2, arg3, args, keyword_names, continuation);
 }
 
 /* extension modules might be compiled with GC support so these

src/runtime/generator.cpp

@@ -429,11 +429,14 @@ extern "C" BoxedGenerator::BoxedGenerator(BoxedFunctionBase* function, Box* arg1
 #endif
 {
     Py_INCREF(function);
-    Py_XINCREF(arg1);
-    Py_XINCREF(arg2);
-    Py_XINCREF(arg3);
 
     int numArgs = function->md->numReceivedArgs();
+    if (numArgs > 0)
+        Py_XINCREF(arg1);
+    if (numArgs > 1)
+        Py_XINCREF(arg2);
+    if (numArgs > 2)
+        Py_XINCREF(arg3);
     if (numArgs > 3) {
         numArgs -= 3;
         this->args = new (numArgs) GCdArray();
@@ -631,15 +634,16 @@ static void generator_dealloc(BoxedGenerator* self) noexcept {
 
     int numArgs = self->function->md->numReceivedArgs();
     if (numArgs > 3) {
-        numArgs -= 3;
-        for (int i = 0; i < numArgs; i++) {
+        for (int i = 0; i < numArgs - 3; i++) {
             Py_CLEAR(self->args->elts[i]);
         }
     }
-
-    Py_CLEAR(self->arg1);
-    Py_CLEAR(self->arg2);
-    Py_CLEAR(self->arg3);
+    if (numArgs > 2)
+        Py_CLEAR(self->arg3);
+    if (numArgs > 1)
+        Py_CLEAR(self->arg2);
+    if (numArgs > 0)
+        Py_CLEAR(self->arg1);
 
     Py_CLEAR(self->function);
 
@@ -667,15 +671,16 @@ static int generator_traverse(BoxedGenerator* self, visitproc visit, void* arg)
 
     int numArgs = self->function->md->numReceivedArgs();
     if (numArgs > 3) {
-        numArgs -= 3;
-        for (int i = 0; i < numArgs; i++) {
+        for (int i = 0; i < numArgs - 3; i++) {
             Py_VISIT(self->args->elts[i]);
         }
     }
-
-    Py_VISIT(self->arg1);
-    Py_VISIT(self->arg2);
-    Py_VISIT(self->arg3);
+    if (numArgs > 2)
+        Py_VISIT(self->arg3);
+    if (numArgs > 1)
+        Py_VISIT(self->arg2);
+    if (numArgs > 0)
+        Py_VISIT(self->arg1);
 
     Py_VISIT(self->function);
 

src/runtime/rewrite_args.h

@@ -292,26 +292,28 @@ struct CompareRewriteArgs {
         : rewriter(rewriter), lhs(lhs), rhs(rhs), destination(destination), out_success(false), out_rtn(NULL) {}
 };
 
-// Passes the output arguments back through oarg.  Passes the rewrite success by setting rewrite_success.
-// Directly modifies rewrite_args args in place, but only if rewrite_success got set.
-// oargs needs to be pre-allocated by the caller, since it's assumed that they will want to use alloca.
+using FunctorPointer
+    = llvm::function_ref<Box*(CallRewriteArgs* rewrite_args, Box* arg1, Box* arg2, Box* arg3, Box** args)>;
+
+// rearrangeArgumentsAndCall maps from a given set of arguments (the structure specified by an ArgPassSpec) to the
+// parameter form than the receiving function expects (given by the ParamReceiveSpec).  After it does this, it will
+// call `continuation` and return the result.
+//
 // The caller is responsible for guarding for paramspec, argspec, param_names, and defaults.
-// TODO Fix this function's signature.  should we pass back out through args?  the common case is that they
-// match anyway.  Or maybe it should call a callback function, which could save on the common case.
 //
-// Reference semantics: takes borrowed references, and everything written out is an owned reference.
-template <Rewritable rewritable = REWRITABLE>
-void rearrangeArguments(ParamReceiveSpec paramspec, const ParamNames* param_names, const char* func_name,
-                        Box** defaults, _CallRewriteArgsBase* rewrite_args, bool& rewrite_success, ArgPassSpec argspec,
-                        Box*& arg1, Box*& arg2, Box*& arg3, Box** args, Box** oargs,
-                        const std::vector<BoxedString*>* keyword_names, bool* oargs_owned);
+// rearrangeArgumentsAndCall supports both CAPI- and CXX- exception styles for continuation, and will propagate them
+// back to the caller.  For now, it can also throw its own exceptions such as "not enough arguments", and will throw
+// them in the CXX style.
+Box* rearrangeArgumentsAndCall(ParamReceiveSpec paramspec, const ParamNames* param_names, const char* func_name,
+                               Box** defaults, CallRewriteArgs* rewrite_args, ArgPassSpec argspec, Box* arg1, Box* arg2,
+                               Box* arg3, Box** args, const std::vector<BoxedString*>* keyword_names,
+                               FunctorPointer continuation);
 
 // new_args should be allocated by the caller if at least three args get passed in.
 // rewrite_args will get modified in place.
 ArgPassSpec bindObjIntoArgs(Box* bind_obj, RewriterVar* r_bind_obj, _CallRewriteArgsBase* rewrite_args,
                             ArgPassSpec argspec, Box*& arg1, Box*& arg2, Box*& arg3, Box** args, Box** new_args);
 
-void decrefOargs(RewriterVar* oargs, bool* oargs_owned, int oargs_size);
 } // namespace pyston
 
 #endif

src/runtime/types.h

@@ -413,41 +413,6 @@ public:
 #define AUTO_DECREF_ARRAY(x, size) AutoDecrefArray<false> CAT(_autodecref_, __LINE__)((x), (size))
 #define AUTO_XDECREF_ARRAY(x, size) AutoDecrefArray<true> CAT(_autodecref_, __LINE__)((x), (size))
 
-class AutoDecrefArgs {
-private:
-    int num_args;
-    Box* arg1, *arg2, *arg3;
-    Box** args;
-
-public:
-    AutoDecrefArgs(int num_args, Box* arg1, Box* arg2, Box* arg3, Box** args)
-        : num_args(num_args), arg1(arg1), arg2(arg2), arg3(arg3), args(args) {}
-    AutoDecrefArgs(ParamReceiveSpec paramspec, Box* arg1, Box* arg2, Box* arg3, Box** args)
-        : num_args(paramspec.totalReceived()), arg1(arg1), arg2(arg2), arg3(arg3), args(args) {}
-
-    ~AutoDecrefArgs() {
-        // TODO Minor optimization: only the last arg (kwargs) is allowed to be NULL.
-        switch (num_args) {
-            default:
-                for (int i = 0; i < num_args - 3; i++) {
-                    Py_XDECREF(args[i]);
-                }
-            case 3:
-                Py_XDECREF(arg3);
-            case 2:
-                Py_XDECREF(arg2);
-            case 1:
-                Py_XDECREF(arg1);
-            case 0:
-                break;
-        }
-    }
-};
-// Note: this captures the first three args by value (like AUTO_DECREF) but the array by reference.
-// You can also pass a ParamReceiveSpec instead of an int for num_args
-#define AUTO_DECREF_ARGS(num_args, arg1, arg2, arg3, args)                                                             \
-    AutoDecrefArgs CAT(_autodecref_, __LINE__)((num_args), (arg1), (arg2), (arg3), (args))
-
 template <typename B> B* incref(B* b) {
     Py_INCREF(b);
     return b;
@@ -457,6 +422,32 @@ template <typename B> B* xincref(B* b) {
     return b;
 }
 
+// Helper function: calls a CXX-style function from a templated function.  This is more efficient than the
+// easier-to-type version:
+//
+// try {
+//     return f();
+// } catch (ExcInfo e) {
+//     if (S == CAPI) {
+//         setCAPIException(e);
+//         return NULL;
+//     } else
+//         throw e;
+// }
+//
+// since this version does not need the try-catch block when called from a CXX-style function
+template <ExceptionStyle S, typename Functor> Box* callCXXFromStyle(Functor f) {
+    if (S == CAPI) {
+        try {
+            return f();
+        } catch (ExcInfo e) {
+            setCAPIException(e);
+            return NULL;
+        }
+    } else
+        return f();
+}
+
 //#define DISABLE_INT_FREELIST
 
 extern "C" int PyInt_ClearFreeList() noexcept;

test/tests/arg_refs.py

+# Tests to see if we add any extra refs to function arguments.
+
+import sys
+print sys.getrefcount(object())
+
+def f(o):
+    print sys.getrefcount(o)
+
+# This gives 3 for CPython and our interpreter, but 2 for the llvm tier:
+# f(object())