From 08f97ec0f25eb7b71f25d8ff2d3deed31121d16b Mon Sep 17 00:00:00 2001 From: phischu Date: Fri, 3 Jan 2025 14:46:46 +0100 Subject: [PATCH] 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 --- .../jvm/src/test/scala/effekt/LLVMTests.scala | 20 ++------- .../src/test/scala/effekt/StdlibTests.scala | 23 ++-------- .../effekt/generator/llvm/Transformer.scala | 3 +- .../main/scala/effekt/machine/Analysis.scala | 10 ++--- examples/stdlib/bytearray/bytearray.check | 2 +- examples/stdlib/bytearray/bytearray.effekt | 7 +++- libraries/common/effekt.effekt | 1 - libraries/common/io/filesystem.effekt | 2 + libraries/llvm/array.c | 6 ++- libraries/llvm/bytearray.c | 42 +++++++++++-------- libraries/llvm/io.c | 10 ++--- libraries/llvm/main.c | 1 - libraries/llvm/ref.c | 2 + libraries/llvm/sanity.c | 16 ------- 14 files changed, 57 insertions(+), 88 deletions(-) delete mode 100644 libraries/llvm/sanity.c diff --git a/effekt/jvm/src/test/scala/effekt/LLVMTests.scala b/effekt/jvm/src/test/scala/effekt/LLVMTests.scala index a65046cd0..356ea86cb 100644 --- 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 index f74cb0e21..e3066302d 100644 --- 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 index 5cb66a27b..476ad2329 100644 --- 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 index d61cd77b5..06bb92ce0 100644 --- 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,7 +44,7 @@ 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) => @@ -52,6 +52,6 @@ def freeVariables(statement: Statement): Set[Variable] = 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 index 1257dbb58..7c50b1575 100644 --- 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 index dfa719b8d..1bcac7ebf 100644 --- 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 index 5fb10bea9..09017e5ac 100644 --- 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 index 1dad752c9..6ec241472 100644 --- a/libraries/common/io/filesystem.effekt +++ b/libraries/common/io/filesystem.effekt @@ -198,6 +198,7 @@ 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 """ @@ -205,6 +206,7 @@ namespace internal { 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 index adb2a1f08..df0950b82 100644 --- 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 index 1866602ea..7cf5a3ade 100644 --- 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 index cff63fc0b..bdc3cfbbb 100644 --- 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 index f11320541..34e22d5c9 100644 --- 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 index aa40032fc..48620a42c 100644 --- 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 deleted file mode 100644 index a64c8f564..000000000 --- a/libraries/llvm/sanity.c +++ /dev/null @@ -1,16 +0,0 @@ -#ifndef EFFEKT_SANITY_C -#define EFFEKT_SANITY_C - - -#if CHAR_BIT != 8 -#error Machine ought to have eight bits in a byte. -#endif - - -#define ASSERT_NON_NULL(PTR) if ((PTR) == NULL) { \ - fprintf(stderr, "*** MALLOC PANIC\n"); \ - fflush(stderr); \ - exit(1); } - - -#endif