Stream: t-compiler/wg-incr-comp

Topic: session locking


view this post on Zulip Eric Huss (May 26 2021 at 17:45):

I have a naive question. Is locking of the session directory necessary when running under Cargo? From my understanding, a single rustc invocation only uses a single session directory for the crate being built. That directory name includes the crate disambiguator. Since cargo always gives unique -C metadata, there should never be a circumstance where there are concurrent rustc processes pointing at the same incremental directory. Does that sound about right?

view this post on Zulip bjorn3 (May 26 2021 at 18:42):

I don't think it should be necessary for successful builds, but it can't hurt.

view this post on Zulip Eric Huss (May 26 2021 at 18:47):

It causes problems for filesystems that don't support locking.

There's more context at https://github.com/rust-lang/rust/pull/85698. I fixed the ICE that happens, but I'm trying to decide if I should go further and change the behavior when it can't lock. Cargo just silently ignores it. I'm thinking of changing rustc to do the same. But then, I was thinking, why does rustc bother locking at all?

WDYT about just ignoring unsupported file locks?

view this post on Zulip bjorn3 (May 26 2021 at 20:29):

When cargo doesn't lock at worst two compilations at the same time could cause an SVH mismatch causing rustc to error out due to not finding an indirect dependency with the right SVH. When rustc doesn't lock the incremental session dir, the depgraph, workproduct index and the cached object files could be read at different times and thus go out of sync with each other. In the best case this will result in a linker error or an ICE. In the worst case this will cause a silent miscompilation.

view this post on Zulip bjorn3 (May 26 2021 at 20:32):

Erroring on out of sync depgraph and workproduct index would be possible if both embed the SVH, but the object files don't have any kind of unique identifier embedded and using a hash of the object files to detecr mismatches would be slow due to the hashing and it would be necessary to copy the object files to a temp dir, then check the hash and finally link the copy to prevent a TOCTOU race.

view this post on Zulip mw (May 27 2021 at 10:07):

Some thoughts:

view this post on Zulip mw (May 27 2021 at 10:16):

Having a good error message instead of a panic is a great improvement! Thanks for implementing that, @Eric Huss!

view this post on Zulip mw (May 27 2021 at 10:26):

Out of curiosity: how does cargo synchronize between concurrent processes trying to build the same directory? From the message on the commandline it looks like it's using file locks too:

$ cargo build
Blocking waiting for file lock on build directory

view this post on Zulip bjorn3 (May 27 2021 at 10:32):

There is a file .cargo-lock inside target/{debug,release}.

view this post on Zulip Eric Huss (May 27 2021 at 15:46):

The locking code is here. It is slightly more complicated than the rustc version, in that it checks for NFS mounts, and will block to wait for the lock instead of returning immediately.

Thanks for the responses! I think keeping it as-is for now is probably good. Hopefully the note added in the PR will guide people on how to resolve the issue.

view this post on Zulip Eric Huss (May 27 2021 at 15:50):

I think not coincidentally, both implementations were written by Alex.


Last updated: Oct 21 2021 at 21:02 UTC