Feldspar / feldspar-compiler

This is the compiler for the Feldspar Language.
Other
22 stars 5 forks source link

Renamer introduces array initialization without free #198

Open emwap opened 9 years ago

emwap commented 9 years ago

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 163e3f05d860ecb8a858e61271dc022b12ef2c27 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?

/cc @pjonsson

pjonsson commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

I have now merged #197. I have reverted https://github.com/Feldspar/feldspar-compiler/commit/163e3f05d860ecb8a858e61271dc022b12ef2c27 so that we can perform further analysis in this issue.