[memory-ownership] Honour ownership attributes when taking and releasing memory

CMakeLists.txt

@@ -11,7 +11,7 @@ include(GNUInstallDirs)
 # Perhaps this should permanently be OFF and users can build their own CppInterOp if they want to run the tests?
 option(CPPJIT_ENABLE_CPPINTEROP_TESTS "enable CppInterOp tests" OFF)
 set(CPPINTEROP_GIT_REPOSITORY "https://github.com/compiler-research/CppInterOp.git" CACHE STRING "")
-set(CPPINTEROP_GIT_TAG "c8126249dd18363000b89333b34313ca2a83fb82" CACHE STRING "")
+set(CPPINTEROP_GIT_TAG "c57d253e7cba94cc0976a9e36547a960f0fd2f83" CACHE STRING "")
 set(CPPINTEROP_SOURCE_DIR "" CACHE PATH
     "Override default CppInterOp built by ExternalProject_Add, with a path to local CppInterOp source")
 

src/cpyrt/CPPClassMethod.cxx

@@ -50,6 +50,24 @@ PyObject* cpyrt::CPPClassMethod::Call(CPPInstance*& self, cpyrt_PyArgs_t args,
   return this->Execute(nullptr, 0, ctxt);
 }
 
+//----------------------------------------------------------------------------
+Py_ssize_t cpyrt::CPPClassMethod::GetDeallocArgShift(bool isBound,
+                                                     PyObject* const* args,
+                                                     Py_ssize_t nargs) {
+  // a static method takes no object argument, so a bound call drops self
+  if (isBound)
+    return 0;
+
+  // Call function above drops a leading instance that the call does not need,
+  // and then args[0] belongs to no parameter. The check below repeats the one
+  // in Call(), so a change to either one belongs in both.
+  if (nargs > 0 && CPPInstance_Check(args[0]) && fArgsRequired <= nargs - 1 &&
+      interop::IsSubclass(((CPPInstance*)args[0])->ObjectIsA(), GetScope()))
+    return -1;
+
+  return 0;
+}
+
 //----------------------------------------------------------------------------
 PyObject* cpyrt::CPPClassMethod::GetTypeName() {
   PyObject* cppname =

src/cpyrt/CPPClassMethod.h

@@ -14,6 +14,11 @@ class CPPClassMethod : public CPPMethod {
   PyObject* GetTypeName() override;
 
 public:
+  Py_ssize_t GetDeallocArgShift(bool isBound, PyObject* const* args,
+                                Py_ssize_t nargs) override;
+  // a static method has no object argument.
+  Py_ssize_t GetDeallocParamShift() override { return 0; }
+
   PyCallable* Clone() override { return new CPPClassMethod(*this); }
   PyObject* Call(CPPInstance*& self, cpyrt_PyArgs_t args, size_t nargsf,
                  PyObject* kwds, CallContext* ctxt = nullptr) override;

src/cpyrt/CPPFunction.h

@@ -15,6 +15,11 @@ class CPPFunction : public CPPMethod {
   PyObject* GetTypeName() override;
 
 public:
+  // A free function takes its object as a plain first parameter, so it has no
+  // implicit this slot. The shift of a bound call stays the one of CPPMethod,
+  // as AdjustSelf() puts self in that first parameter.
+  Py_ssize_t GetDeallocParamShift() override { return 0; }
+
   PyCallable* Clone() override { return new CPPFunction(*this); }
   PyObject* Call(CPPInstance*& self, cpyrt_PyArgs_t args, size_t nargsf,
                  PyObject* kwds, CallContext* ctxt = nullptr) override;

src/cpyrt/CPPInstance.cxx

@@ -17,6 +17,7 @@ using namespace cppjit;
 
 // Standard
 #include <algorithm>
+#include <cstdlib>
 #include <sstream>
 
 //- data _____________________________________________________________________
@@ -227,7 +228,17 @@ void cpyrt::op_dealloc_nofree(CPPInstance* pyobj) {
     if (pyobj->fFlags & CPPInstance::kIsValue) {
       interop::CallDestructor(klass, cppobj);
       interop::Deallocate(klass, cppobj);
-    } else
+    } else if (pyobj->fFlags & CPPInstance::kIsMalloc)
+      std::free(cppobj);
+    else if (pyobj->fFlags & CPPInstance::kIsNoConstruct) {
+      if (pyobj->fFlags & CPPInstance::kIsArrayAlloc)
+        ::operator delete[](cppobj);
+      else
+        ::operator delete(cppobj);
+    } else if (pyobj->fFlags & CPPInstance::kIsArrayAlloc)
+      interop::Destruct(klass, cppobj, 1);
+    // Default case: just kIsOwner set in all of memory-ownership flags
+    else
       interop::Destruct(klass, cppobj);
   }
   cppobj = nullptr;
@@ -938,10 +949,30 @@ static int op_setownership(CPPInstance* pyobj, PyObject* value, void*) {
   return 0;
 }
 
+// Added for testing purposes
+//-----------------------------------------------------------------------------
+static PyObject* op_get_array_alloc(CPPInstance* pyobj, void*) {
+  return PyBool_FromLong((long)(pyobj->fFlags & CPPInstance::kIsArrayAlloc));
+}
+//-----------------------------------------------------------------------------
+static PyObject* op_get_no_construct(CPPInstance* pyobj, void*) {
+  return PyBool_FromLong((long)(pyobj->fFlags & CPPInstance::kIsNoConstruct));
+}
+//-----------------------------------------------------------------------------
+static PyObject* op_get_malloc(CPPInstance* pyobj, void*) {
+  return PyBool_FromLong((long)(pyobj->fFlags & CPPInstance::kIsMalloc));
+}
 //-----------------------------------------------------------------------------
 static PyGetSetDef op_getset[] = {
     {(char*)"__python_owns__", (getter)op_getownership, (setter)op_setownership,
      (char*)"If true, python manages the life time of this object", nullptr},
+    {(char*)"__is_array_alloc__", (getter)op_get_array_alloc, nullptr,
+     (char*)"If true, the object was allocated with new[]/operator new[]",
+     nullptr},
+    {(char*)"__is_no_construct__", (getter)op_get_no_construct, nullptr,
+     (char*)"If true, the memory is raw: no constructor was run", nullptr},
+    {(char*)"__is_malloc__", (getter)op_get_malloc, nullptr,
+     (char*)"If true, the object was allocated with malloc", nullptr},
     {(char*)nullptr, nullptr, nullptr, nullptr, nullptr}};
 
 //= cpyrt type number stubs to allow dynamic overrides =====================

src/cpyrt/CPPInstance.h

@@ -29,20 +29,24 @@ class CPPInstance {
 public:
   enum EFlags {
     kDefault = 0x0000,
-    kNoWrapConv = 0x0001,  // use type as-is (eg. no smart ptr wrap)
-    kIsOwner = 0x0002,     // Python instance owns C++ object/memory
-    kIsExtended = 0x0004,  // has extended data
-    kIsValue = 0x0008,     // was created from a by-value return
-    kIsReference = 0x0010, // represents one indirection
-    kIsArray = 0x0020,     // represents an array of objects
-    kIsSmartPtr = 0x0040,  // is or embeds a smart pointer
-    kIsPtrPtr = 0x0080,    // represents two indirections
-    kIsRValue = 0x0100,    // can be used as an r-value
-    kIsLValue = 0x0200,    // can be used as an l-value
-    kNoMemReg = 0x0400,    // do not register with memory regulator
-    kIsRegulated = 0x0800, // is registered with memory regulator
-    kIsActual = 0x1000,    // has been downcasted to actual type
-    kHasLifeLine = 0x2000, // has a life line set
+    kNoWrapConv = 0x0001,   // use type as-is (eg. no smart ptr wrap)
+    kIsOwner = 0x0002,      // Python instance owns C++ object/memory
+    kIsExtended = 0x0004,   // has extended data
+    kIsValue = 0x0008,      // was created from a by-value return
+    kIsReference = 0x0010,  // represents one indirection
+    kIsArray = 0x0020,      // represents an array of objects
+    kIsSmartPtr = 0x0040,   // is or embeds a smart pointer
+    kIsPtrPtr = 0x0080,     // represents two indirections
+    kIsRValue = 0x0100,     // can be used as an r-value
+    kIsLValue = 0x0200,     // can be used as an l-value
+    kNoMemReg = 0x0400,     // do not register with memory regulator
+    kIsRegulated = 0x0800,  // is registered with memory regulator
+    kIsActual = 0x1000,     // has been downcasted to actual type
+    kHasLifeLine = 0x2000,  // has a life line set
+    kIsArrayAlloc = 0x4000, // represents a heap allocated array of objects
+    kIsNoConstruct =
+        0x8000, // represents constructor is not called in the allocation
+    kIsMalloc = 0x10000, // is allocated with malloc
   };
 
 public: // public, as the python C-API works with C structs

src/cpyrt/CPPMethod.cxx

@@ -85,6 +85,8 @@ inline void cpyrt::CPPMethod::Copy_(const CPPMethod& /* other */) {
   fExecutor = nullptr;
   fArgIndices = nullptr;
   fArgsRequired = -1;
+  fAllocType.reset();
+  fDeallocType.reset();
 }
 
 //----------------------------------------------------------------------------
@@ -703,6 +705,37 @@ PyObject* cpyrt::CPPMethod::GetArgDefault(int iarg, bool silent) {
 
 bool cpyrt::CPPMethod::IsConst() { return interop::IsConstMethod(GetMethod()); }
 
+//----------------------------------------------------------------------------
+interop::AllocType cpyrt::CPPMethod::GetAllocBehaviour() {
+  if (fAllocType.has_value())
+    return *fAllocType;
+  interop::TCppMethod_t method = GetMethod();
+  if (!method) {
+    fAllocType = interop::AllocType::None;
+    return interop::AllocType::None;
+  }
+  if (!IsPointerType(interop::GetMethodReturnType(method))) {
+    fAllocType = interop::AllocType::None;
+    return interop::AllocType::None;
+  }
+  interop::AllocType attrResult = interop::IsAllocator(method);
+  fAllocType = attrResult;
+  return attrResult;
+}
+
+//----------------------------------------------------------------------------
+interop::DeallocType cpyrt::CPPMethod::GetDeallocBehaviour(size_t index) {
+  if (!fDeallocType) {
+    fDeallocType.emplace();
+    if (interop::TCppMethod_t method = GetMethod())
+      interop::IsDeallocator(method, *fDeallocType);
+  }
+  if (index < fDeallocType->size()) {
+    return (*fDeallocType)[index];
+  }
+  return interop::DeallocType::Opaque;
+}
+
 //----------------------------------------------------------------------------
 PyObject* cpyrt::CPPMethod::GetScopeProxy() {
   // Get or build the scope of this method.

src/cpyrt/CPPMethod.h

@@ -5,6 +5,7 @@
 #include "PyCallable.h"
 
 // Standard
+#include <optional>
 #include <string>
 #include <unordered_map>
 #include <vector>
@@ -62,6 +63,17 @@ class CPPMethod : public PyCallable {
   PyObject* GetCoVarNames() override;
   PyObject* GetArgDefault(int iarg, bool silent = true) override;
   bool IsConst() override;
+  interop::AllocType GetAllocBehaviour() override;
+  interop::DeallocType GetDeallocBehaviour(size_t index) override;
+  // A bound call passes self on its own, so args[0] lines up with the second
+  // slot. A direct call goes through the vector call, which skips the
+  // descriptor, and then self sits in args[0] itself.
+  Py_ssize_t GetDeallocArgShift(bool isBound, PyObject* const* /* args */,
+                                Py_ssize_t /* nargs */) override {
+    return isBound ? 1 : 0;
+  }
+  // the first slot belongs to the implicit object argument of the method
+  Py_ssize_t GetDeallocParamShift() override { return 1; }
 
   PyObject* GetScopeProxy() override;
   interop::TCppFuncAddr_t GetFunctionAddress() override;
@@ -121,6 +133,8 @@ class CPPMethod : public PyCallable {
 protected:
   // cached value that doubles as initialized flag (uninitialized if -1)
   int fArgsRequired;
+  std::optional<interop::AllocType> fAllocType;
+  std::optional<std::vector<interop::DeallocType>> fDeallocType;
 };
 
 } // namespace cppjit::cpyrt

src/cpyrt/CPPOverload.cxx

@@ -142,14 +142,23 @@ static inline void ResetCallState(CPPInstance* descr_self,
 
 // helper to factor out return logic of mp_call / mp_vectorcall
 static inline PyObject* HandleReturn(CPPOverload* pymeth, CPPInstance* im_self,
-                                     PyObject* result) {
+                                     PyObject* result, PyCallable* method,
+                                     PyObject* const* args, size_t nargsf,
+                                     PyObject* kwds) {
   // special case for python exceptions, propagated through C++ layer
   if (result) {
     CPPInstance* cppres =
         (CPPInstance*)(CPPInstance_Check(result) ? result : nullptr);
 
+    interop::AllocType AT = method->GetAllocBehaviour();
+    // the overloads of a group share fMethodInfo, so the answer of this one
+    // stays in a local instead of going into the shared flags
+    const bool allocates = AT != interop::AllocType::None &&
+                           AT != interop::AllocType::Null &&
+                           AT != interop::AllocType::Unknown;
+
     // if this method creates new objects, always take ownership
-    if (IsCreator(pymeth->fMethodInfo->fFlags)) {
+    if (allocates || IsCreator(pymeth->fMethodInfo->fFlags)) {
 
       // either be a constructor with a fresh object proxy self ...
       if (IsConstructor(pymeth->fMethodInfo->fFlags)) {
@@ -158,9 +167,76 @@ static inline PyObject* HandleReturn(CPPOverload* pymeth, CPPInstance* im_self,
       }
 
       // ... or be a regular method with an object proxy return value
-      else if (cppres)
+      else if (cppres) {
         cppres->PythonOwns();
+        // After giving ownership, set proper flags to indicate allocation
+        // method/func
+        switch (AT) {
+        case interop::AllocType::Malloc:
+          cppres->fFlags |= CPPInstance::kIsMalloc;
+          break;
+        case interop::AllocType::NewArr:
+          cppres->fFlags |= CPPInstance::kIsArrayAlloc;
+          break;
+        case interop::AllocType::OperatorNew:
+          cppres->fFlags |= CPPInstance::kIsNoConstruct;
+          break;
+        case interop::AllocType::OperatorNewArr:
+          cppres->fFlags |= CPPInstance::kIsArrayAlloc;
+          cppres->fFlags |= CPPInstance::kIsNoConstruct;
+          break;
+        default:
+          break;
+        }
+      }
+    }
+
+    // an argument that the called function releases belongs to C++ from now
+    // on, else Python frees it a second time
+    const Py_ssize_t nargs = cpyrt_PyArgs_GET_SIZE(args, nargsf);
+    const Py_ssize_t numKwds =
+        kwds && PyTuple_CheckExact(kwds) ? PyTuple_GET_SIZE(kwds) : 0;
+    // the keyword values follow the positional ones in call order, so their
+    // names are what tell which parameter each one belongs to
+    PyObject* argnames = numKwds ? method->GetSignatureNames() : nullptr;
+
+    const bool isBound = pymeth->fSelf != nullptr;
+    // a place in the array becomes a slot with this shift
+    const Py_ssize_t argShift =
+        method->GetDeallocArgShift(isBound, args, nargs);
+    // a parameter index becomes a slot with this one
+    const Py_ssize_t paramShift = method->GetDeallocParamShift();
+    // the array holds the positional values first, then the keyword ones
+    for (Py_ssize_t i = 0; i < nargs + numKwds; i++) {
+      // only a C++ object can hold memory that the callee releases
+      if (!CPPInstance_Check(args[i]))
+        continue;
+
+      Py_ssize_t slot = -1; // no slot of its own
+      if (i < nargs)
+        slot = i + argShift;
+      else {
+        // kwds runs parallel to the keyword part of the array
+        // the place of the name in the signature is the parameter index
+        for (Py_ssize_t iarg = 0; iarg < PyTuple_GET_SIZE(argnames); iarg++) {
+          if (PyUnicode_Compare(PyTuple_GET_ITEM(argnames, iarg),
+                                PyTuple_GET_ITEM(kwds, i - nargs)) == 0) {
+            slot = iarg + paramShift;
+            break;
+          }
+        }
+      }
+      // the callee drops this argument, or no name of it matches a parameter
+      if (slot < 0)
+        continue;
+
+      const interop::DeallocType DT = method->GetDeallocBehaviour((size_t)slot);
+      if (DT == interop::DeallocType::None ||
+          DT == interop::DeallocType::Opaque)
+        continue;
+      ((CPPInstance*)args[i])->CppOwns();
     }
+    Py_XDECREF(argnames);
 
     // if this new object falls inside self, make sure its lifetime is proper
     if (!(pymeth->fMethodInfo->fFlags & CallContext::kNeverLifeLine)) {
@@ -576,7 +652,8 @@ static PyObject* mp_vectorcall(CPPOverload* pymeth, PyObject* const* args,
     if (!NoImplicit(&ctxt))
       ctxt.fFlags |= CallContext::kAllowImplicit; // no two rounds needed
     PyObject* result = methods[0]->Call(im_self, args, nargsf, kwds, &ctxt);
-    return HandleReturn(pymeth, im_self, result);
+    return HandleReturn(pymeth, im_self, result, methods[0], args, nargsf,
+                        kwds);
   }
 
   // otherwise, handle overloading
@@ -599,7 +676,8 @@ static PyObject* mp_vectorcall(CPPOverload* pymeth, PyObject* const* args,
       ctxt.fFlags |= CallContext::kAllowImplicit;
     PyObject* result = memoized_pc->Call(im_self, args, nargsf, kwds, &ctxt);
     if (result)
-      return HandleReturn(pymeth, im_self, result);
+      return HandleReturn(pymeth, im_self, result, memoized_pc, args, nargsf,
+                          kwds);
 
     // fall through: python is dynamic, and so, the hashing isn't infallible
     ctxt.fFlags &= ~(CallContext::kAllowImplicit | CallContext::kPyException |
@@ -656,7 +734,8 @@ static PyObject* mp_vectorcall(CPPOverload* pymeth, PyObject* const* args,
           }
         }
 
-        return HandleReturn(pymeth, im_self, result);
+        return HandleReturn(pymeth, im_self, result, methods[i], args, nargsf,
+                            kwds);
       }
 
       // else failure ..

src/cpyrt/PyCallable.h

@@ -37,6 +37,22 @@ class PyCallable {
   virtual PyObject* GetCoVarNames() = 0;
   virtual PyObject* GetArgDefault(int /* iarg */, bool silent = true) = 0;
   virtual bool IsConst() { return false; }
+  virtual interop::AllocType GetAllocBehaviour() {
+    return interop::AllocType::None;
+  }
+  virtual interop::DeallocType GetDeallocBehaviour(size_t /* index */) {
+    return interop::DeallocType::Opaque;
+  }
+  // shift that turns a place in the args array into a GetDeallocBehaviour()
+  // slot, which the shape of the call decides
+  virtual Py_ssize_t GetDeallocArgShift(bool /* isBound */,
+                                        PyObject* const* /* args */,
+                                        Py_ssize_t /* nargs */) {
+    return 0;
+  }
+  // shift that turns a declared parameter index into a GetDeallocBehaviour()
+  // slot, which the declaration decides
+  virtual Py_ssize_t GetDeallocParamShift() { return 0; }
 
   virtual PyObject* GetScopeProxy() = 0;
   virtual interop::TCppFuncAddr_t GetFunctionAddress() = 0;