Update thin-lto cache file atimes when on windows

[gbreynoo]

Summary

Thin-lto cache file atimes are used for expiration based pruning, and since Vista, access file times are not updated by Windows by default:
https://blogs.technet.microsoft.com/filecab/2006/11/07/disabling-last-access-time-in-windows-vista-to-improve-ntfs-performance/

This means on Windows, cache files are currently being pruned from creation time. This change manually updates cache files that are accessed by Thin-lto, when on Windows.

[dmajor]

I don't know this code well enough to be a proper reviewer, but I want to offer moral support for this change. Not having atime updates on NTFS can be frustrating at times.

include/llvm/Support/FileSystem.h

854 ↗

(On Diff #148217)

Needs a @param

lib/Support/Windows/Path.inc

1099 ↗

(On Diff #148217)

This needs a CloseHandle here, right?

[gbreynoo]

Updated to a recent revision and made changes suggested by dmajor

[pcc]

lib/LTO/Caching.cpp

37 ↗

(On Diff #151193)

I'd rather not add another mysterious bool parameter to getFile for something that only one client needs. I'd prefer if the atime update were made more explicit. Specifically, I think we should do this:

• add an OpenFlag that means "update atime"
• change this code to use openFileForRead passing the new OpenFlag to obtain an FD, and then pass the FD to MemoryBuffer::getOpenFile().

[gbreynoo]

Updated to recent revision and made changes suggested by pcc

[gbreynoo]

ping

[pcc]

You should be able to avoid adding the bool argument to openNativeFileInternal as well.

include/llvm/Support/FileSystem.h

726 ↗

(On Diff #151734)

Nit: add space between /// and the start of the comment.

lib/LTO/Caching.cpp

36 ↗

(On Diff #151734)

I think this is leaking the FD; you need to close it after creating the map. Same in ThinLTOCodeGenerator.cpp.

lib/Support/Windows/Path.inc

1083 ↗

(On Diff #151734)

Move this into nativeAccess.

1133 ↗

(On Diff #151734)

Move the call to updateAccessTime here (or better yet just inline the code).

[gbreynoo]

Updated to recent revision and made changes suggested by pcc

[pcc]

LGTM

lib/Support/Windows/Path.inc

1117 ↗

(On Diff #153454)

Close the file handle here.

[gbreynoo]

Updated to recent revision and made change suggested by pcc

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

enum OpenFlags : unsigned {
   F_Append = 2, // For compatibility
 
   /// Delete the file on close. Only makes a difference on windows.
   OF_Delete = 4,
 
   /// When a child process is launched, this file should remain open in the
   /// child process.
   OF_ChildInherit = 8,
+
+  /// Force files Atime to be updated on access. Only makes a difference on windows.
+  OF_UpdateAtime = 16,
 };
 
 /// Create a uniquely named file.
 ///
 /// Generates a unique path suitable for a temporary file and then opens it as a
 /// file. The name is based on \a model with '%' replaced by a random char in
 /// [0-9a-f]. If \a model is not an absolute path, the temporary file will be
 /// created in the current directory.

llvm/trunk/lib/LTO/Caching.cpp

 #include "llvm/LTO/Caching.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/raw_ostream.h"
 
+#if !defined(_MSC_VER) && !defined(__MINGW32__)
+#include <unistd.h>
+#else
+#include <io.h>
+#endif
+
 using namespace llvm;
 using namespace llvm::lto;
 
 Expected<NativeObjectCache> lto::localCache(StringRef CacheDirectoryPath,
                                             AddBufferFn AddBuffer) {
   if (std::error_code EC = sys::fs::create_directories(CacheDirectoryPath))
     return errorCodeToError(EC);
 
   return [=](unsigned Task, StringRef Key) -> AddStreamFn {
     // This choice of file name allows the cache to be pruned (see pruneCache()
     // in include/llvm/Support/CachePruning.h).
     SmallString<64> EntryPath;
     sys::path::append(EntryPath, CacheDirectoryPath, "llvmcache-" + Key);
     // First, see if we have a cache hit.
+    int FD;
+    SmallString<64> ResultPath;
+    std::error_code EC = sys::fs::openFileForRead(
+        Twine(EntryPath), FD, sys::fs::OF_UpdateAtime, &ResultPath);
+    if (!EC) {
       ErrorOr<std::unique_ptr<MemoryBuffer>> MBOrErr =
-        MemoryBuffer::getFile(EntryPath);
+          MemoryBuffer::getOpenFile(FD, EntryPath,
+                                    /*FileSize*/ -1,
+                                    /*RequiresNullTerminator*/ false);
+      close(FD);
       if (MBOrErr) {
         AddBuffer(Task, std::move(*MBOrErr));
         return AddStreamFn();
       }
+      EC = MBOrErr.getError();
+    }
 
     // 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)
+    if (EC != errc::no_such_file_or_directory && EC != errc::permission_denied)
       report_fatal_error(Twine("Failed to open cache file ") + EntryPath +
-                         ": " + MBOrErr.getError().message() + "\n");
+                         ": " + EC.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;

llvm/trunk/lib/LTO/ThinLTOCodeGenerator.cpp

 #include "llvm/Transforms/IPO/FunctionImport.h"
 #include "llvm/Transforms/IPO/Internalize.h"
 #include "llvm/Transforms/IPO/PassManagerBuilder.h"
 #include "llvm/Transforms/ObjCARC.h"
 #include "llvm/Transforms/Utils/FunctionImportUtils.h"
 
 #include <numeric>
 
+#if !defined(_MSC_VER) && !defined(__MINGW32__)
+#include <unistd.h>
+#else
+#include <io.h>
+#endif
+
 using namespace llvm;
 
 #define DEBUG_TYPE "thinlto"
 
 namespace llvm {
 // Flags -discard-value-names, defined in LTOCodeGenerator.cpp
 extern cl::opt<bool> LTODiscardValueNames;
 extern cl::opt<std::string> LTORemarksFilename;
#endif
 
   // Access the path to this entry in the cache.
   StringRef getEntryPath() { return EntryPath; }
 
   // Try loading the buffer for this cache entry.
   ErrorOr<std::unique_ptr<MemoryBuffer>> tryLoadingBuffer() {
     if (EntryPath.empty())
       return std::error_code();
-    return MemoryBuffer::getFile(EntryPath);
+    int FD;
+    SmallString<64> ResultPath;
+    std::error_code EC = sys::fs::openFileForRead(
+        Twine(EntryPath), FD, sys::fs::OF_UpdateAtime, &ResultPath);
+    if (EC)
+      return EC;
+    ErrorOr<std::unique_ptr<MemoryBuffer>> MBOrErr =
+        MemoryBuffer::getOpenFile(FD, EntryPath,
+                                  /*FileSize*/ -1,
+                                  /*RequiresNullTerminator*/ false);
+    close(FD);
+    return MBOrErr;
   }
 
   // Cache the Produced object file
   void write(const MemoryBuffer &OutputBuffer) {
     if (EntryPath.empty())
       return;
 
     // Write to a temporary to avoid race condition

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

 static DWORD nativeAccess(FileAccess Access, OpenFlags Flags) {
   DWORD Result = 0;
   if (Access & FA_Read)
     Result |= GENERIC_READ;
   if (Access & FA_Write)
     Result |= GENERIC_WRITE;
   if (Flags & OF_Delete)
     Result |= DELETE;
+  if (Flags & OF_UpdateAtime)
+    Result |= FILE_WRITE_ATTRIBUTES;
   return Result;
 }
 
 static std::error_code openNativeFileInternal(const Twine &Name,
                                               file_t &ResultFile, DWORD Disp,
                                               DWORD Access, DWORD Flags,
                                               bool Inherit = false) {
   SmallVector<wchar_t, 128> PathUTF16;
Expected<file_t> openNativeFile(const Twine &Name, CreationDisposition Disp,
   if (Flags & OF_ChildInherit)
     Inherit = true;
 
   file_t Result;
   std::error_code EC = openNativeFileInternal(
       Name, Result, NativeDisp, NativeAccess, FILE_ATTRIBUTE_NORMAL, Inherit);
   if (EC)
     return errorCodeToError(EC);
+
+  if (Flags & OF_UpdateAtime) {
+    FILETIME FileTime;
+    SYSTEMTIME SystemTime;
+    GetSystemTime(&SystemTime);
+    if (SystemTimeToFileTime(&SystemTime, &FileTime) == 0 ||
+        SetFileTime(Result, NULL, &FileTime, NULL) == 0) {
+      DWORD LastError = ::GetLastError();
+      ::CloseHandle(Result);
+      return errorCodeToError(mapWindowsError(LastError));
+    }
+  }
+
   if (Flags & OF_Delete) {
     if ((EC = setDeleteDisposition(Result, true))) {
       ::CloseHandle(Result);
       return errorCodeToError(EC);
     }
   }
   return Result;
 }

llvm/trunk/test/ThinLTO/X86/cache.ll

 ; llvmcache.timestamp is current,
 ; cache file is older than default cache expiration value.
 ; RUN: rm -Rf %t.cache && mkdir %t.cache
 ; RUN: touch -t 197001011200 %t.cache/llvmcache-foo
 ; RUN: touch %t.cache/llvmcache.timestamp
 ; RUN: llvm-lto -thinlto-action=run -exported-symbol=globalfunc %t2.bc %t.bc -thinlto-cache-dir %t.cache --thinlto-cache-pruning-interval 0
 ; RUN: not ls %t.cache/llvmcache-foo
 
+; Populate the cache with files with "old" access times, then check llvm-lto updates these file times
+; A negative pruning interval is used to avoid removing cache entries
+; RUN: rm -Rf %t.cache && mkdir %t.cache
+; RUN: llvm-lto -thinlto-action=run -exported-symbol=globalfunc %t2.bc %t.bc -thinlto-cache-dir %t.cache
+; RUN: touch -a -t 197001011200 %t.cache/llvmcache-*
+; RUN: llvm-lto -thinlto-action=run -exported-symbol=globalfunc %t2.bc %t.bc -thinlto-cache-dir %t.cache --thinlto-cache-pruning-interval -1
+; RUN: ls -ltu %t.cache/* | not grep 1970-01-01
+
+; Populate the cache with files with "old" access times, then check llvm-lto2 updates these file times
+; RUN: rm -Rf %t.cache
+; RUN: llvm-lto2 run -o %t.o %t2.bc %t.bc -cache-dir %t.cache \
+; RUN:  -r=%t2.bc,_main,plx \
+; RUN:  -r=%t2.bc,_globalfunc,lx \
+; RUN:  -r=%t.bc,_globalfunc,plx
+; RUN: touch -a -t 197001011200 %t.cache/llvmcache-*
+; RUN: llvm-lto2 run -o %t.o %t2.bc %t.bc -cache-dir %t.cache \
+; RUN:  -r=%t2.bc,_main,plx \
+; RUN:  -r=%t2.bc,_globalfunc,lx \
+; RUN:  -r=%t.bc,_globalfunc,plx
+; RUN: ls -ltu %t.cache/* | not grep 1970-01-01
+
 ; Verify that specifying max size for the cache directory prunes it to this
 ; size, removing the largest files first.
 ; RUN: rm -Rf %t.cache && mkdir %t.cache
 ; Create cache files with different sizes.
 ; Only 8B, 16B and 76B files should stay after pruning.
 ; RUN: "%python" -c "with open(r'%t.cache/llvmcache-foo-1024', 'w') as file: file.truncate(1024)"
 ; RUN: "%python" -c "with open(r'%t.cache/llvmcache-foo-16', 'w') as file: file.truncate(16)"
 ; RUN: "%python" -c "with open(r'%t.cache/llvmcache-foo-8', 'w') as file: file.truncate(8)"

llvm/trunk/unittests/Support/Path.cpp

 #include "llvm/Support/Host.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/raw_ostream.h"
 #include "gtest/gtest.h"
 #include "gmock/gmock.h"
 
 #ifdef _WIN32
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/Support/Chrono.h"
 #include <windows.h>
 #include <winerror.h>
 #endif
 
 #ifdef LLVM_ON_UNIX
 #include <pwd.h>
 #include <sys/stat.h>
 #endif
TEST_F(FileSystemTest, OpenFileForRead) {
   if (!ResultPath.empty()) {
     // The paths returned by createTemporaryFile and getPathFromOpenFD
     // should reference the same file on disk.
     fs::UniqueID D1, D2;
     ASSERT_NO_ERROR(fs::getUniqueID(Twine(TempPath), D1));
     ASSERT_NO_ERROR(fs::getUniqueID(Twine(ResultPath), D2));
     ASSERT_EQ(D1, D2);
   }
+  ::close(FileDescriptor);
+  ::close(FileDescriptor2);
+
+#ifdef _WIN32
+  // Since Windows Vista, file access time is not updated by default.
+  // This is instead updated manually by openFileForRead.
+  // https://blogs.technet.microsoft.com/filecab/2006/11/07/disabling-last-access-time-in-windows-vista-to-improve-ntfs-performance/
+  // This part of the unit test is Windows specific as the updating of
+  // access times can be disabled on Linux using /etc/fstab.
+
+  // Set access time to UNIX epoch.
+  ASSERT_NO_ERROR(sys::fs::openFileForWrite(Twine(TempPath), FileDescriptor,
+                                            fs::CD_OpenExisting));
+  TimePoint<> Epoch(std::chrono::milliseconds(0));
+  ASSERT_NO_ERROR(fs::setLastModificationAndAccessTime(FileDescriptor, Epoch));
+  ::close(FileDescriptor);
 
+  // Open the file and ensure access time is updated, when forced.
+  bool ForceATime = true;
+  ASSERT_NO_ERROR(fs::openFileForRead(Twine(TempPath), FileDescriptor,
+                                      fs::OF_UpdateAtime, &ResultPath));
+
+  sys::fs::file_status Status;
+  ASSERT_NO_ERROR(sys::fs::status(FileDescriptor, Status));
+  auto FileAccessTime = Status.getLastAccessedTime();
+
+  ASSERT_NE(Epoch, FileAccessTime);
   ::close(FileDescriptor);
+
+  // Ideally this test would include a case when ATime is not forced to update,
+  // however the expected behaviour will differ depending on the configuration
+  // of the Windows file system.
+#endif
 }
 
 static void createFileWithData(const Twine &Path, bool ShouldExistBefore,
                                fs::CreationDisposition Disp, StringRef Data) {
   int FD;
   ASSERT_EQ(ShouldExistBefore, fs::exists(Path));
   ASSERT_NO_ERROR(fs::openFileForWrite(Path, FD, Disp));
   FileDescriptorCloser Closer(FD);