From 0283222e3aebc0a3ccaee08265260729a779611e Mon Sep 17 00:00:00 2001 From: Alexandros Naskos Date: Thu, 7 May 2020 12:50:25 +0300 Subject: [PATCH 1/8] Added build option to enable the leak counting allocator. Log the allocation count to the client at the end of the main loop. Fixed two memory leaks in analysis.zig --- build.zig | 6 ++++++ src/analysis.zig | 6 ++++-- src/main.zig | 23 +++++++++++++++++++---- 3 files changed, 29 insertions(+), 6 deletions(-) diff --git a/build.zig b/build.zig index 5bea2c8..b6768c9 100644 --- a/build.zig +++ b/build.zig @@ -16,6 +16,12 @@ pub fn build(b: *std.build.Builder) void { exe.addPackagePath("data", "src/data/0.6.0.zig"); + exe.addBuildOption( + bool, + "leak_detection", + b.option(bool, "leak_detection", "Use testing.LeakCountAllocator to track leaks.") orelse false, + ); + exe.setTarget(target); exe.setBuildMode(mode); exe.install(); diff --git a/src/analysis.zig b/src/analysis.zig index 7940482..2fee411 100644 --- a/src/analysis.zig +++ b/src/analysis.zig @@ -30,12 +30,13 @@ pub fn getDocComments(allocator: *std.mem.Allocator, tree: *std.zig.ast.Tree, no if (func.doc_comments) |doc_comments| { var doc_it = doc_comments.lines.iterator(0); var lines = std.ArrayList([]const u8).init(allocator); + defer lines.deinit(); while (doc_it.next()) |doc_comment| { _ = try lines.append(std.fmt.trim(tree.tokenSlice(doc_comment.*)[3..])); } - return try std.mem.join(allocator, "\n", lines.toOwnedSlice()); + return try std.mem.join(allocator, "\n", lines.items); } else { return null; } @@ -45,12 +46,13 @@ pub fn getDocComments(allocator: *std.mem.Allocator, tree: *std.zig.ast.Tree, no if (var_decl.doc_comments) |doc_comments| { var doc_it = doc_comments.lines.iterator(0); var lines = std.ArrayList([]const u8).init(allocator); + defer lines.deinit(); while (doc_it.next()) |doc_comment| { _ = try lines.append(std.fmt.trim(tree.tokenSlice(doc_comment.*)[3..])); } - return try std.mem.join(allocator, "\n", lines.toOwnedSlice()); + return try std.mem.join(allocator, "\n", lines.items); } else { return null; } diff --git a/src/main.zig b/src/main.zig index b355a2e..9c9f4ea 100644 --- a/src/main.zig +++ b/src/main.zig @@ -164,7 +164,8 @@ pub fn completeGlobal(id: i64, document: types.TextDocument) !void { var completions = std.ArrayList(types.CompletionItem).init(allocator); - // try log("{}", .{&tree.root_node.decls}); + // @TODO + try log("{}", .{&tree.root_node.decls}); var decls = tree.root_node.decls.iterator(0); while (decls.next()) |decl_ptr| { @@ -356,9 +357,14 @@ pub fn processJsonRpc(json: []const u8) !void { } else { try log("Method without return value not implemented: {}", .{method}); } - } +const use_leak_count_alloc = @import("build_options").leak_detection; + +var leak_alloc_global: std.testing.LeakCountAllocator = undefined; +// We can now use if(leak_count_alloc) |alloc| { ... } as a comptime check. +const leak_count_alloc: ?*std.testing.LeakCountAllocator = if (use_leak_count_alloc) &leak_alloc_global else null; + pub fn main() anyerror!void { // Init memory @@ -367,6 +373,12 @@ pub fn main() anyerror!void { defer arena.deinit(); allocator = &arena.allocator; + if (use_leak_count_alloc) { + // Initialize the leak counting allocator. + leak_alloc_global = std.testing.LeakCountAllocator.init(allocator); + allocator = &leak_alloc_global.allocator; + } + // Init buffer for stdin read var buffer = std.ArrayList(u8).init(allocator); @@ -391,7 +403,7 @@ pub fn main() anyerror!void { // var bytes = stdin.read(buffer.items[0..6]) catch return; - if (offset >= 16 and std.mem.eql(u8, "Content-Length: ", buffer.items[0..16])) { + if (offset >= 16 and std.mem.startsWith(u8, buffer.items, "Content-Length: ")) { index = 16; while (index <= offset + 10) : (index += 1) { @@ -430,7 +442,7 @@ pub fn main() anyerror!void { } } else if (offset >= 16) { - try log("Offset is greater than 16!", .{}); + try log("OffseOt is greater than 16!", .{}); return; } @@ -460,5 +472,8 @@ pub fn main() anyerror!void { offset += bytes_read; + if (leak_count_alloc) |leaks| { + try log("Allocations alive after message: {}", .{leaks.count}); + } } } From 3fa5ef4b9643fc1d171f1d0e1efc72c4bdd2084c Mon Sep 17 00:00:00 2001 From: Alexandros Naskos Date: Thu, 7 May 2020 12:58:35 +0300 Subject: [PATCH 2/8] Fixed a couple of typos --- src/main.zig | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/main.zig b/src/main.zig index 9c9f4ea..61c8dee 100644 --- a/src/main.zig +++ b/src/main.zig @@ -164,8 +164,7 @@ pub fn completeGlobal(id: i64, document: types.TextDocument) !void { var completions = std.ArrayList(types.CompletionItem).init(allocator); - // @TODO - try log("{}", .{&tree.root_node.decls}); + // try log("{}", .{&tree.root_node.decls}); var decls = tree.root_node.decls.iterator(0); while (decls.next()) |decl_ptr| { @@ -362,7 +361,7 @@ pub fn processJsonRpc(json: []const u8) !void { const use_leak_count_alloc = @import("build_options").leak_detection; var leak_alloc_global: std.testing.LeakCountAllocator = undefined; -// We can now use if(leak_count_alloc) |alloc| { ... } as a comptime check. +// We can now use if(leak_count_alloc) |alloc| { ... } as a comptime check. const leak_count_alloc: ?*std.testing.LeakCountAllocator = if (use_leak_count_alloc) &leak_alloc_global else null; pub fn main() anyerror!void { @@ -442,7 +441,7 @@ pub fn main() anyerror!void { } } else if (offset >= 16) { - try log("OffseOt is greater than 16!", .{}); + try log("Offset is greater than 16!", .{}); return; } From e9d9c57ff419a93af309cedd0e4594aa0e656792 Mon Sep 17 00:00:00 2001 From: Alexandros Naskos Date: Thu, 7 May 2020 13:56:08 +0300 Subject: [PATCH 3/8] Fixed some additional memory leaks. --- src/main.zig | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/src/main.zig b/src/main.zig index 61c8dee..139d931 100644 --- a/src/main.zig +++ b/src/main.zig @@ -82,6 +82,7 @@ pub fn publishDiagnostics(document: types.TextDocument) !void { defer tree.deinit(); var diagnostics = std.ArrayList(types.Diagnostic).init(allocator); + defer diagnostics.deinit(); if (tree.errors.len > 0) { var index: usize = 0; @@ -107,6 +108,9 @@ pub fn publishDiagnostics(document: types.TextDocument) !void { .severity = types.DiagnosticSeverity.Error, .code = @tagName(err.*), .source = "zls", + // TODO: This is wrong, reference to mem_buffer escapes + // We should probably dupe this (as well as the messages from the other branch) + // And free them in the defer along with the whole array list memory. .message = fbs.getWritten(), // .relatedInformation = undefined }); @@ -150,7 +154,7 @@ pub fn publishDiagnostics(document: types.TextDocument) !void { .params = types.NotificationParams{ .PublishDiagnosticsParams = types.PublishDiagnosticsParams{ .uri = document.uri, - .diagnostics = diagnostics.toOwnedSlice() + .diagnostics = diagnostics.items, } } }); @@ -158,12 +162,28 @@ pub fn publishDiagnostics(document: types.TextDocument) !void { pub fn completeGlobal(id: i64, document: types.TextDocument) !void { const tree = try std.zig.parse(allocator, document.text); - // defer tree.deinit(); + defer tree.deinit(); if (tree.errors.len > 0) return try respondGeneric(id, no_completions_response); var completions = std.ArrayList(types.CompletionItem).init(allocator); + defer { + // Free the completion data. + // @TODO: Check what the tree functions (e.g. tokenSlice) return. + // We may need to dupe them to free the tree + // TODO: Maybe split off into a function or add a method to MarkupContent/CompletionItem? + for (completions.items) |comp| { + if (comp.documentation) |doc| { + if (doc.value.len > 0) { + allocator.free(doc.value); + } + } + } + + completions.deinit(); + } + // try log("{}", .{&tree.root_node.decls}); var decls = tree.root_node.decls.iterator(0); while (decls.next()) |decl_ptr| { @@ -208,7 +228,7 @@ pub fn completeGlobal(id: i64, document: types.TextDocument) !void { .result = types.ResponseParams{ .CompletionList = types.CompletionList{ .isIncomplete = false, - .items = completions.toOwnedSlice() + .items = completions.items, } } }); From a7e4d0b5e317995d2a433fad80bdee3c10dd83f9 Mon Sep 17 00:00:00 2001 From: Alexandros Naskos Date: Thu, 7 May 2020 14:10:58 +0300 Subject: [PATCH 4/8] Use a local arena to free all memory with one call. --- src/main.zig | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-) diff --git a/src/main.zig b/src/main.zig index 139d931..313e31b 100644 --- a/src/main.zig +++ b/src/main.zig @@ -161,28 +161,17 @@ pub fn publishDiagnostics(document: types.TextDocument) !void { } pub fn completeGlobal(id: i64, document: types.TextDocument) !void { + // The tree uses its own arena, so we just pass our main allocator. const tree = try std.zig.parse(allocator, document.text); defer tree.deinit(); if (tree.errors.len > 0) return try respondGeneric(id, no_completions_response); - var completions = std.ArrayList(types.CompletionItem).init(allocator); - - defer { - // Free the completion data. - // @TODO: Check what the tree functions (e.g. tokenSlice) return. - // We may need to dupe them to free the tree - // TODO: Maybe split off into a function or add a method to MarkupContent/CompletionItem? - for (completions.items) |comp| { - if (comp.documentation) |doc| { - if (doc.value.len > 0) { - allocator.free(doc.value); - } - } - } - - completions.deinit(); - } + // We use a local arena allocator to deallocate all temporary data without iterating + var arena = std.heap.ArenaAllocator.init(allocator); + var completions = std.ArrayList(types.CompletionItem).init(&arena.allocator); + // Deallocate all temporary data. + defer arena.deinit(); // try log("{}", .{&tree.root_node.decls}); var decls = tree.root_node.decls.iterator(0); @@ -192,7 +181,7 @@ pub fn completeGlobal(id: i64, document: types.TextDocument) !void { switch (decl.id) { .FnProto => { const func = decl.cast(std.zig.ast.Node.FnProto).?; - var doc_comments = try analysis.getDocComments(allocator, tree, decl); + var doc_comments = try analysis.getDocComments(&arena.allocator, tree, decl); var doc = types.MarkupContent{ .kind = types.MarkupKind.Markdown, .value = doc_comments orelse "" @@ -206,7 +195,7 @@ pub fn completeGlobal(id: i64, document: types.TextDocument) !void { }, .VarDecl => { const var_decl = decl.cast(std.zig.ast.Node.VarDecl).?; - var doc_comments = try analysis.getDocComments(allocator, tree, decl); + var doc_comments = try analysis.getDocComments(&arena.allocator, tree, decl); var doc = types.MarkupContent{ .kind = types.MarkupKind.Markdown, .value = doc_comments orelse "" From c5d3d7902efc4068a1c89c8b781e398efd8a5c02 Mon Sep 17 00:00:00 2001 From: Alexandros Naskos Date: Thu, 7 May 2020 14:36:40 +0300 Subject: [PATCH 5/8] Free json parser state, compute builtin completions at comptime --- src/main.zig | 47 ++++++++++++++++++++++++++++------------------- src/types.zig | 2 +- 2 files changed, 29 insertions(+), 20 deletions(-) diff --git a/src/main.zig b/src/main.zig index 313e31b..eb8131a 100644 --- a/src/main.zig +++ b/src/main.zig @@ -223,11 +223,39 @@ pub fn completeGlobal(id: i64, document: types.TextDocument) !void { }); } + +// Compute builtin completions at comptime. +const builtin_completions = block: { + @setEvalBranchQuota(3_500); + var temp: [data.builtins.len]types.CompletionItem = undefined; + + for (data.builtins) |builtin, i| { + var cutoff = std.mem.indexOf(u8, builtin, "(") orelse builtin.len; + temp[i] = types.CompletionItem{ + .label = builtin[0..cutoff], + .kind = types.CompletionItemKind.Function, + + .filterText = builtin[1..cutoff], + .insertText = builtin[1..], + .insertTextFormat = types.InsertTextFormat.Snippet, + .detail = data.builtin_details[i], + .documentation = types.MarkupContent{ + .kind = types.MarkupKind.Markdown, + .value = data.builtin_docs[i] + } + }; + } + + break :block temp; +}; + // pub fn signature pub fn processJsonRpc(json: []const u8) !void { var parser = std.json.Parser.init(allocator, false); + defer parser.deinit(); + var tree = try parser.parse(json); defer tree.deinit(); @@ -310,25 +338,6 @@ pub fn processJsonRpc(json: []const u8) !void { const char = document.text[pos_index]; if (char == '@') { - var builtin_completions: [data.builtins.len]types.CompletionItem = undefined; - - for (data.builtins) |builtin, i| { - var cutoff = std.mem.indexOf(u8, builtin, "(") orelse builtin.len; - builtin_completions[i] = types.CompletionItem{ - .label = builtin[0..cutoff], - .kind = types.CompletionItemKind.Function, - - .filterText = builtin[1..cutoff], - .insertText = builtin[1..], - .insertTextFormat = types.InsertTextFormat.Snippet, - .detail = data.builtin_details[i], - .documentation = types.MarkupContent{ - .kind = types.MarkupKind.Markdown, - .value = data.builtin_docs[i] - } - }; - } - try send(types.Response{ .id = .{.Integer = id}, .result = types.ResponseParams{ diff --git a/src/types.zig b/src/types.zig index a15aecc..779ba0d 100644 --- a/src/types.zig +++ b/src/types.zig @@ -203,7 +203,7 @@ pub const MarkupContent = struct { pub const CompletionList = struct { isIncomplete: Bool, - items: []CompletionItem, + items: []const CompletionItem, }; pub const CompletionItemKind = enum(Integer) { From a0addc040d6bb6937486b5c0014bca876f8899aa Mon Sep 17 00:00:00 2001 From: Alexandros Naskos Date: Thu, 7 May 2020 15:01:16 +0300 Subject: [PATCH 6/8] Fixed a bug in publishDiagnostics where message data on the stack could be corrupted before being sent --- src/main.zig | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/main.zig b/src/main.zig index eb8131a..733fab9 100644 --- a/src/main.zig +++ b/src/main.zig @@ -81,8 +81,11 @@ pub fn publishDiagnostics(document: types.TextDocument) !void { const tree = try std.zig.parse(allocator, document.text); defer tree.deinit(); - var diagnostics = std.ArrayList(types.Diagnostic).init(allocator); - defer diagnostics.deinit(); + // Use an arena for our local memory allocations. + var arena = std.heap.ArenaAllocator.init(allocator); + defer arena.deinit(); + + var diagnostics = std.ArrayList(types.Diagnostic).init(&arena.allocator); if (tree.errors.len > 0) { var index: usize = 0; @@ -108,10 +111,8 @@ pub fn publishDiagnostics(document: types.TextDocument) !void { .severity = types.DiagnosticSeverity.Error, .code = @tagName(err.*), .source = "zls", - // TODO: This is wrong, reference to mem_buffer escapes - // We should probably dupe this (as well as the messages from the other branch) - // And free them in the defer along with the whole array list memory. - .message = fbs.getWritten(), + // We dupe the string from the stack to our arena + .message = try std.mem.dupe(&arena.allocator, u8, fbs.getWritten()), // .relatedInformation = undefined }); } From cea1222b79cd5907ca8d429cf75f7bca9a091d30 Mon Sep 17 00:00:00 2001 From: Alexandros Naskos Date: Thu, 7 May 2020 16:04:57 +0300 Subject: [PATCH 7/8] Free the old document text after rebuilding the new one, this was a use after free since before, after point into the old memory --- src/main.zig | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/main.zig b/src/main.zig index 733fab9..0da6194 100644 --- a/src/main.zig +++ b/src/main.zig @@ -304,13 +304,15 @@ pub fn processJsonRpc(json: []const u8) !void { .character = range.Object.getValue("end").?.Object.getValue("character").?.Integer }; - const before = document.text[0..try document.positionToIndex(start_pos)]; - const after = document.text[try document.positionToIndex(end_pos)..document.text.len]; - allocator.free(document.text); + const old_text = document.text; + const before = old_text[0..try document.positionToIndex(start_pos)]; + const after = old_text[try document.positionToIndex(end_pos)..document.text.len]; document.text = try std.mem.concat(allocator, u8, &[3][]const u8{ before, change.Object.getValue("text").?.String, after }); + allocator.free(old_text); } else { - allocator.free(document.text); + const old_text = document.text; document.text = try std.mem.dupe(allocator, u8, change.Object.getValue("text").?.String); + allocator.free(old_text); } } From 5c602745fd3ff9b0d8736ed8e91024ff5802a142 Mon Sep 17 00:00:00 2001 From: Alexandros Naskos Date: Thu, 7 May 2020 16:20:45 +0300 Subject: [PATCH 8/8] Use the page allocator as our base allocator for now. --- src/main.zig | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/main.zig b/src/main.zig index 0da6194..1b68b03 100644 --- a/src/main.zig +++ b/src/main.zig @@ -387,11 +387,10 @@ const leak_count_alloc: ?*std.testing.LeakCountAllocator = if (use_leak_count_al pub fn main() anyerror!void { - // Init memory - - var arena = std.heap.ArenaAllocator.init(std.heap.page_allocator); - defer arena.deinit(); - allocator = &arena.allocator; + // TODO: Use a better purpose general allocator once std has one. + // Probably after the generic composable allocators PR? + // This is not too bad for now since most allocations happen in local areans. + allocator = std.heap.page_allocator; if (use_leak_count_alloc) { // Initialize the leak counting allocator.