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

Conversation

andrewrk
Copy link
Member

@andrewrk andrewrk commented Dec 6, 2021

All Zig code is eligible to @import("builtin") which is mapped to a
generated file, build.zig, based on the target and other settings.

Zig invocations which share the same target settings will generate the
same builtin.zig file and thus the path to builtin.zig is in a shared
cache folder, and different projects can sometimes use the same file.

Before this commit, this led to race conditions where multiple
invocations of zig would race to write this file. If one process
wanted to read the file while the other process wrote the file, the
reading process could observe a truncated or partially written
builtin.zig file.

This commit makes the following improvements:

  • limitations:
    • avoid clobbering the inode, mtime in the hot path
    • avoid creating a partially written file
    • builtin.zig needs to be on disk for debug info / stack trace purposes
    • don't mark the task as complete until the file is finished being populated
      (possibly by an external process)
  • strategy:
    • create the @import("builtin") Module.File during the AstGen
      work, based on generating the contents in memory rather than
      loading from disk.
    • write builtin.zig in a separate task that doesn't have
      to complete until the end of the AstGen work queue so that it
      can be done in parallel with everything else.
    • when writing the file, first stat the file path. If it exists, we are done.
    • otherwise, write the file to a temp file in the same directory and atomically
      rename it into place (clobbering the inode, mtime in the cold path).
  • summary:
    • all limitations respected
    • hot path: one stat() syscall that happens in a worker thread

This required adding a missing function to the standard library:
std.fs.Dir.statFile. In this commit, it does open() and then fstat()
which is two syscalls. It should be improved in a future commit to only
make one.

Fixes #9439.

All Zig code is eligible to `@import("builtin")` which is mapped to a
generated file, build.zig, based on the target and other settings.

Zig invocations which share the same target settings will generate the
same builtin.zig file and thus the path to builtin.zig is in a shared
cache folder, and different projects can sometimes use the same file.

Before this commit, this led to race conditions where multiple
invocations of `zig` would race to write this file. If one process
wanted to *read* the file while the other process *wrote* the file, the
reading process could observe a truncated or partially written
builtin.zig file.

This commit makes the following improvements:
 - limitations:
   - avoid clobbering the inode, mtime in the hot path
   - avoid creating a partially written file
   - builtin.zig needs to be on disk for debug info / stack trace purposes
   - don't mark the task as complete until the file is finished being populated
     (possibly by an external process)
 - strategy:
   - create the `@import("builtin")` `Module.File` during the AstGen
     work, based on generating the contents in memory rather than
     loading from disk.
   - write builtin.zig in a separate task that doesn't have
     to complete until the end of the AstGen work queue so that it
     can be done in parallel with everything else.
   - when writing the file, first stat the file path. If it exists, we are done.
   - otherwise, write the file to a temp file in the same directory and atomically
     rename it into place (clobbering the inode, mtime in the cold path).
 - summary:
   - all limitations respected
   - hot path: one stat() syscall that happens in a worker thread

This required adding a missing function to the standard library:
`std.fs.Dir.statFile`. In this commit, it does open() and then fstat()
which is two syscalls. It should be improved in a future commit to only
make one.

Fixes #9439.
@andrewrk
Copy link
Member Author

andrewrk commented Dec 7, 2021

If anyone who was experiencing the race condition wants to try this PR out, here is a build of this branch (cc @motiejus):

zig-linux-x86_64-0.9.0-dev.1914+00c8113c7.tar.xz

@@ -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.

@andrewrk andrewrk merged commit 274555b into master Dec 7, 2021
@andrewrk andrewrk deleted the stage2-builtin-zig-race branch December 7, 2021 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Subcompilation fails when build-exe executed in parallel
2 participants