Skip to content

stage2: improve handling of the generated file builtin.zig #10290

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 7, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions lib/std/fs.zig
Original file line number Diff line number Diff line change
Expand Up @@ -2170,6 +2170,16 @@ pub const Dir = struct {
return file.stat();
}

pub const StatFileError = File.OpenError || StatError;

// TODO: improve this to use the fstatat syscall instead of making 2 syscalls here.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whenever stat involved, a reason about TOCTOU should be added.
A detailed reasoning of what weird things can happen, if one only uses stat before open: https://unparalleled.eu/publications/2021/advisory-unpar-2021-0.txt
So this should at least argue, why it is fine to use stat here.

pub fn statFile(self: Dir, sub_path: []const u8) StatFileError!File.Stat {
var file = try self.openFile(sub_path, .{});
defer file.close();

return file.stat();
}

pub const ChmodError = File.ChmodError;

/// Changes the mode of the directory.
Expand Down
69 changes: 42 additions & 27 deletions src/Compilation.zig
Original file line number Diff line number Diff line change
Expand Up @@ -2152,15 +2152,6 @@ pub fn performAllTheWork(self: *Compilation) error{ TimerUnsupported, OutOfMemor
defer main_progress_node.end();
if (self.color == .off) progress.terminal = null;

// If we need to write out builtin.zig, it needs to be done before starting
// the AstGen tasks.
if (self.bin_file.options.module) |mod| {
if (mod.job_queued_update_builtin_zig) {
mod.job_queued_update_builtin_zig = false;
try self.updateBuiltinZigFile(mod);
}
}

// Here we queue up all the AstGen tasks first, followed by C object compilation.
// We wait until the AstGen tasks are all completed before proceeding to the
// (at least for now) single-threaded main work queue. However, C object compilation
Expand All @@ -2185,6 +2176,21 @@ pub fn performAllTheWork(self: *Compilation) error{ TimerUnsupported, OutOfMemor
self.astgen_wait_group.reset();
defer self.astgen_wait_group.wait();

// builtin.zig is handled specially for two reasons:
// 1. to avoid race condition of zig processes truncating each other's builtin.zig files
// 2. optimization; in the hot path it only incurs a stat() syscall, which happens
// in the `astgen_wait_group`.
if (self.bin_file.options.module) |mod| {
if (mod.job_queued_update_builtin_zig) {
mod.job_queued_update_builtin_zig = false;

self.astgen_wait_group.start();
try self.thread_pool.spawn(workerUpdateBuiltinZigFile, .{
self, mod, &self.astgen_wait_group,
});
}
}

while (self.astgen_work_queue.readItem()) |file| {
self.astgen_wait_group.start();
try self.thread_pool.spawn(workerAstGenFile, .{
Expand Down Expand Up @@ -2748,6 +2754,8 @@ fn workerAstGenFile(
extra_index = item.end;

const import_path = file.zir.nullTerminatedString(item.data.name);
// `@import("builtin")` is handled specially.
if (mem.eql(u8, import_path, "builtin")) continue;

const import_result = blk: {
comp.mutex.lock();
Expand Down Expand Up @@ -2775,6 +2783,29 @@ fn workerAstGenFile(
}
}

fn workerUpdateBuiltinZigFile(
comp: *Compilation,
mod: *Module,
wg: *WaitGroup,
) void {
defer wg.finish();

mod.populateBuiltinFile() catch |err| {
const dir_path: []const u8 = mod.zig_cache_artifact_directory.path orelse ".";

comp.mutex.lock();
defer comp.mutex.unlock();

comp.setMiscFailure(.write_builtin_zig, "unable to write builtin.zig to {s}: {s}", .{
dir_path, @errorName(err),
}) catch |oom| switch (oom) {
error.OutOfMemory => log.err("unable to write builtin.zig to {s}: {s}", .{
dir_path, @errorName(err),
}),
};
};
}

fn workerCheckEmbedFile(
comp: *Compilation,
embed_file: *Module.EmbedFile,
Expand Down Expand Up @@ -4046,22 +4077,6 @@ fn wantBuildLibUnwindFromSource(comp: *Compilation) bool {
comp.bin_file.options.object_format != .c;
}

fn updateBuiltinZigFile(comp: *Compilation, mod: *Module) Allocator.Error!void {
const tracy_trace = trace(@src());
defer tracy_trace.end();

const source = try comp.generateBuiltinZigSource(comp.gpa);
defer comp.gpa.free(source);

mod.zig_cache_artifact_directory.handle.writeFile("builtin.zig", source) catch |err| {
const dir_path: []const u8 = mod.zig_cache_artifact_directory.path orelse ".";
try comp.setMiscFailure(.write_builtin_zig, "unable to write builtin.zig to {s}: {s}", .{
dir_path,
@errorName(err),
});
};
}

fn setMiscFailure(
comp: *Compilation,
tag: MiscTask,
Expand All @@ -4084,7 +4099,7 @@ pub fn dump_argv(argv: []const []const u8) void {
std.debug.print("{s}\n", .{argv[argv.len - 1]});
}

pub fn generateBuiltinZigSource(comp: *Compilation, allocator: Allocator) Allocator.Error![]u8 {
pub fn generateBuiltinZigSource(comp: *Compilation, allocator: Allocator) Allocator.Error![:0]u8 {
const tracy_trace = trace(@src());
defer tracy_trace.end();

Expand Down Expand Up @@ -4290,7 +4305,7 @@ pub fn generateBuiltinZigSource(comp: *Compilation, allocator: Allocator) Alloca
}
}

return buffer.toOwnedSlice();
return buffer.toOwnedSliceSentinel(0);
}

pub fn updateSubCompilation(sub_compilation: *Compilation) !void {
Expand Down
72 changes: 72 additions & 0 deletions src/Module.zig
Original file line number Diff line number Diff line change
Expand Up @@ -2967,6 +2967,78 @@ fn updateZirRefs(gpa: Allocator, file: *File, old_zir: Zir) !void {
}
}

pub fn populateBuiltinFile(mod: *Module) !void {
const tracy = trace(@src());
defer tracy.end();

const comp = mod.comp;
const pkg_and_file = blk: {
comp.mutex.lock();
defer comp.mutex.unlock();

const builtin_pkg = mod.main_pkg.table.get("builtin").?;
const result = try mod.importPkg(builtin_pkg);
break :blk .{
.file = result.file,
.pkg = builtin_pkg,
};
};
const file = pkg_and_file.file;
const builtin_pkg = pkg_and_file.pkg;
const gpa = mod.gpa;
file.source = try comp.generateBuiltinZigSource(gpa);
file.source_loaded = true;

if (builtin_pkg.root_src_directory.handle.statFile(builtin_pkg.root_src_path)) |stat| {
if (stat.size != file.source.len) {
const full_path = try builtin_pkg.root_src_directory.join(gpa, &.{
builtin_pkg.root_src_path,
});
defer gpa.free(full_path);

log.warn(
"the cached file '{s}' had the wrong size. Expected {d}, found {d}. " ++
"Overwriting with correct file contents now",
.{ full_path, file.source.len, stat.size },
);

try writeBuiltinFile(file, builtin_pkg);
} else {
file.stat_size = stat.size;
file.stat_inode = stat.inode;
file.stat_mtime = stat.mtime;
}
} else |err| switch (err) {
error.BadPathName => unreachable, // it's always "builtin.zig"
error.NameTooLong => unreachable, // it's always "builtin.zig"
error.PipeBusy => unreachable, // it's not a pipe
error.WouldBlock => unreachable, // not asking for non-blocking I/O

error.FileNotFound => try writeBuiltinFile(file, builtin_pkg),

else => |e| return e,
}

file.tree = try std.zig.parse(gpa, file.source);
file.tree_loaded = true;
assert(file.tree.errors.len == 0); // builtin.zig must parse

file.zir = try AstGen.generate(gpa, file.tree);
file.zir_loaded = true;
file.status = .success_zir;
}

pub fn writeBuiltinFile(file: *File, builtin_pkg: *Package) !void {
var af = try builtin_pkg.root_src_directory.handle.atomicFile(builtin_pkg.root_src_path, .{});
defer af.deinit();
try af.file.writeAll(file.source);
try af.finish();

file.stat_size = file.source.len;
file.stat_inode = 0; // dummy value
file.stat_mtime = 0; // dummy value
}

pub fn mapOldZirToNew(
gpa: Allocator,
old_zir: Zir,
Expand Down