Actually implement rearrangeArgumentsAndCall

And add an optimization that in the fast paths, we don't incref any args.

src/capi/typeobject.cpp

@@ -3436,21 +3436,15 @@ static Box* tppProxyToTpCall(Box* self, CallRewriteArgs* rewrite_args, ArgPassSp
         }
 
         Box* r = self->cls->tp_call(self, arg1, arg2);
-        if (!r)
+        if (S == CXX && !r)
             throwCAPIException();
         return r;
     };
 
-    try {
+    return callCXXFromStyle<S>([&]() {
         return rearrangeArgumentsAndCall(paramspec, NULL, "", NULL, rewrite_args, argspec, arg1, arg2, arg3, args,
                                          keyword_names, continuation);
-    } catch (ExcInfo e) {
-        if (S == CAPI) {
-            setCAPIException(e);
-            return NULL;
-        } else
-            throw e;
-    }
+    });
 }
 
 extern "C" void PyType_RequestHcAttrs(PyTypeObject* cls, int offset) noexcept {

src/core/types.h

@@ -270,6 +270,7 @@ struct ParamReceiveSpec {
     bool operator!=(ParamReceiveSpec rhs) const { return !(*this == rhs); }
 
     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); }
 };
 

src/runtime/builtin_modules/builtins.cpp

@@ -578,10 +578,17 @@ Box* getattrFuncInternal(BoxedFunctionBase* func, CallRewriteArgs* rewrite_args,
             }
         }
 
-        _str = coerceUnicodeToStr<CXX>(_str);
+        _str = coerceUnicodeToStr<S>(_str);
+
+        if (S == CAPI && !_str)
+            return (Box*)NULL;
 
         if (!PyString_Check(_str)) {
             Py_DECREF(_str);
+            if (S == CAPI) {
+                PyErr_SetString(TypeError, "getattr(): attribute name must be string");
+                return (Box*)NULL;
+            } else
                 raiseExcHelper(TypeError, "getattr(): attribute name must be string");
         }
 
@@ -634,21 +641,15 @@ Box* getattrFuncInternal(BoxedFunctionBase* func, CallRewriteArgs* rewrite_args,
         }
 
         Box* r = getattrFuncHelper(rtn, obj, str, default_value);
-        if (!r)
+        if (S == CXX && !r)
             throwCAPIException();
         return r;
     };
 
-    try {
+    return callCXXFromStyle<S>([&]() {
         return rearrangeArgumentsAndCall(ParamReceiveSpec(3, 1, false, false), NULL, "getattr", defaults, rewrite_args,
                                          argspec, arg1, arg2, arg3, args, keyword_names, continuation);
-    } catch (ExcInfo e) {
-        if (S == CAPI) {
-            setCAPIException(e);
-            return NULL;
-        }
-        throw e;
-    }
+    });
 }
 
 Box* setattrFunc(Box* obj, Box* _str, Box* value) {
@@ -709,10 +710,17 @@ Box* hasattrFuncInternal(BoxedFunctionBase* func, CallRewriteArgs* rewrite_args,
             }
         }
 
-        _str = coerceUnicodeToStr<CXX>(_str);
+        _str = coerceUnicodeToStr<S>(_str);
+
+        if (S == CAPI && !_str)
+            return (Box*)NULL;
 
         if (!PyString_Check(_str)) {
             Py_DECREF(_str);
+            if (S == CAPI) {
+                PyErr_SetString(TypeError, "hasattr(): attribute name must be string");
+                return (Box*)NULL;
+            } else
                 raiseExcHelper(TypeError, "hasattr(): attribute name must be string");
         }
 
@@ -760,21 +768,15 @@ Box* hasattrFuncInternal(BoxedFunctionBase* func, CallRewriteArgs* rewrite_args,
         }
 
         Box* r = hasattrFuncHelper(rtn);
-        if (!r)
+        if (S == CXX && !r)
             throwCAPIException();
         return r;
     };
 
-    try {
+    return callCXXFromStyle<S>([&]() {
         return rearrangeArgumentsAndCall(ParamReceiveSpec(2, 0, false, false), NULL, "hasattr", NULL, rewrite_args,
                                          argspec, arg1, arg2, arg3, args, keyword_names, continuation);
-    } catch (ExcInfo e) {
-        if (S == CAPI) {
-            setCAPIException(e);
-            return NULL;
-        }
-        throw e;
-    }
+    });
 }
 
 Box* map2(Box* f, Box* container) {

src/runtime/descr.cpp

@@ -695,13 +695,15 @@ Box* BoxedWrapperDescriptor::tppCall(Box* _self, CallRewriteArgs* rewrite_args,
             RELEASE_ASSERT(0, "%d", flags);
         }
 
-        if (!rtn)
+        if (S == CXX && !rtn)
             throwCAPIException();
         return rtn;
     };
 
+    return callCXXFromStyle<S>([&]() {
         return rearrangeArgumentsAndCall(paramspec, NULL, self->wrapper->name.data(), NULL, rewrite_args, argspec, arg1,
                                          arg2, arg3, args, keyword_names, continuation);
+    });
 }
 
 static Box* wrapperdescrGetDoc(Box* b, void*) {

src/runtime/objmodel.cpp

@@ -4120,20 +4120,11 @@ public:
     }
 };
 
-void decrefOargs(RewriterVar* oargs, bool* oargs_owned, int num_oargs) {
-    for (int i = 0; i < num_oargs; i++) {
-        if (oargs_owned[i]) {
-            oargs->deregisterOwnedAttr(i * sizeof(Box*));
-            oargs->getAttr(i * sizeof(Box*))->setType(RefType::OWNED);
-        }
-    }
-}
-
 template <Rewritable rewritable, typename FuncNameCB>
-void rearrangeArgumentsInternal(ParamReceiveSpec paramspec, const ParamNames* param_names, FuncNameCB func_name_cb,
-                                Box** defaults, CallRewriteArgs* rewrite_args, bool& rewrite_success,
-                                ArgPassSpec argspec, Box*& oarg1, Box*& oarg2, Box*& oarg3, Box** args, Box** oargs,
-                                const std::vector<BoxedString*>* keyword_names, bool* oargs_owned) {
+Box* rearrangeArgumentsAndCallInternal(ParamReceiveSpec paramspec, const ParamNames* param_names,
+                                       FuncNameCB func_name_cb, Box** defaults, CallRewriteArgs* rewrite_args,
+                                       ArgPassSpec argspec, Box* arg1, Box* arg2, Box* arg3, Box** args,
+                                       const std::vector<BoxedString*>* keyword_names, FunctorPointer continuation) {
     if (rewritable == NOT_REWRITABLE) {
         assert(!rewrite_args);
         rewrite_args = NULL;
@@ -4147,44 +4138,17 @@ void rearrangeArgumentsInternal(ParamReceiveSpec paramspec, const ParamNames* pa
      * - error about missing parameters
      */
 
-    int num_output_args = paramspec.totalReceived();
-    int num_passed_args = argspec.totalPassed();
-
-    assert((oargs != NULL) == (num_output_args > 3));
     assert((defaults != NULL) == (paramspec.num_defaults != 0));
 
-    if (rewrite_args && oargs) {
-        assert(oargs_owned);
-        memset(oargs_owned, 0, (num_output_args - 3) * sizeof(bool));
-    }
-
-    if (rewrite_args) {
-        rewrite_success = false; // default case
-    }
-
-    auto propagate_args = [&]() {
-        if (num_output_args >= 1)
-            Py_XINCREF(oarg1);
-        if (num_output_args >= 2)
-            Py_XINCREF(oarg2);
-        if (num_output_args >= 3)
-            Py_XINCREF(oarg3);
-        if (num_output_args >= 3) {
-            memcpy(oargs, args, sizeof(Box*) * (num_output_args - 3));
-            for (int i = 0; i < num_output_args - 3; i++) {
-                Py_XINCREF(oargs[i]);
-            }
-        }
-    };
-
     // Super fast path:
     if (argspec.num_keywords == 0 && !argspec.has_starargs && !paramspec.takes_varargs && !argspec.has_kwargs
         && argspec.num_args == paramspec.num_args && !paramspec.takes_kwargs) {
-        rewrite_success = true;
-        propagate_args();
-        return;
+        return continuation(rewrite_args, arg1, arg2, arg3, args);
     }
 
+    int num_output_args = paramspec.totalReceived();
+    int num_passed_args = argspec.totalPassed();
+
     // Fast path: if it's a simple-enough call, we don't have to do anything special.  On a simple
     // django-admin test this covers something like 93% of all calls to callFunc.
     if (argspec.num_keywords == 0 && argspec.has_starargs == paramspec.takes_varargs && !argspec.has_kwargs
@@ -4195,7 +4159,7 @@ void rearrangeArgumentsInternal(ParamReceiveSpec paramspec, const ParamNames* pa
             assert(num_output_args == num_passed_args + 1);
             int idx = paramspec.kwargsIndex();
             assert(idx < 3);
-            getArg(idx, oarg1, oarg2, oarg3, NULL) = NULL; // pass NULL for kwargs
+            getArg(idx, arg1, arg2, arg3, NULL) = NULL; // pass NULL for kwargs
             if (rewrite_args) {
                 if (idx == 0)
                     rewrite_args->arg1 = rewrite_args->rewriter->loadConst(0)->setType(RefType::BORROWED);
@@ -4215,31 +4179,28 @@ void rearrangeArgumentsInternal(ParamReceiveSpec paramspec, const ParamNames* pa
         // received by the caller are always tuples).
         // This is why we can't pass kwargs here.
         if (argspec.has_starargs) {
-            Box* given_varargs = getArg(argspec.num_args + argspec.num_keywords, oarg1, oarg2, oarg3, args);
+            Box* given_varargs = getArg(argspec.num_args + argspec.num_keywords, arg1, arg2, arg3, args);
             if (given_varargs->cls == tuple_cls) {
                 if (rewrite_args) {
                     getArg(argspec.num_args + argspec.num_keywords, rewrite_args)
                         ->addAttrGuard(offsetof(Box, cls), (intptr_t)tuple_cls);
                 }
-                rewrite_success = true;
-                propagate_args();
-                return;
+                return continuation(rewrite_args, arg1, arg2, arg3, args);
             }
         } else {
-            rewrite_success = true;
-            propagate_args();
-            return;
+            return continuation(rewrite_args, arg1, arg2, arg3, args);
         }
     }
 
-    // Save the original values:
-    Box* arg1 = oarg1;
-    Box* arg2 = oarg2;
-    Box* arg3 = oarg3;
+    Box* oarg1, *oarg2, *oarg3;
     oarg1 = oarg2 = oarg3 = NULL;
 
-    if (oargs)
+    Box** oargs = NULL;
+    // TODO: could we reuse the args array?
+    if (num_output_args > 3) {
+        oargs = (Box**)alloca(sizeof(Box*) * (num_output_args - 3));
         memset(oargs, 0, sizeof(Box*) * (num_output_args - 3));
+    }
 
     // Clear any increfs we did for when we throw an exception:
     auto clear_refs = [&]() {
@@ -4268,9 +4229,6 @@ void rearrangeArgumentsInternal(ParamReceiveSpec paramspec, const ParamNames* pa
     // At this point we are not allowed to abort the rewrite any more, since we will start
     // modifying rewrite_args.
 
-    if (rewrite_args)
-        rewrite_success = true;
-
     if (rewrite_args) {
         // We might have trouble if we have more output args than input args,
         // such as if we need more space to pass defaults.
@@ -4346,6 +4304,7 @@ void rearrangeArgumentsInternal(ParamReceiveSpec paramspec, const ParamNames* pa
         unused_positional.push_back(PySequence_Fast_GET_ITEM(varargs, i));
     }
 
+    bool varargs_owned = false;
     if (paramspec.takes_varargs) {
         int varargs_idx = paramspec.num_args;
         if (rewrite_args) {
@@ -4354,7 +4313,6 @@ void rearrangeArgumentsInternal(ParamReceiveSpec paramspec, const ParamNames* pa
 
             RewriterVar* varargs_val;
             int varargs_size = unused_positional_rvars.size();
-            bool is_owned = false;
 
             if (varargs_size == 0) {
                 varargs_val
@@ -4374,7 +4332,7 @@ void rearrangeArgumentsInternal(ParamReceiveSpec paramspec, const ParamNames* pa
                 };
                 varargs_val = rewrite_args->rewriter->call(true, create_ptrs[varargs_size], unused_positional_rvars)
                                   ->setType(RefType::OWNED);
-                is_owned = true;
+                varargs_owned = true;
             }
 
             if (varargs_val) {
@@ -4385,13 +4343,12 @@ void rearrangeArgumentsInternal(ParamReceiveSpec paramspec, const ParamNames* pa
                 if (varargs_idx == 2)
                     rewrite_args->arg3 = varargs_val;
                 if (varargs_idx >= 3) {
-                    if (is_owned) {
+                    if (varargs_owned) {
                         rewrite_args->args->registerOwnedAttr((varargs_idx - 3) * sizeof(Box*));
 
                         rewrite_args->args->setAttr((varargs_idx - 3) * sizeof(Box*), varargs_val,
                                                     RewriterVar::SetattrType::HANDED_OFF);
 
-                        oargs_owned[varargs_idx - 3] = true;
                         varargs_val->refConsumed();
                     } else {
                         rewrite_args->args->setAttr((varargs_idx - 3) * sizeof(Box*), varargs_val);
@@ -4607,57 +4564,21 @@ void rearrangeArgumentsInternal(ParamReceiveSpec paramspec, const ParamNames* pa
         getArg(arg_idx, oarg1, oarg2, oarg3, oargs) = xincref(default_obj);
     }
 
-    cleanup.cancel();
-}
-
+    if (rewrite_args && varargs_owned) {
+        Box* r = continuation(rewrite_args, oarg1, oarg2, oarg3, oargs);
 
-// TODO: implement this for real
-template <Rewritable rewritable, typename FuncNameCB>
-Box* rearrangeArgumentsAndCallInternal(ParamReceiveSpec paramspec, const ParamNames* param_names,
-                                       FuncNameCB func_name_cb, Box** defaults, CallRewriteArgs* rewrite_args,
-                                       ArgPassSpec argspec, Box* arg1, Box* arg2, Box* arg3, Box** args,
-                                       const std::vector<BoxedString*>* keyword_names, FunctorPointer continuation) {
-    Box** oargs = NULL;
-    bool* oargs_owned = NULL;
-    if (paramspec.totalReceived() > 3) {
-        oargs = (Box**)alloca(sizeof(Box*) * (paramspec.totalReceived() - 3));
-        oargs_owned = (bool*)alloca(sizeof(bool) * (paramspec.totalReceived() - 3));
+        int varargs_idx = paramspec.varargsIndex();
+        if (varargs_idx >= 3) {
+            rewrite_args->args->deregisterOwnedAttr((varargs_idx - 3) * sizeof(Box*));
+            rewrite_args->args->getAttr((varargs_idx - 3) * sizeof(Box*))->setType(RefType::OWNED);
         }
 
-    bool rewrite_success = false;
-    rearrangeArgumentsInternal<rewritable>(paramspec, param_names, func_name_cb, defaults, rewrite_args,
-                                           rewrite_success, argspec, arg1, arg2, arg3, args, oargs, keyword_names,
-                                           oargs_owned);
-
-    AUTO_DECREF_ARGS(paramspec, arg1, arg2, arg3, oargs);
-
-    if (!rewrite_success)
-        rewrite_args = NULL;
-
-    Box* r = continuation(rewrite_args, arg1, arg2, arg3, oargs);
-
-    if (rewrite_args)
-        decrefOargs(rewrite_args->args, oargs_owned, paramspec.totalReceived() - 3);
-
         return r;
+    } else {
+        return continuation(rewrite_args, oarg1, oarg2, oarg3, oargs);
+    }
 }
 
-template <Rewritable rewritable>
-void rearrangeArguments(ParamReceiveSpec paramspec, const ParamNames* param_names, const char* func_name,
-                        Box** defaults, CallRewriteArgs* 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) {
-    auto func = [func_name]() { return func_name; };
-    return rearrangeArgumentsInternal<rewritable>(paramspec, param_names, func, defaults, rewrite_args, rewrite_success,
-                                                  argspec, arg1, arg2, arg3, args, oargs, keyword_names, oargs_owned);
-}
-template void rearrangeArguments<REWRITABLE>(ParamReceiveSpec, const ParamNames*, const char*, Box**, CallRewriteArgs*,
-                                             bool&, ArgPassSpec, Box*&, Box*&, Box*&, Box**, Box**,
-                                             const std::vector<BoxedString*>*, bool*);
-template void rearrangeArguments<NOT_REWRITABLE>(ParamReceiveSpec, const ParamNames*, const char*, Box**,
-                                                 CallRewriteArgs*, bool&, ArgPassSpec, Box*&, Box*&, Box*&, Box**,
-                                                 Box**, const std::vector<BoxedString*>*, bool*);
-
 template <Rewritable rewritable>
 Box* rearrangeArgumentsAndCall(ParamReceiveSpec paramspec, const ParamNames* param_names, const char* func_name,
                                Box** defaults, CallRewriteArgs* rewrite_args, ArgPassSpec argspec, Box* arg1, Box* arg2,
@@ -4748,19 +4669,14 @@ Box* callFunc(BoxedFunctionBase* func, CallRewriteArgs* rewrite_args, ArgPassSpe
         return res;
     };
 
-    Box* r;
-    try {
     auto func_name_cb = [md]() { return getFunctionName(md).data(); };
-        r = rearrangeArgumentsAndCallInternal<rewritable>(
+    Box* r = callCXXFromStyle<S>([&] {
+        return rearrangeArgumentsAndCallInternal<rewritable>(
             paramspec, &md->param_names, func_name_cb, paramspec.num_defaults ? func->defaults->elts : NULL,
             rewrite_args, argspec, arg1, arg2, arg3, args, keyword_names, continuation);
-    } catch (ExcInfo e) {
-        if (S == CAPI) {
-            setCAPIException(e);
+    });
+    if (S == CAPI && !r)
         return NULL;
-        } else
-            throw e;
-    }
 
     if (rearrange_rewrite_failed) {
 // If we weren't able to rewrite, at least rewrite to callFunc, which helps a little bit.

src/runtime/types.cpp

@@ -1216,16 +1216,10 @@ static Box* typeCallInner(CallRewriteArgs* rewrite_args, ArgPassSpec argspec, Bo
             return InitHelper::call(made, cls, arg2, arg3);
         };
 
-        try {
+        return callCXXFromStyle<S>([&]() {
             return rearrangeArgumentsAndCall(ParamReceiveSpec(1, 0, true, true), NULL, "", NULL, rewrite_args, argspec,
                                              made, arg2, arg3, args, keyword_names, continuation);
-        } catch (ExcInfo e) {
-            if (S == CAPI) {
-                setCAPIException(e);
-                return NULL;
-            } else
-                throw e;
-        }
+        });
     }
 
     // If __new__ returns a subclass, supposed to call that subclass's __init__.
@@ -1330,23 +1324,34 @@ static Box* typeCallInner(CallRewriteArgs* rewrite_args, ArgPassSpec argspec, Bo
 
                 int err = tpinit(made, arg2, arg3);
 
-                if (err == -1) {
-                    throwCAPIException();
-                }
+                if (err == -1)
                     return (Box*)NULL;
+
+                if (rewrite_args) {
+                    auto r_err = rewrite_args->rewriter->call(true, (void*)tpinit, r_made, rewrite_args->arg2,
+                                                              rewrite_args->arg3);
+
+                    assert(S == CXX && "this need to be converted");
+                    rewrite_args->rewriter->checkAndThrowCAPIException(r_err, -1, assembler::MovType::L);
+                    rewrite_args->out_success = true;
+                }
+                return (Box*)1;
             };
 
+            Box* _t;
             try {
-                Box* _t = rearrangeArgumentsAndCall(ParamReceiveSpec(1, 0, true, true), NULL, "", NULL, rewrite_args,
+                _t = rearrangeArgumentsAndCall(ParamReceiveSpec(1, 0, true, true), NULL, "", NULL, rewrite_args,
                                                argspec, made, arg2, arg3, args, keyword_names, continuation);
-                assert(_t == NULL);
             } catch (ExcInfo e) {
-                Py_DECREF(made);
-                if (S == CAPI) {
                 setCAPIException(e);
+                _t = NULL;
+            }
+            if (_t == NULL) {
+                Py_DECREF(made);
+                if (S == CAPI)
                     return NULL;
-                } else
-                    throw e;
+                else
+                    throwCAPIException();
             }
 
             if (rewrite_args) {
@@ -1355,14 +1360,6 @@ static Box* typeCallInner(CallRewriteArgs* rewrite_args, ArgPassSpec argspec, Bo
                 else
                     rewrite_args->out_success = false;
             }
-
-            if (rewrite_args) {
-                auto r_err
-                    = rewrite_args->rewriter->call(true, (void*)tpinit, r_made, rewrite_args->arg2, rewrite_args->arg3);
-
-                assert(S == CXX && "this need to be converted");
-                rewrite_args->rewriter->checkAndThrowCAPIException(r_err, -1, assembler::MovType::L);
-            }
         }
     } else {
         if (new_attr == NULL && npassed_args != 1) {

src/runtime/types.h

@@ -422,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;