[Support] On Windows, ensure that UniqueID is really stable

[aganea]

Summary

Before this patch, UniqueID was thought to store stable file IDs. However a recently reported issue shows that the file IDs are not always stable, at least on Windows. The documentation for the underlying Windows API states: (note the emphasis)

To determine whether two open handles represent the same file, combine the identifier and the volume serial number for each file and compare them.

Currently, UniqueID does not keep the file handles open. The fact that file IDs were stable until now is only due to a NTFS implementation detail. In some cases, such as drives mounted on network locations, the underlying Windows drivers might or might not generate stable file IDs.

To fix the issue, we keep the file handles open during the lifetime of their corresponding UniqueID instances. Since handles will live longer now, this requires particular attention when performing some file actions, such as file deletions.

Should fix #61401 and #22079.

[kadircet]

To fix the issue, we keep the file handles open during the lifetime of their corresponding UniqueID instances. Since handles will live longer now, this requires particular attention when performing some file actions, such as file deletions.

I am a little worried about this in general (what happens if some part of LLVM is keeping a fs::file_status object around when something deep down is trying to delete a file?) but specifically from clang-tooling perspective, especially clangd (i'd expect lldb to have similar concerns).
Today clangd is a long-lived process and it keeps uniqueid's around for the headers included inside a translation unit pretty much indefinitely. What happens if someone outside tries to delete/modify a file while we're keeping handles for it around? (it's quite common for people to switch branches in their VCS while clangd is working on these files)

also filemanager is one of the central pieces depended by many projects, that'll keep uniqueids around for any file that's accessed.

[hans]

This makes me nervous as well. I think it also doesn't fit well with developers' expectations of UniqueID, changing it from a simple piece of data to something which interacts with the OS in a pretty deep way. I don't have specific examples, but I'm sure it will bite us somehow.

[aaron.ballman]

FWIW, I'm also nervous about this change. Keeping the file handle open changes the ways in which you can access the file from other processes (or within the same process), so I'd expect this to cause some heartache, possibly for downstreams as well as Clang users. I mostly worry about things like leaving a file open interacting with things like deleting the file from the file system, including via a user interaction.

[aganea]

Fair enough. There are several choices forward: either we mark the issue as "Will Not Fix" or I can try only scoping this patch to only keep the handle open for network drives/paths. Any other suggestions?

[kadircet]

In D146490#4209495, @aganea wrote:

Fair enough. There are several choices forward: either we mark the issue as "Will Not Fix" or I can try only scoping this patch to only keep the handle open for network drives/paths. Any other suggestions?

I am afraid that a solution that keeps file handles alive indefinitely won't work even for that narrow set of users (unless they're operating on a read-only FS). so another alternative could be to have a different definition for uniqueids on these specific cases, e.g. use filepaths (canonicalized for capitalization & symlinks & hardlinks etc.). it might be slower than how things are working today, but i guess if that's needed for correctness in certain platforms one has to eat that regression?

[mstorsjo]

In D146490#4209495, @aganea wrote:

Fair enough. There are several choices forward: either we mark the issue as "Will Not Fix" or I can try only scoping this patch to only keep the handle for network drives/paths. Any other suggestions?

I also kinda share the sentiment that keeping handles open might be problematic - the cure is worse than the disease. I wouldn't quite start throwing in the towel quite yet though...

While the docs say that the ID only is stable as long as the handles are open (except for NTFS where it's an implementation detail), it's also stable enough for FAT (as long as nobody is defragmenting the drive while the process is open); at least stable enough for compiler/linker use, but possibly not for long running processes like clangd.

It might not even be strictly necessary for all network drives; out of 4 tested cases, it seemed to behave fine (stable enough at least) with a Linux Samba mount and a drive shared from VMWare Fusion, while Remote Desktop folder sharing (and allegedly VirtualBox although I haven't tried that) exhibit the issue. But I don't know of any immediate way of identifying the problematic cases either...

Another potential band-aid solution is to simply hash the pathnames for such drives. We won't get the desired effect of correctly identifying the same individual file via different pathnames (hardlinks/symlinks), but we would at least have a stable representation of a file.

[falhumai96]

Hello devs,

I would like to follow up on this issue whether or not there has been an update on it.

Thanks.

[mstorsjo]

In D146490#4307269, @falhumai96 wrote:

I would like to follow up on this issue whether or not there has been an update on it.

There's no update currently; I think there's some amount of consensus that this approach, while fixing the issue, would cause too much other collateral issues.

I'm considering making a patch that switches to hashing the canonicalized path, for the cases where we know the path isn't a local path; it's not an entirely correct solution, but a rough approximation for when the file IDs are stable or not. I just haven't had time to try doing that yet.

clang-tools-extra/clangd/unittests/FSTests.cpp

TEST(FSTests, PreambleStatusCache) {
   EXPECT_TRUE(ProduceFS->status("y"));
   EXPECT_TRUE(ProduceFS->status("main"));
 
   EXPECT_TRUE(StatCache.lookup(testPath("x")).has_value());
   EXPECT_TRUE(StatCache.lookup(testPath("y")).has_value());
   // Main file is not cached.
   EXPECT_FALSE(StatCache.lookup(testPath("main")).has_value());
 
-  llvm::vfs::Status S("fake", llvm::sys::fs::UniqueID(123, 456),
+  llvm::vfs::Status S("fake", llvm::sys::fs::UniqueID::inMemory(123, 456),
                       std::chrono::system_clock::now(), 0, 0, 1024,
                       llvm::sys::fs::file_type::regular_file,
                       llvm::sys::fs::all_all);
   StatCache.update(*FS, S);
   auto ConsumeFS = StatCache.getConsumingFS(FS);
   auto Cached = ConsumeFS->status(testPath("fake"));
   EXPECT_TRUE(Cached);
   EXPECT_EQ(Cached->getName(), testPath("fake"));

clang/include/clang/Basic/FileEntry.h

 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/DenseMapInfo.h"
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/PointerUnion.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem/UniqueID.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/VirtualFileSystem.h"
 
 #include <optional>
 #include <utility>
 
-namespace llvm {
-
-class MemoryBuffer;
-
-namespace vfs {
-
-class File;
-
-} // namespace vfs
-} // namespace llvm
-
 namespace clang {
 
 class FileEntryRef;
 
 namespace optional_detail {
 
 /// Forward declare a template specialization for OptionalStorage.
 template <> class OptionalStorage<clang::FileEntryRef>;

 /// Cached information about one file (either on disk
 /// or in the virtual file system).
 ///
 /// If the 'File' member is valid, then this FileEntry has an open file
 /// descriptor for the file.
 class FileEntry {
   friend class FileManager;
   friend class FileEntryTestHelper;
-  FileEntry();
+  FileEntry() = default;
   FileEntry(const FileEntry &) = delete;
   FileEntry &operator=(const FileEntry &) = delete;
 
   std::string RealPathName;   // Real path to the file; could be empty.
   off_t Size = 0;             // File size in bytes.
   time_t ModTime = 0;         // Modification time of file.
   const DirectoryEntry *Dir = nullptr; // Directory file lives in.
   llvm::sys::fs::UniqueID UniqueID;

clang/lib/Basic/FileEntry.cpp

 //===----------------------------------------------------------------------===//
 
 #include "clang/Basic/FileEntry.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/VirtualFileSystem.h"
 
 using namespace clang;
 
-FileEntry::FileEntry() : UniqueID(0, 0) {}
-
 FileEntry::~FileEntry() = default;
 
 void FileEntry::closeFile() const { File.reset(); }

clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

 DependencyScanningFilesystemSharedCache::getShardForFilename(
     StringRef Filename) const {
   return CacheShards[llvm::hash_value(Filename) % NumShards];
 }
 
 DependencyScanningFilesystemSharedCache::CacheShard &
 DependencyScanningFilesystemSharedCache::getShardForUID(
     llvm::sys::fs::UniqueID UID) const {
-  auto Hash = llvm::hash_combine(UID.getDevice(), UID.getFile());
-  return CacheShards[Hash % NumShards];
+  return CacheShards[UID.getHashValue() % NumShards];
 }
 
 const CachedFileSystemEntry *
 DependencyScanningFilesystemSharedCache::CacheShard::findEntryByFilename(
     StringRef Filename) const {
   std::lock_guard<std::mutex> LockGuard(CacheLock);
   auto It = EntriesByFilename.find(Filename);
   return It == EntriesByFilename.end() ? nullptr : It->getValue();
return llvm::StringSwitch<PathPolicy>(Ext)
       .Default(PathPolicy::fallThrough());
   // clang-format on
 }
 
 const CachedFileSystemEntry &
 DependencyScanningWorkerFilesystem::getOrEmplaceSharedEntryForUID(
     TentativeEntry TEntry) {
   auto &Shard = SharedCache.getShardForUID(TEntry.Status.getUniqueID());
-  return Shard.getOrEmplaceEntryForUID(TEntry.Status.getUniqueID(),
-                                       std::move(TEntry.Status),
+  llvm::sys::fs::UniqueID Id = TEntry.Status.getUniqueID();
+  return Shard.getOrEmplaceEntryForUID(Id, std::move(TEntry.Status),
                                        std::move(TEntry.Contents));
 }
 
 const CachedFileSystemEntry *
 DependencyScanningWorkerFilesystem::findEntryByFilenameWithWriteThrough(
     StringRef Filename) {
   if (const auto *Entry = LocalCache.findEntryByFilename(Filename))
     return Entry;

clang/tools/libclang/Indexing.cpp

 ///   #2 is identified as the location of "#ifdef BLAH"
 ///   #3 is identified as the location of "#ifdef CAKE"
 ///
 class PPRegion {
   llvm::sys::fs::UniqueID UniqueID;
   time_t ModTime;
   unsigned Offset;
 public:
-  PPRegion() : UniqueID(0, 0), ModTime(), Offset() {}
+  PPRegion() : ModTime(), Offset() {}
   PPRegion(llvm::sys::fs::UniqueID UniqueID, unsigned offset, time_t modTime)
       : UniqueID(UniqueID), ModTime(modTime), Offset(offset) {}
 
   const llvm::sys::fs::UniqueID &getUniqueID() const { return UniqueID; }
   unsigned getOffset() const { return Offset; }
   time_t getModTime() const { return ModTime; }
 
   bool isInvalid() const { return *this == PPRegion(); }
 
   friend bool operator==(const PPRegion &lhs, const PPRegion &rhs) {
     return lhs.UniqueID == rhs.UniqueID && lhs.Offset == rhs.Offset &&
            lhs.ModTime == rhs.ModTime;
   }
 };
 
 } // end anonymous namespace
 
 namespace llvm {
 
   template <>
   struct DenseMapInfo<PPRegion> {
     static inline PPRegion getEmptyKey() {
-      return PPRegion(llvm::sys::fs::UniqueID(0, 0), unsigned(-1), 0);
+    return PPRegion(llvm::sys::fs::UniqueID(), unsigned(-1), 0);
     }
     static inline PPRegion getTombstoneKey() {
-      return PPRegion(llvm::sys::fs::UniqueID(0, 0), unsigned(-2), 0);
+    return PPRegion(llvm::sys::fs::UniqueID(), unsigned(-2), 0);
     }
 
     static unsigned getHashValue(const PPRegion &S) {
       llvm::FoldingSetNodeID ID;
       const llvm::sys::fs::UniqueID &UniqueID = S.getUniqueID();
       ID.AddInteger(UniqueID.getFile());
       ID.AddInteger(UniqueID.getDevice());
       ID.AddInteger(S.getOffset());

clang/unittests/Basic/FileManagerTest.cpp

if (is_style_posix(llvm::sys::path::Style::native)) {
         llvm::sys::path::native(NormalizedStatPath);
         StatPath = NormalizedStatPath.c_str();
       }
     }
 
     auto fileType = IsFile ?
       llvm::sys::fs::file_type::regular_file :
       llvm::sys::fs::file_type::directory_file;
-    llvm::vfs::Status Status(StatPath ? StatPath : Path,
-                             llvm::sys::fs::UniqueID(1, INode),
-                             /*MTime*/{}, /*User*/0, /*Group*/0,
-                             /*Size*/0, fileType,
-                             llvm::sys::fs::perms::all_all);
+    llvm::vfs::Status Status(
+        StatPath ? StatPath : Path, llvm::sys::fs::UniqueID::inMemory(1, INode),
+        /*MTime*/ {}, /*User*/ 0, /*Group*/ 0,
+        /*Size*/ 0, fileType, llvm::sys::fs::perms::all_all);
     if (StatPath)
       Status.ExposesExternalVFSPath = true;
     StatCalls[Path] = Status;
   }
 
 public:
   // Inject a file with the given inode value to the fake file system.
   void InjectFile(const char *Path, ino_t INode,

llvm/include/llvm/Support/FileSystem.h

class file_status : public basic_file_status {
   friend bool equivalent(file_status A, file_status B);
 
   #if defined(LLVM_ON_UNIX)
   dev_t fs_st_dev = 0;
   nlink_t fs_st_nlinks = 0;
   ino_t fs_st_ino = 0;
   #elif defined (_WIN32)
   uint32_t NumLinks = 0;
-  uint32_t VolumeSerialNumber = 0;
-  uint32_t FileIndexHigh = 0;
-  uint32_t FileIndexLow = 0;
-  #endif
+  sys::fs::UniqueID FileID;
 
 public:
   file_status() = default;
 
   explicit file_status(file_type Type) : basic_file_status(Type) {}
 
   #if defined(LLVM_ON_UNIX)
   file_status(file_type Type, perms Perms, dev_t Dev, nlink_t Links, ino_t Ino,
               time_t ATime, uint32_t ATimeNSec,
               time_t MTime, uint32_t MTimeNSec,
               uid_t UID, gid_t GID, off_t Size)
       : basic_file_status(Type, Perms, ATime, ATimeNSec, MTime, MTimeNSec,
                           UID, GID, Size),
         fs_st_dev(Dev), fs_st_nlinks(Links), fs_st_ino(Ino) {}
   #elif defined(_WIN32)
   file_status(file_type Type, perms Perms, uint32_t LinkCount,
               uint32_t LastAccessTimeHigh, uint32_t LastAccessTimeLow,
               uint32_t LastWriteTimeHigh, uint32_t LastWriteTimeLow,
-              uint32_t VolumeSerialNumber, uint32_t FileSizeHigh,
-              uint32_t FileSizeLow, uint32_t FileIndexHigh,
-              uint32_t FileIndexLow)
+              uint32_t FileSizeHigh, uint32_t FileSizeLow,
+              sys::fs::UniqueID FileID)
       : basic_file_status(Type, Perms, LastAccessTimeHigh, LastAccessTimeLow,
                           LastWriteTimeHigh, LastWriteTimeLow, FileSizeHigh,
                           FileSizeLow),
-        NumLinks(LinkCount), VolumeSerialNumber(VolumeSerialNumber),
-        FileIndexHigh(FileIndexHigh), FileIndexLow(FileIndexLow) {}
-  #endif
+        NumLinks(LinkCount), FileID(FileID) {}
 
   UniqueID getUniqueID() const;
   uint32_t getLinkCount() const;
 };
 
 /// @}
 /// @name Physical Operators
 /// @{

llvm/include/llvm/Support/FileSystem/UniqueID.h

 #ifndef LLVM_SUPPORT_FILESYSTEM_UNIQUEID_H
 #define LLVM_SUPPORT_FILESYSTEM_UNIQUEID_H
 
 #include "llvm/ADT/DenseMapInfo.h"
 #include "llvm/ADT/Hashing.h"
 #include <cstdint>
 #include <utility>
 
+#if defined(_WIN32)
+#include "llvm/Support/Windows/ScopedHandle.h"
+#include <memory>
+#undef max
+#endif
+
 namespace llvm {
 namespace sys {
 namespace fs {
 
 class UniqueID {
-  uint64_t Device;
-  uint64_t File;
+  uint64_t Device = 0;
+  uint64_t File = 0;
+
+#if defined(_WIN32)
+  // On Windows, the handle is kept here only to guarantee that the values of
+  // FileIndexHigh and FileIndexLow remain valid during the lifetime of the
+  // UniqueID instance. The underlying Windows API only guarantees valid IDs as
+  // long as the file is open, please see:
+  // https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_id_info
+  // and:
+  // https://learn.microsoft.com/en-us/windows/win32/api/fileapi/ns-fileapi-by_handle_file_information
+
+  using H =
+      llvm::sys::windows::ScopedHandle<llvm::sys::windows::CommonHandleTraits>;
+  std::shared_ptr<H> Handle;
+#endif
 
 public:
   UniqueID() = default;
+
+#if defined(LLVM_ON_UNIX)
   UniqueID(uint64_t Device, uint64_t File) : Device(Device), File(File) {}
+#elif defined(_WIN32)
+  UniqueID(uint64_t VolumeSerialNumber, uint64_t FileIndex, uint64_t Handle)
+      : Device(VolumeSerialNumber), File(FileIndex),
+        Handle(std::make_shared<H>(Handle)) {}
+#endif
 
   bool operator==(const UniqueID &Other) const {
     return Device == Other.Device && File == Other.File;
   }
   bool operator!=(const UniqueID &Other) const { return !(*this == Other); }
   bool operator<(const UniqueID &Other) const {
-    /// Don't use std::tie since it bloats the compile time of this header.
-    if (Device < Other.Device)
-      return true;
-    if (Other.Device < Device)
-      return false;
-    return File < Other.File;
+    return std::make_pair(Device, File) <
+           std::make_pair(Other.Device, Other.File);
   }
 
   uint64_t getDevice() const { return Device; }
   uint64_t getFile() const { return File; }
+
+  // Create a UniqueID that is known to refer to in-memory files. The FileID
+  // argument must be a unique value representing a in-memory file.
+  static UniqueID inMemory(uint64_t DeviceID, uint64_t FileID) {
+#if defined(LLVM_ON_UNIX)
+    return UniqueID(DeviceID, FileID);
+#elif defined(_WIN32)
+    UniqueID ID;
+    ID.Device = DeviceID;
+    ID.File = FileID;
+    return ID;
+#endif
+  }
+
+  hash_code getHashValue() const {
+    return hash_value(std::make_pair(Device, File));
+  }
+
+private:
+  UniqueID(uint64_t Key) : Device(Key), File(Key) {}
+
+  friend struct DenseMapInfo<llvm::sys::fs::UniqueID>;
 };
 
 } // end namespace fs
 } // end namespace sys
 
 // Support UniqueIDs as DenseMap keys.
 template <> struct DenseMapInfo<llvm::sys::fs::UniqueID> {
   static inline llvm::sys::fs::UniqueID getEmptyKey() {
-    auto EmptyKey = DenseMapInfo<std::pair<uint64_t, uint64_t>>::getEmptyKey();
-    return {EmptyKey.first, EmptyKey.second};
+    auto EmptyKey = DenseMapInfo<uint64_t>::getEmptyKey();
+    return llvm::sys::fs::UniqueID(EmptyKey);
   }
 
   static inline llvm::sys::fs::UniqueID getTombstoneKey() {
-    auto TombstoneKey =
-        DenseMapInfo<std::pair<uint64_t, uint64_t>>::getTombstoneKey();
-    return {TombstoneKey.first, TombstoneKey.second};
+    auto TombstoneKey = DenseMapInfo<uint64_t>::getTombstoneKey();
+    return llvm::sys::fs::UniqueID(TombstoneKey);
   }
 
   static hash_code getHashValue(const llvm::sys::fs::UniqueID &Tag) {
-    return hash_value(std::make_pair(Tag.getDevice(), Tag.getFile()));
+    return Tag.getHashValue();
   }
 
   static bool isEqual(const llvm::sys::fs::UniqueID &LHS,
                       const llvm::sys::fs::UniqueID &RHS) {
     return LHS == RHS;
   }
 };
 
 } // end namespace llvm
 
 #endif // LLVM_SUPPORT_FILESYSTEM_UNIQUEID_H

llvm/include/llvm/Support/Windows/ScopedHandle.h

+//===- ScopedHandle.h - Common Windows Include File -------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// A helper that keeps a Windows HANDLE open. This helper is meant to be used
+// in situations where it is not desirable to include windows.h.
+//
+//===----------------------------------------------------------------------===//
+
+//===----------------------------------------------------------------------===//
+//=== WARNING: Implementation here must contain only generic Win32 code that
+//===          is guaranteed to work on *all* Win32 variants.
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_SUPPORT_SCOPEDHANDLE_H
+#define LLVM_SUPPORT_SCOPEDHANDLE_H
+
+namespace llvm {
+namespace sys {
+namespace windows {
+
+template <typename HandleTraits> class ScopedHandle {
+  typedef typename HandleTraits::handle_type handle_type;
+  handle_type Handle = HandleTraits::GetInvalid();
+
+  ScopedHandle(const ScopedHandle &other) = delete;
+  void operator=(const ScopedHandle &other) = delete;
+
+public:
+  ScopedHandle() = default;
+
+  explicit ScopedHandle(handle_type h) : Handle(h) {}
+
+  ~ScopedHandle() {
+    if (Handle != HandleTraits::GetInvalid())
+      HandleTraits::Close(Handle);
+  }
+
+  handle_type take() {
+    handle_type t = Handle;
+    Handle = HandleTraits::GetInvalid();
+    return t;
+  }
+
+  ScopedHandle &operator=(handle_type h) {
+    if (Handle != HandleTraits::GetInvalid())
+      HandleTraits::Close(Handle);
+    Handle = h;
+    return *this;
+  }
+
+  // True if Handle is valid.
+  explicit operator bool() const {
+    return Handle != HandleTraits::GetInvalid();
+  }
+
+  operator handle_type() const { return Handle; }
+};
+
+struct CommonHandleTraits {
+  // Don't use HANDLE here to avoid including windows.h.
+  using handle_type = uint64_t;
+
+  static handle_type GetInvalid() { return (uint64_t)-1ULL; }
+
+  static void Close(handle_type h);
+};
+
+} // end namespace windows.
+} // end namespace sys
+} // end namespace llvm.
+
+#endif

llvm/include/llvm/Support/Windows/WindowsSupport.h

 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Config/llvm-config.h" // Get build system configuration settings
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/Chrono.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/VersionTuple.h"
+#include "llvm/Support/Windows/ScopedHandle.h"
 #include <cassert>
 #include <string>
 #include <system_error>
 #include <windows.h>
 
 // Must be included after windows.h
 #include <wincrypt.h>
 
+using llvm::sys::windows;
+
 namespace llvm {
 
 /// Determines if the program is running on Windows 8 or newer. This
 /// reimplements one of the helpers in the Windows 8.1 SDK, which are intended
 /// to supercede raw calls to GetVersionEx. Old SDKs, Cygwin, and MinGW don't
 /// yet have VersionHelpers.h, so we have our own helper.
 bool RunningWindows8OrGreater();
 

 
 // Include GetLastError() in a fatal error message.
 [[noreturn]] inline void ReportLastErrorFatal(const char *Msg) {
   std::string ErrMsg;
   MakeErrMsg(&ErrMsg, Msg);
   llvm::report_fatal_error(Twine(ErrMsg));
 }
 
-template <typename HandleTraits>
-class ScopedHandle {
-  typedef typename HandleTraits::handle_type handle_type;
-  handle_type Handle;
-
-  ScopedHandle(const ScopedHandle &other) = delete;
-  void operator=(const ScopedHandle &other) = delete;
-public:
-  ScopedHandle()
-    : Handle(HandleTraits::GetInvalid()) {}
-
-  explicit ScopedHandle(handle_type h)
-    : Handle(h) {}
-
-  ~ScopedHandle() {
-    if (HandleTraits::IsValid(Handle))
-      HandleTraits::Close(Handle);
-  }
-
-  handle_type take() {
-    handle_type t = Handle;
-    Handle = HandleTraits::GetInvalid();
-    return t;
-  }
-
-  ScopedHandle &operator=(handle_type h) {
-    if (HandleTraits::IsValid(Handle))
-      HandleTraits::Close(Handle);
-    Handle = h;
-    return *this;
-  }
-
-  // True if Handle is valid.
-  explicit operator bool() const {
-    return HandleTraits::IsValid(Handle) ? true : false;
-  }
-
-  operator handle_type() const {
-    return Handle;
-  }
-};
+struct TypedHandleTraits {
+  using handle_type = HANDLE;
 
-struct CommonHandleTraits {
-  typedef HANDLE handle_type;
-
-  static handle_type GetInvalid() {
-    return INVALID_HANDLE_VALUE;
-  }
+  static handle_type GetInvalid() { return INVALID_HANDLE_VALUE; }
 
   static void Close(handle_type h) {
-    ::CloseHandle(h);
-  }
-
-  static bool IsValid(handle_type h) {
-    return h != GetInvalid();
+    CommonHandleTraits::Close((CommonHandleTraits::handle_type)h);
   }
 };
 
-struct JobHandleTraits : CommonHandleTraits {
-  static handle_type GetInvalid() {
-    return NULL;
-  }
+struct JobHandleTraits : TypedHandleTraits {
+  static handle_type GetInvalid() { return NULL; }
 };
 
-struct CryptContextTraits : CommonHandleTraits {
-  typedef HCRYPTPROV handle_type;
-
-  static handle_type GetInvalid() {
-    return 0;
-  }
+struct CryptContextTraits : TypedHandleTraits {
+  using handle_type = HCRYPTPROV;
 
-  static void Close(handle_type h) {
-    ::CryptReleaseContext(h, 0);
-  }
+  static handle_type GetInvalid() { return 0; }
 
-  static bool IsValid(handle_type h) {
-    return h != GetInvalid();
-  }
+  static void Close(handle_type h) { ::CryptReleaseContext(h, 0); }
 };
 
-struct RegTraits : CommonHandleTraits {
-  typedef HKEY handle_type;
+struct RegTraits : TypedHandleTraits {
+  using handle_type = HKEY;
 
-  static handle_type GetInvalid() {
-    return NULL;
-  }
-
-  static void Close(handle_type h) {
-    ::RegCloseKey(h);
-  }
+  static handle_type GetInvalid() { return NULL; }
 
-  static bool IsValid(handle_type h) {
-    return h != GetInvalid();
-  }
+  static void Close(handle_type h) { ::RegCloseKey(h); }
 };
 
-struct FindHandleTraits : CommonHandleTraits {
-  static void Close(handle_type h) {
-    ::FindClose(h);
-  }
+struct FindHandleTraits : TypedHandleTraits {
+  static void Close(handle_type h) { ::FindClose((HANDLE)h); }
 };
 
-struct FileHandleTraits : CommonHandleTraits {};
+struct FileHandleTraits : TypedHandleTraits {};
 
-typedef ScopedHandle<CommonHandleTraits> ScopedCommonHandle;
-typedef ScopedHandle<FileHandleTraits>   ScopedFileHandle;
+typedef ScopedHandle<TypedHandleTraits> ScopedCommonHandle;
+typedef ScopedHandle<FileHandleTraits> ScopedFileHandle;
 typedef ScopedHandle<CryptContextTraits> ScopedCryptContext;
-typedef ScopedHandle<RegTraits>          ScopedRegHandle;
-typedef ScopedHandle<FindHandleTraits>   ScopedFindHandle;
-typedef ScopedHandle<JobHandleTraits>    ScopedJobHandle;
+typedef ScopedHandle<RegTraits> ScopedRegHandle;
+typedef ScopedHandle<FindHandleTraits> ScopedFindHandle;
+typedef ScopedHandle<JobHandleTraits> ScopedJobHandle;
 
 template <class T>
 class SmallVectorImpl;
 
 template <class T>
 typename SmallVectorImpl<T>::const_pointer
 c_str(SmallVectorImpl<T> &str) {
   str.push_back(0);

llvm/lib/Support/VirtualFileSystem.cpp

 };
 
 } // namespace detail
 
 // The UniqueID of in-memory files is derived from path and content.
 // This avoids difficulties in creating exactly equivalent in-memory FSes,
 // as often needed in multithreaded programs.
 static sys::fs::UniqueID getUniqueID(hash_code Hash) {
-  return sys::fs::UniqueID(std::numeric_limits<uint64_t>::max(),
+  return sys::fs::UniqueID::inMemory(std::numeric_limits<uint64_t>::max(),
                                      uint64_t(size_t(Hash)));
 }
 static sys::fs::UniqueID getFileID(sys::fs::UniqueID Parent,
                                    llvm::StringRef Name,
                                    llvm::StringRef Contents) {
   return getUniqueID(llvm::hash_combine(Parent.getFile(), Name, Contents));
 }
 static sys::fs::UniqueID getDirectoryID(sys::fs::UniqueID Parent,
                                         llvm::StringRef Name) {
void vfs::collectVFSFromYAML(std::unique_ptr<MemoryBuffer> Buffer,
   getVFSEntries(RootResult->E, Components, CollectedEntries);
 }
 
 UniqueID vfs::getNextVirtualUniqueID() {
   static std::atomic<unsigned> UID;
   unsigned ID = ++UID;
   // The following assumes that uint64_t max will never collide with a real
   // dev_t value from the OS.
-  return UniqueID(std::numeric_limits<uint64_t>::max(), ID);
+  return UniqueID::inMemory(std::numeric_limits<uint64_t>::max(), ID);
 }
 
 void YAMLVFSWriter::addEntry(StringRef VirtualPath, StringRef RealPath,
                              bool IsDirectory) {
   assert(sys::path::is_absolute(VirtualPath) && "virtual path not absolute");
   assert(sys::path::is_absolute(RealPath) && "real path not absolute");
   assert(!pathHasTraversal(VirtualPath) && "path traversal is not supported");
   Mappings.emplace_back(VirtualPath, RealPath, IsDirectory);

llvm/lib/Support/Windows/Path.inc

static bool is_separator(const wchar_t value) {
   case L'\\':
   case L'/':
     return true;
   default:
     return false;
   }
 }
 
+static HANDLE duplicateHandle(HANDLE Handle) {
+  if (Handle == INVALID_HANDLE_VALUE)
+    return INVALID_HANDLE_VALUE;
+  HANDLE NewHandle;
+  if (!::DuplicateHandle(::GetCurrentProcess(), Handle, ::GetCurrentProcess(),
+                         &NewHandle, 0, 0, DUPLICATE_SAME_ACCESS)) {
+    return INVALID_HANDLE_VALUE;
+  }
+  return NewHandle;
+}
+
 namespace llvm {
 namespace sys {
 namespace windows {
 
 // Convert a UTF-8 path to UTF-16. Also, if the absolute equivalent of the path
 // is longer than the limit that the Win32 Unicode File API can tolerate, make
 // it an absolute normalized path prefixed by '\\?\'.
 std::error_code widenPath(const Twine &Path8, SmallVectorImpl<wchar_t> &Path16,
if (RootName[1] != ':') { // Check if UNC.
     FullPath.append("UNC\\");
     FullPath.append(Path8Str.begin() + 2, Path8Str.end());
   } else
     FullPath.append(Path8Str);
 
   return UTF8ToUTF16(FullPath, Path16);
 }
 
+void CommonHandleTraits::Close(handle_type h) { ::CloseHandle((HANDLE)h); }
+
 } // end namespace windows
 
 namespace fs {
 
 const file_t kInvalidFile = INVALID_HANDLE_VALUE;
 
 std::string getMainExecutable(const char *argv0, void *MainExecAddr) {
   SmallVector<wchar_t, MAX_PATH> PathName;
std::string getMainExecutable(const char *argv0, void *MainExecAddr) {
   SmallVector<char, MAX_PATH> PathNameUTF8;
   if (UTF16ToUTF8(PathName.data(), PathName.size(), PathNameUTF8))
     return "";
 
   llvm::sys::path::make_preferred(PathNameUTF8);
   return std::string(PathNameUTF8.data());
 }
 
-UniqueID file_status::getUniqueID() const {
-  // The file is uniquely identified by the volume serial number along
-  // with the 64-bit file identifier.
-  uint64_t FileID = (static_cast<uint64_t>(FileIndexHigh) << 32ULL) |
-                    static_cast<uint64_t>(FileIndexLow);
-
-  return UniqueID(VolumeSerialNumber, FileID);
-}
+UniqueID file_status::getUniqueID() const { return FileID; }
 
 ErrorOr<space_info> disk_space(const Twine &Path) {
   ULARGE_INTEGER Avail, Total, Free;
   if (!::GetDiskFreeSpaceExA(Path.str().c_str(), &Avail, &Total, &Free))
     return mapWindowsError(::GetLastError());
   space_info SpaceInfo;
   SpaceInfo.capacity =
       (static_cast<uint64_t>(Total.HighPart) << 32) + Total.LowPart;

 
 bool can_execute(const Twine &Path) {
   return !access(Path, AccessMode::Execute) ||
          !access(Path + ".exe", AccessMode::Execute);
 }
 
 bool equivalent(file_status A, file_status B) {
   assert(status_known(A) && status_known(B));
-  return A.FileIndexHigh == B.FileIndexHigh &&
-         A.FileIndexLow == B.FileIndexLow && A.FileSizeHigh == B.FileSizeHigh &&
+  return A.FileID == B.FileID && A.FileSizeHigh == B.FileSizeHigh &&
          A.FileSizeLow == B.FileSizeLow &&
          A.LastWriteTimeHigh == B.LastWriteTimeHigh &&
-         A.LastWriteTimeLow == B.LastWriteTimeLow &&
-         A.VolumeSerialNumber == B.VolumeSerialNumber;
+         A.LastWriteTimeLow == B.LastWriteTimeLow;
 }
 
 std::error_code equivalent(const Twine &A, const Twine &B, bool &result) {
   file_status fsA, fsB;
   if (std::error_code ec = status(A, fsA))
     return ec;
   if (std::error_code ec = status(B, fsB))
     return ec;
static std::error_code getStatus(HANDLE FileHandle, file_status &Result) {
   if (!::GetFileInformationByHandle(FileHandle, &Info))
     goto handle_status_error;
 
   Result = file_status(
       file_type_from_attrs(Info.dwFileAttributes),
       perms_from_attrs(Info.dwFileAttributes), Info.nNumberOfLinks,
       Info.ftLastAccessTime.dwHighDateTime, Info.ftLastAccessTime.dwLowDateTime,
       Info.ftLastWriteTime.dwHighDateTime, Info.ftLastWriteTime.dwLowDateTime,
-      Info.dwVolumeSerialNumber, Info.nFileSizeHigh, Info.nFileSizeLow,
-      Info.nFileIndexHigh, Info.nFileIndexLow);
+      Info.nFileSizeHigh, Info.nFileSizeLow,
+      llvm::sys::fs::UniqueID(Info.dwVolumeSerialNumber,
+                              ((uint64_t)Info.nFileIndexHigh << 32) |
+                                  Info.nFileIndexLow,
+                              (uint64_t)duplicateHandle(FileHandle)));
   return std::error_code();
 
 handle_status_error:
   DWORD LastError = ::GetLastError();
   if (LastError == ERROR_FILE_NOT_FOUND || LastError == ERROR_PATH_NOT_FOUND)
     Result = file_status(file_type::file_not_found);
   else if (LastError == ERROR_SHARING_VIOLATION)
     Result = file_status(file_type::type_unknown);

llvm/unittests/Support/FSUniqueIDTest.cpp

 #include "gtest/gtest.h"
 
 using namespace llvm;
 using namespace llvm::sys::fs;
 
 namespace {
 
 TEST(FSUniqueIDTest, construct) {
-  EXPECT_EQ(20u, UniqueID(20, 10).getDevice());
-  EXPECT_EQ(10u, UniqueID(20, 10).getFile());
+  EXPECT_EQ(20u, UniqueID::inMemory(20, 10).getDevice());
+  EXPECT_EQ(10u, UniqueID::inMemory(20, 10).getFile());
 }
 
 TEST(FSUniqueIDTest, equals) {
-  EXPECT_EQ(UniqueID(20, 10), UniqueID(20, 10));
-  EXPECT_NE(UniqueID(20, 20), UniqueID(20, 10));
-  EXPECT_NE(UniqueID(10, 10), UniqueID(20, 10));
+  EXPECT_EQ(UniqueID::inMemory(20, 10), UniqueID::inMemory(20, 10));
+  EXPECT_NE(UniqueID::inMemory(20, 20), UniqueID::inMemory(20, 10));
+  EXPECT_NE(UniqueID::inMemory(10, 10), UniqueID::inMemory(20, 10));
 }
 
 TEST(FSUniqueIDTest, less) {
-  EXPECT_FALSE(UniqueID(20, 2) < UniqueID(20, 2));
-  EXPECT_FALSE(UniqueID(20, 3) < UniqueID(20, 2));
-  EXPECT_FALSE(UniqueID(30, 2) < UniqueID(20, 2));
-  EXPECT_FALSE(UniqueID(30, 2) < UniqueID(20, 40));
-  EXPECT_TRUE(UniqueID(20, 2) < UniqueID(20, 3));
-  EXPECT_TRUE(UniqueID(20, 2) < UniqueID(30, 2));
-  EXPECT_TRUE(UniqueID(20, 40) < UniqueID(30, 2));
+  EXPECT_FALSE(UniqueID::inMemory(20, 2) < UniqueID::inMemory(20, 2));
+  EXPECT_FALSE(UniqueID::inMemory(20, 3) < UniqueID::inMemory(20, 2));
+  EXPECT_FALSE(UniqueID::inMemory(30, 2) < UniqueID::inMemory(20, 2));
+  EXPECT_FALSE(UniqueID::inMemory(30, 2) < UniqueID::inMemory(20, 40));
+  EXPECT_TRUE(UniqueID::inMemory(20, 2) < UniqueID::inMemory(20, 3));
+  EXPECT_TRUE(UniqueID::inMemory(20, 2) < UniqueID::inMemory(30, 2));
+  EXPECT_TRUE(UniqueID::inMemory(20, 40) < UniqueID::inMemory(30, 2));
 }
 
 } // anonymous namespace

llvm/unittests/Support/Path.cpp

TEST_F(FileSystemTest, Unique) {
   SmallString<64> TempPath2;
   ASSERT_NO_ERROR(
       fs::createTemporaryFile("prefix", "temp", FileDescriptor2, TempPath2));
 
   fs::UniqueID D;
   ASSERT_NO_ERROR(fs::getUniqueID(Twine(TempPath2), D));
   ASSERT_NE(D, F1);
   ::close(FileDescriptor2);
+  D = fs::UniqueID();
 
   ASSERT_NO_ERROR(fs::remove(Twine(TempPath2)));
 
   // Two paths representing the same file on disk should still provide the
   // same unique id.  We can test this by making a hard link.
   ASSERT_NO_ERROR(fs::create_link(Twine(TempPath), Twine(TempPath2)));
   fs::UniqueID D2;
   ASSERT_NO_ERROR(fs::getUniqueID(Twine(TempPath2), D2));
   ASSERT_EQ(D2, F1);
 
+  D2 = fs::UniqueID();
   ::close(FileDescriptor);
 
   SmallString<128> Dir1;
   ASSERT_NO_ERROR(
      fs::createUniqueDirectory("dir1", Dir1));
   ASSERT_NO_ERROR(fs::getUniqueID(Dir1.c_str(), F1));
   ASSERT_NO_ERROR(fs::getUniqueID(Dir1.c_str(), F2));
   ASSERT_EQ(F1, F2);
 
   SmallString<128> Dir2;
   ASSERT_NO_ERROR(
      fs::createUniqueDirectory("dir2", Dir2));
   ASSERT_NO_ERROR(fs::getUniqueID(Dir2.c_str(), F2));
   ASSERT_NE(F1, F2);
+
+  F1 = fs::UniqueID();
+  F2 = fs::UniqueID();
+
   ASSERT_NO_ERROR(fs::remove(Dir1));
   ASSERT_NO_ERROR(fs::remove(Dir2));
   ASSERT_NO_ERROR(fs::remove(TempPath2));
   ASSERT_NO_ERROR(fs::remove(TempPath));
 }
 
 TEST_F(FileSystemTest, RealPath) {
   ASSERT_NO_ERROR(
TEST_F(FileSystemTest, TempFiles) {
   ASSERT_NE(TempPath.str(), TempPath2.str());
 
   fs::file_status A, B;
   ASSERT_NO_ERROR(fs::status(Twine(TempPath), A));
   ASSERT_NO_ERROR(fs::status(Twine(TempPath2), B));
   EXPECT_FALSE(fs::equivalent(A, B));
 
   ::close(FD2);
+  // Also close the file statuses which contain instances of UniqueID, which on
+  // Windows keep the file handle alive. See comment in UniqueID.h
+  A = fs::file_status();
+  B = fs::file_status();
 
   // Remove Temp2.
   ASSERT_NO_ERROR(fs::remove(Twine(TempPath2)));
   ASSERT_NO_ERROR(fs::remove(Twine(TempPath2)));
   ASSERT_EQ(fs::remove(Twine(TempPath2), false),
             errc::no_such_file_or_directory);
 
   std::error_code EC = fs::status(TempPath2.c_str(), B);
TEST_F(FileSystemTest, TempFiles) {
   ASSERT_NO_ERROR(fs::create_link(Twine(TempPath), Twine(TempPath2)));
   bool equal;
   ASSERT_NO_ERROR(fs::equivalent(Twine(TempPath), Twine(TempPath2), equal));
   EXPECT_TRUE(equal);
   ASSERT_NO_ERROR(fs::status(Twine(TempPath), A));
   ASSERT_NO_ERROR(fs::status(Twine(TempPath2), B));
   EXPECT_TRUE(fs::equivalent(A, B));
 
+  // Also close the file statuses which contain instances of UniqueID, which on
+  // Windows keep the file handle alive. See comment in UniqueID.h
+  A = fs::file_status();
+  B = fs::file_status();
+
   // Remove Temp1.
   ::close(FileDescriptor);
   ASSERT_NO_ERROR(fs::remove(Twine(TempPath)));
 
   // Remove the hard link.
   ASSERT_NO_ERROR(fs::remove(Twine(TempPath2)));
 
   // Make sure Temp1 doesn't exist.

llvm/unittests/Support/VirtualFileSystemTest.cpp

const_iterator findEntry(const Twine &Path) const {
     SmallString<128> P;
     Path.toVector(P);
     std::error_code EC = makeAbsolute(P);
     assert(!EC);
     (void)EC;
     return FilesAndDirs.find(std::string(P.str()));
   }
 
+  UniqueID makeUniqueID() { return UniqueID::inMemory(FSID, FileID++); }
+
   void addRegularFile(StringRef Path, sys::fs::perms Perms = sys::fs::all_all) {
-    vfs::Status S(Path, UniqueID(FSID, FileID++),
-                  std::chrono::system_clock::now(), 0, 0, 1024,
-                  sys::fs::file_type::regular_file, Perms);
+    vfs::Status S(Path, makeUniqueID(), std::chrono::system_clock::now(), 0, 0,
+                  1024, sys::fs::file_type::regular_file, Perms);
     addEntry(Path, S);
   }
 
   void addDirectory(StringRef Path, sys::fs::perms Perms = sys::fs::all_all) {
-    vfs::Status S(Path, UniqueID(FSID, FileID++),
-                  std::chrono::system_clock::now(), 0, 0, 0,
-                  sys::fs::file_type::directory_file, Perms);
+    vfs::Status S(Path, makeUniqueID(), std::chrono::system_clock::now(), 0, 0,
+                  0, sys::fs::file_type::directory_file, Perms);
     addEntry(Path, S);
   }
 
   void addSymlink(StringRef Path) {
-    vfs::Status S(Path, UniqueID(FSID, FileID++),
-                  std::chrono::system_clock::now(), 0, 0, 0,
-                  sys::fs::file_type::symlink_file, sys::fs::all_all);
+    vfs::Status S(Path, makeUniqueID(), std::chrono::system_clock::now(), 0, 0,
+                  0, sys::fs::file_type::symlink_file, sys::fs::all_all);
     addEntry(Path, S);
   }
 
 protected:
   void printImpl(raw_ostream &OS, PrintType Type,
                  unsigned IndentLevel) const override {
     printIndent(OS, IndentLevel);
     OS << "DummyFileSystem (";

llvm/utils/split-file/split-file.cpp

if (output.empty())
     fatal("", "output directory is not specified");
   ErrorOr<std::unique_ptr<MemoryBuffer>> bufferOrErr =
       MemoryBuffer::getFileOrSTDIN(input);
   if (std::error_code ec = bufferOrErr.getError())
     fatal(input, ec.message());
 
   // Delete output if it is a file or an empty directory, so that we can create
   // a directory.
+  {
     sys::fs::file_status status;
     if (std::error_code ec = sys::fs::status(output, status))
       if (ec.value() != static_cast<int>(std::errc::no_such_file_or_directory))
         fatal(output, ec.message());
     if (status.type() != sys::fs::file_type::file_not_found &&
         status.type() != sys::fs::file_type::directory_file &&
         status.type() != sys::fs::file_type::regular_file)
       fatal(output, "output cannot be a special file");
+  }
   if (std::error_code ec = sys::fs::remove(output, /*IgnoreNonExisting=*/true))
     if (ec.value() != static_cast<int>(std::errc::directory_not_empty) &&
         ec.value() != static_cast<int>(std::errc::file_exists))
       fatal(output, ec.message());
   return handle(**bufferOrErr, input);
 }