Skip to content

Introducing proper deallocation ways for CPPInstance - #58

Open
keremsahn wants to merge 3 commits into
compiler-research:mainfrom
keremsahn:cppins-prop-dealloc
Open

Introducing proper deallocation ways for CPPInstance#58
keremsahn wants to merge 3 commits into
compiler-research:mainfrom
keremsahn:cppins-prop-dealloc

Conversation

@keremsahn

@keremsahn keremsahn commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Built upon #37, just last commit is related to this PR
I added three flags to the CPPInstance to distinguish different allocation methods, and in HandleReturn ,where ownership is given, corresponding flags are set after analyzer/attribute check.

Using these new flags CPPInstance::op_dealloc_nofree introduces different kind of deallocations.
Also tests changed a lot after this feature

And I need to explain why I added kIsArrayAlloc instead of using kIsArray, users can set kIsArray flag by reshape feature:

Klass* alloc(size_t count){  return ::operator new(sizeof(Klass)*count);}

and if user reshapes the returned object, that will lead UB because object is deallocated by operator delete[]

@Vipul-Cariappa @aaronj0 @vgvassilev

@keremsahn keremsahn changed the title Cppins prop dealloc Introducing proper deallocation ways for CPPInstance Sep 1, 2026
@keremsahn
keremsahn force-pushed the cppins-prop-dealloc branch 3 times, most recently from 622f8b2 to 82a9b99 Compare September 1, 2026 11:52
@keremsahn
keremsahn marked this pull request as draft September 1, 2026 12:06
@keremsahn
keremsahn force-pushed the cppins-prop-dealloc branch 3 times, most recently from 9d29a43 to a706d92 Compare September 1, 2026 12:44
@keremsahn

Copy link
Copy Markdown
Contributor Author

I do not have access to a mac and I am trying to figure out why macos tests fail, that is why I am pushing constantly

@keremsahn
keremsahn force-pushed the cppins-prop-dealloc branch 4 times, most recently from 7d54a18 to eba937e Compare September 1, 2026 15:46
@keremsahn
keremsahn marked this pull request as ready for review September 1, 2026 16:12
Comment on lines +207 to +208
alloc()
assert cppjit.gbl.memory.memOwn.dtorCount == 0

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
alloc()
assert cppjit.gbl.memory.memOwn.dtorCount == 0
alloc()
gc.collect()
assert cppjit.gbl.memory.memOwn.dtorCount == 0

…p information and this information effects kIsCreator flag of overload group , currently analyzer is not called, just attribute checker is called
…mory-related attributes in the FunctionDecl, analyzer is disabled by default and set by cppjit.use_alloc_analyzer(True/False)
… delete

AllocType was collapsed into kIsCreator in HandleReturn and never reached
op_dealloc_nofree, so malloc'd, new[]'d and raw operator-new'd memory were
all freed with a scalar delete.

Carry it on the instance as three flags saying what to do: kIsArrayAlloc,
kIsNoConstruct, kIsMalloc. HandleReturn sets them when taking ownership;
op_dealloc_nofree branches on them to delete[], free, ::operator delete or
::operator delete[]. No destructor runs for the raw cases. All flags clear
keeps the old delete path, so constructors and explicit __python_owns__ are
unaffected.

delete[] goes through CppInterOp's dtor wrapper, so interop::Destruct just
stops pinning its count to 0; the rest are plain host-side calls.

Fixed two fixtures that claimed malloc via attributes but called new, added
coverage for the new paths plus a destructor counter, and exposed read-only
__is_array_alloc__ / __is_no_construct__ / __is_malloc__ for the tests.
Verified under valgrind: no mismatched free or memory leak.

Also manual ownership setting is added to test07.
@aaronj0
aaronj0 force-pushed the cppins-prop-dealloc branch from eba937e to ddee366 Compare September 8, 2026 11:51
@aaronj0

aaronj0 commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Heads-up: main was force-pushed before the first release as a one-time fix for authorship on two commits ported from the cppyy forks, which credited claude instead of the original patch authors. Since this branch was based on the old history, I rebased it onto the current main and force-pushed, your commits are unchanged. If you have local work on this branch, fetch and put it on top with git rebase <your-remote>/<branch> (or git reset --hard <your-remote>/<branch> if your local copy has nothing extra).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants