| author |  Matthew Lugg <mlugg@mlugg.co.uk> 2025-11-18 12:03:06+00:00 |
| committer | Matthew Lugg <mlugg@mlugg.co.uk> 2025-11-19 09:44:22+00:00 |
| log | 806470b492f15c4e3350ce4a48e607f2c8e94ff8 |
| tree | 3bbc7184dc62cc4f1e583c9e907620517f44e4c7 |
| parent | 4ea472808437c932f6240c0df5e4d5912d282052 |
| signature |  Commit is signed but in an unrecognized format. |
compiler: fix crash if file contents change during update
When reporting a compile error, we would load the new file, but assume
we could apply old AST/token indices (etc) to it, potentially causing
crashes. Instead, if the file stat has changed since it was loaded, just
emit an error that the file was modified mid-update.2 files changed, 48 insertions(+), 39 deletions(-)
src/Compilation.zig+10-9
| ... | @@ -3935,17 +3935,13 @@ pub fn getAllErrorsAlloc(comp: *Compilation) error{OutOfMemory}!ErrorBundle { | ... | @@ -3935,17 +3935,13 @@ pub fn getAllErrorsAlloc(comp: *Compilation) error{OutOfMemory}!ErrorBundle { |
| 3935 | for (zcu.failed_imports.items) |failed| { | 3935 | for (zcu.failed_imports.items) |failed| { |
| 3936 | assert(zcu.alive_files.contains(failed.file_index)); // otherwise it wouldn't have been added | 3936 | assert(zcu.alive_files.contains(failed.file_index)); // otherwise it wouldn't have been added |
| 3937 | const file = zcu.fileByIndex(failed.file_index); | 3937 | const file = zcu.fileByIndex(failed.file_index); |
| 3938 | const source = file.getSource(zcu) catch |err| { | ||
| 3939 | try unableToLoadZcuFile(zcu, &bundle, file, err); | ||
| 3940 | continue; | ||
| 3941 | }; | ||
| 3942 | const tree = file.getTree(zcu) catch |err| { | 3938 | const tree = file.getTree(zcu) catch |err| { |
| 3943 | try unableToLoadZcuFile(zcu, &bundle, file, err); | 3939 | try unableToLoadZcuFile(zcu, &bundle, file, err); |
| 3944 | continue; | 3940 | continue; |
| 3945 | }; | 3941 | }; |
| 3946 | const start = tree.tokenStart(failed.import_token); | 3942 | const start = tree.tokenStart(failed.import_token); |
| 3947 | const end = start + tree.tokenSlice(failed.import_token).len; | 3943 | const end = start + tree.tokenSlice(failed.import_token).len; |
| 3948 | const loc = std.zig.findLineColumn(source.bytes, start); | 3944 | const loc = std.zig.findLineColumn(tree.source, start); |
| 3949 | try bundle.addRootErrorMessage(.{ | 3945 | try bundle.addRootErrorMessage(.{ |
| 3950 | .msg = switch (failed.kind) { | 3946 | .msg = switch (failed.kind) { |
| 3951 | .file_outside_module_root => try bundle.addString("import of file outside module path"), | 3947 | .file_outside_module_root => try bundle.addString("import of file outside module path"), |
| ... | @@ -4338,7 +4334,7 @@ pub fn addModuleErrorMsg( | ... | @@ -4338,7 +4334,7 @@ pub fn addModuleErrorMsg( |
| 4338 | const err_span = err_src_loc.span(zcu) catch |err| { | 4334 | const err_span = err_src_loc.span(zcu) catch |err| { |
| 4339 | return unableToLoadZcuFile(zcu, eb, err_src_loc.file_scope, err); | 4335 | return unableToLoadZcuFile(zcu, eb, err_src_loc.file_scope, err); |
| 4340 | }; | 4336 | }; |
| 4341 | const err_loc = std.zig.findLineColumn(err_source.bytes, err_span.main); | 4337 | const err_loc = std.zig.findLineColumn(err_source, err_span.main); |
| 4342 | 4338 | ||
| 4343 | var ref_traces: std.ArrayListUnmanaged(ErrorBundle.ReferenceTrace) = .empty; | 4339 | var ref_traces: std.ArrayListUnmanaged(ErrorBundle.ReferenceTrace) = .empty; |
| 4344 | defer ref_traces.deinit(gpa); | 4340 | defer ref_traces.deinit(gpa); |
| ... | @@ -4434,7 +4430,7 @@ pub fn addModuleErrorMsg( | ... | @@ -4434,7 +4430,7 @@ pub fn addModuleErrorMsg( |
| 4434 | const span = note_src_loc.span(zcu) catch |err| { | 4430 | const span = note_src_loc.span(zcu) catch |err| { |
| 4435 | return unableToLoadZcuFile(zcu, eb, note_src_loc.file_scope, err); | 4431 | return unableToLoadZcuFile(zcu, eb, note_src_loc.file_scope, err); |
| 4436 | }; | 4432 | }; |
| 4437 | const loc = std.zig.findLineColumn(source.bytes, span.main); | 4433 | const loc = std.zig.findLineColumn(source, span.main); |
| 4438 | 4434 | ||
| 4439 | const omit_source_line = loc.eql(err_loc) or (last_note_loc != null and loc.eql(last_note_loc.?)); | 4435 | const omit_source_line = loc.eql(err_loc) or (last_note_loc != null and loc.eql(last_note_loc.?)); |
| 4440 | last_note_loc = loc; | 4436 | last_note_loc = loc; |
| ... | @@ -4489,7 +4485,7 @@ fn addReferenceTraceFrame( | ... | @@ -4489,7 +4485,7 @@ fn addReferenceTraceFrame( |
| 4489 | try unableToLoadZcuFile(zcu, eb, src.file_scope, err); | 4485 | try unableToLoadZcuFile(zcu, eb, src.file_scope, err); |
| 4490 | return error.AlreadyReported; | 4486 | return error.AlreadyReported; |
| 4491 | }; | 4487 | }; |
| 4492 | const loc = std.zig.findLineColumn(source.bytes, span.main); | 4488 | const loc = std.zig.findLineColumn(source, span.main); |
| 4493 | try ref_traces.append(gpa, .{ | 4489 | try ref_traces.append(gpa, .{ |
| 4494 | .decl_name = try eb.printString("{s}{s}", .{ name, if (inlined) " [inlined]" else "" }), | 4490 | .decl_name = try eb.printString("{s}{s}", .{ name, if (inlined) " [inlined]" else "" }), |
| 4495 | .src_loc = try eb.addSourceLocation(.{ | 4491 | .src_loc = try eb.addSourceLocation(.{ |
| ... | @@ -4545,8 +4541,13 @@ pub fn unableToLoadZcuFile( | ... | @@ -4545,8 +4541,13 @@ pub fn unableToLoadZcuFile( |
| 4545 | file: *Zcu.File, | 4541 | file: *Zcu.File, |
| 4546 | err: Zcu.File.GetSourceError, | 4542 | err: Zcu.File.GetSourceError, |
| 4547 | ) Allocator.Error!void { | 4543 | ) Allocator.Error!void { |
| 4544 | const msg = switch (err) { | ||
| 4545 | error.OutOfMemory => |e| return e, | ||
| 4546 | error.FileChanged => try eb.addString("file contents changed during update"), | ||
| 4547 | else => |e| try eb.printString("unable to load: {t}", .{e}), | ||
| 4548 | }; | ||
| 4548 | try eb.addRootErrorMessage(.{ | 4549 | try eb.addRootErrorMessage(.{ |
| 4549 | .msg = try eb.printString("unable to load: {t}", .{err}), | 4550 | .msg = msg, |
| 4550 | .src_loc = try file.errorBundleWholeFileSrc(zcu, eb), | 4551 | .src_loc = try file.errorBundleWholeFileSrc(zcu, eb), |
| 4551 | }); | 4552 | }); |
| 4552 | } | 4553 | } |
src/Zcu.zig+38-30
| ... | @@ -925,8 +925,11 @@ pub const File = struct { | ... | @@ -925,8 +925,11 @@ pub const File = struct { |
| 925 | /// allocated into `gpa`. | 925 | /// allocated into `gpa`. |
| 926 | path: Compilation.Path, | 926 | path: Compilation.Path, |
| 927 | 927 | ||
| 928 | /// Populated only when emitting error messages; see `getSource`. | ||
| 928 | source: ?[:0]const u8, | 929 | source: ?[:0]const u8, |
| 930 | /// Populated only when emitting error messages; see `getTree`. | ||
| 929 | tree: ?Ast, | 931 | tree: ?Ast, |
| 932 | |||
| 930 | zir: ?Zir, | 933 | zir: ?Zir, |
| 931 | zoir: ?Zoir, | 934 | zoir: ?Zoir, |
| 932 | 935 | ||
| ... | @@ -1033,25 +1036,27 @@ pub const File = struct { | ... | @@ -1033,25 +1036,27 @@ pub const File = struct { |
| 1033 | } | 1036 | } |
| 1034 | } | 1037 | } |
| 1035 | 1038 | ||
| 1036 | pub const Source = struct { | ||
| 1037 | bytes: [:0]const u8, | ||
| 1038 | stat: Cache.File.Stat, | ||
| 1039 | }; | ||
| 1040 | |||
| 1041 | pub const GetSourceError = error{ | 1039 | pub const GetSourceError = error{ |
| 1042 | OutOfMemory, | 1040 | OutOfMemory, |
| 1043 | FileTooBig, | 1041 | FileChanged, |
| 1044 | Streaming, | 1042 | } || std.Io.File.OpenError || std.Io.File.Reader.Error; |
| 1045 | } || std.fs.File.OpenError || std.fs.File.ReadError; | ||
| 1046 | 1043 | ||
| 1047 | pub fn getSource(file: *File, zcu: *const Zcu) GetSourceError!Source { | 1044 | /// This must only be called in error conditions where `stat` *is* populated. It returns the |
| 1045 | /// contents of the source file, assuming the stat has not changed since it was originally | ||
| 1046 | /// loaded. | ||
| 1047 | pub fn getSource(file: *File, zcu: *const Zcu) GetSourceError![:0]const u8 { | ||
| 1048 | const gpa = zcu.gpa; | 1048 | const gpa = zcu.gpa; |
| 1049 | const io = zcu.comp.io; | 1049 | const io = zcu.comp.io; |
| 1050 | 1050 | ||
| 1051 | if (file.source) |source| return .{ | 1051 | if (file.source) |source| return source; |
| 1052 | .bytes = source, | 1052 | |
| 1053 | .stat = file.stat, | 1053 | switch (file.status) { |
| 1054 | }; | 1054 | .never_loaded => unreachable, // stat must be populated |
| 1055 | .retryable_failure => unreachable, // stat must be populated | ||
| 1056 | .astgen_failure, .success => {}, | ||
| 1057 | } | ||
| 1058 | |||
| 1059 | assert(file.stat.size <= std.math.maxInt(u32)); // `PerThread.updateFile` checks this | ||
| 1055 | 1060 | ||
| 1056 | var f = f: { | 1061 | var f = f: { |
| 1057 | const dir, const sub_path = file.path.openInfo(zcu.comp.dirs); | 1062 | const dir, const sub_path = file.path.openInfo(zcu.comp.dirs); |
| ... | @@ -1059,40 +1064,43 @@ pub const File = struct { | ... | @@ -1059,40 +1064,43 @@ pub const File = struct { |
| 1059 | }; | 1064 | }; |
| 1060 | defer f.close(); | 1065 | defer f.close(); |
| 1061 | 1066 | ||
| 1062 | const stat = try f.stat(); | 1067 | const stat = f.stat() catch |err| switch (err) { |
| 1068 | error.Streaming => { | ||
| 1069 | // Since `file.stat` is populated, this was previously a file stream; since it is | ||
| 1070 | // now not a file stream, it must have changed. | ||
| 1071 | return error.FileChanged; | ||
| 1072 | }, | ||
| 1073 | else => |e| return e, | ||
| 1074 | }; | ||
| 1063 | 1075 | ||
| 1064 | if (stat.size > std.math.maxInt(u32)) | 1076 | if (stat.inode != file.stat.inode or |
| 1065 | return error.FileTooBig; | 1077 | stat.size != file.stat.size or |
| 1078 | stat.mtime.nanoseconds != file.stat.mtime.nanoseconds) | ||
| 1079 | { | ||
| 1080 | return error.FileChanged; | ||
| 1081 | } | ||
| 1066 | 1082 | ||
| 1067 | const source = try gpa.allocSentinel(u8, @intCast(stat.size), 0); | 1083 | const source = try gpa.allocSentinel(u8, @intCast(file.stat.size), 0); |
| 1068 | errdefer gpa.free(source); | 1084 | errdefer gpa.free(source); |
| 1069 | 1085 | ||
| 1070 | var file_reader = f.reader(io, &.{}); | 1086 | var file_reader = f.reader(io, &.{}); |
| 1071 | file_reader.size = stat.size; | 1087 | file_reader.size = stat.size; |
| 1072 | file_reader.interface.readSliceAll(source) catch return file_reader.err.?; | 1088 | file_reader.interface.readSliceAll(source) catch return file_reader.err.?; |
| 1073 | 1089 | ||
| 1074 | // Here we do not modify stat fields because this function is the one | ||
| 1075 | // used for error reporting. We need to keep the stat fields stale so that | ||
| 1076 | // updateFile can know to regenerate ZIR. | ||
| 1077 | |||
| 1078 | file.source = source; | 1090 | file.source = source; |
| 1079 | errdefer comptime unreachable; // don't error after populating `source` | 1091 | errdefer comptime unreachable; // don't error after populating `source` |
| 1080 | 1092 | ||
| 1081 | return .{ | 1093 | return source; |
| 1082 | .bytes = source, | ||
| 1083 | .stat = .{ | ||
| 1084 | .size = stat.size, | ||
| 1085 | .inode = stat.inode, | ||
| 1086 | .mtime = stat.mtime, | ||
| 1087 | }, | ||
| 1088 | }; | ||
| 1089 | } | 1094 | } |
| 1090 | 1095 | ||
| 1096 | /// This must only be called in error conditions where `stat` *is* populated. It returns the | ||
| 1097 | /// parsed AST of the source file, assuming the stat has not changed since it was originally | ||
| 1098 | /// loaded. | ||
| 1091 | pub fn getTree(file: *File, zcu: *const Zcu) GetSourceError!*const Ast { | 1099 | pub fn getTree(file: *File, zcu: *const Zcu) GetSourceError!*const Ast { |
| 1092 | if (file.tree) |*tree| return tree; | 1100 | if (file.tree) |*tree| return tree; |
| 1093 | 1101 | ||
| 1094 | const source = try file.getSource(zcu); | 1102 | const source = try file.getSource(zcu); |
| 1095 | file.tree = try .parse(zcu.gpa, source.bytes, file.getMode()); | 1103 | file.tree = try .parse(zcu.gpa, source, file.getMode()); |
| 1096 | return &file.tree.?; | 1104 | return &file.tree.?; |
| 1097 | } | 1105 | } |
| 1098 | 1106 |