LTO: Keep file handles open for memory mapped files.

[pcc]

Summary

On Windows we've observed that if you open a file,
write to it, map it into memory and close the file handle, the
contents of the memory mapping can sometimes be incorrect. That
was what we did when adding an entry to the ThinLTO cache using the
TempFile and MemoryBuffer classes, and it was causing intermittent
build failures on Chromium's ThinLTO bots on Windows. More details
are in the associated Chromium bug (crbug.com/786127).

We can prevent this from happening by keeping a handle to the
file open while the mapping is active. So this patch changes the
mapped_file_region class to duplicate the file handle when mapping
the file and close it upon unmapping it.

One gotcha is that the file handle that we keep open must not have
been created with FILE_FLAG_DELETE_ON_CLOSE, as otherwise the operating
system will prevent other processes from opening the file. We can achieve
this by avoiding the use of FILE_FLAG_DELETE_ON_CLOSE altogether.
Instead, we use SetFileInformationByHandle with FileDispositionInfo to manage
the delete-on-close bit. This lets us remove the hack that we used to use to clear
the delete-on-close bit on a file opened with FILE_FLAG_DELETE_ON_CLOSE.

A downside of using SetFileInformationByHandle/FileDispositionInfo as opposed
to FILE_FLAG_DELETE_ON_CLOSE is that it prevents us from using CreateFile to open
the file while the flag is set, even within the same process. This doesn't seem to matter
for almost every client of TempFile, except for LockFileManager, which calls sys::fs::create_link
to create a hard link from the lock file, and in the process of doing so tries to open the
file. To prevent this change from breaking LockFileManager I changed it to stop using
TempFile by effectively reverting r318550.

[zturner]

FWIW, I'm no longer convinced this is a Windows bug based on the reading of the blog post linked in comment 52 of the crbug.

But this comment worries me: "One gotcha is that the file handle that we keep open must not have been created with FILE_FLAG_DELETE_ON_CLOSE, as otherwise the operating system will prevent other processes from opening the file". The aforementioned blog post hints that if other processes are being prevented from opening the file, then you've already gone astray.

Is there any we can just transfer ownership of the original handle (with FILE_FLAG_DELETE_ON_CLOSE) from whoever had it before to the TempFile. This way we can just not close that handle until we're sure we're done with it.

[inglorion]

Thanks for the fix! A couple of suggestions:

I wouldn't call it a bug unless we are sure that it is. My reading of the documentation of FILE_FLAG_DELETE_ON_CLOSE is that what we're hitting may actually be the intended behavior. Instead of "bug", can we describe is at "observed behavior" or words to that effect? That way, we aren't implying that the behavior is incorrect while still explaining why our code needs to be the way it is.

The documentation for FILE_FLAG_DELETE_ON_CLOSE also mentions "Subsequent open requests for the file fail, unless the FILE_SHARE_DELETE share mode is specified." I wonder if we could use that to avoid having to split keep(). I think that would make the tempfile API a little nicer, although I'm ok splitting keep() if that's what we have to do to make it correct.

[pcc]

In D48051#1128998, @zturner wrote:

Is there any we can just transfer ownership of the original handle (with FILE_FLAG_DELETE_ON_CLOSE) from whoever had it before to the TempFile. This way we can just not close that handle until we're sure we're done with it.

I'm not sure what you mean. The FILE_FLAG_DELETE_ON_CLOSE handle is created and owned by TempFile. Do you mean transferring ownership from TempFile to MemoryBuffer when we map the file?

[pcc]

In D48051#1129001, @inglorion wrote:

Thanks for the fix! A couple of suggestions:

I wouldn't call it a bug unless we are sure that it is. My reading of the documentation of FILE_FLAG_DELETE_ON_CLOSE is that what we're hitting may actually be the intended behavior. Instead of "bug", can we describe is at "observed behavior" or words to that effect? That way, we aren't implying that the behavior is incorrect while still explaining why our code needs to be the way it is.

Okay.

The documentation for FILE_FLAG_DELETE_ON_CLOSE also mentions "Subsequent open requests for the file fail, unless the FILE_SHARE_DELETE share mode is specified." I wonder if we could use that to avoid having to split keep(). I think that would make the tempfile API a little nicer, although I'm ok splitting keep() if that's what we have to do to make it correct.

By "that" do you mean the "unless the FILE_SHARE_DELETE share mode is specified" part? Unfortunately that doesn't seem to work; even if both processes pass FILE_SHARE_DELETE the open still fails.

[zturner]

In D48051#1129014, @pcc wrote:

In D48051#1128998, @zturner wrote:

Is there any we can just transfer ownership of the original handle (with FILE_FLAG_DELETE_ON_CLOSE) from whoever had it before to the TempFile. This way we can just not close that handle until we're sure we're done with it.

I'm not sure what you mean. The FILE_FLAG_DELETE_ON_CLOSE handle is created and owned by TempFile. Do you mean transferring ownership from TempFile to MemoryBuffer when we map the file?

Yea, sorry. I'm just worried that by copying and duplicating the handle, and cancelling the delete on close, and all this other stuff we're just exposing ourselves to even more strange problems. It seems like the "correct" thing to do is to just keep the original handle open until we're absolutely done with that file.

[pcc]

In D48051#1129025, @zturner wrote:

In D48051#1129014, @pcc wrote:

In D48051#1128998, @zturner wrote:

Is there any we can just transfer ownership of the original handle (with FILE_FLAG_DELETE_ON_CLOSE) from whoever had it before to the TempFile. This way we can just not close that handle until we're sure we're done with it.

I'm not sure what you mean. The FILE_FLAG_DELETE_ON_CLOSE handle is created and owned by TempFile. Do you mean transferring ownership from TempFile to MemoryBuffer when we map the file?

Yea, sorry. I'm just worried that by copying and duplicating the handle, and cancelling the delete on close, and all this other stuff we're just exposing ourselves to even more strange problems. It seems like the "correct" thing to do is to just keep the original handle open until we're absolutely done with that file.

As I wrote in the commit message, that would prevent other processes from opening the file. What that would mean is that if we have two concurrent link processes they would not be able to share results.

I guess what we could do instead is exclusively use SetFileInformationByHandle/FileDispositionInfo for delete-on-close to avoid needing to do weird things to cancel FILE_FLAG_DELETE_ON_CLOSE. That should solve the sharing problem as well.

Also it would mean that we would need to change all other clients of MemoryBuffer::getOpenFile{,Slice}() to pass FD ownership to the MemoryBuffer. While that would seem to work for most in-tree clients there does appear to be at least one case where the FD is owned by external code: http://llvm-cs.pcc.me.uk/tools/gold/gold-plugin.cpp#498

So we'd probably either want the FD to be optionally owned or require the client to duplicate the handle if it doesn't own it. But that would mean that someone would still end up needing to duplicate on Windows in the non-owned case.

[zturner]

In D48051#1129059, @pcc wrote:

In D48051#1129025, @zturner wrote:

In D48051#1129014, @pcc wrote:

In D48051#1128998, @zturner wrote:

Is there any we can just transfer ownership of the original handle (with FILE_FLAG_DELETE_ON_CLOSE) from whoever had it before to the TempFile. This way we can just not close that handle until we're sure we're done with it.

I'm not sure what you mean. The FILE_FLAG_DELETE_ON_CLOSE handle is created and owned by TempFile. Do you mean transferring ownership from TempFile to MemoryBuffer when we map the file?

Yea, sorry. I'm just worried that by copying and duplicating the handle, and cancelling the delete on close, and all this other stuff we're just exposing ourselves to even more strange problems. It seems like the "correct" thing to do is to just keep the original handle open until we're absolutely done with that file.

As I wrote in the commit message, that would prevent other processes from opening the file. What that would mean is that if we have two concurrent link processes they would not be able to share results.

I guess what we could do instead is exclusively use SetFileInformationByHandle/FileDispositionInfo for delete-on-close to avoid needing to do weird things to cancel FILE_FLAG_DELETE_ON_CLOSE. That should solve the sharing problem as well.

Also it would mean that we would need to change all other clients of MemoryBuffer::getOpenFile{,Slice}() to pass FD ownership to the MemoryBuffer. While that would seem to work for most in-tree clients there does appear to be at least one case where the FD is owned by external code: http://llvm-cs.pcc.me.uk/tools/gold/gold-plugin.cpp#498

So we'd probably either want the FD to be optionally owned or require the client to duplicate the handle if it doesn't own it. But that would mean that someone would still end up needing to duplicate on Windows in the non-owned case.

I don't think that would prevent other processes from opening the file. Both processes just have to make sure they both specify FILE_SHARE_DELETE.

[zturner]

Do we have any kind of guarantee that one entity will outlive all other entities that may attempt to share the file?

[pcc]

In D48051#1129078, @zturner wrote:

In D48051#1129059, @pcc wrote:

In D48051#1129025, @zturner wrote:

In D48051#1129014, @pcc wrote:

In D48051#1128998, @zturner wrote:

Is there any we can just transfer ownership of the original handle (with FILE_FLAG_DELETE_ON_CLOSE) from whoever had it before to the TempFile. This way we can just not close that handle until we're sure we're done with it.

I'm not sure what you mean. The FILE_FLAG_DELETE_ON_CLOSE handle is created and owned by TempFile. Do you mean transferring ownership from TempFile to MemoryBuffer when we map the file?

Yea, sorry. I'm just worried that by copying and duplicating the handle, and cancelling the delete on close, and all this other stuff we're just exposing ourselves to even more strange problems. It seems like the "correct" thing to do is to just keep the original handle open until we're absolutely done with that file.

As I wrote in the commit message, that would prevent other processes from opening the file. What that would mean is that if we have two concurrent link processes they would not be able to share results.

I guess what we could do instead is exclusively use SetFileInformationByHandle/FileDispositionInfo for delete-on-close to avoid needing to do weird things to cancel FILE_FLAG_DELETE_ON_CLOSE. That should solve the sharing problem as well.

Also it would mean that we would need to change all other clients of MemoryBuffer::getOpenFile{,Slice}() to pass FD ownership to the MemoryBuffer. While that would seem to work for most in-tree clients there does appear to be at least one case where the FD is owned by external code: http://llvm-cs.pcc.me.uk/tools/gold/gold-plugin.cpp#498

So we'd probably either want the FD to be optionally owned or require the client to duplicate the handle if it doesn't own it. But that would mean that someone would still end up needing to duplicate on Windows in the non-owned case.

I don't think that would prevent other processes from opening the file. Both processes just have to make sure they both specify FILE_SHARE_DELETE.

As I wrote in my message to Bob that doesn't seem to work. According to process monitor the second process fails to open the file with "DELETE PENDING".

[zturner]

In D48051#1129100, @pcc wrote:

In D48051#1129078, @zturner wrote:

In D48051#1129059, @pcc wrote:

In D48051#1129025, @zturner wrote:

In D48051#1129014, @pcc wrote:

In D48051#1128998, @zturner wrote:

Is there any we can just transfer ownership of the original handle (with FILE_FLAG_DELETE_ON_CLOSE) from whoever had it before to the TempFile. This way we can just not close that handle until we're sure we're done with it.

I'm not sure what you mean. The FILE_FLAG_DELETE_ON_CLOSE handle is created and owned by TempFile. Do you mean transferring ownership from TempFile to MemoryBuffer when we map the file?

Yea, sorry. I'm just worried that by copying and duplicating the handle, and cancelling the delete on close, and all this other stuff we're just exposing ourselves to even more strange problems. It seems like the "correct" thing to do is to just keep the original handle open until we're absolutely done with that file.

As I wrote in the commit message, that would prevent other processes from opening the file. What that would mean is that if we have two concurrent link processes they would not be able to share results.

I guess what we could do instead is exclusively use SetFileInformationByHandle/FileDispositionInfo for delete-on-close to avoid needing to do weird things to cancel FILE_FLAG_DELETE_ON_CLOSE. That should solve the sharing problem as well.

Also it would mean that we would need to change all other clients of MemoryBuffer::getOpenFile{,Slice}() to pass FD ownership to the MemoryBuffer. While that would seem to work for most in-tree clients there does appear to be at least one case where the FD is owned by external code: http://llvm-cs.pcc.me.uk/tools/gold/gold-plugin.cpp#498

So we'd probably either want the FD to be optionally owned or require the client to duplicate the handle if it doesn't own it. But that would mean that someone would still end up needing to duplicate on Windows in the non-owned case.

I don't think that would prevent other processes from opening the file. Both processes just have to make sure they both specify FILE_SHARE_DELETE.

As I wrote in my message to Bob that doesn't seem to work. According to process monitor the second process fails to open the file with "DELETE PENDING".

That's because a handle has been opened with FILE_FLAG_DELETE_ON_CLOSE and then subsequently closed. As soon as you close a file that has been openend with that flag, no other opens can happen on the file. That's why I suggested transferring ownership of it.

Are we guaranteed that there is one entity that will outlive all other entities who may wish to share the file? As long as that one keeps the file open for its entire life, there shouldn't be any problems.

[pcc]

In D48051#1129090, @zturner wrote:

Do we have any kind of guarantee that one entity will outlive all other entities that may attempt to share the file?

I don't think so, basically the process that creates a cache entry could exit at any time before or after any processes that might read the cache entry.

[pcc]

In D48051#1129109, @zturner wrote:

In D48051#1129100, @pcc wrote:

I don't think that would prevent other processes from opening the file. Both processes just have to make sure they both specify FILE_SHARE_DELETE.

As I wrote in my message to Bob that doesn't seem to work. According to process monitor the second process fails to open the file with "DELETE PENDING".

That's because a handle has been opened with FILE_FLAG_DELETE_ON_CLOSE and then subsequently closed. As soon as you close a file that has been openend with that flag, no other opens can happen on the file. That's why I suggested transferring ownership of it.

But we need to close the handle to clear FILE_FLAG_DELETE_ON_CLOSE. Or are you suggesting that we do this:

• open file with FILE_FLAG_DELETE_ON_CLOSE
• map the file
• use it
• unmap it
• do magic to clear FILE_FLAG_DELETE_ON_CLOSE

? I'm not sure that would be simpler because it would mean transferring ownership back to the TempFile.

[zturner]

In D48051#1129112, @pcc wrote:

In D48051#1129090, @zturner wrote:

Do we have any kind of guarantee that one entity will outlive all other entities that may attempt to share the file?

I don't think so, basically the process that creates a cache entry could exit at any time before or after any processes that might read the cache entry.

What happens if process A creates a cache entry, then exits, then process B tries to re-use the same cache entry? Since it got closed (and deleted) before anyone could try to re-use it, does it just create it again? It seems like it would be more efficient from a hit/miss ratio perspective if once a cache file was created, it would never be created again.

That would also sidestep this problem because we wouldn't even have to use FILE_FLAG_DELETE_ON_CLOSE in the first place, you could wipe the cache at the beginning of a multi-process link operation and let it build up over time as processes spawn and die.

I'm concerned that we're tacking more and more complexity and strange combinations of flags and sequences of operations that we're just inviting even more (and even harder to find) problems.

[zturner]

In D48051#1129116, @pcc wrote:

In D48051#1129109, @zturner wrote:

In D48051#1129100, @pcc wrote:

I don't think that would prevent other processes from opening the file. Both processes just have to make sure they both specify FILE_SHARE_DELETE.

As I wrote in my message to Bob that doesn't seem to work. According to process monitor the second process fails to open the file with "DELETE PENDING".

That's because a handle has been opened with FILE_FLAG_DELETE_ON_CLOSE and then subsequently closed. As soon as you close a file that has been openend with that flag, no other opens can happen on the file. That's why I suggested transferring ownership of it.

But we need to close the handle to clear FILE_FLAG_DELETE_ON_CLOSE. Or are you suggesting that we do this:

• open file with FILE_FLAG_DELETE_ON_CLOSE
• map the file
• use it
• unmap it
• do magic to clear FILE_FLAG_DELETE_ON_CLOSE

? I'm not sure that would be simpler because it would mean transferring ownership back to the TempFile.

If you could guarantee that the person who originally opened it with FILE_FLAG_DELETE_ON_CLOSE could outlive any other process that may wish to share it, then you could just do:

1. Process A - Open it with FILE_FLAG_DELETE_ON_CLOSE.
2. Processes B1, B2, ..., Bn - Open it with FILE_SHARE_DELETE (this should work)
3. Wait for all those processes to exit.
4. Process A - close the file.

[pcc]

In D48051#1129117, @zturner wrote:

In D48051#1129112, @pcc wrote:

In D48051#1129090, @zturner wrote:

Do we have any kind of guarantee that one entity will outlive all other entities that may attempt to share the file?

I don't think so, basically the process that creates a cache entry could exit at any time before or after any processes that might read the cache entry.

What happens if process A creates a cache entry, then exits, then process B tries to re-use the same cache entry? Since it got closed (and deleted) before anyone could try to re-use it, does it just create it again?

It doesn't normally get deleted because we clear the delete-on-close bit. It might get deleted later as a result of cache pruning, but that's a separate mechanism. But if it does get deleted through cache pruning, we do create it again.

It seems like it would be more efficient from a hit/miss ratio perspective if once a cache file was created, it would never be created again.

That's exactly what happens, provided that the cache entry isn't pruned.

[zturner]

In D48051#1129121, @pcc wrote:

In D48051#1129117, @zturner wrote:

In D48051#1129112, @pcc wrote:

In D48051#1129090, @zturner wrote:

Do we have any kind of guarantee that one entity will outlive all other entities that may attempt to share the file?

I don't think so, basically the process that creates a cache entry could exit at any time before or after any processes that might read the cache entry.

What happens if process A creates a cache entry, then exits, then process B tries to re-use the same cache entry? Since it got closed (and deleted) before anyone could try to re-use it, does it just create it again?

It doesn't normally get deleted because we clear the delete-on-close bit. It might get deleted later as a result of cache pruning, but that's a separate mechanism. But if it does get deleted through cache pruning, we do create it again.

It seems like it would be more efficient from a hit/miss ratio perspective if once a cache file was created, it would never be created again.

That's exactly what happens, provided that the cache entry isn't pruned.

So after the entire link operation is finished, there are still cache files lying around on the hard drive (because their delete on close bit was cleared)? If that's the case, can we just not even specify it in the first place? What would be the implications of that?

[pcc]

In D48051#1129126, @zturner wrote:

In D48051#1129121, @pcc wrote:

In D48051#1129117, @zturner wrote:

In D48051#1129112, @pcc wrote:

In D48051#1129090, @zturner wrote:

Do we have any kind of guarantee that one entity will outlive all other entities that may attempt to share the file?

I don't think so, basically the process that creates a cache entry could exit at any time before or after any processes that might read the cache entry.

What happens if process A creates a cache entry, then exits, then process B tries to re-use the same cache entry? Since it got closed (and deleted) before anyone could try to re-use it, does it just create it again?

It doesn't normally get deleted because we clear the delete-on-close bit. It might get deleted later as a result of cache pruning, but that's a separate mechanism. But if it does get deleted through cache pruning, we do create it again.

It seems like it would be more efficient from a hit/miss ratio perspective if once a cache file was created, it would never be created again.

That's exactly what happens, provided that the cache entry isn't pruned.

So after the entire link operation is finished, there are still cache files lying around on the hard drive (because their delete on close bit was cleared)? If that's the case, can we just not even specify it in the first place? What would be the implications of that?

It would mean that if a link process is terminated forcefully or if there is a power cut, the link process's temporary files would be left on the disk.

That's why I was thinking about using SetFileInformationByHandle/FileDispositionInfo instead of FILE_FLAG_DELETE_ON_CLOSE to manage the delete-on-close bit for temporary files. It would mean that if the process is terminated in the window between CreateFile and SetFileInformationByHandle the file would be left on the disk, but other than that we should basically get the same behaviour. Since getting terminated in that window is pretty unlikely, it seems okay to me.

[zturner]

In D48051#1129127, @pcc wrote:

In D48051#1129126, @zturner wrote:

In D48051#1129121, @pcc wrote:

In D48051#1129117, @zturner wrote:

In D48051#1129112, @pcc wrote:

In D48051#1129090, @zturner wrote:

Do we have any kind of guarantee that one entity will outlive all other entities that may attempt to share the file?

I don't think so, basically the process that creates a cache entry could exit at any time before or after any processes that might read the cache entry.

What happens if process A creates a cache entry, then exits, then process B tries to re-use the same cache entry? Since it got closed (and deleted) before anyone could try to re-use it, does it just create it again?

It doesn't normally get deleted because we clear the delete-on-close bit. It might get deleted later as a result of cache pruning, but that's a separate mechanism. But if it does get deleted through cache pruning, we do create it again.

It seems like it would be more efficient from a hit/miss ratio perspective if once a cache file was created, it would never be created again.

That's exactly what happens, provided that the cache entry isn't pruned.

So after the entire link operation is finished, there are still cache files lying around on the hard drive (because their delete on close bit was cleared)? If that's the case, can we just not even specify it in the first place? What would be the implications of that?

It would mean that if a link process is terminated forcefully or if there is a power cut, the link process's temporary files would be left on the disk.

That's why I was thinking about using SetFileInformationByHandle/FileDispositionInfo instead of FILE_FLAG_DELETE_ON_CLOSE to manage the delete-on-close bit for temporary files. It would mean that if the process is terminated in the window between CreateFile and SetFileInformationByHandle the file would be left on the disk, but other than that we should basically get the same behaviour. Since getting terminated in that window is pretty unlikely, it seems okay to me.

Would the temporary files potentially be re-used on a subsequent link (if it were restarted by the user) and then cause mysterious failures? If not, I'm honestly fine to just never set the flag at all. We've been burned by it hard enough that who knows if we're triggering some additional strange / subtle codepath. That said, your idea is probably fine. At least it solves all the problems and eliminates all the duplicate handles and complexity.

[pcc]

In D48051#1129142, @zturner wrote:

In D48051#1129127, @pcc wrote:

In D48051#1129126, @zturner wrote:

In D48051#1129121, @pcc wrote:

In D48051#1129117, @zturner wrote:

In D48051#1129112, @pcc wrote:

In D48051#1129090, @zturner wrote:

Do we have any kind of guarantee that one entity will outlive all other entities that may attempt to share the file?

I don't think so, basically the process that creates a cache entry could exit at any time before or after any processes that might read the cache entry.

What happens if process A creates a cache entry, then exits, then process B tries to re-use the same cache entry? Since it got closed (and deleted) before anyone could try to re-use it, does it just create it again?

It doesn't normally get deleted because we clear the delete-on-close bit. It might get deleted later as a result of cache pruning, but that's a separate mechanism. But if it does get deleted through cache pruning, we do create it again.

It seems like it would be more efficient from a hit/miss ratio perspective if once a cache file was created, it would never be created again.

That's exactly what happens, provided that the cache entry isn't pruned.

So after the entire link operation is finished, there are still cache files lying around on the hard drive (because their delete on close bit was cleared)? If that's the case, can we just not even specify it in the first place? What would be the implications of that?

It would mean that if a link process is terminated forcefully or if there is a power cut, the link process's temporary files would be left on the disk.

That's why I was thinking about using SetFileInformationByHandle/FileDispositionInfo instead of FILE_FLAG_DELETE_ON_CLOSE to manage the delete-on-close bit for temporary files. It would mean that if the process is terminated in the window between CreateFile and SetFileInformationByHandle the file would be left on the disk, but other than that we should basically get the same behaviour. Since getting terminated in that window is pretty unlikely, it seems okay to me.

Would the temporary files potentially be re-used on a subsequent link (if it were restarted by the user) and then cause mysterious failures? If not, I'm honestly fine to just never set the flag at all. We've been burned by it hard enough that who knows if we're triggering some additional strange / subtle codepath. That said, your idea is probably fine. At least it solves all the problems and eliminates all the duplicate handles and complexity.

As I mentioned I think we'd still need to duplicate in cases where we didn't create the file handle (and there do seem to be at least a couple), so from an API simplicity perspective we might as well do it unconditionally. But I think we should at least be able to eliminate the magic for clearing the delete-on-close bit and the two-stage keep/close for temporary files.

[zturner]

We can’t transfer ownership of the handle in that case? With the flag
cleared we shouldn’t have to worry about the pending delete error

[pcc]

In D48051#1129149, @zturner wrote:

We can’t transfer ownership of the handle in that case? With the flag
cleared we shouldn’t have to worry about the pending delete error

No because in those cases another program owns the file handle.

In this case gold owns the file handle: http://llvm-cs.pcc.me.uk/tools/gold/gold-plugin.cpp#498
and in this case the user of the C LTO API owns it: http://llvm-cs.pcc.me.uk/lib/LTO/LTOModule.cpp#134

In both cases the owner of the file handle is on the other end of a stable API.

[zturner]

Alright, if there is no way to get rid of the Duplicates then we can live with them. As you said, the proposed solution is already better than before. I mildly prefer just never setting the delete on close flag in the first place because the case that it address is so rare that it almost doesn't even matter IMO, and I think just reducing the surface area of the Windows' kernels code paths that we touch is a win given how subtle this bug was and how long it took us to figure out. But if you think we should have it then I won't make a big fuss over it :)

[inglorion]

Alright, sounds like we are in agreement about the code. @pcc, can you still change the wording from "bug" to something else (like "observed behavior")?

[pcc]

• Replace FILE_FLAG_DELETE_ON_CLOSE with SetFileInformationByHandle/FileDispositionInfo

[inglorion]

LGTM, thanks!

llvm/trunk/include/llvm/Support/FileSystem.h

enum mapmode {
     readwrite, ///< May access map via data and modify it. Written to path.
     priv ///< May modify via data, but changes are lost on destruction.
   };
 
 private:
   /// Platform-specific mapping state.
   size_t Size;
   void *Mapping;
-  int FD;
+#ifdef _WIN32
+  void *FileHandle;
+#endif
   mapmode Mode;
 
   std::error_code init(int FD, uint64_t Offset, mapmode Mode);
 
 public:
   mapped_file_region() = delete;
   mapped_file_region(mapped_file_region&) = delete;
   mapped_file_region &operator =(mapped_file_region&) = delete;

llvm/trunk/include/llvm/Support/LockFileManager.h

 //===--- LockFileManager.h - File-level locking utility ---------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 #ifndef LLVM_SUPPORT_LOCKFILEMANAGER_H
 #define LLVM_SUPPORT_LOCKFILEMANAGER_H
 
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallString.h"
-#include "llvm/Support/FileSystem.h"
 #include <system_error>
 #include <utility> // for std::pair
 
 namespace llvm {
 class StringRef;
 
 /// Class that manages the creation of a lock file to aid
 /// implicit coordination between different processes.
enum WaitForUnlockResult {
     Res_OwnerDied,
     /// Reached timeout while waiting for the owner to release the lock.
     Res_Timeout
   };
 
 private:
   SmallString<128> FileName;
   SmallString<128> LockFileName;
-  Optional<sys::fs::TempFile> UniqueLockFile;
+  SmallString<128> UniqueLockFileName;
 
   Optional<std::pair<std::string, int> > Owner;
   std::error_code ErrorCode;
   std::string ErrorDiagMsg;
 
   LockFileManager(const LockFileManager &) = delete;
   LockFileManager &operator=(const LockFileManager &) = delete;
 

llvm/trunk/lib/LTO/Caching.cpp

return [=](unsigned Task, StringRef Key) -> AddStreamFn {
     // First, see if we have a cache hit.
     ErrorOr<std::unique_ptr<MemoryBuffer>> MBOrErr =
         MemoryBuffer::getFile(EntryPath);
     if (MBOrErr) {
       AddBuffer(Task, std::move(*MBOrErr));
       return AddStreamFn();
     }
 
-    if (MBOrErr.getError() != errc::no_such_file_or_directory)
+    // On Windows we can fail to open a cache file with a permission denied
+    // error. This generally means that another process has requested to delete
+    // the file while it is still open, but it could also mean that another
+    // process has opened the file without the sharing permissions we need.
+    // Since the file is probably being deleted we handle it in the same way as
+    // if the file did not exist at all.
+    if (MBOrErr.getError() != errc::no_such_file_or_directory &&
+        MBOrErr.getError() != errc::permission_denied)
       report_fatal_error(Twine("Failed to open cache file ") + EntryPath +
                          ": " + MBOrErr.getError().message() + "\n");
 
     // This native object stream is responsible for commiting the resulting
     // file to the cache and calling AddBuffer to add it to the link.
     struct CacheStream : NativeObjectStream {
       AddBufferFn AddBuffer;
       sys::fs::TempFile TempFile;

llvm/trunk/lib/Support/LockFileManager.cpp

 //===--- LockFileManager.cpp - File-level Locking Utility------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/LockFileManager.h"
 #include "llvm/ADT/None.h"
-#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringExtras.h"
-#include "llvm/Config/llvm-config.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cerrno>
 #include <ctime>
#if LLVM_ON_UNIX && !defined(__ANDROID__)
   // Check whether the process is dead. If so, we're done.
   if (StoredHostID == HostID && getsid(PID) == -1 && errno == ESRCH)
     return false;
 #endif
 
   return true;
 }
 
+namespace {
+
+/// An RAII helper object ensure that the unique lock file is removed.
+///
+/// Ensures that if there is an error or a signal before we finish acquiring the
+/// lock, the unique file will be removed. And if we successfully take the lock,
+/// the signal handler is left in place so that signals while the lock is held
+/// will remove the unique lock file. The caller should ensure there is a
+/// matching call to sys::DontRemoveFileOnSignal when the lock is released.
+class RemoveUniqueLockFileOnSignal {
+  StringRef Filename;
+  bool RemoveImmediately;
+public:
+  RemoveUniqueLockFileOnSignal(StringRef Name)
+  : Filename(Name), RemoveImmediately(true) {
+    sys::RemoveFileOnSignal(Filename, nullptr);
+  }
+
+  ~RemoveUniqueLockFileOnSignal() {
+    if (!RemoveImmediately) {
+      // Leave the signal handler enabled. It will be removed when the lock is
+      // released.
+      return;
+    }
+    sys::fs::remove(Filename);
+    sys::DontRemoveFileOnSignal(Filename);
+  }
+
+  void lockAcquired() { RemoveImmediately = false; }
+};
+
+} // end anonymous namespace
+
 LockFileManager::LockFileManager(StringRef FileName)
 {
   this->FileName = FileName;
   if (std::error_code EC = sys::fs::make_absolute(this->FileName)) {
     std::string S("failed to obtain absolute path for ");
     S.append(this->FileName.str());
     setError(EC, S);
     return;
   }
   LockFileName = this->FileName;
   LockFileName += ".lock";
 
   // If the lock file already exists, don't bother to try to create our own
   // lock file; it won't work anyway. Just figure out who owns this lock file.
   if ((Owner = readLockFile(LockFileName)))
     return;
 
   // Create a lock file that is unique to this instance.
-  Expected<sys::fs::TempFile> Temp =
-      sys::fs::TempFile::create(LockFileName + "-%%%%%%%%");
-  if (!Temp) {
-    std::error_code EC = errorToErrorCode(Temp.takeError());
-    std::string S("failed to create unique file with prefix ");
-    S.append(LockFileName.str());
+  UniqueLockFileName = LockFileName;
+  UniqueLockFileName += "-%%%%%%%%";
+  int UniqueLockFileID;
+  if (std::error_code EC = sys::fs::createUniqueFile(
+          UniqueLockFileName, UniqueLockFileID, UniqueLockFileName)) {
+    std::string S("failed to create unique file ");
+    S.append(UniqueLockFileName.str());
     setError(EC, S);
     return;
   }
-  UniqueLockFile = std::move(*Temp);
-
-  // Make sure we discard the temporary file on exit.
-  auto RemoveTempFile = llvm::make_scope_exit([&]() {
-    if (Error E = UniqueLockFile->discard())
-      setError(errorToErrorCode(std::move(E)));
-  });
 
   // Write our process ID to our unique lock file.
   {
     SmallString<256> HostID;
     if (auto EC = getHostID(HostID)) {
       setError(EC, "failed to get host id");
       return;
     }
 
-    raw_fd_ostream Out(UniqueLockFile->FD, /*shouldClose=*/false);
+    raw_fd_ostream Out(UniqueLockFileID, /*shouldClose=*/true);
     Out << HostID << ' ';
 #if LLVM_ON_UNIX
     Out << getpid();
 #else
     Out << "1";
 #endif
-    Out.flush();
+    Out.close();
 
     if (Out.has_error()) {
       // We failed to write out PID, so report the error, remove the
       // unique lock file, and fail.
       std::string S("failed to write to ");
-      S.append(UniqueLockFile->TmpName);
+      S.append(UniqueLockFileName.str());
       setError(Out.error(), S);
+      sys::fs::remove(UniqueLockFileName);
       return;
     }
   }
 
+  // Clean up the unique file on signal, which also releases the lock if it is
+  // held since the .lock symlink will point to a nonexistent file.
+  RemoveUniqueLockFileOnSignal RemoveUniqueFile(UniqueLockFileName);
+
   while (true) {
     // Create a link from the lock file name. If this succeeds, we're done.
     std::error_code EC =
-        sys::fs::create_link(UniqueLockFile->TmpName, LockFileName);
+        sys::fs::create_link(UniqueLockFileName, LockFileName);
     if (!EC) {
-      RemoveTempFile.release();
+      RemoveUniqueFile.lockAcquired();
       return;
     }
 
     if (EC != errc::file_exists) {
       std::string S("failed to create link ");
       raw_string_ostream OSS(S);
-      OSS << LockFileName.str() << " to " << UniqueLockFile->TmpName;
+      OSS << LockFileName.str() << " to " << UniqueLockFileName.str();
       setError(EC, OSS.str());
       return;
     }
 
     // Someone else managed to create the lock file first. Read the process ID
     // from the lock file.
-    if ((Owner = readLockFile(LockFileName)))
-      return; // RemoveTempFile will delete out our unique lock file.
+    if ((Owner = readLockFile(LockFileName))) {
+      // Wipe out our unique lock file (it's useless now)
+      sys::fs::remove(UniqueLockFileName);
+      return;
+    }
 
     if (!sys::fs::exists(LockFileName)) {
       // The previous owner released the lock file before we could read it.
       // Try to get ownership again.
       continue;
     }
 
     // There is a lock file that nobody owns; try to clean it up and get
     // ownership.
     if ((EC = sys::fs::remove(LockFileName))) {
       std::string S("failed to remove lockfile ");
-      S.append(LockFileName.str());
+      S.append(UniqueLockFileName.str());
       setError(EC, S);
       return;
     }
   }
 }
 
 LockFileManager::LockFileState LockFileManager::getState() const {
   if (Owner)

 }
 
 LockFileManager::~LockFileManager() {
   if (getState() != LFS_Owned)
     return;
 
   // Since we own the lock, remove the lock file and our own unique lock file.
   sys::fs::remove(LockFileName);
-  consumeError(UniqueLockFile->discard());
+  sys::fs::remove(UniqueLockFileName);
+  // The unique file is now gone, so remove it from the signal handler. This
+  // matches a sys::RemoveFileOnSignal() in LockFileManager().
+  sys::DontRemoveFileOnSignal(UniqueLockFileName);
 }
 
 LockFileManager::WaitForUnlockResult LockFileManager::waitForUnlock() {
   if (getState() != LFS_Shared)
     return Res_Success;
 
 #if _WIN32
   unsigned long Interval = 1;

llvm/trunk/lib/Support/Path.cpp

 }
 
 Error TempFile::keep(const Twine &Name) {
   assert(!Done);
   Done = true;
   // Always try to close and rename.
 #ifdef _WIN32
   // If we cant't cancel the delete don't rename.
-  std::error_code RenameEC = cancelDeleteOnClose(FD);
+  auto H = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
+  std::error_code RenameEC = setDeleteDisposition(H, false);
   if (!RenameEC)
     RenameEC = rename_fd(FD, Name);
   // If we can't rename, discard the temporary file.
   if (RenameEC)
-    removeFD(FD);
+    setDeleteDisposition(H, true);
 #else
   std::error_code RenameEC = fs::rename(TmpName, Name);
   // If we can't rename, discard the temporary file.
   if (RenameEC)
     remove(TmpName);
   sys::DontRemoveFileOnSignal(TmpName);
 #endif
 
#endif
   return errorCodeToError(RenameEC);
 }
 
 Error TempFile::keep() {
   assert(!Done);
   Done = true;
 
 #ifdef _WIN32
-  if (std::error_code EC = cancelDeleteOnClose(FD))
+  auto H = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
+  if (std::error_code EC = setDeleteDisposition(H, false))
     return errorCodeToError(EC);
 #else
   sys::DontRemoveFileOnSignal(TmpName);
 #endif
 
   TmpName = "";
 
   if (close(FD) == -1) {

llvm/trunk/lib/Support/Unix/Path.inc

#endif // #if defined (__APPLE__)
   Mapping = ::mmap(nullptr, Size, prot, flags, FD, Offset);
   if (Mapping == MAP_FAILED)
     return std::error_code(errno, std::generic_category());
   return std::error_code();
 }
 
 mapped_file_region::mapped_file_region(int fd, mapmode mode, size_t length,
                                        uint64_t offset, std::error_code &ec)
-    : Size(length), Mapping(), FD(fd), Mode(mode) {
-  (void)FD;
+    : Size(length), Mapping(), Mode(mode) {
   (void)Mode;
   ec = init(fd, offset, mode);
   if (ec)
     Mapping = nullptr;
 }
 
 mapped_file_region::~mapped_file_region() {
   if (Mapping)

llvm/trunk/lib/Support/Windows/Path.inc

static std::error_code setDeleteDisposition(HANDLE Handle, bool Delete) {
   FILE_DISPOSITION_INFO Disposition;
   Disposition.DeleteFile = Delete;
   if (!SetFileInformationByHandle(Handle, FileDispositionInfo, &Disposition,
                                   sizeof(Disposition)))
     return mapWindowsError(::GetLastError());
   return std::error_code();
 }
 
-static std::error_code removeFD(int FD) {
-  HANDLE Handle = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
-  return setDeleteDisposition(Handle, true);
-}
-
-/// In order to handle temporary files we want the following properties
-///
-/// * The temporary file is deleted on crashes
-/// * We can use (read, rename, etc) the temporary file.
-/// * We can cancel the delete to keep the file.
-///
-/// Using FILE_DISPOSITION_INFO with DeleteFile=true will create a file that is
-/// deleted on close, but it has a few problems:
-///
-/// * The file cannot be used. An attempt to open or rename the file will fail.
-///   This makes the temporary file almost useless, as it cannot be part of
-///   any other CreateFileW call in the current or in another process.
-/// * It is not atomic. A crash just after CreateFileW or just after canceling
-///   the delete will leave the file on disk.
-///
-/// Using FILE_FLAG_DELETE_ON_CLOSE solves the first issues and the first part
-/// of the second one, but there is no way to cancel it in place. What works is
-/// to create a second handle to prevent the deletion, close the first one and
-/// then clear DeleteFile with SetFileInformationByHandle. This requires
-/// changing the handle and file descriptor the caller uses.
-static std::error_code cancelDeleteOnClose(int &FD) {
-  HANDLE Handle = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
-  SmallVector<wchar_t, MAX_PATH> Name;
-  if (std::error_code EC = realPathFromHandle(Handle, Name))
-    return EC;
-  HANDLE NewHandle =
-      ::CreateFileW(Name.data(), GENERIC_READ | GENERIC_WRITE | DELETE,
-                    FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
-                    NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
-  if (NewHandle == INVALID_HANDLE_VALUE)
-    return mapWindowsError(::GetLastError());
-  if (close(FD))
-    return mapWindowsError(::GetLastError());
-
-  if (std::error_code EC = setDeleteDisposition(NewHandle, false))
-    return EC;
-
-  FD = ::_open_osfhandle(intptr_t(NewHandle), 0);
-  if (FD == -1) {
-    ::CloseHandle(NewHandle);
-    return mapWindowsError(ERROR_INVALID_HANDLE);
-  }
-  return std::error_code();
-}
-
 static std::error_code rename_internal(HANDLE FromHandle, const Twine &To,
                                        bool ReplaceIfExists) {
   SmallVector<wchar_t, 0> ToWide;
   if (auto EC = widenPath(To, ToWide))
     return EC;
 
   std::vector<char> RenameInfoBuf(sizeof(FILE_RENAME_INFO) - sizeof(wchar_t) +
                                   (ToWide.size() * sizeof(wchar_t)));
std::error_code setLastModificationAndAccessTime(int FD, TimePoint<> Time) {
   HANDLE FileHandle = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
   if (!SetFileTime(FileHandle, NULL, &FT, &FT))
     return mapWindowsError(::GetLastError());
   return std::error_code();
 }
 
 std::error_code mapped_file_region::init(int FD, uint64_t Offset,
                                          mapmode Mode) {
-  this->FD = FD;
   this->Mode = Mode;
-  HANDLE FileHandle = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
-  if (FileHandle == INVALID_HANDLE_VALUE)
+  HANDLE OrigFileHandle = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
+  if (OrigFileHandle == INVALID_HANDLE_VALUE)
     return make_error_code(errc::bad_file_descriptor);
 
   DWORD flprotect;
   switch (Mode) {
   case readonly:  flprotect = PAGE_READONLY; break;
   case readwrite: flprotect = PAGE_READWRITE; break;
   case priv:      flprotect = PAGE_WRITECOPY; break;
   }
 
   HANDLE FileMappingHandle =
-      ::CreateFileMappingW(FileHandle, 0, flprotect,
+      ::CreateFileMappingW(OrigFileHandle, 0, flprotect,
                            Hi_32(Size),
                            Lo_32(Size),
                            0);
   if (FileMappingHandle == NULL) {
     std::error_code ec = mapWindowsError(GetLastError());
     return ec;
   }
 
if (Result == 0) {
       std::error_code ec = mapWindowsError(GetLastError());
       ::UnmapViewOfFile(Mapping);
       ::CloseHandle(FileMappingHandle);
       return ec;
     }
     Size = mbi.RegionSize;
   }
 
-  // Close all the handles except for the view. It will keep the other handles
-  // alive.
+  // Close the file mapping handle, as it's kept alive by the file mapping. But
+  // neither the file mapping nor the file mapping handle keep the file handle
+  // alive, so we need to keep a reference to the file in case all other handles
+  // are closed and the file is deleted, which may cause invalid data to be read
+  // from the file.
   ::CloseHandle(FileMappingHandle);
+  if (!::DuplicateHandle(::GetCurrentProcess(), OrigFileHandle,
+                         ::GetCurrentProcess(), &FileHandle, 0, 0,
+                         DUPLICATE_SAME_ACCESS)) {
+    std::error_code ec = mapWindowsError(GetLastError());
+    ::UnmapViewOfFile(Mapping);
+    return ec;
+  }
+
   return std::error_code();
 }
 
 mapped_file_region::mapped_file_region(int fd, mapmode mode, size_t length,
                                        uint64_t offset, std::error_code &ec)
     : Size(length), Mapping() {
   ec = init(fd, offset, mode);
   if (ec)
     Mapping = 0;
 }
 
 mapped_file_region::~mapped_file_region() {
   if (Mapping) {
     ::UnmapViewOfFile(Mapping);
 
     if (Mode == mapmode::readwrite) {
       // There is a Windows kernel bug, the exact trigger conditions of which
       // are not well understood.  When triggered, dirty pages are not properly
       // flushed and subsequent process's attempts to read a file can return
       // invalid data.  Calling FlushFileBuffers on the write handle is
       // sufficient to ensure that this bug is not triggered.
-      HANDLE FileHandle = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
-      if (FileHandle != INVALID_HANDLE_VALUE)
-        ::FlushFileBuffers(FileHandle);
     }
+
+    ::CloseHandle(FileHandle);
   }
 }
 
 size_t mapped_file_region::size() const {
   assert(Mapping && "Mapping failed but used anyway!");
   return Size;
 }
 
static std::error_code nativeFileToFd(Expected<HANDLE> H, int &ResultFD,
   ResultFD = ::_open_osfhandle(intptr_t(*H), CrtOpenFlags);
   if (ResultFD == -1) {
     ::CloseHandle(*H);
     return mapWindowsError(ERROR_INVALID_HANDLE);
   }
   return std::error_code();
 }
 
-static DWORD nativeOpenFlags(OpenFlags Flags) {
-  DWORD Result = 0;
-  if (Flags & OF_Delete)
-    Result |= FILE_FLAG_DELETE_ON_CLOSE;
-
-  if (Result == 0)
-    Result = FILE_ATTRIBUTE_NORMAL;
-  return Result;
-}
-
 static DWORD nativeDisposition(CreationDisposition Disp, OpenFlags Flags) {
   // This is a compatibility hack.  Really we should respect the creation
   // disposition, but a lot of old code relied on the implicit assumption that
   // OF_Append implied it would open an existing file.  Since the disposition is
   // now explicit and defaults to CD_CreateAlways, this assumption would cause
   // any usage of OF_Append to append to a new file, even if the file already
   // existed.  A better solution might have two new creation dispositions:
   // CD_AppendAlways and CD_AppendNew.  This would also address the problem of

 
 Expected<file_t> openNativeFile(const Twine &Name, CreationDisposition Disp,
                                 FileAccess Access, OpenFlags Flags,
                                 unsigned Mode) {
   // Verify that we don't have both "append" and "excl".
   assert((!(Disp == CD_CreateNew) || !(Flags & OF_Append)) &&
          "Cannot specify both 'CreateNew' and 'Append' file creation flags!");
 
-  DWORD NativeFlags = nativeOpenFlags(Flags);
   DWORD NativeDisp = nativeDisposition(Disp, Flags);
   DWORD NativeAccess = nativeAccess(Access, Flags);
 
   bool Inherit = false;
   if (Flags & OF_ChildInherit)
     Inherit = true;
 
   file_t Result;
   std::error_code EC = openNativeFileInternal(
-      Name, Result, NativeDisp, NativeAccess, NativeFlags, Inherit);
+      Name, Result, NativeDisp, NativeAccess, FILE_ATTRIBUTE_NORMAL, Inherit);
   if (EC)
     return errorCodeToError(EC);
+  if (Flags & OF_Delete) {
+    if ((EC = setDeleteDisposition(Result, true))) {
+      ::CloseHandle(Result);
+      return errorCodeToError(EC);
+    }
+  }
   return Result;
 }
 
 std::error_code openFile(const Twine &Name, int &ResultFD,
                          CreationDisposition Disp, FileAccess Access,
                          OpenFlags Flags, unsigned int Mode) {
   Expected<file_t> Result = openNativeFile(Name, Disp, Access, Flags);
   if (!Result)