Skip to content

Plug memory leak #375

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

mandolaerik
Copy link
Contributor

@mandolaerik mandolaerik commented Jun 11, 2025

No description provided.

@syssimics
Copy link
Contributor

PR Verification: ❌ failure

void
_DML_free_qname_cache(qname_cache_t *cache)
{
for (int i = 0; i < 4; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this magic 4 be a define?

Copy link
Contributor

Choose a reason for hiding this comment

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

sizeof(cache->bufs)/sizeof(cache->bufs[0]) is also a pretty common C pattern (though typically broken out to a macro)

Copy link
Contributor

Choose a reason for hiding this comment

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

That works for me too, I wasn't sure if cache->bufs was an array or pointer

Copy link
Contributor

@lwaern-intel lwaern-intel left a comment

Choose a reason for hiding this comment

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

There's a potential pitfall, here; model code can be called following _destroy due to immediate after (see c_backend.generate_deinit,) and that model code could call _qname and reallocate the buffers.
The surefire way to address that is to free the qname cache separately from _destroy. Say, introduce the method _destroy_qname_cache and then codegen_inline_byname it in generate_deinit after the final _DML_execute_immediate_afters_now. Or we could just be fine with that extremely unlikely leak.

void
_DML_free_qname_cache(qname_cache_t *cache)
{
for (int i = 0; i < 4; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

sizeof(cache->bufs)/sizeof(cache->bufs[0]) is also a pretty common C pattern (though typically broken out to a macro)

method _destroy() { _rec_destroy(); }
method _destroy() {
_rec_destroy();
_DML_free_qname_cache();
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing its argument.

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.

4 participants