Change the PyGC_AddRoot API

I think there's a theoretical issue where a GC happens between the
assignment to a static slot and the call to PyGC_AddRoot.  I don't
think this was causing any issues, but I think it possibly could.

from_cpython/Include/Python.h

@@ -110,7 +110,14 @@ PyObject* PyModule_GetDict(PyObject*) PYSTON_NOEXCEPT;
 // Pyston addition:
 // Our goal is to not make exception modules declare their static memory.  But until we can identify
 // that in an automated way, we have to modify extension modules to call this:
-void PyGC_AddRoot(PyObject*) PYSTON_NOEXCEPT;
+PyObject* PyGC_AddRoot(PyObject*) PYSTON_NOEXCEPT;
+// PyGC_AddRoot returns its argument, with the intention that you do something like
+//      static PyObject* obj = PyGC_AddRoot(foo());
+// rather than
+//      static PyObject* obj = foo();
+//      PyGC_AddRoot(obj);
+// to reduce any chances of compiler reorderings or a GC somehow happening between the assignment
+// to the static slot and the call to PyGC_AddRoot.
 
 #define PyDoc_VAR(name) static char name[]
 #define PyDoc_STRVAR(name, str) PyDoc_VAR(name) = PyDoc_STR(str)

from_cpython/Modules/_io/_iomodule.c

@@ -712,90 +712,64 @@ init_io(void)
     /* IncrementalNewlineDecoder */
     ADD_TYPE(&PyIncrementalNewlineDecoder_Type, "IncrementalNewlineDecoder");
 
+    // Pyston change: register with gc
+    // TODO: automatically register static variables as roots
     /* Interned strings */
-    if (!(_PyIO_str_close = PyString_InternFromString("close")))
+    if (!(_PyIO_str_close = PyGC_AddRoot(PyString_InternFromString("close"))))
         goto fail;
-    if (!(_PyIO_str_closed = PyString_InternFromString("closed")))
+    if (!(_PyIO_str_closed = PyGC_AddRoot(PyString_InternFromString("closed"))))
         goto fail;
-    if (!(_PyIO_str_decode = PyString_InternFromString("decode")))
+    if (!(_PyIO_str_decode = PyGC_AddRoot(PyString_InternFromString("decode"))))
         goto fail;
-    if (!(_PyIO_str_encode = PyString_InternFromString("encode")))
+    if (!(_PyIO_str_encode = PyGC_AddRoot(PyString_InternFromString("encode"))))
         goto fail;
-    if (!(_PyIO_str_fileno = PyString_InternFromString("fileno")))
+    if (!(_PyIO_str_fileno = PyGC_AddRoot(PyString_InternFromString("fileno"))))
         goto fail;
-    if (!(_PyIO_str_flush = PyString_InternFromString("flush")))
+    if (!(_PyIO_str_flush = PyGC_AddRoot(PyString_InternFromString("flush"))))
         goto fail;
-    if (!(_PyIO_str_getstate = PyString_InternFromString("getstate")))
+    if (!(_PyIO_str_getstate = PyGC_AddRoot(PyString_InternFromString("getstate"))))
         goto fail;
-    if (!(_PyIO_str_isatty = PyString_InternFromString("isatty")))
+    if (!(_PyIO_str_isatty = PyGC_AddRoot(PyString_InternFromString("isatty"))))
         goto fail;
-    if (!(_PyIO_str_newlines = PyString_InternFromString("newlines")))
+    if (!(_PyIO_str_newlines = PyGC_AddRoot(PyString_InternFromString("newlines"))))
         goto fail;
-    if (!(_PyIO_str_nl = PyString_InternFromString("\n")))
+    if (!(_PyIO_str_nl = PyGC_AddRoot(PyString_InternFromString("\n"))))
         goto fail;
-    if (!(_PyIO_str_read = PyString_InternFromString("read")))
+    if (!(_PyIO_str_read = PyGC_AddRoot(PyString_InternFromString("read"))))
         goto fail;
-    if (!(_PyIO_str_read1 = PyString_InternFromString("read1")))
+    if (!(_PyIO_str_read1 = PyGC_AddRoot(PyString_InternFromString("read1"))))
         goto fail;
-    if (!(_PyIO_str_readable = PyString_InternFromString("readable")))
+    if (!(_PyIO_str_readable = PyGC_AddRoot(PyString_InternFromString("readable"))))
         goto fail;
-    if (!(_PyIO_str_readinto = PyString_InternFromString("readinto")))
+    if (!(_PyIO_str_readinto = PyGC_AddRoot(PyString_InternFromString("readinto"))))
         goto fail;
-    if (!(_PyIO_str_readline = PyString_InternFromString("readline")))
+    if (!(_PyIO_str_readline = PyGC_AddRoot(PyString_InternFromString("readline"))))
         goto fail;
-    if (!(_PyIO_str_reset = PyString_InternFromString("reset")))
+    if (!(_PyIO_str_reset = PyGC_AddRoot(PyString_InternFromString("reset"))))
         goto fail;
-    if (!(_PyIO_str_seek = PyString_InternFromString("seek")))
+    if (!(_PyIO_str_seek = PyGC_AddRoot(PyString_InternFromString("seek"))))
         goto fail;
-    if (!(_PyIO_str_seekable = PyString_InternFromString("seekable")))
+    if (!(_PyIO_str_seekable = PyGC_AddRoot(PyString_InternFromString("seekable"))))
         goto fail;
-    if (!(_PyIO_str_setstate = PyString_InternFromString("setstate")))
+    if (!(_PyIO_str_setstate = PyGC_AddRoot(PyString_InternFromString("setstate"))))
         goto fail;
-    if (!(_PyIO_str_tell = PyString_InternFromString("tell")))
+    if (!(_PyIO_str_tell = PyGC_AddRoot(PyString_InternFromString("tell"))))
         goto fail;
-    if (!(_PyIO_str_truncate = PyString_InternFromString("truncate")))
+    if (!(_PyIO_str_truncate = PyGC_AddRoot(PyString_InternFromString("truncate"))))
         goto fail;
-    if (!(_PyIO_str_write = PyString_InternFromString("write")))
+    if (!(_PyIO_str_write = PyGC_AddRoot(PyString_InternFromString("write"))))
         goto fail;
-    if (!(_PyIO_str_writable = PyString_InternFromString("writable")))
+    if (!(_PyIO_str_writable = PyGC_AddRoot(PyString_InternFromString("writable"))))
         goto fail;
     
+    // this is already registered as root:
     if (!(_PyIO_empty_str = PyUnicode_FromStringAndSize(NULL, 0)))
         goto fail;
-    if (!(_PyIO_empty_bytes = PyBytes_FromStringAndSize(NULL, 0)))
+    if (!(_PyIO_empty_bytes = PyGC_AddRoot(PyBytes_FromStringAndSize(NULL, 0))))
         goto fail;
-    if (!(_PyIO_zero = PyLong_FromLong(0L)))
+    if (!(_PyIO_zero = PyGC_AddRoot(PyLong_FromLong(0L))))
         goto fail;
 
-    // Pyston change: register with gc
-    // TODO: automatically register static variables as roots
-    PyGC_AddRoot(_PyIO_str_close);
-    PyGC_AddRoot(_PyIO_str_closed);
-    PyGC_AddRoot(_PyIO_str_decode);
-    PyGC_AddRoot(_PyIO_str_encode);
-    PyGC_AddRoot(_PyIO_str_fileno);
-    PyGC_AddRoot(_PyIO_str_flush);
-    PyGC_AddRoot(_PyIO_str_getstate);
-    PyGC_AddRoot(_PyIO_str_isatty);
-    PyGC_AddRoot(_PyIO_str_newlines);
-    PyGC_AddRoot(_PyIO_str_nl);
-    PyGC_AddRoot(_PyIO_str_read);
-    PyGC_AddRoot(_PyIO_str_read1);
-    PyGC_AddRoot(_PyIO_str_readable);
-    PyGC_AddRoot(_PyIO_str_readinto);
-    PyGC_AddRoot(_PyIO_str_readline);
-    PyGC_AddRoot(_PyIO_str_reset);
-    PyGC_AddRoot(_PyIO_str_seek);
-    PyGC_AddRoot(_PyIO_str_seekable);
-    PyGC_AddRoot(_PyIO_str_setstate);
-    PyGC_AddRoot(_PyIO_str_tell);
-    PyGC_AddRoot(_PyIO_str_truncate);
-    PyGC_AddRoot(_PyIO_str_writable);
-    PyGC_AddRoot(_PyIO_str_write);
-    // PyGC_AddRoot(_PyIO_empty_str); // this is already registered as root
-    PyGC_AddRoot(_PyIO_empty_bytes);
-    PyGC_AddRoot(_PyIO_zero);
-
     return;
 
   fail:

from_cpython/Modules/_struct.c

@@ -1816,10 +1816,9 @@ cache_struct(PyObject *fmt)
     PyObject * s_object;
 
     if (cache == NULL) {
-        cache = PyDict_New();
+        cache = PyGC_AddRoot(PyDict_New());
         if (cache == NULL)
             return NULL;
-        PyGC_AddRoot(cache);
     }
 
     s_object = PyDict_GetItem(cache, fmt);

from_cpython/Objects/unicodeobject.c

@@ -108,10 +108,9 @@ static PyUnicodeObject *unicode_empty = NULL;
         if (unicode_empty != NULL)                      \
             Py_INCREF(unicode_empty);                   \
         else {                                          \
-            unicode_empty = _PyUnicode_New(0);          \
+            unicode_empty = (PyUnicodeObject*)PyGC_AddRoot((PyObject*)_PyUnicode_New(0)); \
             if (unicode_empty != NULL) {                \
                 Py_INCREF(unicode_empty);               \
-                PyGC_AddRoot((PyObject*)unicode_empty); \
             }                                           \
         }                                               \
         return (PyObject *)unicode_empty;               \
@@ -474,10 +473,9 @@ PyObject *PyUnicode_FromUnicode(const Py_UNICODE *u,
         if (size == 1 && *u < 256) {
             unicode = unicode_latin1[*u];
             if (!unicode) {
-                unicode = _PyUnicode_New(1);
+                unicode = (PyUnicodeObject*)PyGC_AddRoot((PyObject*)_PyUnicode_New(1));
                 if (!unicode)
                     return NULL;
-                PyGC_AddRoot((PyObject*)unicode);
                 unicode->str[0] = *u;
                 unicode_latin1[*u] = unicode;
             }
@@ -522,10 +520,9 @@ PyObject *PyUnicode_FromStringAndSize(const char *u, Py_ssize_t size)
         if (size == 1 && Py_CHARMASK(*u) < 128) {
             unicode = unicode_latin1[Py_CHARMASK(*u)];
             if (!unicode) {
-                unicode = _PyUnicode_New(1);
+                unicode = (PyUnicodeObject*)PyGC_AddRoot((PyObject*)_PyUnicode_New(1));
                 if (!unicode)
                     return NULL;
-                PyGC_AddRoot((PyObject*)unicode);
                 unicode->str[0] = Py_CHARMASK(*u);
                 unicode_latin1[Py_CHARMASK(*u)] = unicode;
             }
@@ -8929,10 +8926,9 @@ void _PyUnicode_Init(void)
 
     /* Init the implementation */
     if (!unicode_empty) {
-        unicode_empty = _PyUnicode_New(0);
+        unicode_empty = (PyUnicodeObject*)PyGC_AddRoot((PyObject*)_PyUnicode_New(0));
         if (!unicode_empty)
             return;
-        PyGC_AddRoot((PyObject*)unicode_empty);
     }
 
     /* initialize the linebreak bloom filter */

src/gc/collector.cpp

@@ -132,8 +132,10 @@ void registerPermanentRoot(void* obj) {
 #endif
 }
 
-extern "C" void PyGC_AddRoot(PyObject* obj) noexcept {
-    registerPermanentRoot(obj);
+extern "C" PyObject* PyGC_AddRoot(PyObject* obj) noexcept {
+    if (obj)
+        registerPermanentRoot(obj);
+    return obj;
 }
 
 static std::unordered_set<void*> nonheap_roots;