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?
I don't think it should be necessary for successful builds, but it can't hurt.
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?
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.
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.
rustcruns under cargo I'd still feel a lot better to have precautions in place that (try to) make sure that no two
rustcprocess use the same cache directory. As @bjorn3 points out anything can happen in that case.
rustcprocesses would read data from the cache directories of upstream crates. I don't know if that is still true but if it is, there can be any number of processes reading a cache directory concurrently and locks are the way to make sure garbage collection knows about that.
:/I suspect that it isn't optimal either performance-wise. Unfortunately, since anything interacting with filesystems is rather brittle and platform-dependent, it would be quite a bit of work to change things here. Might be an interesting project for @wg-incr-comp at some point.
Having a good error message instead of a panic is a great improvement! Thanks for implementing that, @Eric Huss!
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
There is a file
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.
I think not coincidentally, both implementations were written by Alex.
Last updated: Oct 21 2021 at 21:02 UTC