Add setattrInternal

This should fix the bad behavior we had where setGlobal would only get rewritten
by having its call to setattr() get inlined, and having setattr() then think it
was being called directly.  This was both brittle (only worked on release builds)
and incorrect (there were checks that happened before calling into setattr, which
needed to be emitted as guards).

I had hacked in an attribute((always_inline)) to at least deal with the brittleness,
but this should be a fix for real.  And let us reenable the gcc builds.

src/runtime/objmodel.cpp

@@ -3105,37 +3105,14 @@ void setattrGeneric(Box* obj, BoxedString* attr, STOLEN(Box*) val, SetattrRewrit
 template void setattrGeneric<NOT_REWRITABLE>(Box* obj, BoxedString* attr, Box* val, SetattrRewriteArgs* rewrite_args);
 template void setattrGeneric<REWRITABLE>(Box* obj, BoxedString* attr, Box* val, SetattrRewriteArgs* rewrite_args);
 
-extern "C" void setattr(Box* obj, BoxedString* attr, STOLEN(Box*) attr_val) {
-    STAT_TIMER(t0, "us_timer_slowpath_setattr", 10);
-
-    static StatCounter slowpath_setattr("slowpath_setattr");
-    slowpath_setattr.log();
-
-    if (obj->cls->tp_setattr) {
-        STAT_TIMER(t1, "us_timer_slowpath_tpsetattr", 10);
-
-        assert(attr->data()[attr->size()] == '\0');
-        AUTO_DECREF(attr_val);
-        int rtn = obj->cls->tp_setattr(obj, const_cast<char*>(attr->data()), attr_val);
-        if (rtn)
-            throwCAPIException();
-        return;
-    }
-
-    std::unique_ptr<Rewriter> rewriter(
-        Rewriter::createRewriter(__builtin_extract_return_addr(__builtin_return_address(0)), 3, "setattr"));
-
+void setattrInternal(Box* obj, BoxedString* attr, STOLEN(Box*) attr_val, SetattrRewriteArgs* rewrite_args) {
     setattrofunc tp_setattro = obj->cls->tp_setattro;
     assert(tp_setattro);
 
     assert(!obj->cls->tp_setattr);
 
-    if (rewriter.get()) {
-        rewriter->getArg(0)->setType(RefType::BORROWED);
-        rewriter->getArg(1)->setType(RefType::BORROWED);
-        rewriter->getArg(2)->setType(RefType::OWNED);
-
-        auto r_cls = rewriter->getArg(0)->getAttr(offsetof(Box, cls));
+    if (rewrite_args) {
+        auto r_cls = rewrite_args->obj->getAttr(offsetof(Box, cls));
         // rewriter->trap();
         r_cls->addAttrGuard(offsetof(BoxedClass, tp_setattr), 0);
         r_cls->addAttrGuard(offsetof(BoxedClass, tp_setattro), (intptr_t)tp_setattro);
@@ -3147,19 +3124,14 @@ extern "C" void setattr(Box* obj, BoxedString* attr, STOLEN(Box*) attr_val) {
     RewriterVar* r_setattr;
 
     if (tp_setattro == instance_setattro) {
-        if (rewriter.get()) {
-            SetattrRewriteArgs rewrite_args(rewriter.get(), rewriter->getArg(0), rewriter->getArg(2));
-            instanceSetattroInternal(obj, attr, attr_val, &rewrite_args);
-            if (rewrite_args.out_success)
-                rewriter->commit();
-        } else
-            instanceSetattroInternal(obj, attr, attr_val, NULL);
-
+        instanceSetattroInternal(obj, attr, attr_val, rewrite_args);
         return;
     } else if (tp_setattro != PyObject_GenericSetAttr) {
+// TODO: rewrite these cases?
+#if 0
         static BoxedString* setattr_str = getStaticString("__setattr__");
-        if (rewriter.get()) {
-            GetattrRewriteArgs rewrite_args(rewriter.get(), rewriter->getArg(0)->getAttr(offsetof(Box, cls)),
+        if (rewrite_args) {
+            GetattrRewriteArgs rewrite_args(rewrite_args->rewriter, rewrite_args->obj->getAttr(offsetof(Box, cls)),
                                             Location::any());
             setattr = typeLookup(obj->cls, setattr_str, &rewrite_args);
             assert(setattr);
@@ -3174,37 +3146,28 @@ extern "C" void setattr(Box* obj, BoxedString* attr, STOLEN(Box*) attr_val) {
         } else {
             // setattr = typeLookup(obj->cls, setattr_str);
         }
+#endif
     }
 
-    // This is a borrowed reference so we don't need to register it
-    static Box* object_setattr = object_cls->getattr(getStaticString("__setattr__"));
-
-    // I guess this check makes it ok for us to just rely on having guarded on the value of setattr without
-    // invalidating on deallocation, since we assume that object.__setattr__ will never get deallocated.
     if (tp_setattro == PyObject_GenericSetAttr) {
-        if (rewriter.get()) {
-            // rewriter->trap();
-            SetattrRewriteArgs rewrite_args(rewriter.get(), rewriter->getArg(0), rewriter->getArg(2));
-            setattrGeneric<REWRITABLE>(obj, attr, attr_val, &rewrite_args);
-            if (rewrite_args.out_success)
-                rewriter->commit();
-        } else {
-            setattrGeneric<NOT_REWRITABLE>(obj, attr, attr_val, NULL);
-        }
+        setattrGeneric<REWRITABLE>(obj, attr, attr_val, rewrite_args);
         return;
     }
 
     AUTO_DECREF(attr_val);
 
-    if (rewriter.get()) {
+// TODO actually rewrite this?
+#if 0
+    if (rewrite_args) {
         assert(setattr);
 
-        // TODO actually rewrite this?
         setattr = processDescriptor(setattr, obj, obj->cls);
         AUTO_DECREF(setattr);
         autoDecref(
             runtimeCallInternal<CXX, REWRITABLE>(setattr, NULL, ArgPassSpec(2), attr, attr_val, NULL, NULL, NULL));
-    } else {
+    } else
+#endif
+    {
         STAT_TIMER(t0, "us_timer_slowpath_tpsetattro", 10);
         int r = tp_setattro(obj, attr, attr_val);
         if (r)
@@ -3212,6 +3175,39 @@ extern "C" void setattr(Box* obj, BoxedString* attr, STOLEN(Box*) attr_val) {
     }
 }
 
+extern "C" void setattr(Box* obj, BoxedString* attr, STOLEN(Box*) attr_val) {
+    STAT_TIMER(t0, "us_timer_slowpath_setattr", 10);
+
+    static StatCounter slowpath_setattr("slowpath_setattr");
+    slowpath_setattr.log();
+
+    if (obj->cls->tp_setattr) {
+        STAT_TIMER(t1, "us_timer_slowpath_tpsetattr", 10);
+
+        assert(attr->data()[attr->size()] == '\0');
+        AUTO_DECREF(attr_val);
+        int rtn = obj->cls->tp_setattr(obj, const_cast<char*>(attr->data()), attr_val);
+        if (rtn)
+            throwCAPIException();
+        return;
+    }
+
+    std::unique_ptr<Rewriter> rewriter(
+        Rewriter::createRewriter(__builtin_extract_return_addr(__builtin_return_address(0)), 3, "setattr"));
+
+    if (rewriter.get()) {
+        SetattrRewriteArgs rewrite_args(rewriter.get(), rewriter->getArg(0)->setType(RefType::BORROWED),
+                                        rewriter->getArg(2)->setType(RefType::OWNED));
+        rewriter->getArg(1)->setType(RefType::BORROWED);
+        setattrInternal(obj, attr, attr_val, &rewrite_args);
+
+        if (rewrite_args.out_success)
+            rewriter->commit();
+    } else {
+        setattrInternal(obj, attr, attr_val, NULL);
+    }
+}
+
 static bool nonzeroHelper(STOLEN(Box*) r) {
     AUTO_DECREF(r);
 
@@ -7284,14 +7280,23 @@ extern "C" void setGlobal(Box* globals, BoxedString* name, STOLEN(Box*) value) {
     }
 
     if (globals->cls == module_cls) {
-        // Note: in optimized builds, this will be a tail call, which will
-        // preserve the return address, letting the setattr() call rewrite itself.
-        // XXX this isn't really safe in general, since the guards that led to this
-        // path need to end up in the rewrite.  I think this is safe for now since
-        // writing the module case won't accidentally work for the dict case, but
-        // we should make all the entrypoints (the ones that look at the return address)
-        // be noinline.
-        setattr(static_cast<BoxedModule*>(globals), name, value);
+        std::unique_ptr<Rewriter> rewriter(
+            Rewriter::createRewriter(__builtin_extract_return_addr(__builtin_return_address(0)), 3, "setattr"));
+
+        if (rewriter.get()) {
+            SetattrRewriteArgs rewrite_args(rewriter.get(), rewriter->getArg(0)->setType(RefType::BORROWED),
+                                            rewriter->getArg(2)->setType(RefType::OWNED));
+            rewriter->getArg(1)->setType(RefType::BORROWED);
+
+            rewrite_args.obj->addAttrGuard(offsetof(Box, cls), (uint64_t)globals->cls);
+
+            setattrInternal(globals, name, value, &rewrite_args);
+
+            if (rewrite_args.out_success)
+                rewriter->commit();
+        } else {
+            setattrInternal(globals, name, value, NULL);
+        }
     } else {
         RELEASE_ASSERT(globals->cls == dict_cls, "%s", globals->cls->tp_name);
         int r = PyDict_SetItem(globals, name, value);

src/runtime/objmodel.h

@@ -56,7 +56,7 @@ extern "C" Box* getattr_capi(Box* obj, BoxedString* attr) noexcept __attribute__
 extern "C" Box* getattrMaybeNonstring(Box* obj, Box* attr);
 // XXX: testing.  this tail-calls in optimized builds so force it to inline for unoptimized as well to get the same
 // behavior.
-extern "C" void setattr(Box* obj, BoxedString* attr, STOLEN(Box*) attr_val) __attribute__((always_inline));
+extern "C" void setattr(Box* obj, BoxedString* attr, STOLEN(Box*) attr_val) __attribute__((noinline));
 extern "C" void delattr(Box* obj, BoxedString* attr) __attribute__((noinline));
 extern "C" void delattrGeneric(Box* obj, BoxedString* attr, DelattrRewriteArgs* rewrite_args);
 extern "C" bool nonzero(Box* obj) __attribute__((noinline));