(re-land) use func watchers

Summary:
This reverts D47602760, and re-lands D47156535, D47165628, and D47306492, with changes to adapt to the new `cinder.cpp` compilation unit for Cinder-wide infra. Since Static Python relies on function watchers, function watchers go there rather than in the JIT.

The revert wasn't due to any problems discovered in this code, it was just to minimize conflicts in the dict watchers revert.

With this diff, all `#ifdef ENABLE_CINDERX` couplings have been removed from `funcobject.c`.

This removes the `InitFunction` HIR opcode from the JIT, since that opcode existed to do Cinder entrypoint initialization for new functions, and this now happens automatically as part of `MakeFunction`, due to the function watcher.

In order to properly initialize functions created before `Cinder_Init` is called, we use the new (upstream and backported) `PyUnstable_GC_VisitObjects` API to visit all GC objects and initialize the ones that are functions.

I eliminated the call to `PyEntry_init` after a change to function defaults. This only happened if the function was not JIT-compiled. I think this dates back to Cinder 3.8 where we had many more (non-JIT) function entrypoint variants, depending on number of arguments, argument defaults, etc, and needed to ensure we set the right one. But in 3.10 we eliminated all those custom entry points, so I don't think this is needed anymore.

I had to make a fix to the func watchers tests so they would work correctly when running with another func watcher active. I also submitted this fix upstream:  python/cpython#106286

And I had to delete a C++ test that was passing only due to a series of accidents. The func-modified callbacks (both before and after this diff) are global and dispatch only to the global singleton `jit_ctx` in `pyjit.cpp`, so they can't be tested correctly by a unit test that never globally enables the JIT and only constructs its own private JIT context. The function-modified callback in this test was doing nothing, but the entrypoint of the function was getting re-set to `_PyFunction_Vectorcall` anyway due to `PyEntry_init` seeing the JIT as not enabled; this seems unlikely to be a realistic scenario the test was intended to check.

There is already a Python-level test (`test_funcattrs.FunctionPropertiesTest.test_copying___code__`) that verifies that re-assigning `__code__` changes the behavior of the function; we run this test under the JIT, and it fails if we fail to deopt the function on `__code__` reassignment. So the behavior we care about is already tested.

Reviewed By: alexmalyshev

Differential Revision: D48128982

fbshipit-source-id: cd56b4c485f62e33580b4838882cd4c610653a4f

Cinder/cinder.cpp

@@ -5,6 +5,7 @@
 #include "Jit/pyjit.h"
 
 static int cinder_dict_watcher_id = -1;
+static int cinder_func_watcher_id = -1;
 
 static int cinder_dict_watcher(
     PyDict_WatchEvent event,
@@ -55,10 +56,72 @@ void Cinder_UnwatchDict(PyObject* dict) {
   }
 }
 
+static int cinder_func_watcher(
+    PyFunction_WatchEvent event,
+    PyFunctionObject* func,
+    PyObject* new_value) {
+  switch (event) {
+    case PyFunction_EVENT_CREATE:
+      PyEntry_init(func);
+      break;
+    case PyFunction_EVENT_MODIFY_CODE:
+      _PyJIT_FuncModified(func);
+      // having deopted the func, we want to immediately consider recompiling.
+      // func_set_code will assign this again later, but we do it early so
+      // PyEntry_init can consider the new code object now
+      Py_INCREF(new_value);
+      Py_XSETREF(func->func_code, new_value);
+      PyEntry_init(func);
+      break;
+    case PyFunction_EVENT_MODIFY_DEFAULTS:
+      break;
+    case PyFunction_EVENT_MODIFY_KWDEFAULTS:
+      break;
+    case PyFunction_EVENT_MODIFY_QUALNAME:
+      // allow reconsideration of whether this function should be compiled
+      if (!_PyJIT_IsCompiled((PyObject*)func)) {
+        // func_set_qualname will assign this again, but we need to assign it
+        // now so that PyEntry_init can consider the new qualname
+        Py_INCREF(new_value);
+        Py_XSETREF(func->func_qualname, new_value);
+        PyEntry_init(func);
+      }
+      break;
+    case PyFunction_EVENT_DESTROY:
+      _PyJIT_FuncDestroyed(func);
+      break;
+  }
+  return 0;
+}
+
+static int init_funcs_visitor(PyObject* obj, void*) {
+  if (PyFunction_Check(obj)) {
+    PyEntry_init((PyFunctionObject*)obj);
+  }
+  return 1;
+}
+
+static void init_already_existing_funcs() {
+  PyUnstable_GC_VisitObjects(init_funcs_visitor, NULL);
+}
+
+static int cinder_install_func_watcher() {
+  int watcher_id = PyFunction_AddWatcher(cinder_func_watcher);
+  if (watcher_id < 0) {
+    return -1;
+  }
+  cinder_func_watcher_id = watcher_id;
+  return 0;
+}
+
 int Cinder_Init() {
   if (cinder_install_dict_watcher() < 0) {
     return -1;
   }
+  if (cinder_install_func_watcher() < 0) {
+    return -1;
+  }
+  init_already_existing_funcs();
   return _PyJIT_Initialize();
 }
 
@@ -67,18 +130,29 @@ int Cinder_Fini() {
 }
 
 int Cinder_InitSubInterp() {
-  // HACK: for now we assume we are the only dict watcher out there, so that we
-  // can just keep track of a single dict watcher ID rather than one per
-  // interpreter.
-  int prev_watcher_id = cinder_dict_watcher_id;
+  // HACK: for now we assume we are the only watcher out there, so that we can
+  // just keep track of a single watcher ID rather than one per interpreter.
+  int prev_dict_watcher_id = cinder_dict_watcher_id;
   JIT_CHECK(
-      prev_watcher_id >= 0,
+      prev_dict_watcher_id >= 0,
       "Initializing sub-interpreter without main interpreter?");
   if (cinder_install_dict_watcher() < 0) {
     return -1;
   }
   JIT_CHECK(
-      cinder_dict_watcher_id == prev_watcher_id,
+      cinder_dict_watcher_id == prev_dict_watcher_id,
       "Somebody else watching dicts?");
+
+  int prev_func_watcher_id = cinder_func_watcher_id;
+  JIT_CHECK(
+      prev_func_watcher_id >= 0,
+      "Initializing sub-interpreter without main interpreter?");
+  if (cinder_install_func_watcher() < 0) {
+    return -1;
+  }
+  JIT_CHECK(
+      cinder_func_watcher_id == prev_func_watcher_id,
+      "Somebody else watching functions?");
+
   return 0;
 }

Jit/hir/analysis.cpp

@@ -178,7 +178,6 @@ bool isPassthrough(const Instr& instr) {
     case Opcode::kHintType:
     case Opcode::kSnapshot:
     case Opcode::kIncref:
-    case Opcode::kInitFunction:
     case Opcode::kReturn:
     case Opcode::kSetCellItem:
     case Opcode::kSetFunctionAttr:

Jit/hir/builder.cpp

@@ -2917,7 +2917,6 @@ void HIRBuilder::emitMakeFunction(
     tc.emit<SetFunctionAttr>(defaults, func, FunctionAttr::kDefaults);
   }
 
-  tc.emit<InitFunction>(func);
   tc.frame.stack.push(func);
 }
 

Jit/hir/hir.cpp

@@ -216,7 +216,6 @@ bool Instr::isReplayable() const {
     case Opcode::kInPlaceOp:
     case Opcode::kIncref:
     case Opcode::kInitialYield:
-    case Opcode::kInitFunction:
     case Opcode::kInvokeIterNext:
     case Opcode::kInvokeStaticFunction:
     case Opcode::kInvokeMethod:

Jit/hir/hir.h

@@ -267,7 +267,6 @@ struct FrameState {
   V(ImportName)                        \
   V(InPlaceOp)                         \
   V(Incref)                            \
-  V(InitFunction)                      \
   V(InitialYield)                      \
   V(IntBinaryOp)                       \
   V(PrimitiveBoxBool)                  \
@@ -1267,9 +1266,6 @@ DEFINE_SIMPLE_INSTR(
     Operands<2>,
     DeoptBase);
 
-// Calls PyEntry_Init(func)
-DEFINE_SIMPLE_INSTR(InitFunction, (TFunc), Operands<1>);
-
 // Takes a list as operand 0
 // Takes an item as operand 1
 DEFINE_SIMPLE_INSTR(

Jit/hir/memory_effects.cpp

@@ -50,7 +50,6 @@ MemoryEffects memoryEffects(const Instr& inst) {
     case Opcode::kMakeCell:
     case Opcode::kMakeCheckedDict:
     case Opcode::kMakeDict:
-    case Opcode::kMakeFunction:
     case Opcode::kMakeSet:
     case Opcode::kMakeTupleFromList:
     case Opcode::kPrimitiveCompare:
@@ -179,9 +178,8 @@ MemoryEffects memoryEffects(const Instr& inst) {
     case Opcode::kXDecref:
       return {false, AEmpty, {1, 1}, AManagedHeapAny};
 
-    case Opcode::kInitFunction:
-      // InitFunction mostly writes to a bunch of func fields we don't track,
-      // but it can also invoke the JIT which may at some point have effects
+    case Opcode::kMakeFunction:
+      // MakeFunction can invoke the JIT which may at some point have effects
       // worth tracking.
       return commonEffects(inst, AOther);
 

Jit/hir/printer.cpp

@@ -253,7 +253,6 @@ static std::string format_immediates(const Instr& instr) {
     case Opcode::kGuard:
     case Opcode::kIncref:
     case Opcode::kInitialYield:
-    case Opcode::kInitFunction:
     case Opcode::kInvokeIterNext:
     case Opcode::kIsInstance:
     case Opcode::kIsNegativeAndErrOccurred:

Jit/hir/ssa.cpp

@@ -696,7 +696,6 @@ Type outputType(
     case Opcode::kGuard:
     case Opcode::kHintType:
     case Opcode::kIncref:
-    case Opcode::kInitFunction:
     case Opcode::kRaise:
     case Opcode::kRaiseAwaitableError:
     case Opcode::kRaiseStatic:

Jit/lir/generator.cpp

@@ -2345,12 +2345,6 @@ LIRGenerator::TranslatedBlock LIRGenerator::TranslateOneBasicBlock(
         bbb.AppendCode(ss.str());
         break;
       }
-      case Opcode::kInitFunction: {
-        auto instr = static_cast<const InitFunction*>(&i);
-
-        bbb.AppendInvoke(PyEntry_init, instr->GetOperand(0));
-        break;
-      }
       case Opcode::kMakeFunction: {
         auto instr = static_cast<const MakeFunction*>(&i);
         auto qualname = instr->GetOperand(0);

Jit/pyjit.cpp

@@ -1739,7 +1739,7 @@ static int install_jit_audit_hook() {
 int _PyJIT_Initialize() {
   // If we have data symbols which are public but not used within CPython code,
   // we need to ensure the linker doesn't GC the .data section containing them.
-  // We can do this by referencing at least symbol from that sourfe module.
+  // We can do this by referencing at least symbol from that source module.
   // In future versions of clang/gcc we may be able to eliminate this with
   // 'keep' and/or 'used' attributes.
   //

Modules/_testcapimodule.c

@@ -6406,9 +6406,9 @@ allocate_too_many_code_watchers(PyObject *self, PyObject *args)
 
 // Test function watchers
 
-#define NUM_FUNC_WATCHERS 2
-static PyObject *pyfunc_watchers[NUM_FUNC_WATCHERS];
-static int func_watcher_ids[NUM_FUNC_WATCHERS] = {-1, -1};
+#define NUM_TEST_FUNC_WATCHERS 2
+static PyObject *pyfunc_watchers[NUM_TEST_FUNC_WATCHERS];
+static int func_watcher_ids[NUM_TEST_FUNC_WATCHERS] = {-1, -1};
 
 static PyObject *
 get_id(PyObject *obj)
@@ -6482,7 +6482,7 @@ second_func_watcher_callback(PyFunction_WatchEvent event,
     return call_pyfunc_watcher(pyfunc_watchers[1], event, func, new_value);
 }
 
-static PyFunction_WatchCallback func_watcher_callbacks[NUM_FUNC_WATCHERS] = {
+static PyFunction_WatchCallback func_watcher_callbacks[NUM_TEST_FUNC_WATCHERS] = {
     first_func_watcher_callback,
     second_func_watcher_callback
 };
@@ -6507,26 +6507,25 @@ add_func_watcher(PyObject *self, PyObject *func)
         return NULL;
     }
     int idx = -1;
-    for (int i = 0; i < NUM_FUNC_WATCHERS; i++) {
+    for (int i = 0; i < NUM_TEST_FUNC_WATCHERS; i++) {
         if (func_watcher_ids[i] == -1) {
             idx = i;
             break;
         }
     }
     if (idx == -1) {
-        PyErr_SetString(PyExc_RuntimeError, "no free watchers");
-        return NULL;
-    }
-    PyObject *result = PyLong_FromLong(idx);
-    if (result == NULL) {
+        PyErr_SetString(PyExc_RuntimeError, "no free test watchers");
         return NULL;
     }
     func_watcher_ids[idx] = PyFunction_AddWatcher(func_watcher_callbacks[idx]);
     if (func_watcher_ids[idx] < 0) {
-        Py_DECREF(result);
         return NULL;
     }
     pyfunc_watchers[idx] = Py_NewRef(func);
+    PyObject *result = PyLong_FromLong(func_watcher_ids[idx]);
+    if (result == NULL) {
+        return NULL;
+    }
     return result;
 }
 
@@ -6543,7 +6542,7 @@ clear_func_watcher(PyObject *self, PyObject *watcher_id_obj)
         return NULL;
     }
     int idx = -1;
-    for (int i = 0; i < NUM_FUNC_WATCHERS; i++) {
+    for (int i = 0; i < NUM_TEST_FUNC_WATCHERS; i++) {
         if (func_watcher_ids[i] == wid) {
             idx = i;
             break;

Objects/funcobject.c

@@ -6,7 +6,6 @@
 #include "pycore_object.h"        // _PyObject_GC_UNTRACK()
 #include "pycore_pyerrors.h"      // _PyErr_Occurred()
 #include "structmember.h"         // PyMemberDef
-#include "Jit/pyjit.h"
 
 #include "cinder/exports.h"
 
@@ -173,11 +172,7 @@ PyFunction_NewWithQualName(PyObject *code, PyObject *globals, PyObject *qualname
     op->func_weakreflist = NULL;
     op->func_module = module;
     op->func_annotations = NULL;
-#ifdef ENABLE_CINDERX
-    op->vectorcall = (vectorcallfunc)PyEntry_LazyInit;
-#else
     op->vectorcall = (vectorcallfunc)_PyFunction_Vectorcall;
-#endif
 
     _PyObject_GC_TRACK(op);
     handle_func_event(PyFunction_EVENT_CREATE, op, NULL);
@@ -460,10 +455,6 @@ func_set_code(PyFunctionObject *op, PyObject *value, void *Py_UNUSED(ignored))
     handle_func_event(PyFunction_EVENT_MODIFY_CODE, op, value);
     Py_INCREF(value);
     Py_XSETREF(op->func_code, value);
-#ifdef ENABLE_CINDERX
-    _PyJIT_FuncModified(op);
-    PyEntry_init(op);
-#endif
     return 0;
 }
 
@@ -510,11 +501,6 @@ func_set_qualname(PyFunctionObject *op, PyObject *value, void *Py_UNUSED(ignored
                       (PyFunctionObject *)op, value);
     Py_INCREF(value);
     Py_XSETREF(op->func_qualname, value);
-#ifdef ENABLE_CINDERX
-    if (!_PyJIT_IsCompiled((PyObject *)op)) {
-        PyEntry_init(op);
-    }
-#endif
     return 0;
 }
 
@@ -556,14 +542,6 @@ func_set_defaults(PyFunctionObject *op, PyObject *value, void *Py_UNUSED(ignored
     handle_func_event(PyFunction_EVENT_MODIFY_DEFAULTS, op, value);
     Py_XINCREF(value);
     Py_XSETREF(op->func_defaults, value);
-#ifdef ENABLE_CINDERX
-    // JIT-compiled functions load their defaults at runtime if needed. Others
-    // need their entrypoint recomputed.
-    // TODO(T126790232): Don't load defaults at runtime and recompile as needed.
-    if (!_PyJIT_IsCompiled((PyObject *)op)) {
-        PyEntry_init(op);
-    }
-#endif
     return 0;
 }
 
@@ -757,10 +735,6 @@ func_new_impl(PyTypeObject *type, PyCodeObject *code, PyObject *globals,
         newfunc->func_closure = closure;
     }
 
-#ifdef ENABLE_CINDERX
-    PyEntry_init(newfunc);
-#endif
-
     return (PyObject *)newfunc;
 }
 
@@ -785,9 +759,6 @@ func_clear(PyFunctionObject *op)
 static void
 func_dealloc(PyFunctionObject *op)
 {
-#ifdef ENABLE_CINDERX
-    _PyJIT_FuncDestroyed(op);
-#endif
     assert(Py_REFCNT(op) == 0);
     Py_SET_REFCNT(op, 1);
     handle_func_event(PyFunction_EVENT_DESTROY, op, NULL);

Python/ceval.c

@@ -4363,10 +4363,6 @@ f->lazy_imports_cache_seq = -1;
                 func->func_defaults = POP();
             }
 
-#ifdef ENABLE_CINDERX
-            PyEntry_init(func);
-#endif
-
             PUSH((PyObject *)func);
             DISPATCH();
         }

RuntimeTests/hir_frame_state_test.cpp

@@ -860,7 +860,6 @@ def test(x):
       }
     }
     SetFunctionAttr<func_defaults> v2 v5
-    InitFunction v5
     Snapshot {
       NextInstrOffset 10
       Locals<2> v0 v1

RuntimeTests/hir_tests/dead_code_elimination_test.txt

@@ -105,7 +105,6 @@ fun jittestmodule:test {
         Locals<1> v6
       }
     }
-    InitFunction v9
     Snapshot
     v12:Object = VectorCall<0> v9 {
       FrameState {
@@ -136,4 +135,4 @@ fun test {
     Return<CInt32> v0
   }
 }
----
+---