Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix addCallback binding #1322

Merged

Conversation

cnheitman
Copy link
Collaborator

Fix PR fixes a small memory leak in the addCallback function. This issue was spotted than to LIEF and their new nanobind implementation of its Python bindings (ref here).

@JonathanSalwan
Copy link
Owner

Mmmh, I remember adding these INC_REF due to a bad counting. I think that's why unit tests segfault =/

@cnheitman
Copy link
Collaborator Author

I'm checking the tests to understand better what's going on.

@JonathanSalwan
Copy link
Owner

Here is the commit that added these INC. Maybe it can help you. 23f5c21

@cnheitman
Copy link
Collaborator Author

cnheitman commented May 6, 2024

I updated the PR. Now tests pass (at least locally), and I'm not getting the nanobind leak messages when using Triton with LIEF v0.14.1. It does not seem necessary to increase the refs in addCallback as the are "borrowed" refs and, in a similar way, they don't need to be decreased in removeCallback.

Let's wait until all tests run and see what happens.

@JonathanSalwan JonathanSalwan added this to the v1.0 milestone May 6, 2024
@JonathanSalwan JonathanSalwan merged commit 6474d96 into JonathanSalwan:dev-v1.0 May 6, 2024
28 checks passed
@cnheitman cnheitman deleted the fix/fix-python-binding branch May 7, 2024 13:29
@jordan9001
Copy link
Contributor

jordan9001 commented Jul 25, 2024

These changes brought back this SEGFAULT

#1035

@JonathanSalwan
Copy link
Owner

Erf, you're right and I don't know why we didn't have this unittest... @cnheitman can you backport the patch and try another fix for the memleak?

@jordan9001
Copy link
Contributor

I think it makes sense to me that we require the addition of a reference to any function object sent to addCallback (and subtraction on removeCallback).

Could the cause of the memory leak be that removeCallback isn't always being called when the context is destroyed? Do we need to add logic to TritonContext_dealloc to remove any remaining references?

JonathanSalwan pushed a commit that referenced this pull request Jul 25, 2024
@JonathanSalwan
Copy link
Owner

JonathanSalwan commented Jul 25, 2024

I've reverted #1322 and added an unit test for #1035. Now, let's see how we can fix the memory leak.

@cnheitman
Copy link
Collaborator Author

Hey! A bit late... I'll try to check this again (and how it interacts with #1035) later this week.

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

Successfully merging this pull request may close these issues.

None yet

3 participants