Commit 10d2f08d37

Andrew Kelley <superjoe30@gmail.com>
2018-07-23 20:28:14
self-hosted: fix error messages not cleaning up correctly
1 parent d767fae
src-self-hosted/compilation.zig
@@ -24,6 +24,7 @@ const Visib = @import("visib.zig").Visib;
 const Value = @import("value.zig").Value;
 const Type = Value.Type;
 const Span = errmsg.Span;
+const Msg = errmsg.Msg;
 const codegen = @import("codegen.zig");
 const Package = @import("package.zig").Package;
 const link = @import("link.zig").link;
@@ -40,8 +41,6 @@ pub const EventLoopLocal = struct {
 
     native_libc: event.Future(LibCInstallation),
 
-    cwd: []const u8,
-
     var lazy_init_targets = std.lazyInit(void);
 
     fn init(loop: *event.Loop) !EventLoopLocal {
@@ -54,21 +53,16 @@ pub const EventLoopLocal = struct {
         try std.os.getRandomBytes(seed_bytes[0..]);
         const seed = std.mem.readInt(seed_bytes, u64, builtin.Endian.Big);
 
-        const cwd = try os.getCwd(loop.allocator);
-        errdefer loop.allocator.free(cwd);
-
         return EventLoopLocal{
             .loop = loop,
             .llvm_handle_pool = std.atomic.Stack(llvm.ContextRef).init(),
             .prng = event.Locked(std.rand.DefaultPrng).init(loop, std.rand.DefaultPrng.init(seed)),
             .native_libc = event.Future(LibCInstallation).init(loop),
-            .cwd = cwd,
         };
     }
 
     /// Must be called only after EventLoop.run completes.
     fn deinit(self: *EventLoopLocal) void {
-        self.loop.allocator.free(self.cwd);
         while (self.llvm_handle_pool.pop()) |node| {
             c.LLVMContextDispose(node.data);
             self.loop.allocator.destroy(node);
@@ -234,7 +228,7 @@ pub const Compilation = struct {
     const PtrTypeTable = std.HashMap(*const Type.Pointer.Key, *Type.Pointer, Type.Pointer.Key.hash, Type.Pointer.Key.eql);
     const TypeTable = std.HashMap([]const u8, *Type, mem.hash_slice_u8, mem.eql_slice_u8);
 
-    const CompileErrList = std.ArrayList(*errmsg.Msg);
+    const CompileErrList = std.ArrayList(*Msg);
 
     // TODO handle some of these earlier and report them in a way other than error codes
     pub const BuildError = error{
@@ -286,7 +280,7 @@ pub const Compilation = struct {
     pub const Event = union(enum) {
         Ok,
         Error: BuildError,
-        Fail: []*errmsg.Msg,
+        Fail: []*Msg,
     };
 
     pub const DarwinVersionMin = union(enum) {
@@ -761,21 +755,35 @@ pub const Compilation = struct {
                 };
                 errdefer self.gpa().free(source_code);
 
-                var tree = try std.zig.parse(self.gpa(), source_code);
-                errdefer tree.deinit();
+                const tree = try self.gpa().createOne(ast.Tree);
+                tree.* = try std.zig.parse(self.gpa(), source_code);
+                errdefer {
+                    tree.deinit();
+                    self.gpa().destroy(tree);
+                }
 
                 break :blk try Scope.Root.create(self, tree, root_src_real_path);
             };
             defer root_scope.base.deref(self);
+            const tree = root_scope.tree;
+
+            var error_it = tree.errors.iterator(0);
+            while (error_it.next()) |parse_error| {
+                const msg = try Msg.createFromParseErrorAndScope(self, root_scope, parse_error);
+                errdefer msg.destroy();
 
-            const tree = &root_scope.tree;
+                try await (async self.addCompileErrorAsync(msg) catch unreachable);
+            }
+            if (tree.errors.len != 0) {
+                return;
+            }
 
             const decls = try Scope.Decls.create(self, &root_scope.base);
             defer decls.base.deref(self);
 
             var decl_group = event.Group(BuildError!void).init(self.loop);
-            // TODO https://github.com/ziglang/zig/issues/1261
-            //errdefer decl_group.cancelAll();
+            var decl_group_consumed = false;
+            errdefer if (!decl_group_consumed) decl_group.cancelAll();
 
             var it = tree.root_node.decls.iterator(0);
             while (it.next()) |decl_ptr| {
@@ -817,6 +825,7 @@ pub const Compilation = struct {
                     else => unreachable,
                 }
             }
+            decl_group_consumed = true;
             try await (async decl_group.wait() catch unreachable);
 
             // Now other code can rely on the decls scope having a complete list of names.
@@ -897,7 +906,7 @@ pub const Compilation = struct {
     }
 
     async fn addTopLevelDecl(self: *Compilation, decls: *Scope.Decls, decl: *Decl) !void {
-        const tree = &decl.findRootScope().tree;
+        const tree = decl.findRootScope().tree;
         const is_export = decl.isExported(tree);
 
         var add_to_table_resolved = false;
@@ -927,25 +936,17 @@ pub const Compilation = struct {
         const text = try std.fmt.allocPrint(self.gpa(), fmt, args);
         errdefer self.gpa().free(text);
 
-        try self.prelink_group.call(addCompileErrorAsync, self, root, span, text);
+        const msg = try Msg.createFromScope(self, root, span, text);
+        errdefer msg.destroy();
+
+        try self.prelink_group.call(addCompileErrorAsync, self, msg);
     }
 
     async fn addCompileErrorAsync(
         self: *Compilation,
-        root: *Scope.Root,
-        span: Span,
-        text: []u8,
+        msg: *Msg,
     ) !void {
-        const relpath = try os.path.relative(self.gpa(), self.event_loop_local.cwd, root.realpath);
-        errdefer self.gpa().free(relpath);
-
-        const msg = try self.gpa().create(errmsg.Msg{
-            .path = if (relpath.len < root.realpath.len) relpath else root.realpath,
-            .text = text,
-            .span = span,
-            .tree = &root.tree,
-        });
-        errdefer self.gpa().destroy(msg);
+        errdefer msg.destroy();
 
         const compile_errors = await (async self.compile_errors.acquire() catch unreachable);
         defer compile_errors.release();
src-self-hosted/errmsg.zig
@@ -4,6 +4,8 @@ const os = std.os;
 const Token = std.zig.Token;
 const ast = std.zig.ast;
 const TokenIndex = std.zig.ast.TokenIndex;
+const Compilation = @import("compilation.zig").Compilation;
+const Scope = @import("scope.zig").Scope;
 
 pub const Color = enum {
     Auto,
@@ -31,77 +33,205 @@ pub const Span = struct {
 };
 
 pub const Msg = struct {
-    path: []const u8,
-    text: []u8,
     span: Span,
-    tree: *ast.Tree,
-};
+    text: []u8,
+    data: Data,
+
+    const Data = union(enum) {
+        PathAndTree: PathAndTree,
+        ScopeAndComp: ScopeAndComp,
+    };
+
+    const PathAndTree = struct {
+        realpath: []const u8,
+        tree: *ast.Tree,
+        allocator: *mem.Allocator,
+    };
+
+    const ScopeAndComp = struct {
+        root_scope: *Scope.Root,
+        compilation: *Compilation,
+    };
+
+    pub fn destroy(self: *Msg) void {
+        switch (self.data) {
+            Data.PathAndTree => |path_and_tree| {
+                path_and_tree.allocator.free(self.text);
+                path_and_tree.allocator.destroy(self);
+            },
+            Data.ScopeAndComp => |scope_and_comp| {
+                scope_and_comp.root_scope.base.deref(scope_and_comp.compilation);
+                scope_and_comp.compilation.gpa().free(self.text);
+                scope_and_comp.compilation.gpa().destroy(self);
+            },
+        }
+    }
+
+    fn getAllocator(self: *const Msg) *mem.Allocator {
+        switch (self.data) {
+            Data.PathAndTree => |path_and_tree| {
+                return path_and_tree.allocator;
+            },
+            Data.ScopeAndComp => |scope_and_comp| {
+                return scope_and_comp.compilation.gpa();
+            },
+        }
+    }
+
+    pub fn getRealPath(self: *const Msg) []const u8 {
+        switch (self.data) {
+            Data.PathAndTree => |path_and_tree| {
+                return path_and_tree.realpath;
+            },
+            Data.ScopeAndComp => |scope_and_comp| {
+                return scope_and_comp.root_scope.realpath;
+            },
+        }
+    }
+
+    pub fn getTree(self: *const Msg) *ast.Tree {
+        switch (self.data) {
+            Data.PathAndTree => |path_and_tree| {
+                return path_and_tree.tree;
+            },
+            Data.ScopeAndComp => |scope_and_comp| {
+                return scope_and_comp.root_scope.tree;
+            },
+        }
+    }
+
+    /// Takes ownership of text
+    /// References root_scope, and derefs when the msg is freed
+    pub fn createFromScope(comp: *Compilation, root_scope: *Scope.Root, span: Span, text: []u8) !*Msg {
+        const msg = try comp.gpa().create(Msg{
+            .text = text,
+            .span = span,
+            .data = Data{
+                .ScopeAndComp = ScopeAndComp{
+                    .root_scope = root_scope,
+                    .compilation = comp,
+                },
+            },
+        });
+        root_scope.base.ref();
+        return msg;
+    }
+
+    pub fn createFromParseErrorAndScope(
+        comp: *Compilation,
+        root_scope: *Scope.Root,
+        parse_error: *const ast.Error,
+    ) !*Msg {
+        const loc_token = parse_error.loc();
+        var text_buf = try std.Buffer.initSize(comp.gpa(), 0);
+        defer text_buf.deinit();
+
+        var out_stream = &std.io.BufferOutStream.init(&text_buf).stream;
+        try parse_error.render(&root_scope.tree.tokens, out_stream);
+
+        const msg = try comp.gpa().create(Msg{
+            .text = undefined,
+            .span = Span{
+                .first = loc_token,
+                .last = loc_token,
+            },
+            .data = Data{
+                .ScopeAndComp = ScopeAndComp{
+                    .root_scope = root_scope,
+                    .compilation = comp,
+                },
+            },
+        });
+        root_scope.base.ref();
+        msg.text = text_buf.toOwnedSlice();
+        return msg;
+    }
+
+    /// `realpath` must outlive the returned Msg
+    /// `tree` must outlive the returned Msg
+    /// Caller owns returned Msg and must free with `allocator`
+    /// allocator will additionally be used for printing messages later.
+    pub fn createFromParseError(
+        allocator: *mem.Allocator,
+        parse_error: *const ast.Error,
+        tree: *ast.Tree,
+        realpath: []const u8,
+    ) !*Msg {
+        const loc_token = parse_error.loc();
+        var text_buf = try std.Buffer.initSize(allocator, 0);
+        defer text_buf.deinit();
+
+        var out_stream = &std.io.BufferOutStream.init(&text_buf).stream;
+        try parse_error.render(&tree.tokens, out_stream);
+
+        const msg = try allocator.create(Msg{
+            .text = undefined,
+            .data = Data{
+                .PathAndTree = PathAndTree{
+                    .allocator = allocator,
+                    .realpath = realpath,
+                    .tree = tree,
+                },
+            },
+            .span = Span{
+                .first = loc_token,
+                .last = loc_token,
+            },
+        });
+        msg.text = text_buf.toOwnedSlice();
+        errdefer allocator.destroy(msg);
+
+        return msg;
+    }
+
+    pub fn printToStream(msg: *const Msg, stream: var, color_on: bool) !void {
+        const allocator = msg.getAllocator();
+        const realpath = msg.getRealPath();
+        const tree = msg.getTree();
+
+        const cwd = try os.getCwd(allocator);
+        defer allocator.free(cwd);
+
+        const relpath = try os.path.relative(allocator, cwd, realpath);
+        defer allocator.free(relpath);
+
+        const path = if (relpath.len < realpath.len) relpath else realpath;
+
+        const first_token = tree.tokens.at(msg.span.first);
+        const last_token = tree.tokens.at(msg.span.last);
+        const start_loc = tree.tokenLocationPtr(0, first_token);
+        const end_loc = tree.tokenLocationPtr(first_token.end, last_token);
+        if (!color_on) {
+            try stream.print(
+                "{}:{}:{}: error: {}\n",
+                path,
+                start_loc.line + 1,
+                start_loc.column + 1,
+                msg.text,
+            );
+            return;
+        }
 
-/// `path` must outlive the returned Msg
-/// `tree` must outlive the returned Msg
-/// Caller owns returned Msg and must free with `allocator`
-pub fn createFromParseError(
-    allocator: *mem.Allocator,
-    parse_error: *const ast.Error,
-    tree: *ast.Tree,
-    path: []const u8,
-) !*Msg {
-    const loc_token = parse_error.loc();
-    var text_buf = try std.Buffer.initSize(allocator, 0);
-    defer text_buf.deinit();
-
-    var out_stream = &std.io.BufferOutStream.init(&text_buf).stream;
-    try parse_error.render(&tree.tokens, out_stream);
-
-    const msg = try allocator.create(Msg{
-        .tree = tree,
-        .path = path,
-        .text = text_buf.toOwnedSlice(),
-        .span = Span{
-            .first = loc_token,
-            .last = loc_token,
-        },
-    });
-    errdefer allocator.destroy(msg);
-
-    return msg;
-}
-
-pub fn printToStream(stream: var, msg: *const Msg, color_on: bool) !void {
-    const first_token = msg.tree.tokens.at(msg.span.first);
-    const last_token = msg.tree.tokens.at(msg.span.last);
-    const start_loc = msg.tree.tokenLocationPtr(0, first_token);
-    const end_loc = msg.tree.tokenLocationPtr(first_token.end, last_token);
-    if (!color_on) {
         try stream.print(
-            "{}:{}:{}: error: {}\n",
-            msg.path,
+            "{}:{}:{}: error: {}\n{}\n",
+            path,
             start_loc.line + 1,
             start_loc.column + 1,
             msg.text,
+            tree.source[start_loc.line_start..start_loc.line_end],
         );
-        return;
+        try stream.writeByteNTimes(' ', start_loc.column);
+        try stream.writeByteNTimes('~', last_token.end - first_token.start);
+        try stream.write("\n");
     }
 
-    try stream.print(
-        "{}:{}:{}: error: {}\n{}\n",
-        msg.path,
-        start_loc.line + 1,
-        start_loc.column + 1,
-        msg.text,
-        msg.tree.source[start_loc.line_start..start_loc.line_end],
-    );
-    try stream.writeByteNTimes(' ', start_loc.column);
-    try stream.writeByteNTimes('~', last_token.end - first_token.start);
-    try stream.write("\n");
-}
-
-pub fn printToFile(file: *os.File, msg: *const Msg, color: Color) !void {
-    const color_on = switch (color) {
-        Color.Auto => file.isTty(),
-        Color.On => true,
-        Color.Off => false,
-    };
-    var stream = &std.io.FileOutStream.init(file).stream;
-    return printToStream(stream, msg, color_on);
-}
+    pub fn printToFile(msg: *const Msg, file: *os.File, color: Color) !void {
+        const color_on = switch (color) {
+            Color.Auto => file.isTty(),
+            Color.On => true,
+            Color.Off => false,
+        };
+        var stream = &std.io.FileOutStream.init(file).stream;
+        return msg.printToStream(stream, color_on);
+    }
+};
src-self-hosted/main.zig
@@ -485,7 +485,8 @@ async fn processBuildEvents(comp: *Compilation, color: errmsg.Color) void {
         },
         Compilation.Event.Fail => |msgs| {
             for (msgs) |msg| {
-                errmsg.printToFile(&stderr_file, msg, color) catch os.exit(1);
+                defer msg.destroy();
+                msg.printToFile(&stderr_file, color) catch os.exit(1);
             }
         },
     }
@@ -646,10 +647,10 @@ fn cmdFmt(allocator: *Allocator, args: []const []const u8) !void {
 
         var error_it = tree.errors.iterator(0);
         while (error_it.next()) |parse_error| {
-            const msg = try errmsg.createFromParseError(allocator, parse_error, &tree, "<stdin>");
-            defer allocator.destroy(msg);
+            const msg = try errmsg.Msg.createFromParseError(allocator, parse_error, &tree, "<stdin>");
+            defer msg.destroy();
 
-            try errmsg.printToFile(&stderr_file, msg, color);
+            try msg.printToFile(&stderr_file, color);
         }
         if (tree.errors.len != 0) {
             os.exit(1);
@@ -702,10 +703,10 @@ fn cmdFmt(allocator: *Allocator, args: []const []const u8) !void {
 
         var error_it = tree.errors.iterator(0);
         while (error_it.next()) |parse_error| {
-            const msg = try errmsg.createFromParseError(allocator, parse_error, &tree, file_path);
-            defer allocator.destroy(msg);
+            const msg = try errmsg.Msg.createFromParseError(allocator, parse_error, &tree, file_path);
+            defer msg.destroy();
 
-            try errmsg.printToFile(&stderr_file, msg, color);
+            try msg.printToFile(&stderr_file, color);
         }
         if (tree.errors.len != 0) {
             fmt.any_error = true;
src-self-hosted/scope.zig
@@ -93,13 +93,13 @@ pub const Scope = struct {
 
     pub const Root = struct {
         base: Scope,
-        tree: ast.Tree,
+        tree: *ast.Tree,
         realpath: []const u8,
 
         /// Creates a Root scope with 1 reference
         /// Takes ownership of realpath
-        /// Caller must set tree
-        pub fn create(comp: *Compilation, tree: ast.Tree, realpath: []u8) !*Root {
+        /// Takes ownership of tree, will deinit and destroy when done.
+        pub fn create(comp: *Compilation, tree: *ast.Tree, realpath: []u8) !*Root {
             const self = try comp.gpa().create(Root{
                 .base = Scope{
                     .id = Id.Root,
@@ -117,6 +117,7 @@ pub const Scope = struct {
         pub fn destroy(self: *Root, comp: *Compilation) void {
             comp.gpa().free(self.tree.source);
             self.tree.deinit();
+            comp.gpa().destroy(self.tree);
             comp.gpa().free(self.realpath);
             comp.gpa().destroy(self);
         }
src-self-hosted/test.zig
@@ -184,7 +184,8 @@ pub const TestContext = struct {
                 var stderr = try std.io.getStdErr();
                 try stderr.write("build incorrectly failed:\n");
                 for (msgs) |msg| {
-                    try errmsg.printToFile(&stderr, msg, errmsg.Color.Auto);
+                    defer msg.destroy();
+                    try msg.printToFile(&stderr, errmsg.Color.Auto);
                 }
             },
         }
@@ -211,10 +212,10 @@ pub const TestContext = struct {
             Compilation.Event.Fail => |msgs| {
                 assertOrPanic(msgs.len != 0);
                 for (msgs) |msg| {
-                    if (mem.endsWith(u8, msg.path, path) and mem.eql(u8, msg.text, text)) {
-                        const first_token = msg.tree.tokens.at(msg.span.first);
-                        const last_token = msg.tree.tokens.at(msg.span.first);
-                        const start_loc = msg.tree.tokenLocationPtr(0, first_token);
+                    if (mem.endsWith(u8, msg.getRealPath(), path) and mem.eql(u8, msg.text, text)) {
+                        const first_token = msg.getTree().tokens.at(msg.span.first);
+                        const last_token = msg.getTree().tokens.at(msg.span.first);
+                        const start_loc = msg.getTree().tokenLocationPtr(0, first_token);
                         if (start_loc.line + 1 == line and start_loc.column + 1 == column) {
                             return;
                         }
@@ -231,7 +232,8 @@ pub const TestContext = struct {
                 std.debug.warn("\n====found:========\n");
                 var stderr = try std.io.getStdErr();
                 for (msgs) |msg| {
-                    try errmsg.printToFile(&stderr, msg, errmsg.Color.Auto);
+                    defer msg.destroy();
+                    try msg.printToFile(&stderr, errmsg.Color.Auto);
                 }
                 std.debug.warn("============\n");
                 return error.TestFailed;
std/mem.zig
@@ -35,6 +35,7 @@ pub const Allocator = struct {
     freeFn: fn (self: *Allocator, old_mem: []u8) void,
 
     /// Call `destroy` with the result
+    /// TODO this is deprecated. use createOne instead
     pub fn create(self: *Allocator, init: var) Error!*@typeOf(init) {
         const T = @typeOf(init);
         if (@sizeOf(T) == 0) return &(T{});
@@ -44,6 +45,14 @@ pub const Allocator = struct {
         return ptr;
     }
 
+    /// Call `destroy` with the result.
+    /// Returns undefined memory.
+    pub fn createOne(self: *Allocator, comptime T: type) Error!*T {
+        if (@sizeOf(T) == 0) return &(T{});
+        const slice = try self.alloc(T, 1);
+        return &slice[0];
+    }
+
     /// `ptr` should be the return value of `create`
     pub fn destroy(self: *Allocator, ptr: var) void {
         const non_const_ptr = @intToPtr([*]u8, @ptrToInt(ptr));
@@ -149,13 +158,12 @@ pub fn copyBackwards(comptime T: type, dest: []T, source: []const T) void {
     @setRuntimeSafety(false);
     assert(dest.len >= source.len);
     var i = source.len;
-    while(i > 0){
+    while (i > 0) {
         i -= 1;
         dest[i] = source[i];
     }
 }
 
-
 pub fn set(comptime T: type, dest: []T, value: T) void {
     for (dest) |*d|
         d.* = value;