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 some memory leaks #736

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Fix some memory leaks #736

wants to merge 9 commits into from

Conversation

phischu
Copy link
Collaborator

@phischu phischu commented Dec 10, 2024

@marvinborner With these fixes the following does not leak anymore:

import array

def main() = {
  array::allocate[Int](1)
  ()
}

Does this also fix other tests that are commented out?

@phischu phischu changed the title Fix/free variables leak Fix memory leak caused by bug in finding free variables Dec 10, 2024
@marvinborner
Copy link
Member

No, the other leaking tests are not fixed by this entirely. My additional pos erasers in the stdlib make Valgrind even angrier.

@phischu
Copy link
Collaborator Author

phischu commented Dec 12, 2024

Could you please add your additional pos erasers in the stdlib to this pull request?

@marvinborner
Copy link
Member

I've added them. I'm still not 100% sure when we are supposed to add erasers and whether my additions are correct. We should really start writing a detailed sharer/eraser guide :)

@phischu phischu changed the title Fix memory leak caused by bug in finding free variables Fix memory leaks Dec 16, 2024
@phischu phischu changed the title Fix memory leaks Fix some memory leaks Dec 16, 2024
@phischu phischu force-pushed the fix/free_variables_leak branch 2 times, most recently from 27cf964 to c752e08 Compare December 16, 2024 11:52
@phischu
Copy link
Collaborator Author

phischu commented Dec 16, 2024

Thank you. I have applied some fixes, please check if we can enable more tests for llvm now.

@phischu phischu force-pushed the fix/free_variables_leak branch from c752e08 to 87ab60d Compare December 16, 2024 13:25
@marvinborner
Copy link
Member

marvinborner commented Dec 16, 2024

This seems to improve things by a lot! There are still some tests left that fail with segfaults (use-after-free most probably) in UV/IO code, as well as some IO/bytearray-related code in stdlib. I've pushed the uncommented tests to see some immediate CI response on further fixes.

@jiribenes
Copy link
Contributor

jiribenes commented Dec 17, 2024

Does this fix the leak in #711?

@phischu
Copy link
Collaborator Author

phischu commented Dec 18, 2024

examples/stdlib/io/filesystem/files.effekt (llvm) and examples/stdlib/io/filesystem/async_file_io.effekt (llvm) fail with "Syscall param write(buf) points to uninitialised byte(s)", @marvinborner could you investigate? I'll try to hunt down the other ones.

@phischu phischu force-pushed the fix/free_variables_leak branch from dc20bb9 to e1ddadc Compare December 18, 2024 16:46
@phischu phischu force-pushed the fix/free_variables_leak branch from 862de57 to efa2512 Compare December 20, 2024 09:16
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