From f75d4cbe56f9f8212581f00700600a57ce545ba1 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Mon, 28 Dec 2020 11:24:53 -0700 Subject: [PATCH] Revert "stage2: add compile log statement (#7191)" The addition of `addDeclErr` introduced a memory leak at every call site, and I also would like to push back on having more than 1 compilation error per `Decl`. This reverts commit 1634d45f1d53c8d7bfefa56ab4d2fa4cc8218b6d. --- src/Compilation.zig | 25 ++++++++++++------------ src/Module.zig | 46 ++++++++++++++++---------------------------- src/astgen.zig | 13 ------------- src/codegen/c.zig | 2 +- src/link/C.zig | 2 +- src/link/Coff.zig | 2 +- src/link/Elf.zig | 2 +- src/link/MachO.zig | 2 +- src/value.zig | 2 +- src/zir.zig | 25 ++++-------------------- src/zir_sema.zig | 23 ---------------------- test/stage2/test.zig | 16 +-------------- 12 files changed, 40 insertions(+), 120 deletions(-) diff --git a/src/Compilation.zig b/src/Compilation.zig index 6b5ae3abed65..885ecbb95c7a 100644 --- a/src/Compilation.zig +++ b/src/Compilation.zig @@ -1350,11 +1350,8 @@ pub fn totalErrorCount(self: *Compilation) usize { var total: usize = self.failed_c_objects.items().len; if (self.bin_file.options.module) |module| { - for (module.failed_decls.items()) |entry| { - assert(entry.value.items.len > 0); - total += entry.value.items.len; - } - total += module.failed_exports.items().len + + total += module.failed_decls.items().len + + module.failed_exports.items().len + module.failed_files.items().len + @boolToInt(module.failed_root_src_file != null); } @@ -1388,11 +1385,9 @@ pub fn getAllErrorsAlloc(self: *Compilation) !AllErrors { } for (module.failed_decls.items()) |entry| { const decl = entry.key; - const err_msg_list = entry.value; - for (err_msg_list.items) |err_msg| { - const source = try decl.scope.getSource(module); - try AllErrors.add(&arena, &errors, decl.scope.subFilePath(), source, err_msg.*); - } + const err_msg = entry.value; + const source = try decl.scope.getSource(module); + try AllErrors.add(&arena, &errors, decl.scope.subFilePath(), source, err_msg.*); } for (module.failed_exports.items()) |entry| { const decl = entry.key.owner_decl; @@ -1485,6 +1480,7 @@ pub fn performAllTheWork(self: *Compilation) error{ TimerUnsupported, OutOfMemor } assert(decl.typed_value.most_recent.typed_value.ty.hasCodeGenBits()); + self.bin_file.updateDecl(module, decl) catch |err| { switch (err) { error.OutOfMemory => return error.OutOfMemory, @@ -1492,7 +1488,8 @@ pub fn performAllTheWork(self: *Compilation) error{ TimerUnsupported, OutOfMemor decl.analysis = .dependency_failure; }, else => { - try module.addDeclErr(decl, try ErrorMsg.create( + try module.failed_decls.ensureCapacity(module.gpa, module.failed_decls.items().len + 1); + module.failed_decls.putAssumeCapacityNoClobber(decl, try ErrorMsg.create( module.gpa, decl.src(), "unable to codegen: {}", @@ -1511,7 +1508,8 @@ pub fn performAllTheWork(self: *Compilation) error{ TimerUnsupported, OutOfMemor decl.analysis = .dependency_failure; }, else => { - try module.addDeclErr(decl, try ErrorMsg.create( + try module.failed_decls.ensureCapacity(module.gpa, module.failed_decls.items().len + 1); + module.failed_decls.putAssumeCapacityNoClobber(decl, try ErrorMsg.create( module.gpa, decl.src(), "unable to generate C header: {}", @@ -1533,7 +1531,8 @@ pub fn performAllTheWork(self: *Compilation) error{ TimerUnsupported, OutOfMemor .update_line_number => |decl| { const module = self.bin_file.options.module.?; self.bin_file.updateDeclLineNumber(module, decl) catch |err| { - try module.addDeclErr(decl, try ErrorMsg.create( + try module.failed_decls.ensureCapacity(module.gpa, module.failed_decls.items().len + 1); + module.failed_decls.putAssumeCapacityNoClobber(decl, try ErrorMsg.create( module.gpa, decl.src(), "unable to update line number: {}", diff --git a/src/Module.zig b/src/Module.zig index 5a4a76150ce3..c0b7011f43df 100644 --- a/src/Module.zig +++ b/src/Module.zig @@ -53,7 +53,7 @@ decl_table: std.ArrayHashMapUnmanaged(Scope.NameHash, *Decl, Scope.name_hash_has /// The ErrorMsg memory is owned by the decl, using Module's general purpose allocator. /// Note that a Decl can succeed but the Fn it represents can fail. In this case, /// a Decl can have a failed_decls entry but have analysis status of success. -failed_decls: std.AutoArrayHashMapUnmanaged(*Decl, ArrayListUnmanaged(*Compilation.ErrorMsg)) = .{}, +failed_decls: std.AutoArrayHashMapUnmanaged(*Decl, *Compilation.ErrorMsg) = .{}, /// Using a map here for consistency with the other fields here. /// The ErrorMsg memory is owned by the `Scope`, using Module's general purpose allocator. failed_files: std.AutoArrayHashMapUnmanaged(*Scope, *Compilation.ErrorMsg) = .{}, @@ -849,11 +849,8 @@ pub fn deinit(self: *Module) void { } self.decl_table.deinit(gpa); - for (self.failed_decls.items()) |*entry| { - for (entry.value.items) |compile_err| { - compile_err.destroy(gpa); - } - entry.value.deinit(gpa); + for (self.failed_decls.items()) |entry| { + entry.value.destroy(gpa); } self.failed_decls.deinit(gpa); @@ -949,7 +946,8 @@ pub fn ensureDeclAnalyzed(self: *Module, decl: *Decl) InnerError!void { error.OutOfMemory => return error.OutOfMemory, error.AnalysisFail => return error.AnalysisFail, else => { - try self.addDeclErr(decl, try Compilation.ErrorMsg.create( + try self.failed_decls.ensureCapacity(self.gpa, self.failed_decls.items().len + 1); + self.failed_decls.putAssumeCapacityNoClobber(decl, try Compilation.ErrorMsg.create( self.gpa, decl.src(), "unable to analyze: {}", @@ -1556,7 +1554,7 @@ pub fn analyzeContainer(self: *Module, container_scope: *Scope.Container) !void decl.analysis = .sema_failure; const err_msg = try Compilation.ErrorMsg.create(self.gpa, tree.token_locs[name_tok].start, "redefinition of '{}'", .{decl.name}); errdefer err_msg.destroy(self.gpa); - try self.addDeclErr(decl, err_msg); + try self.failed_decls.putNoClobber(self.gpa, decl, err_msg); } else { if (!srcHashEql(decl.contents_hash, contents_hash)) { try self.markOutdatedDecl(decl); @@ -1598,7 +1596,7 @@ pub fn analyzeContainer(self: *Module, container_scope: *Scope.Container) !void decl.analysis = .sema_failure; const err_msg = try Compilation.ErrorMsg.create(self.gpa, name_loc.start, "redefinition of '{}'", .{decl.name}); errdefer err_msg.destroy(self.gpa); - try self.addDeclErr(decl, err_msg); + try self.failed_decls.putNoClobber(self.gpa, decl, err_msg); } else if (!srcHashEql(decl.contents_hash, contents_hash)) { try self.markOutdatedDecl(decl); decl.contents_hash = contents_hash; @@ -1724,11 +1722,8 @@ pub fn deleteDecl(self: *Module, decl: *Decl) !void { try self.markOutdatedDecl(dep); } } - if (self.failed_decls.remove(decl)) |*entry| { - for (entry.value.items) |compile_err| { - compile_err.destroy(self.gpa); - } - entry.value.deinit(self.gpa); + if (self.failed_decls.remove(decl)) |entry| { + entry.value.destroy(self.gpa); } self.deleteDeclExports(decl); self.comp.bin_file.freeDecl(decl); @@ -1807,11 +1802,8 @@ pub fn analyzeFnBody(self: *Module, decl: *Decl, func: *Fn) !void { fn markOutdatedDecl(self: *Module, decl: *Decl) !void { log.debug("mark {} outdated\n", .{decl.name}); try self.comp.work_queue.writeItem(.{ .analyze_decl = decl }); - if (self.failed_decls.remove(decl)) |*entry| { - for (entry.value.items) |compile_err| { - compile_err.destroy(self.gpa); - } - entry.value.deinit(self.gpa); + if (self.failed_decls.remove(decl)) |entry| { + entry.value.destroy(self.gpa); } decl.analysis = .outdated; } @@ -2956,14 +2948,10 @@ pub fn failNode( return self.fail(scope, src, format, args); } -pub fn addDeclErr(self: *Module, decl: *Decl, err: *Compilation.ErrorMsg) error{OutOfMemory}!void { - const entry = try self.failed_decls.getOrPutValue(self.gpa, decl, .{}); - try entry.value.append(self.gpa, err); -} - fn failWithOwnedErrorMsg(self: *Module, scope: *Scope, src: usize, err_msg: *Compilation.ErrorMsg) InnerError { { errdefer err_msg.destroy(self.gpa); + try self.failed_decls.ensureCapacity(self.gpa, self.failed_decls.items().len + 1); try self.failed_files.ensureCapacity(self.gpa, self.failed_files.items().len + 1); } switch (scope.tag) { @@ -2971,7 +2959,7 @@ fn failWithOwnedErrorMsg(self: *Module, scope: *Scope, src: usize, err_msg: *Com const decl = scope.cast(Scope.DeclAnalysis).?.decl; decl.analysis = .sema_failure; decl.generation = self.generation; - try self.addDeclErr(decl, err_msg); + self.failed_decls.putAssumeCapacityNoClobber(decl, err_msg); }, .block => { const block = scope.cast(Scope.Block).?; @@ -2981,25 +2969,25 @@ fn failWithOwnedErrorMsg(self: *Module, scope: *Scope, src: usize, err_msg: *Com block.decl.analysis = .sema_failure; block.decl.generation = self.generation; } - try self.addDeclErr(block.decl, err_msg); + self.failed_decls.putAssumeCapacityNoClobber(block.decl, err_msg); }, .gen_zir => { const gen_zir = scope.cast(Scope.GenZIR).?; gen_zir.decl.analysis = .sema_failure; gen_zir.decl.generation = self.generation; - try self.addDeclErr(gen_zir.decl, err_msg); + self.failed_decls.putAssumeCapacityNoClobber(gen_zir.decl, err_msg); }, .local_val => { const gen_zir = scope.cast(Scope.LocalVal).?.gen_zir; gen_zir.decl.analysis = .sema_failure; gen_zir.decl.generation = self.generation; - try self.addDeclErr(gen_zir.decl, err_msg); + self.failed_decls.putAssumeCapacityNoClobber(gen_zir.decl, err_msg); }, .local_ptr => { const gen_zir = scope.cast(Scope.LocalPtr).?.gen_zir; gen_zir.decl.analysis = .sema_failure; gen_zir.decl.generation = self.generation; - try self.addDeclErr(gen_zir.decl, err_msg); + self.failed_decls.putAssumeCapacityNoClobber(gen_zir.decl, err_msg); }, .zir_module => { const zir_module = scope.cast(Scope.ZIRModule).?; diff --git a/src/astgen.zig b/src/astgen.zig index 100b2b9d02dd..672fe343e27b 100644 --- a/src/astgen.zig +++ b/src/astgen.zig @@ -2333,17 +2333,6 @@ fn compileError(mod: *Module, scope: *Scope, call: *ast.Node.BuiltinCall) InnerE return addZIRUnOp(mod, scope, src, .compileerror, target); } -fn compileLog(mod: *Module, scope: *Scope, call: *ast.Node.BuiltinCall) InnerError!*zir.Inst { - const tree = scope.tree(); - const arena = scope.arena(); - const src = tree.token_locs[call.builtin_token].start; - const params = call.params(); - var targets = try arena.alloc(*zir.Inst, params.len); - for (params) |param, param_i| - targets[param_i] = try expr(mod, scope, .none, param); - return addZIRInst(mod, scope, src, zir.Inst.CompileLog, .{ .to_log = targets }, .{}); -} - fn typeOf(mod: *Module, scope: *Scope, rl: ResultLoc, call: *ast.Node.BuiltinCall) InnerError!*zir.Inst { const tree = scope.tree(); const arena = scope.arena(); @@ -2389,8 +2378,6 @@ fn builtinCall(mod: *Module, scope: *Scope, rl: ResultLoc, call: *ast.Node.Built return rlWrap(mod, scope, rl, try import(mod, scope, call)); } else if (mem.eql(u8, builtin_name, "@compileError")) { return compileError(mod, scope, call); - } else if (mem.eql(u8, builtin_name, "@compileLog")) { - return compileLog(mod, scope, call); } else { return mod.failTok(scope, call.builtin_token, "invalid builtin function: '{}'", .{builtin_name}); } diff --git a/src/codegen/c.zig b/src/codegen/c.zig index 73ac3e2f3810..589e2f17e0f3 100644 --- a/src/codegen/c.zig +++ b/src/codegen/c.zig @@ -106,7 +106,7 @@ pub fn generateHeader( const writer = header.buf.writer(); renderFunctionSignature(&ctx, header, writer, decl) catch |err| { if (err == error.AnalysisFail) { - try module.addDeclErr(decl, ctx.error_msg); + try module.failed_decls.put(module.gpa, decl, ctx.error_msg); } return err; }; diff --git a/src/link/C.zig b/src/link/C.zig index cadbcf105f0d..325f2ee0484b 100644 --- a/src/link/C.zig +++ b/src/link/C.zig @@ -106,7 +106,7 @@ pub fn deinit(self: *C) void { pub fn updateDecl(self: *C, module: *Module, decl: *Module.Decl) !void { codegen.generate(self, decl) catch |err| { if (err == error.AnalysisFail) { - try module.addDeclErr(decl, self.error_msg); + try module.failed_decls.put(module.gpa, decl, self.error_msg); } return err; }; diff --git a/src/link/Coff.zig b/src/link/Coff.zig index 5ec428462357..bea3033f8860 100644 --- a/src/link/Coff.zig +++ b/src/link/Coff.zig @@ -658,7 +658,7 @@ pub fn updateDecl(self: *Coff, module: *Module, decl: *Module.Decl) !void { .appended => code_buffer.items, .fail => |em| { decl.analysis = .codegen_failure; - try module.addDeclErr(decl, em); + try module.failed_decls.put(module.gpa, decl, em); return; }, }; diff --git a/src/link/Elf.zig b/src/link/Elf.zig index 2cd668c09fdd..6232a64e4af1 100644 --- a/src/link/Elf.zig +++ b/src/link/Elf.zig @@ -2248,7 +2248,7 @@ pub fn updateDecl(self: *Elf, module: *Module, decl: *Module.Decl) !void { .appended => code_buffer.items, .fail => |em| { decl.analysis = .codegen_failure; - try module.addDeclErr(decl, em); + try module.failed_decls.put(module.gpa, decl, em); return; }, }; diff --git a/src/link/MachO.zig b/src/link/MachO.zig index 8c3001938fed..17816959a340 100644 --- a/src/link/MachO.zig +++ b/src/link/MachO.zig @@ -1062,7 +1062,7 @@ pub fn updateDecl(self: *MachO, module: *Module, decl: *Module.Decl) !void { .appended => code_buffer.items, .fail => |em| { decl.analysis = .codegen_failure; - try module.addDeclErr(decl, em); + try module.failed_decls.put(module.gpa, decl, em); return; }, }; diff --git a/src/value.zig b/src/value.zig index 56b343440013..4271ae66f4bc 100644 --- a/src/value.zig +++ b/src/value.zig @@ -350,7 +350,7 @@ pub const Value = extern union { val = elem_ptr.array_ptr; }, .empty_array => return out_stream.writeAll(".{}"), - .enum_literal => return out_stream.print(".{z}", .{@fieldParentPtr(Payload.Bytes, "base", self.ptr_otherwise).data}), + .enum_literal => return out_stream.print(".{z}", .{self.cast(Payload.Bytes).?.data}), .bytes => return out_stream.print("\"{Z}\"", .{self.cast(Payload.Bytes).?.data}), .repeated => { try out_stream.writeAll("(repeated) "); diff --git a/src/zir.zig b/src/zir.zig index 10f64be609cc..47e1abc24b6d 100644 --- a/src/zir.zig +++ b/src/zir.zig @@ -126,8 +126,6 @@ pub const Inst = struct { coerce_to_ptr_elem, /// Emit an error message and fail compilation. compileerror, - /// Log compile time variables and emit an error message. - compilelog, /// Conditional branch. Splits control flow based on a boolean condition value. condbr, /// Special case, has no textual representation. @@ -394,7 +392,6 @@ pub const Inst = struct { .declval => DeclVal, .declval_in_module => DeclValInModule, .coerce_result_block_ptr => CoerceResultBlockPtr, - .compilelog => CompileLog, .loop => Loop, .@"const" => Const, .str => Str, @@ -524,7 +521,6 @@ pub const Inst = struct { .slice_start, .import, .switch_range, - .compilelog, .typeof_peer, => false, @@ -709,19 +705,6 @@ pub const Inst = struct { kw_args: struct {}, }; - pub const CompileLog = struct { - pub const base_tag = Tag.compilelog; - base: Inst, - - positionals: struct { - to_log: []*Inst, - }, - kw_args: struct { - /// If we have seen it already so don't make another error - seen: bool = false, - }, - }; - pub const Const = struct { pub const base_tag = Tag.@"const"; base: Inst, @@ -1940,7 +1923,7 @@ const EmitZIR = struct { .sema_failure, .sema_failure_retryable, .dependency_failure, - => if (self.old_module.failed_decls.get(ir_decl)) |err_msg_list| { + => if (self.old_module.failed_decls.get(ir_decl)) |err_msg| { const fail_inst = try self.arena.allocator.create(Inst.UnOp); fail_inst.* = .{ .base = .{ @@ -1949,7 +1932,7 @@ const EmitZIR = struct { }, .positionals = .{ .operand = blk: { - const msg_str = try self.arena.allocator.dupe(u8, err_msg_list.items[0].msg); + const msg_str = try self.arena.allocator.dupe(u8, err_msg.msg); const str_inst = try self.arena.allocator.create(Inst.Str); str_inst.* = .{ @@ -1958,7 +1941,7 @@ const EmitZIR = struct { .tag = Inst.Str.base_tag, }, .positionals = .{ - .bytes = msg_str, + .bytes = err_msg.msg, }, .kw_args = .{}, }; @@ -2085,7 +2068,7 @@ const EmitZIR = struct { try self.emitBody(body, &inst_table, &instructions); }, .sema_failure => { - const err_msg = self.old_module.failed_decls.get(module_fn.owner_decl).?.items[0]; + const err_msg = self.old_module.failed_decls.get(module_fn.owner_decl).?; const fail_inst = try self.arena.allocator.create(Inst.UnOp); fail_inst.* = .{ .base = .{ diff --git a/src/zir_sema.zig b/src/zir_sema.zig index f3dae8f5b1b9..5700c1857883 100644 --- a/src/zir_sema.zig +++ b/src/zir_sema.zig @@ -45,7 +45,6 @@ pub fn analyzeInst(mod: *Module, scope: *Scope, old_inst: *zir.Inst) InnerError! .coerce_result_ptr => return analyzeInstCoerceResultPtr(mod, scope, old_inst.castTag(.coerce_result_ptr).?), .coerce_to_ptr_elem => return analyzeInstCoerceToPtrElem(mod, scope, old_inst.castTag(.coerce_to_ptr_elem).?), .compileerror => return analyzeInstCompileError(mod, scope, old_inst.castTag(.compileerror).?), - .compilelog => return analyzeInstCompileLog(mod, scope, old_inst.castTag(.compilelog).?), .@"const" => return analyzeInstConst(mod, scope, old_inst.castTag(.@"const").?), .dbg_stmt => return analyzeInstDbgStmt(mod, scope, old_inst.castTag(.dbg_stmt).?), .declref => return analyzeInstDeclRef(mod, scope, old_inst.castTag(.declref).?), @@ -503,28 +502,6 @@ fn analyzeInstCompileError(mod: *Module, scope: *Scope, inst: *zir.Inst.UnOp) In return mod.fail(scope, inst.base.src, "{}", .{msg}); } -fn analyzeInstCompileLog(mod: *Module, scope: *Scope, inst: *zir.Inst.CompileLog) InnerError!*Inst { - std.debug.print("| ", .{}); - for (inst.positionals.to_log) |item, i| { - const to_log = try resolveInst(mod, scope, item); - if (to_log.value()) |val| { - std.debug.print("{}", .{val}); - } else { - std.debug.print("(runtime value)", .{}); - } - if (i != inst.positionals.to_log.len - 1) std.debug.print(", ", .{}); - } - std.debug.print("\n", .{}); - if (!inst.kw_args.seen) { - inst.kw_args.seen = true; // so that we do not give multiple compile errors if it gets evaled twice - switch (mod.fail(scope, inst.base.src, "found compile log statement", .{})) { - error.AnalysisFail => {}, // analysis continues - else => |e| return e, - } - } - return mod.constVoid(scope, inst.base.src); -} - fn analyzeInstArg(mod: *Module, scope: *Scope, inst: *zir.Inst.Arg) InnerError!*Inst { const b = try mod.requireRuntimeBlock(scope, inst.base.src); const fn_ty = b.func.?.owner_decl.typed_value.most_recent.typed_value.ty; diff --git a/test/stage2/test.zig b/test/stage2/test.zig index 4e57bbf9f7e6..5a200e9cf4f5 100644 --- a/test/stage2/test.zig +++ b/test/stage2/test.zig @@ -1171,27 +1171,13 @@ pub fn addCases(ctx: *TestContext) !void { \\fn entry() void {} , &[_][]const u8{":2:4: error: redefinition of 'entry'"}); - ctx.compileError("compileLog", linux_x64, - \\export fn _start() noreturn { - \\ const b = true; - \\ var f: u32 = 1; - \\ @compileLog(b, 20, f, x, .foo); - \\ var y: u32 = true; - \\ unreachable; - \\} - \\fn x() void {} - , &[_][]const u8{ - ":4:3: error: found compile log statement", ":5:16: error: expected u32, found bool", - }); - - // "| true, 20, (runtime value), (function)" // TODO if this is here it invalidates the compile error checker. Need a way to check though. - ctx.compileError("compileError", linux_x64, \\export fn _start() noreturn { \\ @compileError("this is an error"); \\ unreachable; \\} , &[_][]const u8{":2:3: error: this is an error"}); + { var case = ctx.obj("variable shadowing", linux_x64); case.addError(