Add more asserts to aggressively enforce return conventions

You can't get the return value from a RewriteArgs without
looking at the return convention.  Debug mode will check the
return convention during the execution of rewrites.

Split "VALID_RETURN" into "HAS_RETURN" and "CAPI_RETURN" since some
places used it to mean either.

Hopefully this helps keep things clean.  There are a lot of places that
were implictly assuming a certain return convention, but now that they
have to explicitly assume it, the issues are more obvious.  Plus
they will get checked in the debug builds.

Also tried to fix things up while going through and doing this refactoring;
I think I found a number of issues.

src/asm_writing/rewriter.h

@@ -277,6 +277,8 @@ public:
         assert(rewriter);
     }
 
+    Rewriter* getRewriter() { return rewriter; }
+
     friend class Rewriter;
     friend class JitFragmentWriter;
 };

src/capi/typeobject.cpp

@@ -908,7 +908,11 @@ Box* slotTpGetattrHookInternal(Box* self, BoxedString* name, GetattrRewriteArgs*
     getattr = typeLookup(self->cls, _getattr_str, NULL);
 
     if (getattr == NULL) {
-        assert(!rewrite_args || !rewrite_args->out_success);
+        if (rewrite_args) {
+            // Don't bother rewriting this case:
+            assert(!rewrite_args->isSuccessful());
+            rewrite_args = NULL;
+        }
 
         /* No __getattr__ hook: use a simpler dispatcher */
         self->cls->tp_getattro = slot_tp_getattro;
@@ -931,13 +935,12 @@ Box* slotTpGetattrHookInternal(Box* self, BoxedString* name, GetattrRewriteArgs*
         GetattrRewriteArgs grewrite_args(rewrite_args->rewriter, r_obj_cls, Location::any());
 
         getattribute = typeLookup(self->cls, _getattribute_str, &grewrite_args);
-        if (!grewrite_args.out_success)
+        if (!grewrite_args.isSuccessful())
             rewrite_args = NULL;
         else if (getattribute) {
-            assert(grewrite_args.out_return_convention == GetattrRewriteArgs::VALID_RETURN);
-            r_getattribute = grewrite_args.out_rtn;
+            r_getattribute = grewrite_args.getReturn(ReturnConvention::HAS_RETURN);
         } else {
-            assert(grewrite_args.out_return_convention == GetattrRewriteArgs::NO_RETURN);
+            grewrite_args.assertReturnConvention(ReturnConvention::NO_RETURN);
         }
     } else {
         getattribute = typeLookup(self->cls, _getattribute_str, NULL);
@@ -973,35 +976,38 @@ Box* slotTpGetattrHookInternal(Box* self, BoxedString* name, GetattrRewriteArgs*
                         throw e;
                 }
 
-                grewrite_args.out_success = false;
+                if (grewrite_args.isSuccessful()) {
+                    grewrite_args.getReturn(); // to make the asserts happy
+                    grewrite_args.clearReturn();
+                }
                 res = NULL;
             }
 
-            if (!grewrite_args.out_success)
+            if (!grewrite_args.isSuccessful())
                 rewrite_args = NULL;
-            else if (res) {
-                rewrite_args->out_rtn = grewrite_args.out_rtn;
-                rewrite_args->out_return_convention = grewrite_args.out_return_convention;
-            }
+            else {
+                RewriterVar* rtn;
+                ReturnConvention return_convention;
+                std::tie(rtn, return_convention) = grewrite_args.getReturn();
 
-            // Guarding here is a bit tricky, since we need to make sure that we call getattr
-            // (or not) at the right times.
-            // Right now this section is a bit conservative.
-            if (rewrite_args) {
-                if (grewrite_args.out_return_convention == GetattrRewriteArgs::NO_RETURN) {
-                    // Do nothing
-                } else if (grewrite_args.out_return_convention == GetattrRewriteArgs::VALID_RETURN) {
-                    // TODO we should have a HAS_RETURN that takes out the NULL case
+                if (return_convention == ReturnConvention::HAS_RETURN) {
                     assert(res);
-                    if (res)
-                        grewrite_args.out_rtn->addGuardNotEq(0);
-                    else
-                        grewrite_args.out_rtn->addGuard(0);
-                } else if (grewrite_args.out_return_convention == GetattrRewriteArgs::NOEXC_POSSIBLE) {
-                    // TODO maybe we could handle this
-                    rewrite_args = NULL;
+                    rewrite_args->setReturn(rtn, ReturnConvention::HAS_RETURN);
+                    return res;
+                } else if (return_convention == ReturnConvention::NO_RETURN) {
+                    assert(!res);
+                } else if (return_convention == ReturnConvention::CAPI_RETURN) {
+                    // If we get a CAPI return, we probably did a function call, and these guards
+                    // will probably just make the rewrite fail:
+                    if (res) {
+                        rtn->addGuardNotEq(0);
+                        rewrite_args->setReturn(rtn, ReturnConvention::HAS_RETURN);
+                        return res;
+                    } else
+                        rtn->addGuard(0);
                 } else {
-                    RELEASE_ASSERT(0, "%d", grewrite_args.out_return_convention);
+                    assert(return_convention == ReturnConvention::NOEXC_POSSIBLE);
+                    rewrite_args = NULL;
                 }
             }
         } else {
@@ -1044,8 +1050,7 @@ Box* slotTpGetattrHookInternal(Box* self, BoxedString* name, GetattrRewriteArgs*
     // doesn't exist.
 
     if (res) {
-        if (rewrite_args)
-            rewrite_args->out_success = true;
+        assert(!rewrite_args); // should have been handled already
         return res;
     }
 
@@ -1059,7 +1064,7 @@ Box* slotTpGetattrHookInternal(Box* self, BoxedString* name, GetattrRewriteArgs*
         // - we have no way of signalling that "we didn't get an attribute this time but that may be different
         //   in future executions through the IC".
         // I think this should only end up mattering anyway if the getattr site throws every single time.
-        CallRewriteArgs crewrite_args(rewrite_args->rewriter, rewrite_args->obj, rewrite_args->destination);
+        CallattrRewriteArgs crewrite_args(rewrite_args->rewriter, rewrite_args->obj, rewrite_args->destination);
         assert(PyString_CHECK_INTERNED(name) == SSTATE_INTERNED_IMMORTAL);
         crewrite_args.arg1 = rewrite_args->rewriter->loadConst((intptr_t)name, Location::forArg(1));
 
@@ -1067,11 +1072,10 @@ Box* slotTpGetattrHookInternal(Box* self, BoxedString* name, GetattrRewriteArgs*
                                   NULL, NULL, NULL, NULL);
         assert(res || S == CAPI);
 
-        if (!crewrite_args.out_success)
+        if (!crewrite_args.isSuccessful())
             rewrite_args = NULL;
         else {
-            rewrite_args->out_rtn = crewrite_args.out_rtn;
-            rewrite_args->out_return_convention = GetattrRewriteArgs::VALID_RETURN;
+            rewrite_args->setReturn(crewrite_args.getReturn());
         }
     } else {
         // TODO: we already fetched the getattr attribute, it would be faster to call it rather than do
@@ -1084,8 +1088,6 @@ Box* slotTpGetattrHookInternal(Box* self, BoxedString* name, GetattrRewriteArgs*
         assert(res || S == CAPI);
     }
 
-    if (rewrite_args)
-        rewrite_args->out_success = true;
     return res;
 }
 // Force instantiation of the template

src/runtime/builtin_modules/builtins.cpp

@@ -569,16 +569,15 @@ Box* getattrFuncInternal(BoxedFunctionBase* func, CallRewriteArgs* rewrite_args,
         GetattrRewriteArgs grewrite_args(rewrite_args->rewriter, rewrite_args->arg1, rewrite_args->destination);
         rtn = getattrInternal<CAPI>(obj, str, &grewrite_args);
         // TODO could make the return valid in the NOEXC_POSSIBLE case via a helper
-        if (!grewrite_args.out_success || grewrite_args.out_return_convention == GetattrRewriteArgs::NOEXC_POSSIBLE)
+        if (!grewrite_args.isSuccessful())
             rewrite_args = NULL;
         else {
-            if (!rtn && !PyErr_Occurred()) {
-                assert(grewrite_args.out_return_convention == GetattrRewriteArgs::NO_RETURN);
+            ReturnConvention return_convention;
+            std::tie(r_rtn, return_convention) = grewrite_args.getReturn();
+
+            // Convert to NOEXC_POSSIBLE:
+            if (return_convention == ReturnConvention::NO_RETURN)
                 r_rtn = rewrite_args->rewriter->loadConst(0);
-            } else {
-                assert(grewrite_args.out_return_convention == GetattrRewriteArgs::VALID_RETURN);
-                r_rtn = grewrite_args.out_rtn;
-            }
         }
     } else {
         rtn = getattrInternal<CAPI>(obj, str, NULL);
@@ -681,16 +680,15 @@ Box* hasattrFuncInternal(BoxedFunctionBase* func, CallRewriteArgs* rewrite_args,
     if (rewrite_args) {
         GetattrRewriteArgs grewrite_args(rewrite_args->rewriter, rewrite_args->arg1, rewrite_args->destination);
         rtn = getattrInternal<CAPI>(obj, str, &grewrite_args);
-        if (!grewrite_args.out_success || grewrite_args.out_return_convention == GetattrRewriteArgs::NOEXC_POSSIBLE)
+        if (!grewrite_args.isSuccessful())
             rewrite_args = NULL;
         else {
-            if (!rtn && !PyErr_Occurred()) {
-                assert(grewrite_args.out_return_convention == GetattrRewriteArgs::NO_RETURN);
+            ReturnConvention return_convention;
+            std::tie(r_rtn, return_convention) = grewrite_args.getReturn();
+
+            // Convert to NOEXC_POSSIBLE:
+            if (return_convention == ReturnConvention::NO_RETURN)
                 r_rtn = rewrite_args->rewriter->loadConst(0);
-            } else {
-                assert(grewrite_args.out_return_convention == GetattrRewriteArgs::VALID_RETURN);
-                r_rtn = grewrite_args.out_rtn;
-            }
         }
     } else {
         rtn = getattrInternal<CAPI>(obj, str, NULL);

src/runtime/classobj.cpp

@@ -32,15 +32,20 @@ BoxedClass* classobj_cls, *instance_cls;
 
 static Box* classLookup(BoxedClassobj* cls, BoxedString* attr, GetattrRewriteArgs* rewrite_args = NULL) {
     if (rewrite_args)
-        assert(!rewrite_args->out_success);
+        assert(!rewrite_args->isSuccessful());
 
     Box* r = cls->getattr(attr, rewrite_args);
-    if (r)
+    if (r) {
+        if (rewrite_args)
+            rewrite_args->assertReturnConvention(ReturnConvention::HAS_RETURN);
         return r;
+    }
 
     if (rewrite_args) {
-        // abort rewriting because we currenly don't guard the particular 'bases' hierarchy
-        rewrite_args->out_success = false;
+        if (rewrite_args->isSuccessful()) {
+            rewrite_args->getReturn(); // just to make the asserts happy
+            rewrite_args->clearReturn();
+        }
         rewrite_args = NULL;
     }
 
@@ -328,62 +333,71 @@ Box* classobjRepr(Box* _obj) {
 
 // Analogous to CPython's instance_getattr2
 static Box* instanceGetattributeSimple(BoxedInstance* inst, BoxedString* attr_str,
-                                       GetattrRewriteArgs* rewriter_args = NULL) {
-    Box* r = inst->getattr(attr_str, rewriter_args);
-    if (r)
+                                       GetattrRewriteArgs* rewrite_args = NULL) {
+    Box* r = inst->getattr(attr_str, rewrite_args);
+    if (r) {
+        if (rewrite_args)
+            rewrite_args->assertReturnConvention(ReturnConvention::HAS_RETURN);
         return r;
+    }
 
     RewriterVar* r_inst = NULL;
     RewriterVar* r_inst_cls = NULL;
-    if (rewriter_args) {
-        if (!rewriter_args->out_success)
-            rewriter_args = NULL;
+    if (rewrite_args) {
+        if (!rewrite_args->isSuccessful())
+            rewrite_args = NULL;
         else {
-            rewriter_args->out_success = false;
-            r_inst = rewriter_args->obj;
+            rewrite_args->assertReturnConvention(ReturnConvention::NO_RETURN);
+            rewrite_args->clearReturn();
+
+            r_inst = rewrite_args->obj;
             r_inst_cls = r_inst->getAttr(offsetof(BoxedInstance, inst_cls));
         }
     }
-    GetattrRewriteArgs grewriter_inst_args(rewriter_args ? rewriter_args->rewriter : NULL, r_inst_cls,
-                                           rewriter_args ? rewriter_args->rewriter->getReturnDestination()
-                                                         : Location());
-    r = classLookup(inst->inst_cls, attr_str, rewriter_args ? &grewriter_inst_args : NULL);
-    if (!grewriter_inst_args.out_success)
-        rewriter_args = NULL;
-    else
-        assert(grewriter_inst_args.out_return_convention == GetattrRewriteArgs::VALID_RETURN);
+    GetattrRewriteArgs grewriter_inst_args(rewrite_args ? rewrite_args->rewriter : NULL, r_inst_cls,
+                                           rewrite_args ? rewrite_args->rewriter->getReturnDestination() : Location());
+    r = classLookup(inst->inst_cls, attr_str, rewrite_args ? &grewriter_inst_args : NULL);
+    if (!grewriter_inst_args.isSuccessful())
+        rewrite_args = NULL;
 
     if (r) {
         Box* rtn = processDescriptor(r, inst, inst->inst_cls);
-        if (rewriter_args) {
-            RewriterVar* r_rtn = rewriter_args->rewriter->call(true, (void*)processDescriptor,
-                                                               grewriter_inst_args.out_rtn, r_inst, r_inst_cls);
-            rewriter_args->out_rtn = r_rtn;
-            rewriter_args->out_success = true;
-            rewriter_args->out_return_convention = GetattrRewriteArgs::VALID_RETURN;
+        if (rewrite_args) {
+            RewriterVar* r_rtn = rewrite_args->rewriter->call(
+                true, (void*)processDescriptor, grewriter_inst_args.getReturn(ReturnConvention::HAS_RETURN), r_inst,
+                r_inst_cls);
+            rewrite_args->setReturn(r_rtn, ReturnConvention::HAS_RETURN);
         }
         return rtn;
     }
 
+    if (rewrite_args)
+        grewriter_inst_args.assertReturnConvention(ReturnConvention::NO_RETURN);
+
     return NULL;
 }
 
 static Box* instanceGetattributeWithFallback(BoxedInstance* inst, BoxedString* attr_str,
-                                             GetattrRewriteArgs* rewriter_args = NULL) {
-    Box* attr_obj = instanceGetattributeSimple(inst, attr_str, rewriter_args);
+                                             GetattrRewriteArgs* rewrite_args = NULL) {
+    Box* attr_obj = instanceGetattributeSimple(inst, attr_str, rewrite_args);
 
     if (attr_obj) {
+        if (rewrite_args && rewrite_args->isSuccessful())
+            rewrite_args->assertReturnConvention(
+                ReturnConvention::HAS_RETURN); // otherwise need to guard on the success
         return attr_obj;
     }
 
-    if (rewriter_args) {
-        if (!rewriter_args->out_success)
-            rewriter_args = NULL;
-        else
-            rewriter_args->out_success = false;
+    if (rewrite_args) {
+        if (!rewrite_args->isSuccessful())
+            rewrite_args = NULL;
+        else {
+            rewrite_args->assertReturnConvention(ReturnConvention::NO_RETURN);
+            rewrite_args->clearReturn();
+        }
 
         // abort rewriting for now
-        rewriter_args = NULL;
+        rewrite_args = NULL;
     }
 
     static BoxedString* getattr_str = internStringImmortal("__getattr__");
@@ -398,7 +412,7 @@ static Box* instanceGetattributeWithFallback(BoxedInstance* inst, BoxedString* a
 }
 
 static Box* _instanceGetattribute(Box* _inst, BoxedString* attr_str, bool raise_on_missing,
-                                  GetattrRewriteArgs* rewriter_args = NULL) {
+                                  GetattrRewriteArgs* rewrite_args = NULL) {
     RELEASE_ASSERT(_inst->cls == instance_cls, "");
     BoxedInstance* inst = static_cast<BoxedInstance*>(_inst);
 
@@ -411,7 +425,7 @@ static Box* _instanceGetattribute(Box* _inst, BoxedString* attr_str, bool raise_
             return inst->inst_cls;
     }
 
-    Box* attr = instanceGetattributeWithFallback(inst, attr_str, rewriter_args);
+    Box* attr = instanceGetattributeWithFallback(inst, attr_str, rewrite_args);
     if (attr) {
         return attr;
     } else if (!raise_on_missing) {

src/runtime/cxx_unwind.cpp

@@ -92,6 +92,7 @@ static thread_local Timer per_thread_cleanup_timer(-1);
 #ifndef NDEBUG
 static __thread bool in_cleanup_code = false;
 #endif
+static __thread bool is_unwinding = false;
 
 extern "C" {
 
@@ -567,6 +568,7 @@ static inline void unwind_loop(ExcInfo* exc_data) {
 #if STAT_TIMERS
             pyston::StatTimer::finishOverride();
 #endif
+            pyston::is_unwinding = false;
         }
         static_assert(THREADING_USE_GIL, "have to make the unwind session usage in this file thread safe!");
         // there is a python unwinding implementation detail leaked
@@ -610,6 +612,10 @@ void std::terminate() noexcept {
     RELEASE_ASSERT(0, "std::terminate() called!");
 }
 
+bool std::uncaught_exception() noexcept {
+    return pyston::is_unwinding;
+}
+
 // wrong type signature, but that's okay, it's extern "C"
 extern "C" void __gxx_personality_v0() {
     RELEASE_ASSERT(0, "__gxx_personality_v0 should never get called");
@@ -684,9 +690,13 @@ extern "C" void __cxa_throw(void* exc_obj, std::type_info* tinfo, void (*dtor)(v
     pyston::ExcInfo* exc_data = (pyston::ExcInfo*)exc_obj;
     checkExcInfo(exc_data);
 
+    ASSERT(!pyston::is_unwinding, "We don't support throwing exceptions in destructors!");
+
+    pyston::is_unwinding = true;
 #if STAT_TIMERS
     pyston::StatTimer::overrideCounter(unwinding_stattimer);
 #endif
+
     // let unwinding.cpp know we've started unwinding
     pyston::logException(exc_data);
     pyston::unwind(exc_data);

src/runtime/objmodel.h

@@ -141,8 +141,9 @@ enum LookupScope {
     INST_ONLY = 2,
     CLASS_OR_INST = 3,
 };
+struct CallattrRewriteArgs;
 template <ExceptionStyle S>
-Box* callattrInternal(Box* obj, BoxedString* attr, LookupScope, CallRewriteArgs* rewrite_args, ArgPassSpec argspec,
+Box* callattrInternal(Box* obj, BoxedString* attr, LookupScope, CallattrRewriteArgs* rewrite_args, ArgPassSpec argspec,
                       Box* arg1, Box* arg2, Box* arg3, Box** args,
                       const std::vector<BoxedString*>* keyword_names) noexcept(S == CAPI);
 extern "C" void delattr_internal(Box* obj, BoxedString* attr, bool allow_custom, DelattrRewriteArgs* rewrite_args);

src/runtime/types.cpp

@@ -905,13 +905,12 @@ static Box* typeCallInner(CallRewriteArgs* rewrite_args, ArgPassSpec argspec, Bo
         GetattrRewriteArgs grewrite_args(rewrite_args->rewriter, r_ccls, rewrite_args->destination);
         // TODO: if tp_new != Py_CallPythonNew, call that instead?
         new_attr = typeLookup(cls, new_str, &grewrite_args);
+        assert(new_attr);
 
-        if (!grewrite_args.out_success)
+        if (!grewrite_args.isSuccessful())
             rewrite_args = NULL;
         else {
-            assert(new_attr);
-            assert(grewrite_args.out_return_convention = GetattrRewriteArgs::VALID_RETURN);
-            r_new = grewrite_args.out_rtn;
+            r_new = grewrite_args.getReturn(ReturnConvention::HAS_RETURN);
             r_new->addGuard((intptr_t)new_attr);
         }
 
@@ -1049,15 +1048,14 @@ static Box* typeCallInner(CallRewriteArgs* rewrite_args, ArgPassSpec argspec, Bo
             GetattrRewriteArgs grewrite_args(rewrite_args->rewriter, r_ccls, rewrite_args->destination);
             init_attr = typeLookup(cls, init_str, &grewrite_args);
 
-            if (!grewrite_args.out_success)
+            if (!grewrite_args.isSuccessful())
                 rewrite_args = NULL;
             else {
                 if (init_attr) {
-                    assert(grewrite_args.out_return_convention = GetattrRewriteArgs::VALID_RETURN);
-                    r_init = grewrite_args.out_rtn;
+                    r_init = grewrite_args.getReturn(ReturnConvention::HAS_RETURN);
                     r_init->addGuard((intptr_t)init_attr);
                 } else {
-                    assert(grewrite_args.out_return_convention = GetattrRewriteArgs::NO_RETURN);
+                    grewrite_args.assertReturnConvention(ReturnConvention::NO_RETURN);
                 }
             }
         } else {

test/tests/binop_guarding.py

+class C(object):
+    def __add__(self, rhs):
+        if rhs == 50:
+            return NotImplemented
+        return 0
+
+    __eq__ = __add__
+
+def f():
+    c = C()
+    for i in xrange(100):
+        try:
+            print i, c + i
+        except TypeError as e:
+            print e
+f()
+
+def f2():
+    c = C()
+    for i in xrange(100):
+        try:
+            print i, c == i
+        except TypeError as e:
+            print e
+f2()