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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/lib/Support/Windows/Path.inc:368-369
 
-  // Retry while we see recoverable errors.
-  // System scanners (eg. indexer) might open the source file when it is written
-  // and closed.
+  std::vector<char> RenameInfoBuf(sizeof(FILE_RENAME_INFO) +
+                                  (ToWide.size() * sizeof(wchar_t)));
+  FILE_RENAME_INFO &RenameInfo =
----------------
zturner wrote:
> I think this is allocating 1 character too many.  `ToWide.size()` already includes the null terminator, but so does `sizeof(FILE_RENAME_INFO)` since it has a 1 character array.
Yeah, looks like we can subtract 2 here, I'll do that.


================
Comment at: llvm/lib/Support/Windows/Path.inc:374
+  RenameInfo.RootDirectory = 0;
+  RenameInfo.FileNameLength = ToWide.size();
+  std::copy(ToWide.begin(), ToWide.end(), RenameInfo.FileName);
----------------
zturner wrote:
> If I understand correctly, `ToWide.size()` contains the number of characters including null terminator, but this value should be number of characters *not* including null terminator.
http://llvm-cs.pcc.me.uk/lib/Support/Windows/Path.inc#1130

Looks like the string is null terminated, but will not contain a null terminator.


================
Comment at: llvm/lib/Support/Windows/Path.inc:381
-    if (i > 0)
-      ::Sleep(1);
 
----------------
rnk wrote:
> Are you sure you want to remove the Sleep? Consider the case where LLD is rapidly re-linking the same executable, and there is some silly virus scanner opening the file with a bad sharing mode. @dyung added this.
Actually it looks like this was added by Takumi back in r156380. Seems like the equivalent thing in the new code would be to do a sleep/retry when calling CreateFile on the source file. I'll do that.


================
Comment at: llvm/lib/Support/Windows/Path.inc:400
 
-      DWORD ReplaceError = ::GetLastError();
-      ec = mapWindowsError(ReplaceError);
+  while (1) {
+    auto EC = rename_internal(FromHandle, To, true);
----------------
rnk wrote:
> Let's make this fail after some time. Say 200 attempts? I doubt we need 2000... We can probably return something like `errc::resource_unavailable_try_again`` (EAGAIN)?
My instinct was to not limit the number of retries because a limit seems unsound, but limiting to a large number is probably fine if the probability of a false negative becomes infinitesimal. In general I think we shouldn't expect more than the number of logical CPUs to be trying to replace the file, and I wouldn't expect them all to be trying to replace the same file, so 200 seems like an ok number.


================
Comment at: llvm/lib/Support/Windows/Path.inc:429-430
+    // Try to find a unique new name for the destination file.
+    int UniqueId = 0;
+    while (1) {
+      std::string TmpFilename = (To + ".tmp" + utostr(UniqueId)).str();
----------------
rnk wrote:
> At first, I wanted to say we should use the same random file name generation used for createUniqueEntity, but these files should not accumulate if FILE_FLAG_DELETE_ON_CLOSE works as intended.
> 
> We should limit the number of retries, though. (also 10?)
I think I'd also make this based on a reasonable upper limit for the number of logical CPUs (i.e. 200).


https://reviews.llvm.org/D38570