[Support] call FlushFileBuffers when closing raw_fd_ostream on Windows

[inglorion]

Summary

We have observed a problem where files we write on Windows are
sometimes read back with bytes having value 0, when that was not the
value that was written. D42925 works around this problem for files we
write to by memory-mapping them. This change applies a similar fix for
files we write using raw_fd_ostream, as we do for archives and in the
LTO cache.

[inglorion]

My hope is that this will help with https://crbug.com/786127. I haven't seen the undefined symbol errors in local builds for several weeks now, but they still happen on the bots.

[zturner]

This doesn’t seem necessary, as the problem in the referenced cl only
affects memory mapped files. Furthermore, this will probably adversely
impact performance.

[pcc]

Is this a speculative fix or have you observed that this fixes the problem in practice?

[inglorion]

It's a speculative fix. I'm not getting the error locally and it doesn't consistently reproduce anywhere I know of, which makes it hard to think of a way to ascertain that it actually fixed the problem. It's just that I noticed that a number of files we write when doing ThinLTO are written through raw_fd_ostream, and we then occasionally see bytes being read as 0 when they shouldn't be, which is a problem we fixed elsewhere by calling FlushFileBuffers. If we land this and the problem persists I think we should revert it (for the reasons zturner mentioned), but given the similarity to the problem we worked around with D42925, I would like to give this a shot and see if it helps.

[zturner]

I won’t block or approve, but I would be extraordinarily surprised if it
turns out to be the culprit since the Windows team already confirmed it to
only be a problem with mapped file i/o (and if it weren’t, it would
probably be so common that the whole world would have blown up by now).

So probably a good idea to think about next steps (eg obtaining one of the
“corrupt” files from a buildbot)

[inglorion]

Bruce's blog post (https://randomascii.wordpress.com/2018/02/25/compiler-bug-linker-bug-windows-kernel-bug/) does seem to suggest that this issue should only ever be hit when files are written through memory maps. If that is the case, this change should not make a difference. I'll shelve it for now and see if I can think of something better. Thanks!

[inglorion]

We don't need this. Abandoning.

llvm/lib/Support/raw_ostream.cpp

#endif
   else
     pos = static_cast<uint64_t>(loc);
 }
 
 raw_fd_ostream::~raw_fd_ostream() {
   if (FD >= 0) {
     flush();
     if (ShouldClose) {
+#if defined(LLVM_ON_WIN32)
+      // On Windows, we have observed that programs that read a file after it
+      // was written will sometimes read bytes with value 0, when that isn't
+      // actually the value that was written. Calling FlushFileBuffers before
+      // closing the file seems to avoid the issue.
+      HANDLE FileHandle = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
+      if (FileHandle != INVALID_HANDLE_VALUE)
+        ::FlushFileBuffers(FileHandle);
+#endif
       if (auto EC = sys::Process::SafelyCloseFileDescriptor(FD))
         error_detected(EC);
     }
   }
 
 #ifdef __MINGW32__
   // On mingw, global dtors should not call exit().
   // report_fatal_error() invokes exit(). We know report_fatal_error()