[PATCH] D38570: Support: Rewrite Windows implementation of sys::fs::rename to be more POSIXy.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

In https://reviews.llvm.org/D38570#889021, @zturner wrote:

> So if I understand the problem correctly, you don't have any issue with `ReplaceFile`'s behavior on the destination, only the source.  So, for example, you call `ReplaceFile`, it returns, and then some other process tries to open the //source// file and fails.


Not quite. From the ThinLTO perspective I don't care about the behaviour on the source file before it is renamed, because all source files are temporary files whose names are unknown to other processes. The problem is with the behaviour on the source file after it has been renamed to the destination file. ThinLTO does this for each new cache file that it creates:

  rename("thinlto-cache/temporary file name", "thinlto-cache/llvmcache-ABC123") // ABC123 is a unique hash that is computed from the linker's input files and command line options. There is a many-to-one mapping from hashes to object files (modulo collisions, but that's unlikely enough not to matter).

In parallel, another process will be doing this in order to try and open an existing cache file:

  open("thinlto-cache/llvmcache-ABC123") // This is expected to either succeed (cache hit) or return "no such file or directory" (cache miss).

Where this breaks is when `rename` is implemented in terms of `ReplaceFile`. In pseudocode, here's what I think is going on in the important part of `ReplaceFile`:

  void ReplaceFile(char *Src, char *Dst) {
    HANDLE h = CreateFile(Src, no sharing); // (1) Src cannot be opened
    SetFileInformationByHandle(h, Rename, Dst); // (2) Now Dst cannot be opened
    CloseFile(h); // (3) Now Dst can be opened
  }

So if the `open` happens between 2 and 3, it will get a permission denied error, which is not what the caller of `open` is expecting.


https://reviews.llvm.org/D38570