Repository navigation
Conversation
4507254 to
5dfd251
Compare
keremsahn
changed the title
7e33f9b to
b2c668e
Compare
Vipul-Cariappa
left a comment
Vipul-Cariappa
left a comment
There was a problem hiding this comment.
Old comments that we discussed during the meeting. I forgot to submit them.
30a0993 to
f8a202b
Compare
Vipul-Cariappa
left a comment
Vipul-Cariappa
left a comment
There was a problem hiding this comment.
Initial round of reviews. I need to look at the tests. I do not completely understand the changes in the BindObject function.
| {(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, |
There was a problem hiding this comment.
Change to __is_construct__.
There was a problem hiding this comment.
But we can at least do it here, right?
| ((CPPInstance*)pyobj)->fFlags |= allocFlags; | ||
| } |
There was a problem hiding this comment.
I do not completely understand this.
There was a problem hiding this comment.
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
f8a202b to
eef3dbb
Compare
Vipul-Cariappa
left a comment
Vipul-Cariappa
left a comment
There was a problem hiding this comment.
From yesterday's meeting.
…ing memory 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 collapsed into kIsCreator and never reached op_dealloc_nofree, so malloc'd, new[]'d and raw operator-new'd memory were all freed with a scalar delete. It now rides on the instance as kIsArrayAlloc, kIsNoConstruct and kIsMalloc; op_dealloc_nofree branches on them to delete[], free, ::operator delete or ::operator delete[], with no destructor for the raw cases. All flags clear keeps the old delete path. bind_object carries those flags to the new proxy along with the ownership, as BindCppObjectNoCast() masks them out, so a cast object frees its memory the way it was allocated. Mirrored that with Cpp::IsDeallocator, cached the same way, and passed the call arguments to HandleReturn: handing a Python-owned instance to an ownership-taking function now gives ownership back to C++ instead of leaving it to be 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, because an implicit object parameter and the shape of the call each move it: GetDeallocArgShift() answers for the args array and GetDeallocParamShift() for the declaration. CPPMethod, CPPFunction and CPPClassMethod answer both 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. CPPINTEROP_GIT_TAG moves to the CppInterOp commit that carries those two entry points, and test/Makefile builds the memory_analysis dictionary. Ownership is only released, never nullified: the instance keeps a stale pointer after the callee frees the memory. GetAllocBehaviour is restricted to raw-pointer returns. Exposed read-only __is_array_alloc__ / __is_no_construct__ / __is_malloc__ and added tests for the deallocation paths and for ownership release via free, via an 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.
eef3dbb to
158c08d
Compare
PyCallable carried four virtual functions for the two slot shifts. Only CPPClassMethod needed a real computation, so HandleReturn now picks the shifts with a dynamic_cast chain. CPPClassMethod::Call and HandleReturn share one ShouldDropSelf, which ends the repeated condition. As an optimization, the loop also skips an argument that C++ already owns.
158c08d to
a0bc038
Compare
|
This looks good to me. All of my major concerns have been addressed. If you could let give me time until Monday, it will be helpful. I would like to try this changes locally once and than merge. In a mean time, may be @aaronj0 and @vgvassilev can take a look at this. |
|
|
||
| // if this method creates new objects, always take ownership | ||
| if (IsCreator(pymeth->fMethodInfo->fFlags)) { | ||
| if (allocates || IsCreator(pymeth->fMethodInfo->fFlags)) { |
There was a problem hiding this comment.
I believe we should
if (allocates && ~IsCreator(...))
pymeth->fMethodInfo->fFlags;But that assumes that all overloads are allocating, and that is bad. @keremsahn could you open an issue for this, once this PR is merged? I believe you are working on this already?
We should then be able to have some optimised code paths.
FYI, the current implementation is correct. This is a further improvement.
There was a problem hiding this comment.
It does not assume all overloads are allocating, IsCreator is global flag for all overloads the condition still checks if the flag is set, but does not write to it. I agree overload flags should be changed but in terms of this PR, I did not want to touch it, therefore we are checking analyzer/attributes result for each overload and do not write to their global flag.
if (allocates && ~IsCreator(...))
pymeth->fMethodInfo->fFlags;This code is also wrong,
- ~IsCreator check will evaluate to false after first call because it is set with pymeth->fMethodInfo->fFlags;
- The flag should not be set because it is global for all overloads and assumes other overloads are also allocating
There was a problem hiding this comment.
I meant !IsCreator. Yeah, I completely agree with everything you said.
But if we can store the IsCreator info per overload, we should do it. Of course not in this PR.
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.