Fix some memory leaks (#736)

@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? --------- Co-authored-by: Marvin Borner <[email protected]>
effekt-lang · Jan 3, 2025 · 08f97ec · 08f97ec
1 parent 2fcfc6d
commit 08f97ec
Show file tree

Hide file tree

Showing 14 changed files with 57 additions and 88 deletions.
diff --git a/effekt/jvm/src/test/scala/effekt/LLVMTests.scala b/effekt/jvm/src/test/scala/effekt/LLVMTests.scala
@@ -22,25 +22,11 @@ class LLVMTests extends EffektTests {
   lazy val bugs: List[File] = List(
     // names not sanitized (even?)
     examplesDir / "pos" / "special_names.effekt",
-
-    // sanitizer/valgrind: segfault
-    examplesDir / "pos" / "parametrized.effekt",
-
-    // Valgrind leak, unclear
-    examplesDir / "llvm" / "polymorphism_map.effekt",
-    examplesDir / "pos" / "type_parameters_blocks.effekt",
-
-    // Valgrind leak in array_new
-    examplesDir / "benchmarks" / "are_we_fast_yet" / "sieve.effekt",
-    examplesDir / "benchmarks" / "are_we_fast_yet" / "nbody.effekt",
-    examplesDir / "benchmarks" / "are_we_fast_yet" / "bounce.effekt", // + c_ref_fresh
-    examplesDir / "benchmarks" / "are_we_fast_yet" / "towers.effekt",
-    examplesDir / "benchmarks" / "are_we_fast_yet" / "permute.effekt",
-    examplesDir / "benchmarks" / "are_we_fast_yet" / "queens.effekt",
-    examplesDir / "benchmarks" / "input_output" / "word_count_ascii.effekt",
-    examplesDir / "benchmarks" / "input_output" / "word_count_utf8.effekt",
+    // Jump to the invalid address stated on the next line
     examplesDir / "benchmarks" / "input_output" / "dyck_one.effekt",
     examplesDir / "benchmarks" / "input_output" / "number_matrix.effekt",
+    examplesDir / "benchmarks" / "input_output" / "word_count_ascii.effekt",
+    examplesDir / "benchmarks" / "input_output" / "word_count_utf8.effekt",
   )
 
   /**

diff --git a/effekt/jvm/src/test/scala/effekt/StdlibTests.scala b/effekt/jvm/src/test/scala/effekt/StdlibTests.scala
@@ -43,27 +43,10 @@ class StdlibLLVMTests extends StdlibTests {
   override def ignored: List[File] = List(
     // Toplevel let-bindings (for ANSI-color-codes in output) not supported
     examplesDir / "stdlib" / "test" / "test.effekt",
-
-    // Valgrind leak/failure
-    examplesDir / "stdlib" / "bytearray" / "bytearray.effekt",
-    examplesDir / "stdlib" / "stream" / "characters.effekt",
-    examplesDir / "stdlib" / "stream" / "fuse_newlines.effekt",
-    examplesDir / "stdlib" / "io" / "filesystem" / "async_file_io.effekt",
+    // Syscall param write(buf) points to uninitialised byte(s)
     examplesDir / "stdlib" / "io" / "filesystem" / "files.effekt",
+    examplesDir / "stdlib" / "io" / "filesystem" / "async_file_io.effekt",
+    // Conditional jump or move depends on uninitialised value(s)
     examplesDir / "stdlib" / "io" / "filesystem" / "wordcount.effekt",
-    examplesDir / "stdlib" / "io" / "time.effekt",
-    examplesDir / "stdlib" / "list" / "flatmap.effekt",
-    examplesDir / "stdlib" / "list" / "zip.effekt",
-    examplesDir / "stdlib" / "list" / "deleteat.effekt",
-    examplesDir / "stdlib" / "list" / "join.effekt",
-    examplesDir / "stdlib" / "list" / "modifyat.effekt",
-    examplesDir / "stdlib" / "list" / "updateat.effekt",
-    examplesDir / "stdlib" / "list" / "insert.effekt",
-    examplesDir / "stdlib" / "list" / "fill.effekt",
-    examplesDir / "stdlib" / "list" / "zipwith.effekt",
-    examplesDir / "stdlib" / "list" / "collect.effekt",
-    examplesDir / "stdlib" / "list" / "build.effekt",
-    examplesDir / "stdlib" / "string" / "strings.effekt",
-    examplesDir / "stdlib" / "string" / "unicode.effekt",
   )
 }
diff --git a/effekt/shared/src/main/scala/effekt/generator/llvm/Transformer.scala b/effekt/shared/src/main/scala/effekt/generator/llvm/Transformer.scala
@@ -365,11 +365,12 @@ object Transformer {
         eraseValues(List(v), freeVariables(rest));
         transform(rest)
 
-      case machine.ForeignCall(machine.Variable(resultName, resultType), foreign, values, rest) =>
+      case machine.ForeignCall(variable @ machine.Variable(resultName, resultType), foreign, values, rest) =>
         emit(Comment(s"foreignCall $resultName : $resultType, foreign $foreign, ${values.length} values"))
         val functionType = PointerType();
         shareValues(values, freeVariables(rest));
         emit(Call(resultName, Ccc(), transform(resultType), ConstantGlobal(foreign), values.map(transform)));
+        eraseValues(List(variable), freeVariables(rest))
         transform(rest)
 
       case machine.Statement.Hole =>

diff --git a/effekt/shared/src/main/scala/effekt/machine/Analysis.scala b/effekt/shared/src/main/scala/effekt/machine/Analysis.scala
@@ -20,7 +20,7 @@ def freeVariables(statement: Statement): Set[Variable] =
     case Jump(Label(_, environment)) =>
       environment.toSet
     case Substitute(bindings, rest) =>
-      freeVariables(rest) -- bindings.map(_._1).toSet ++ bindings.map(_._2).toSet
+      freeVariables(rest).map { variable => bindings.toMap.getOrElse(variable, variable) }
     case Construct(name, tag, values, rest) =>
       Set.from(values) ++ (freeVariables(rest) -- Set(name))
     case Switch(value, clauses, default: Option[Clause]) =>
@@ -30,9 +30,9 @@ def freeVariables(statement: Statement): Set[Variable] =
     case Invoke(value, tag, values) =>
       Set(value) ++ Set.from(values)
     case Var(name, init, tpe, rest) =>
-      freeVariables(rest) ++ Set(init) -- Set(name)
+      Set(init) ++ (freeVariables(rest) -- Set(name))
     case LoadVar(name, ref, rest) =>
-      Set(ref) ++ freeVariables(rest) -- Set(name)
+      Set(ref) ++ (freeVariables(rest) -- Set(name))
     case StoreVar(ref, value, rest) =>
       Set(ref, value) ++ freeVariables(rest)
     case PushFrame(frame, rest) =>
@@ -44,14 +44,14 @@ def freeVariables(statement: Statement): Set[Variable] =
     case Resume(value, rest) =>
       Set(value) ++ freeVariables(rest)
     case Shift(name, prompt, rest) =>
-      freeVariables(rest) -- Set(name) ++ Set(prompt)
+      Set(prompt) ++ (freeVariables(rest) -- Set(name))
     case LiteralInt(name, value, rest) =>
       freeVariables(rest) - name
     case LiteralDouble(name, value, rest) =>
       freeVariables(rest) - name
     case LiteralUTF8String(name, utf8, rest) =>
       freeVariables(rest) - name
     case ForeignCall(name, builtin, arguments, rest) =>
-      arguments.toSet ++ freeVariables(rest) - name
+      arguments.toSet ++ (freeVariables(rest) - name)
     case Hole => Set.empty
   }
diff --git a/examples/stdlib/bytearray/bytearray.check b/examples/stdlib/bytearray/bytearray.check
@@ -1 +1 @@
-hfllo
+hfllo!!!
diff --git a/examples/stdlib/bytearray/bytearray.effekt b/examples/stdlib/bytearray/bytearray.effekt
@@ -2,14 +2,17 @@ import bytearray
 
 def main() = {
 
-  val b = bytearray::allocate(16);
+  val b = bytearray::allocate(8);
   b.unsafeSet(0, 104.toByte)
   b.unsafeSet(1, 101.toByte)
   b.unsafeSet(2, 108.toByte)
   b.unsafeSet(3, 108.toByte)
   b.unsafeSet(4, 111.toByte)
+  b.unsafeSet(5, 33.toByte)
+  b.unsafeSet(6, 33.toByte)
+  b.unsafeSet(7, 33.toByte)
 
   b.unsafeSet(1, 102.toByte)
 
-  println(b.toString) // hfllo
+  println(b.toString) // hfllo!!!
 }
diff --git a/libraries/common/effekt.effekt b/libraries/common/effekt.effekt
@@ -115,7 +115,6 @@ extern pure def length(str: String): Int =
   chez "(string-length ${str})"
   llvm """
     %x = call %Int @c_bytearray_size(%Pos ${str})
-    call void @erasePositive(%Pos ${str})
     ret %Int %x
   """
 

diff --git a/libraries/common/io/filesystem.effekt b/libraries/common/io/filesystem.effekt
@@ -198,13 +198,15 @@ namespace internal {
       ret void
     """
 
+  /// The buffer must be kept alive by using it after the call
   extern async def read(file: Int, buffer: ByteArray, offset: Int, size: Int, position: Int): Int =
     jsNode "$effekt.capture(callback => read(${file}, ${buffer}, ${offset}, ${size}, ${position}, callback))"
     llvm """
       call void @c_fs_read(%Int ${file}, %Pos ${buffer}, %Int ${offset}, %Int ${size}, %Int ${position}, %Stack %stack)
       ret void
     """
 
+  /// The buffer must be kept alive by using it after the call
   extern async def write(file: Int, buffer: ByteArray, offset: Int, size: Int, position: Int): Int =
     jsNode "$effekt.capture(callback => write(${file}, ${buffer}, ${offset}, ${size}, ${position}, callback))"
     llvm """

diff --git a/libraries/llvm/array.c b/libraries/llvm/array.c
@@ -32,13 +32,16 @@ struct Pos c_array_new(const Int size) {
 
 Int c_array_size(const struct Pos arr) {
   uint64_t *sizePtr = arr.obj + sizeof(struct Header);
-  return *sizePtr;
+  uint64_t size = *sizePtr;
+  erasePositive(arr);
+  return size;
 }
 
 struct Pos c_array_get(const struct Pos arr, const Int index) {
   struct Pos *dataPtr = arr.obj + sizeof(struct Header) + sizeof(uint64_t);
   struct Pos element = dataPtr[index];
   sharePositive(element);
+  erasePositive(arr);
   return element;
 }
 
@@ -47,6 +50,7 @@ struct Pos c_array_set(const struct Pos arr, const Int index, const struct Pos v
   struct Pos element = dataPtr[index];
   erasePositive(element);
   dataPtr[index] = value;
+  erasePositive(arr);
   return Unit;
 }
 

diff --git a/libraries/llvm/bytearray.c b/libraries/llvm/bytearray.c
@@ -29,30 +29,34 @@ struct Pos c_bytearray_new(const Int size) {
 }
 
 Int c_bytearray_size(const struct Pos arr) {
+  erasePositive(arr);
   return arr.tag;
 }
 
-uint8_t* c_bytearray_data(const struct Pos arr) {
-  return arr.obj + sizeof(struct Header);
-}
-
 Byte c_bytearray_get(const struct Pos arr, const Int index) {
   Byte *dataPtr = arr.obj + sizeof(struct Header);
   Byte element = dataPtr[index];
+  erasePositive(arr);
   return element;
 }
 
 struct Pos c_bytearray_set(const struct Pos arr, const Int index, const Byte value) {
   Byte *dataPtr = arr.obj + sizeof(struct Header);
   dataPtr[index] = value;
+  erasePositive(arr);
   return Unit;
 }
 
+// Internal Operations
+
+uint8_t* c_bytearray_data(const struct Pos arr) {
+    uint8_t *data = arr.obj + sizeof(struct Header);
+    return data;
+}
+
 struct Pos c_bytearray_construct(const uint64_t n, const uint8_t *data) {
     struct Pos arr = c_bytearray_new(n);
-
     memcpy(c_bytearray_data(arr), data, n);
-
     return arr;
 }
 
@@ -66,7 +70,7 @@ struct Pos c_bytearray_from_nullterminated_string(const char *data) {
 }
 
 char* c_bytearray_into_nullterminated_string(const struct Pos arr) {
-    uint64_t size = c_bytearray_size(arr);
+    uint64_t size = arr.tag;
 
     char* result = (char*)malloc(size + 1);
 
@@ -125,12 +129,14 @@ struct Pos c_bytearray_show_Double(const Double x) {
 
 // TODO do this in Effekt
 struct Pos c_bytearray_concatenate(const struct Pos left, const struct Pos right) {
-    const struct Pos concatenated = c_bytearray_new(c_bytearray_size(left) + c_bytearray_size(right));
-    for (int64_t j = 0; j < c_bytearray_size(concatenated); ++j)
+    uint64_t left_size = left.tag;
+    uint64_t right_size = right.tag;
+    const struct Pos concatenated = c_bytearray_new(left_size + right_size);
+    for (uint64_t j = 0; j < concatenated.tag; ++j)
         c_bytearray_data(concatenated)[j]
-            = j < c_bytearray_size(left)
+            = j < left_size
             ? c_bytearray_data(left)[j]
-            : c_bytearray_data(right)[j - c_bytearray_size(left)];
+            : c_bytearray_data(right)[j - left_size];
 
     erasePositive(left);
     erasePositive(right);
@@ -139,8 +145,8 @@ struct Pos c_bytearray_concatenate(const struct Pos left, const struct Pos right
 
 // TODO do this in Effekt
 struct Pos c_bytearray_equal(const struct Pos left, const struct Pos right) {
-    uint64_t left_size = c_bytearray_size(left);
-    uint64_t right_size = c_bytearray_size(right);
+    uint64_t left_size = left.tag;
+    uint64_t right_size = right.tag;
     if (left_size != right_size) {
         erasePositive(left);
         erasePositive(right);
@@ -161,7 +167,7 @@ struct Pos c_bytearray_equal(const struct Pos left, const struct Pos right) {
 // TODO deprecate
 struct Pos c_bytearray_substring(const struct Pos str, uint64_t start, uint64_t end) {
     const struct Pos substr = c_bytearray_new(end - start);
-    for (int64_t j = 0; j < c_bytearray_size(substr); ++j) {
+    for (uint64_t j = 0; j < substr.tag; ++j) {
         c_bytearray_data(substr)[j] = c_bytearray_data(str)[start+j];
     }
     erasePositive(str);
@@ -174,27 +180,27 @@ uint32_t c_bytearray_character_at(const struct Pos str, const uint64_t index) {
     uint8_t first_byte = bytes[index];
     uint32_t character = 0;
 
-    uint32_t length = c_bytearray_size(str);
+    uint32_t size = str.tag;
 
     if (first_byte < 0x80) {
         // Single-byte character (0xxxxxxx)
         character = first_byte;
     } else if ((first_byte & 0xE0) == 0xC0) {
         // Two-byte character (110xxxxx 10xxxxxx)
-        if (index + 1 < length) {
+        if (index + 1 < size) {
             character = ((first_byte & 0x1F) << 6) |
                         (bytes[index + 1] & 0x3F);
         }
     } else if ((first_byte & 0xF0) == 0xE0) {
         // Three-byte character (1110xxxx 10xxxxxx 10xxxxxx)
-        if (index + 2 < length) {
+        if (index + 2 < size) {
             character = ((first_byte & 0x0F) << 12) |
                         ((bytes[index + 1] & 0x3F) << 6) |
                         (bytes[index + 2] & 0x3F);
         }
     } else if ((first_byte & 0xF8) == 0xF0) {
         // Four-byte character (11110xxx 10xxxxxx 10xxxxxx 10xxxxxx)
-        if (index + 3 < length) {
+        if (index + 3 < size) {
             character = ((first_byte & 0x07) << 18) |
                         ((bytes[index + 1] & 0x3F) << 12) |
                         ((bytes[index + 2] & 0x3F) << 6) |

diff --git a/libraries/llvm/io.c b/libraries/llvm/io.c
@@ -20,7 +20,7 @@ void c_io_println_Double(const Double x) {
 }
 
 void c_io_println_String(String text) {
-    for (int64_t j = 0; j < c_bytearray_size(text); ++j)
+    for (uint64_t j = 0; j < text.tag; ++j)
         putchar(c_bytearray_data(text)[j]);
     erasePositive(text);
     putchar('\n');
@@ -126,8 +126,8 @@ void c_fs_open(String path, struct Pos mode, Stack stack) {
 void c_fs_read(Int file, struct Pos buffer, Int offset, Int size, Int position, Stack stack) {
 
     uv_buf_t buf = uv_buf_init((char*)(c_bytearray_data(buffer) + offset), size);
-    // erasePositive(buffer);
-    // TODO we should erase the buffer but abort if this was the last reference
+    erasePositive(buffer);
+    // TODO panic if this was the last reference
 
     uv_fs_t* request = malloc(sizeof(uv_fs_t));
     request->data = stack;
@@ -144,8 +144,8 @@ void c_fs_read(Int file, struct Pos buffer, Int offset, Int size, Int position,
 void c_fs_write(Int file, struct Pos buffer, Int offset, Int size, Int position, Stack stack) {
 
     uv_buf_t buf = uv_buf_init((char*)(c_bytearray_data(buffer) + offset), size);
-    // erasePositive(buffer);
-    // TODO we should erase the buffer but abort if this was the last reference
+    erasePositive(buffer);
+    // TODO panic if this was the last reference
 
     uv_fs_t* request = malloc(sizeof(uv_fs_t));
     request->data = stack;

diff --git a/libraries/llvm/main.c b/libraries/llvm/main.c
@@ -9,7 +9,6 @@
 
 #define DEBUG_REFCOUNT (false)
 
-#include "sanity.c"
 #include "types.c"
 #include "bytearray.c"
 #include "io.c"

diff --git a/libraries/llvm/ref.c b/libraries/llvm/ref.c
@@ -31,6 +31,7 @@ struct Pos c_ref_get(const struct Pos ref) {
   struct Pos *fieldPtr = ref.obj + sizeof(struct Header);
   struct Pos element = *fieldPtr;
   sharePositive(element);
+  erasePositive(ref);
   return element;
 }
 
@@ -39,6 +40,7 @@ struct Pos c_ref_set(const struct Pos ref, const struct Pos value) {
   struct Pos element = *fieldPtr;
   erasePositive(element);
   *fieldPtr = value;
+  erasePositive(ref);
   return Unit;
 }
 

diff --git a/libraries/llvm/sanity.c b/libraries/llvm/sanity.c