From 50960fac80b1d04f7858215d963fa64a7583210b Mon Sep 17 00:00:00 2001 From: mlugg Date: Tue, 13 Aug 2024 20:20:38 +0100 Subject: [PATCH] compiler: be more cautious about source locations Two fixes here. * Prevent a crash when sorting the list of analysis errors when some errors refer to lost source locations. These errors can be sorted anywhere in the list, because they are (in theory) guaranteed to never be emitted by the `resolveReferences` logic. This case occurs, for instance, when a declaration has compile errors in the initial update and is deleted in the second update. * Prevent a crash when resolving the source location for `entire_file` errors for a non-existent file. This is the bug underlying #20954. Resolves: #20954. --- src/Compilation.zig | 10 ++++++++-- src/Sema.zig | 4 ++-- src/Zcu.zig | 15 ++++++++++++--- src/crash_report.zig | 16 ++++++++++++++-- 4 files changed, 36 insertions(+), 9 deletions(-) diff --git a/src/Compilation.zig b/src/Compilation.zig index 5c49e96db22e9f78b5532e968118ff24e869c581..8787d679e6ca3c4da57af00c4ed39cb188d4c0bb 100644 --- a/src/Compilation.zig +++ b/src/Compilation.zig @@ -3203,8 +3203,14 @@ pub fn getAllErrorsAlloc(comp: *Compilation) !ErrorBundle { pub fn lessThan(ctx: @This(), lhs_index: usize, rhs_index: usize) bool { if (ctx.err.*) |_| return lhs_index < rhs_index; const errors = ctx.zcu.failed_analysis.values(); - const lhs_src_loc = errors[lhs_index].src_loc.upgrade(ctx.zcu); - const rhs_src_loc = errors[rhs_index].src_loc.upgrade(ctx.zcu); + const lhs_src_loc = errors[lhs_index].src_loc.upgradeOrLost(ctx.zcu) orelse { + // LHS source location lost, so should never be referenced. Just sort it to the end. + return false; + }; + const rhs_src_loc = errors[rhs_index].src_loc.upgradeOrLost(ctx.zcu) orelse { + // RHS source location lost, so should never be referenced. Just sort it to the end. + return true; + }; return if (lhs_src_loc.file_scope != rhs_src_loc.file_scope) std.mem.order( u8, lhs_src_loc.file_scope.sub_file_path, diff --git a/src/Sema.zig b/src/Sema.zig index 9b6ff7cd8efdc3801080654834ee19b0d9f1fb81..a679f69a9ca20b53984e18fcb71084ba201c4715 100644 --- a/src/Sema.zig +++ b/src/Sema.zig @@ -17729,7 +17729,7 @@ fn zirClosureGet(sema: *Sema, block: *Block, extended: Zir.Inst.Extended.InstDat const msg = msg: { const name = name: { // TODO: we should probably store this name in the ZIR to avoid this complexity. - const file, const src_base_node = Module.LazySrcLoc.resolveBaseNode(block.src_base_inst, mod); + const file, const src_base_node = Module.LazySrcLoc.resolveBaseNode(block.src_base_inst, mod).?; const tree = file.getTree(sema.gpa) catch |err| { // In this case we emit a warning + a less precise source location. log.warn("unable to load {s}: {s}", .{ @@ -17757,7 +17757,7 @@ fn zirClosureGet(sema: *Sema, block: *Block, extended: Zir.Inst.Extended.InstDat if (!block.is_typeof and !block.is_comptime and sema.func_index != .none) { const msg = msg: { const name = name: { - const file, const src_base_node = Module.LazySrcLoc.resolveBaseNode(block.src_base_inst, mod); + const file, const src_base_node = Module.LazySrcLoc.resolveBaseNode(block.src_base_inst, mod).?; const tree = file.getTree(sema.gpa) catch |err| { // In this case we emit a warning + a less precise source location. log.warn("unable to load {s}: {s}", .{ diff --git a/src/Zcu.zig b/src/Zcu.zig index c78abb69bfc157d262c3bec0cc5a3d83edf42021..8626a147b617894f356adf825dc3b876a406ec7c 100644 --- a/src/Zcu.zig +++ b/src/Zcu.zig @@ -2042,10 +2042,11 @@ pub const LazySrcLoc = struct { .offset = .unneeded, }; - pub fn resolveBaseNode(base_node_inst: InternPool.TrackedInst.Index, zcu: *Zcu) struct { *File, Ast.Node.Index } { + /// Returns `null` if the ZIR instruction has been lost across incremental updates. + pub fn resolveBaseNode(base_node_inst: InternPool.TrackedInst.Index, zcu: *Zcu) ?struct { *File, Ast.Node.Index } { const ip = &zcu.intern_pool; const file_index, const zir_inst = inst: { - const info = base_node_inst.resolveFull(ip) orelse @panic("TODO: resolve source location relative to lost inst"); + const info = base_node_inst.resolveFull(ip) orelse return null; break :inst .{ info.file, info.inst }; }; const file = zcu.fileByIndex(file_index); @@ -2071,7 +2072,15 @@ pub const LazySrcLoc = struct { /// Resolve the file and AST node of `base_node_inst` to get a resolved `SrcLoc`. /// The resulting `SrcLoc` should only be used ephemerally, as it is not correct across incremental updates. pub fn upgrade(lazy: LazySrcLoc, zcu: *Zcu) SrcLoc { - const file, const base_node = resolveBaseNode(lazy.base_node_inst, zcu); + return lazy.upgradeOrLost(zcu).?; + } + + /// Like `upgrade`, but returns `null` if the source location has been lost across incremental updates. + pub fn upgradeOrLost(lazy: LazySrcLoc, zcu: *Zcu) ?SrcLoc { + const file, const base_node: Ast.Node.Index = if (lazy.offset == .entire_file) .{ + zcu.fileByIndex(lazy.base_node_inst.resolveFile(&zcu.intern_pool)), + 0, + } else resolveBaseNode(lazy.base_node_inst, zcu) orelse return null; return .{ .file_scope = file, .base_node = base_node, diff --git a/src/crash_report.zig b/src/crash_report.zig index d4fe72a8e8a7abf88b8f0d79150b56d81aa603db..67ec0e0eb0460ff32626ec01b9d04b6fa72989a9 100644 --- a/src/crash_report.zig +++ b/src/crash_report.zig @@ -78,7 +78,13 @@ fn dumpStatusReport() !void { const block: *Sema.Block = anal.block; const zcu = anal.sema.pt.zcu; - const file, const src_base_node = Zcu.LazySrcLoc.resolveBaseNode(block.src_base_inst, zcu); + const file, const src_base_node = Zcu.LazySrcLoc.resolveBaseNode(block.src_base_inst, zcu) orelse { + const file = zcu.fileByIndex(block.src_base_inst.resolveFile(&zcu.intern_pool)); + try stderr.writeAll("Analyzing lost instruction in file '"); + try writeFilePath(file, stderr); + try stderr.writeAll("'. This should not happen!\n\n"); + return; + }; try stderr.writeAll("Analyzing "); try writeFilePath(file, stderr); @@ -104,7 +110,13 @@ fn dumpStatusReport() !void { while (parent) |curr| { fba.reset(); try stderr.writeAll(" in "); - const cur_block_file, const cur_block_src_base_node = Zcu.LazySrcLoc.resolveBaseNode(curr.block.src_base_inst, zcu); + const cur_block_file, const cur_block_src_base_node = Zcu.LazySrcLoc.resolveBaseNode(curr.block.src_base_inst, zcu) orelse { + const cur_block_file = zcu.fileByIndex(curr.block.src_base_inst.resolveFile(&zcu.intern_pool)); + try writeFilePath(cur_block_file, stderr); + try stderr.writeAll("\n > [lost instruction; this should not happen]\n"); + parent = curr.parent; + continue; + }; try writeFilePath(cur_block_file, stderr); try stderr.writeAll("\n > "); print_zir.renderSingleInstruction( -- 2.54.0