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/include/llvm/Support/FileSystem.h

public:
   int FD = -1;
 
   // Keep this with the given name.
   Error keep(const Twine &Name);
 
   // Keep this with the temporary name.
   Error keep();
 
-  // Delete the file.
+  // Close the file. This needs to be called after calling keep() unless keep()
+  // returned an error.
+  Error close();
+
+  // Delete and close the file.
   Error discard();
 
-  // This checks that keep or delete was called.
+  // This checks that keep and close or discard was called.
   ~TempFile();
 };
 
 /// Create a file in the system temporary directory.
 ///
 /// The filename is of the form prefix-random_chars.suffix. Since the directory
 /// is not know to the caller, Prefix and Suffix cannot have path separators.
 /// The files are created with mode 0600.
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/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;
       std::string EntryPath;
       unsigned Task;
 
       CacheStream(std::unique_ptr<raw_pwrite_stream> OS, AddBufferFn AddBuffer,
                   sys::fs::TempFile TempFile, std::string EntryPath,
                   unsigned Task)
           : NativeObjectStream(std::move(OS)), AddBuffer(std::move(AddBuffer)),
             TempFile(std::move(TempFile)), EntryPath(std::move(EntryPath)),
             Task(Task) {}
 
       ~CacheStream() {
         // Make sure the stream is closed before committing it.
         OS.reset();
 
-        // Open the file first to avoid racing with a cache pruner.
-        ErrorOr<std::unique_ptr<MemoryBuffer>> MBOrErr =
-            MemoryBuffer::getOpenFile(TempFile.FD, TempFile.TmpName,
-                                      /*FileSize*/ -1,
-                                      /*RequiresNullTerminator*/ false);
-        if (!MBOrErr)
-          report_fatal_error(Twine("Failed to open new cache file ") +
-                             TempFile.TmpName + ": " +
-                             MBOrErr.getError().message() + "\n");
-
         // On POSIX systems, this will atomically replace the destination if
         // it already exists. We try to emulate this on Windows, but this may
         // fail with a permission denied error (for example, if the destination
         // is currently opened by another process that does not give us the
         // sharing permissions we need). Since the existing file should be
         // semantically equivalent to the one we are trying to write, we give
         // AddBuffer a copy of the bytes we wrote in that case. We do this
         // instead of just using the existing file, because the pruner might
         // delete the file before we get a chance to use it.
         Error E = TempFile.keep(EntryPath);
         E = handleErrors(std::move(E), [&](const ECError &E) -> Error {
           std::error_code EC = E.convertToErrorCode();
           if (EC != errc::permission_denied)
             return errorCodeToError(EC);
 
+          ErrorOr<std::unique_ptr<MemoryBuffer>> MBOrErr =
+              MemoryBuffer::getOpenFile(TempFile.FD, TempFile.TmpName,
+                                        /*FileSize*/ -1,
+                                        /*RequiresNullTerminator*/ false);
+          if (!MBOrErr)
+            report_fatal_error(Twine("Failed to map cache file ") +
+                               TempFile.TmpName + ": " +
+                               MBOrErr.getError().message() + "\n");
+
           auto MBCopy = MemoryBuffer::getMemBufferCopy((*MBOrErr)->getBuffer(),
                                                        EntryPath);
           MBOrErr = std::move(MBCopy);
 
           // FIXME: should we consume the discard error?
           consumeError(TempFile.discard());
 
           return Error::success();
         });
 
         if (E)
           report_fatal_error(Twine("Failed to rename temporary file ") +
                              TempFile.TmpName + " to " + EntryPath + ": " +
                              toString(std::move(E)) + "\n");
 
+        // We must map the file after renaming it because on Windows that
+        // replaces the original file handle (which was opened with
+        // delete-on-close) with a new file handle that was opened without
+        // delete-on-close, and if a file handle opened with delete-on-close is
+        // kept alive that prevents other processes from opening the file.
+        ErrorOr<std::unique_ptr<MemoryBuffer>> MBOrErr =
+            MemoryBuffer::getOpenFile(TempFile.FD, EntryPath,
+                                      /*FileSize*/ -1,
+                                      /*RequiresNullTerminator*/ false);
+        if (!MBOrErr)
+          report_fatal_error(Twine("Failed to map cache file ") +
+                             EntryPath + ": " +
+                             MBOrErr.getError().message() + "\n");
+
+        if (Error E = TempFile.close())
+          report_fatal_error(Twine("Failed to close ") + EntryPath + ": " +
+                             toString(std::move(E)) + "\n");
+
         AddBuffer(Task, std::move(*MBOrErr));
       }
     };
 
     return [=](size_t Task) -> std::unique_ptr<NativeObjectStream> {
       // Write to a temporary to avoid race condition
       SmallString<64> TempFilenameModel;
       sys::path::append(TempFilenameModel, CacheDirectoryPath, "Thin-%%%%%%.tmp.o");

llvm/lib/Object/ArchiveWriter.cpp

Error llvm::writeArchive(StringRef ArcName,
   // rename would still succeed, but it would leave behind a
   // temporary file (actually the original file renamed) because
   // a file cannot be deleted while there's a handle open on it,
   // only renamed. So by freeing this buffer, this ensures that
   // the last open handle on the destination file, if any, is
   // closed before we attempt to rename.
   OldArchiveBuf.reset();
 
-  return Temp->keep(ArcName);
+  if (Error E = Temp->keep(ArcName))
+    return E;
+  return Temp->close();
 }

llvm/lib/Support/FileOutputBuffer.cpp

public:
 
   size_t getBufferSize() const override { return Buffer->size(); }
 
   Error commit() override {
     // Unmap buffer, letting OS flush dirty pages to file on disk.
     Buffer.reset();
 
     // Atomically replace the existing file with the new one.
-    return Temp.keep(FinalPath);
+    if (Error E = Temp.keep(FinalPath))
+      return E;
+    return Temp.close();
   }
 
   ~OnDiskBuffer() override {
     // Close the mapping before deleting the temp file, so that the removal
     // succeeds.
     Buffer.reset();
     consumeError(Temp.discard());
   }

llvm/lib/Support/Path.cpp

 namespace sys {
 namespace fs {
 TempFile::TempFile(StringRef Name, int FD) : TmpName(Name), FD(FD) {}
 TempFile::TempFile(TempFile &&Other) { *this = std::move(Other); }
 TempFile &TempFile::operator=(TempFile &&Other) {
   TmpName = std::move(Other.TmpName);
   FD = Other.FD;
   Other.Done = true;
+  Other.FD = -1;
   return *this;
 }
 
-TempFile::~TempFile() { assert(Done); }
+TempFile::~TempFile() { assert(Done && FD == -1); }
 
 Error TempFile::discard() {
   Done = true;
   std::error_code RemoveEC;
 // On windows closing will remove the file.
 #ifndef _WIN32
   // Always try to close and remove.
   if (!TmpName.empty()) {
     RemoveEC = fs::remove(TmpName);
     sys::DontRemoveFileOnSignal(TmpName);
   }
 #endif
 
   if (!RemoveEC)
     TmpName = "";
 
-  if (FD != -1 && close(FD) == -1) {
+  if (FD != -1 && ::close(FD) == -1) {
+    FD = -1;
     std::error_code EC = std::error_code(errno, std::generic_category());
     return errorCodeToError(EC);
   }
   FD = -1;
 
   return errorCodeToError(RemoveEC);
 }
 

 #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
 
-  if (!RenameEC)
+  if (RenameEC) {
+    ::close(FD);
+    FD = -1;
+  } else {
     TmpName = "";
-
-  if (close(FD) == -1) {
-    std::error_code EC(errno, std::generic_category());
-    return errorCodeToError(EC);
   }
-  FD = -1;
 
   return errorCodeToError(RenameEC);
 }
 
 Error TempFile::keep() {
   assert(!Done);
   Done = true;
 
 #ifdef _WIN32
-  if (std::error_code EC = cancelDeleteOnClose(FD))
+  if (std::error_code EC = cancelDeleteOnClose(FD)) {
+    ::close(FD);
+    FD = -1;
     return errorCodeToError(EC);
+  }
 #else
   sys::DontRemoveFileOnSignal(TmpName);
 #endif
 
   TmpName = "";
 
-  if (close(FD) == -1) {
+  return Error::success();
+}
+
+Error TempFile::close() {
+  assert(Done && FD != -1);
+
+  if (::close(FD) == -1) {
+    FD = -1;
     std::error_code EC(errno, std::generic_category());
     return errorCodeToError(EC);
   }
   FD = -1;
 
   return Error::success();
 }
 

llvm/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/lib/Support/Windows/Path.inc

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 due to what is believed to be a Windows kernel bug, 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;
 }
 

llvm/tools/bugpoint/BugDriver.cpp

 using namespace llvm;
 
 namespace llvm {
 Triple TargetTriple;
 }
 
 DiscardTemp::~DiscardTemp() {
   if (SaveTemps) {
-    if (Error E = File.keep())
+    if (Error E = File.keep()) {
       errs() << "Failed to keep temp file " << toString(std::move(E)) << '\n';
       return;
     }
+    if (Error E = File.close())
+      errs() << "Failed to close temp file " << toString(std::move(E)) << '\n';
+    return;
+  }
   if (Error E = File.discard())
     errs() << "Failed to delete temp file " << toString(std::move(E)) << '\n';
 }
 
 // Anonymous namespace to define command line options for debugging.
 //
 namespace {
 // Output - The user can specify a file containing the expected output of the