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

[keremsahn]

Disregard previous PRs, I merged #35, #58 into one commit and added mechanism to prevent double free. So this PR is consist of one commit and should be merged at once.

PR content:

Added an AllocType field on CPPMethod, cached from Cpp::IsAllocator, that now decides the ownership of a returned object. The overloads of a group share fMethodInfo, so the answer of the called overload stays in a local instead of going into the shared kIsCreator flag, which nothing ever clears. The analyzer is not called; this is the attribute-checker path only.

AllocType was folded into kIsCreator and never reached op_dealloc_nofree, so memory from malloc, new[] and raw operator new was all freed with delete. It now sits on the instance as kIsArrayAlloc, kIsNoConstruct and kIsMalloc, and op_dealloc_nofree uses them to pick delete[], free, ::operator delete or ::operator delete[]. The raw cases run no destructor. With all flags clear the old delete path stays as it was. bind_object hands those flags to the new proxy together with the ownership, because BindCppObjectNoCast masks them out, so a cast object frees its memory the way it was allocated.

The same is done for Cpp::IsDeallocator, cached the same way, and the call arguments are now given to HandleReturn: when a Python-owned instance is passed to a function that takes ownership, ownership goes back to C++ instead of the object being freed twice.

IsDeallocator reports one DeallocType per parameter, so every argument of a call is matched to the parameter that the attribute labels. A positional argument finds its slot from its place in the args array, and a keyword argument from its name in the signature. Two shifts turn either number into a slot, since an implicit object parameter and the shape of the call each move it: GetDeallocArgShift gives the one for the args array and GetDeallocParamShift the one for the declaration. CPPMethod, CPPFunction and CPPClassMethod give both answers for their own kind of callee, and the defaults on PyCallable keep every other callable out of this path.

The interop layer carries what the above needs. interop::AllocType and interop::DeallocType name the CppInterOp enums, and IsAllocator and IsDeallocator join their neighbours in the wrapper, each holding the interpreter mutex. interop::Destruct takes an object count, which it used to pin to 0; a count other than 0 makes the CppInterOp destructor wrapper run delete[] in place of delete, which is what the array path wants.

Ownership is still only given back, never nullified, so the instance keeps a stale pointer after the callee frees the memory.

GetAllocBehaviour is restricted to raw-pointer returns because records are not handled properly for now.

Exposed read-only is_array_alloc / is_no_construct / is_malloc and added tests for the deallocation paths and for ownership release via free, ownership_takes function, and the negative case. An attribute reaches the tests three ways: a clang::annotate on the declaration, a redeclaration that carries it, and APINotes behind a module map. The release tests cover the bound, unbound and stored-bound forms of a member method, a static method and
a free function, each in a positional and a keyword shape, a keyword shape with the names reversed, an argument that is no object at all, a cppDeallocNone parameter, a Python overload that carries no attribute, and a cast through bind_object. Verified under valgrind.

[keremsahn]

@vgvassilev @aaronj0 @Vipul-Cariappa

[Vipul-Cariappa]

Old comments that we discussed during the meeting. I forgot to submit them.

[Vipul-Cariappa]

Initial round of reviews. I need to look at the tests. I do not completely understand the changes in the BindObject function.

[Vipul-Cariappa]

Let's not have this in a negative way. It becomes difficult to read.

Suggested change

	kIsNoConstruct =
	0x8000, // represents constructor is not called in the allocation
	kIsConstructed =
	0x8000, // represents if the constructor is called during allocation

[keremsahn]

Since before allocation details the assumption was constructor was run for each instance, and we are adding a kIsNoConstruct to reflect the opposite possibility. If I have added kIsConstruct I would have to revert each instance creation mechanism and probably more things.

Also git diff is still large for flags because of clang-format

[Vipul-Cariappa]

Ok, this is a good argument.

[Vipul-Cariappa]

Change to __is_construct__.

[Vipul-Cariappa]

But we can at least do it here, right?

[keremsahn]

Yep

[Vipul-Cariappa]

Suggested change

	if (!CPPInstance_Check(args[i]))
	continue;
	if (!CPPInstance_Check(args[i]) && !IsPythonOwns(args[i]))
	continue;

?

[keremsahn]

Do you suggest this for LowLevelView? It is the content of next PR, so this PR only touch to the CPPInstance

[Vipul-Cariappa]

If the object is already owned by C++, then do we need to perform any kinds of checks? I believe not. So perform these changes if the object is currently owned by Python. This is an optimisation.
This is unrelated to LowLevelViews, as I understand.

[keremsahn]

I thought you are putting a check for LL because of &&, it should be || !IsPythonOwns(args[i])). I did not put this because the next lines just iterate over 2 array which size of 10 at max which I thought does not worth to put a early continue. We can add it but I prefer to do in next PR since it also changed this line.

[Vipul-Cariappa]

Are GetDeallocArgShift and GetDeallocParamShift required?
You can use interop::IsNonStaticMethod(method->GetMethod()). You now know the the function requires self/this. You can im_self == nullptr, and know how this/self is passed, through im_self or arguments (args/kwds).
You can add a wrapper

bool CPPMethod::NeedsSelf() {
  return interop::IsNonStaticMetod(GetMethod());
}

[Vipul-Cariappa]

I will prefer removing GetDeallocArgShift and GetDeallocParamShift if possible.

[keremsahn]

I agree, I would just put casts for every type of function and calculate offsets, but since CppJIT's style is mostly based on inheritance structure I decided to go this way.

And for your suggestions: 1) for using IsStaticMethod(method->getMethod()) i need to add a virtual function to the PyCallable, so there will be no gain in terms of number of functions, and code will be more messy because there will be exceptions for some classes, eg. TPythonCallback.

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;
}

fArgsRequired is a protected member, of course there are solutions but things get messy as we try to move offset logic into handleReturn.

So my opinion is: we either use casts and do everything in handleReturn (I could not find any example of this kind of code in CppJIT),, or keep these getArg/ParamShift functions, because otherwise code just becomes low-quality

[Vipul-Cariappa]

I do not completely understand this.

[keremsahn]

Basically when a copy proxy is created with binding function we need to pass the ownership information and allocation details, so we save the allocation flags in allocFlags variable and set them in the new proxy object