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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added a comment.

FYI, I had a user report a bug regarding this to me recently, https://github.com/mstorsjo/llvm-mingw/issues/233. This fix makes `TempFile` simply leave the files behind, when on a network drive. Ping @rnk @amccarth 
@jhenderson @rdwampler.

Background: `TempFile::create` calls `createUniqueFile` with `OF_Delete` set, which passes it down through `createUniqueFile`, `createUniqueEntity`, `openFileForReadWrite`, `openFile`, `openNativeFile`, which calls `setDeleteDisposition` if `OF_Delete` was set.

With this fix, `setDeleteDisposition` returns early (with a success error code) even though it didn't set the delete disposition as requested.

It should be simple to carry a bool flag in the `TempFile` class, to remove the file manually when closed. We'd still leak temp files when interrupted, but it'd be way better than leaking temp files on every single successful run too.

I considered fixing it by making `setDeleteDisposition` take a `bool &Delete` which can be updated indicating whether it actually was marked as to be implicitly deleted or not, but we'd need to plumb such a return value back through all those layers. I have a WIP patch for that, but it's ugly.

I also considered making `TempFile::create` check if the delete disposition flag actually was set correctly - however, one can't do `GetFileInformationByHandleEx` with `FileDispositionInfo`, apparently this is a write-only attribute. (The MSDN docs seem to agree.) So we need to carry the information from whoever called `setDeleteDisposition` back to the `TempFile` class.

We can't remove the `OF_Delete` flag from the call in `TempFile::create`, because the flag is needed for adding the `DELETE` flag to the `dwDesiredAccess` attribute to `CreateFileW`. We can't remove the `setDeleteDisposition` call from `openNativeFile` without breaking the `OF_Delete` flag as it is documented.

We could do a separate, duplicate call to `setDeleteDisposition` in `TempFile::create`, to allow checking whether it seems to work or not, but that's a bit ugly.

WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81803