[PATCH] D81803: [Support] PR42623: Avoid setting the delete-on-close bit if a TempFile doesn't reside on a local drive

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D81803#2636317 <https://reviews.llvm.org/D81803#2636317>, @aganea wrote:

> It would be nice to retain Windows 7 support for now, seeing the number of users affected by this issue.

The point may be a bit moot, since there's been no sign of a revert being picked up in the 12.0.0 release candidates, so it looks like Windows 7 support is going to be broken for at least some use cases with LLVM 12. I guess someone could propose an alternative fix for a future RC or 12.0.1, which downstream projects could adopt if needed.



================
Comment at: llvm/lib/Support/Windows/Path.inc:407
+  // set DeleteFile to true, since it prevents opening the file for writes.
+  SmallVector<wchar_t, 128> FinalPath;
+  if (std::error_code EC = realPathFromHandle(Handle, FinalPath))
----------------
aganea wrote:
> The error occurs on a second call with `setDeleteDisposition(..., false)` initiated by `TempFile::keep()`. A simple repro from https://bugs.llvm.org/show_bug.cgi?id=48378#c0 can be found here: https://pastebin.com/v51m3uBU
> 
> We could enclose this new block of code into `if (Delete) { ... }`. eg.
> ```
> static std::error_code setDeleteDisposition(HANDLE Handle, bool Delete) {
>   if (Delete) {
>     // First, check if the file is on a network (non-local) drive. If so, don't
>     // set DeleteFile to true, since it prevents opening the file for writes.
>     SmallVector<wchar_t, 128> FinalPath;
>     ...
>     // At this point, the file is on a local drive.
>   }
> 
>   FILE_DISPOSITION_INFO Disposition;
>   Disposition.DeleteFile = Delete;
>   if (!SetFileInformationByHandle(Handle, FileDispositionInfo, &Disposition,
>                                   sizeof(Disposition)))
>     return mapWindowsError(::GetLastError());
>   return std::error_code();
> }
> 
> ```
> The original report from @rdwampler suggests that the problem is not `SetFileInformationByHandle`, but writes that happen later in the process. In this particular case, the writes already happened.
> 
> WDYT?
Not sure who the WDYT is directed at here, but I'm happy for alternative approaches that solve the same issue.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81803/new/

https://reviews.llvm.org/D81803