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

Renamer introduces array initialization without free #198

Open
emwap opened this issue Jan 8, 2015 · 3 comments
Open

Renamer introduces array initialization without free #198

emwap opened this issue Jan 8, 2015 · 3 comments
Labels

Comments

@emwap
Copy link
Member

emwap commented Jan 8, 2015

Synopsis

rename introduces calls to initArray, but does not insert a corresponding freeArray, which leads to a potential memory leak.

Background

Discovered in #197.

In 163e3f0 the compilation of Let constructs is changed to not initialize and free the variable bound, since doing that may lead to multiple frees of the same variable through aliasing. Instead, the variable is declared as an alias.

This change uncovered an issue in renameExp where the call to deepCopy introduces a call to initArray without a corresponding freeArray.

Solutions?

  • A crude solution is to emit an epilogue snippet in deepCopy and applying that when the let-bound variable goes out of scope.

/cc @pjonsson

@emwap emwap added the bug label Jan 8, 2015
@pjonsson
Copy link
Member

pjonsson commented Jan 8, 2015

As things currently are it's the variable declaration code that is responsible for inserting calls to free. I have to run to a meeting now but I'm fairly sure the code that inserts calls to freeArray is morally doing nub (boundVariables prog).

It's legal to call initArray multiple times on the same structure (setArray uses that). If multiple frees are a problem we can't have deepCopy insert calls to free since that might insert multiple calls to free.

The change to let expressions looks like it's papering over some issue with name shadowing. Do you have a small test case for that?

@pjonsson
Copy link
Member

Having pondered this some more I still think that violating some of the undocumented assumptions about shadowing is the most likely root cause for what you're seeing. We've run into that before and investigated things. The proper fix is pretty invasive so that should be handled as a long term thing rather than an afternoon hack.

@emwap
Copy link
Member Author

emwap commented Apr 24, 2015

I have now merged #197. I have reverted 163e3f0 so that we can perform further analysis in this issue.

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

No branches or pull requests

2 participants