| commit 76f1baa7875acd88bdd4b431eed6e2d2decfc0fe |
| Author: Saleem Abdulrasool <compnerd@compnerd.org> |
| Date: Fri Jun 11 19:05:42 2021 -0700 |
| |
| Revert "Revert "DirectoryWatcher: add an implementation for Windows"" |
| |
| This reverts commit 0ec1cf13f2a4e31aa2c5ccc665c5fbdcd3a94577. |
| |
| Restore the implementation with some minor tweaks: |
| - Use std::unique_ptr for the path instead of std::vector |
| * Stylistic improvement as the buffer is already heap allocated, this |
| just makes it clearer. |
| - Correct the notification buffer allocation size |
| * Memory usage fix: we were allocating 4x the computed size |
| - Correct the passing of the buffer size to RDC |
| * Memory usage fix: we were reporting 1/4th of the size |
| - Convert the operation event to auto-reset |
| * Bug Fix: we never reset the event |
| - Remove `FILE_NOTIFY_CHANGE_LAST_ACCESS` from RDC events |
| * Memory usage fix: we never needed this notification |
| - Fold events for the notification action |
| * Stylistic improvement to be clear how the events map |
| - Update comment |
| * Stylistic improvement to be clear what the RAII controls |
| - Fix the race condition that was uncovered previously |
| * We would return from the construction before the watcher thread |
| began execution. The test would then proceed to begin execution, |
| and we would miss the initial notifications. We now ensure that the |
| watcher thread is initialized before we return. This ensures that |
| we do not miss the initial notifications. |
| |
| Running the test on a SSD was able to uncover the access pattern. This |
| now seems to pass reliably where it was previously flaky locally. |
| |
| diff --git a/clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp b/clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp |
| index 25cbcf536388..6bcfb86e7f99 100644 |
| --- a/clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp |
| +++ b/clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp |
| @@ -6,19 +6,12 @@ |
| // |
| //===----------------------------------------------------------------------===// |
| |
| -// TODO: This is not yet an implementation, but it will make it so Windows |
| -// builds don't fail. |
| - |
| #include "DirectoryScanner.h" |
| #include "clang/DirectoryWatcher/DirectoryWatcher.h" |
| - |
| #include "llvm/ADT/STLExtras.h" |
| -#include "llvm/ADT/ScopeExit.h" |
| -#include "llvm/Support/AlignOf.h" |
| -#include "llvm/Support/Errno.h" |
| -#include "llvm/Support/Mutex.h" |
| +#include "llvm/Support/ConvertUTF.h" |
| #include "llvm/Support/Path.h" |
| -#include <atomic> |
| +#include "llvm/Support/Windows/WindowsSupport.h" |
| #include <condition_variable> |
| #include <mutex> |
| #include <queue> |
| @@ -28,23 +21,271 @@ |
| |
| namespace { |
| |
| +using DirectoryWatcherCallback = |
| + std::function<void(llvm::ArrayRef<clang::DirectoryWatcher::Event>, bool)>; |
| + |
| using namespace llvm; |
| using namespace clang; |
| |
| class DirectoryWatcherWindows : public clang::DirectoryWatcher { |
| + OVERLAPPED Overlapped; |
| + |
| + std::vector<DWORD> Notifications; |
| + |
| + std::thread WatcherThread; |
| + std::thread HandlerThread; |
| + std::function<void(ArrayRef<DirectoryWatcher::Event>, bool)> Callback; |
| + SmallString<MAX_PATH> Path; |
| + HANDLE Terminate; |
| + |
| + std::mutex Mutex; |
| + bool WatcherActive = false; |
| + std::condition_variable Ready; |
| + |
| + class EventQueue { |
| + std::mutex M; |
| + std::queue<DirectoryWatcher::Event> Q; |
| + std::condition_variable CV; |
| + |
| + public: |
| + void emplace(DirectoryWatcher::Event::EventKind Kind, StringRef Path) { |
| + { |
| + std::unique_lock<std::mutex> L(M); |
| + Q.emplace(Kind, Path); |
| + } |
| + CV.notify_one(); |
| + } |
| + |
| + DirectoryWatcher::Event pop_front() { |
| + std::unique_lock<std::mutex> L(M); |
| + while (true) { |
| + if (!Q.empty()) { |
| + DirectoryWatcher::Event E = Q.front(); |
| + Q.pop(); |
| + return E; |
| + } |
| + CV.wait(L, [this]() { return !Q.empty(); }); |
| + } |
| + } |
| + } Q; |
| + |
| public: |
| - ~DirectoryWatcherWindows() override { } |
| - void InitialScan() { } |
| - void EventReceivingLoop() { } |
| - void StopWork() { } |
| + DirectoryWatcherWindows(HANDLE DirectoryHandle, bool WaitForInitialSync, |
| + DirectoryWatcherCallback Receiver); |
| + |
| + ~DirectoryWatcherWindows() override; |
| + |
| + void InitialScan(); |
| + void WatcherThreadProc(HANDLE DirectoryHandle); |
| + void NotifierThreadProc(bool WaitForInitialSync); |
| }; |
| + |
| +DirectoryWatcherWindows::DirectoryWatcherWindows( |
| + HANDLE DirectoryHandle, bool WaitForInitialSync, |
| + DirectoryWatcherCallback Receiver) |
| + : Callback(Receiver), Terminate(INVALID_HANDLE_VALUE) { |
| + // Pre-compute the real location as we will be handing over the directory |
| + // handle to the watcher and performing synchronous operations. |
| + { |
| + DWORD Size = GetFinalPathNameByHandleW(DirectoryHandle, NULL, 0, 0); |
| + std::unique_ptr<WCHAR[]> Buffer{new WCHAR[Size]}; |
| + Size = GetFinalPathNameByHandleW(DirectoryHandle, Buffer.get(), Size, 0); |
| + Buffer[Size] = L'\0'; |
| + llvm::sys::windows::UTF16ToUTF8(Buffer.get(), Size, Path); |
| + } |
| + |
| + size_t EntrySize = sizeof(FILE_NOTIFY_INFORMATION) + MAX_PATH * sizeof(WCHAR); |
| + Notifications.resize((4 * EntrySize) / sizeof(DWORD)); |
| + |
| + memset(&Overlapped, 0, sizeof(Overlapped)); |
| + Overlapped.hEvent = |
| + CreateEventW(NULL, /*bManualReset=*/FALSE, /*bInitialState=*/FALSE, NULL); |
| + assert(Overlapped.hEvent && "unable to create event"); |
| + |
| + Terminate = |
| + CreateEventW(NULL, /*bManualReset=*/TRUE, /*bInitialState=*/FALSE, NULL); |
| + |
| + WatcherThread = std::thread([this, DirectoryHandle]() { |
| + this->WatcherThreadProc(DirectoryHandle); |
| + }); |
| + |
| + if (WaitForInitialSync) |
| + InitialScan(); |
| + |
| + HandlerThread = std::thread([this, WaitForInitialSync]() { |
| + this->NotifierThreadProc(WaitForInitialSync); |
| + }); |
| + |
| + std::unique_lock<std::mutex> lock(Mutex); |
| + Ready.wait(lock, [this] { return this->WatcherActive; }); |
| +} |
| + |
| +DirectoryWatcherWindows::~DirectoryWatcherWindows() { |
| + // Signal the Watcher to exit. |
| + SetEvent(Terminate); |
| + HandlerThread.join(); |
| + WatcherThread.join(); |
| + CloseHandle(Terminate); |
| + CloseHandle(Overlapped.hEvent); |
| +} |
| + |
| +void DirectoryWatcherWindows::InitialScan() { |
| + Callback(getAsFileEvents(scanDirectory(Path.data())), /*IsInitial=*/true); |
| +} |
| + |
| +void DirectoryWatcherWindows::WatcherThreadProc(HANDLE DirectoryHandle) { |
| + { |
| + std::unique_lock<std::mutex> lock(Mutex); |
| + WatcherActive = true; |
| + } |
| + Ready.notify_one(); |
| + |
| + while (true) { |
| + // We do not guarantee subdirectories, but macOS already provides |
| + // subdirectories, might as well as ... |
| + BOOL WatchSubtree = TRUE; |
| + DWORD NotifyFilter = FILE_NOTIFY_CHANGE_FILE_NAME |
| + | FILE_NOTIFY_CHANGE_DIR_NAME |
| + | FILE_NOTIFY_CHANGE_SIZE |
| + | FILE_NOTIFY_CHANGE_LAST_WRITE |
| + | FILE_NOTIFY_CHANGE_CREATION; |
| + |
| + DWORD BytesTransferred; |
| + if (!ReadDirectoryChangesW(DirectoryHandle, Notifications.data(), |
| + Notifications.size() * sizeof(DWORD), |
| + WatchSubtree, NotifyFilter, &BytesTransferred, |
| + &Overlapped, NULL)) { |
| + Q.emplace(DirectoryWatcher::Event::EventKind::WatcherGotInvalidated, |
| + ""); |
| + break; |
| + } |
| + |
| + HANDLE Handles[2] = { Terminate, Overlapped.hEvent }; |
| + switch (WaitForMultipleObjects(2, Handles, FALSE, INFINITE)) { |
| + case WAIT_OBJECT_0: // Terminate Request |
| + case WAIT_FAILED: // Failure |
| + Q.emplace(DirectoryWatcher::Event::EventKind::WatcherGotInvalidated, |
| + ""); |
| + (void)CloseHandle(DirectoryHandle); |
| + return; |
| + case WAIT_TIMEOUT: // Spurious wakeup? |
| + continue; |
| + case WAIT_OBJECT_0 + 1: // Directory change |
| + break; |
| + } |
| + |
| + if (!GetOverlappedResult(DirectoryHandle, &Overlapped, &BytesTransferred, |
| + FALSE)) { |
| + Q.emplace(DirectoryWatcher::Event::EventKind::WatchedDirRemoved, |
| + ""); |
| + Q.emplace(DirectoryWatcher::Event::EventKind::WatcherGotInvalidated, |
| + ""); |
| + break; |
| + } |
| + |
| + // There was a buffer underrun on the kernel side. We may have lost |
| + // events, please re-synchronize. |
| + if (BytesTransferred == 0) { |
| + Q.emplace(DirectoryWatcher::Event::EventKind::WatcherGotInvalidated, |
| + ""); |
| + break; |
| + } |
| + |
| + for (FILE_NOTIFY_INFORMATION *I = |
| + (FILE_NOTIFY_INFORMATION *)Notifications.data(); |
| + I; |
| + I = I->NextEntryOffset |
| + ? (FILE_NOTIFY_INFORMATION *)((CHAR *)I + I->NextEntryOffset) |
| + : NULL) { |
| + DirectoryWatcher::Event::EventKind Kind = |
| + DirectoryWatcher::Event::EventKind::WatcherGotInvalidated; |
| + switch (I->Action) { |
| + case FILE_ACTION_ADDED: |
| + case FILE_ACTION_MODIFIED: |
| + case FILE_ACTION_RENAMED_NEW_NAME: |
| + Kind = DirectoryWatcher::Event::EventKind::Modified; |
| + break; |
| + case FILE_ACTION_REMOVED: |
| + case FILE_ACTION_RENAMED_OLD_NAME: |
| + Kind = DirectoryWatcher::Event::EventKind::Removed; |
| + break; |
| + } |
| + |
| + SmallString<MAX_PATH> filename; |
| + sys::windows::UTF16ToUTF8(I->FileName, I->FileNameLength / sizeof(WCHAR), |
| + filename); |
| + Q.emplace(Kind, filename); |
| + } |
| + } |
| + |
| + (void)CloseHandle(DirectoryHandle); |
| +} |
| + |
| +void DirectoryWatcherWindows::NotifierThreadProc(bool WaitForInitialSync) { |
| + // If we did not wait for the initial sync, then we should perform the |
| + // scan when we enter the thread. |
| + if (!WaitForInitialSync) |
| + this->InitialScan(); |
| + |
| + while (true) { |
| + DirectoryWatcher::Event E = Q.pop_front(); |
| + Callback(E, /*IsInitial=*/false); |
| + if (E.Kind == DirectoryWatcher::Event::EventKind::WatcherGotInvalidated) |
| + break; |
| + } |
| +} |
| + |
| +auto error(DWORD ErrorCode) { |
| + DWORD Flags = FORMAT_MESSAGE_ALLOCATE_BUFFER |
| + | FORMAT_MESSAGE_FROM_SYSTEM |
| + | FORMAT_MESSAGE_IGNORE_INSERTS; |
| + |
| + LPSTR Buffer; |
| + if (!FormatMessageA(Flags, NULL, ErrorCode, |
| + MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), (LPSTR)&Buffer, |
| + 0, NULL)) { |
| + return make_error<llvm::StringError>("error " + utostr(ErrorCode), |
| + inconvertibleErrorCode()); |
| + } |
| + std::string Message{Buffer}; |
| + LocalFree(Buffer); |
| + return make_error<llvm::StringError>(Message, inconvertibleErrorCode()); |
| +} |
| + |
| } // namespace |
| |
| llvm::Expected<std::unique_ptr<DirectoryWatcher>> |
| -clang::DirectoryWatcher::create( |
| - StringRef Path, |
| - std::function<void(llvm::ArrayRef<DirectoryWatcher::Event>, bool)> Receiver, |
| - bool WaitForInitialSync) { |
| - return llvm::Expected<std::unique_ptr<DirectoryWatcher>>( |
| - llvm::errorCodeToError(std::make_error_code(std::errc::not_supported))); |
| +clang::DirectoryWatcher::create(StringRef Path, |
| + DirectoryWatcherCallback Receiver, |
| + bool WaitForInitialSync) { |
| + if (Path.empty()) |
| + llvm::report_fatal_error( |
| + "DirectoryWatcher::create can not accept an empty Path."); |
| + |
| + if (!sys::fs::is_directory(Path)) |
| + llvm::report_fatal_error( |
| + "DirectoryWatcher::create can not accept a filepath."); |
| + |
| + SmallVector<wchar_t, MAX_PATH> WidePath; |
| + if (sys::windows::UTF8ToUTF16(Path, WidePath)) |
| + return llvm::make_error<llvm::StringError>( |
| + "unable to convert path to UTF-16", llvm::inconvertibleErrorCode()); |
| + |
| + DWORD DesiredAccess = FILE_LIST_DIRECTORY; |
| + DWORD ShareMode = FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE; |
| + DWORD CreationDisposition = OPEN_EXISTING; |
| + DWORD FlagsAndAttributes = FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OVERLAPPED; |
| + |
| + HANDLE DirectoryHandle = |
| + CreateFileW(WidePath.data(), DesiredAccess, ShareMode, |
| + /*lpSecurityAttributes=*/NULL, CreationDisposition, |
| + FlagsAndAttributes, NULL); |
| + if (DirectoryHandle == INVALID_HANDLE_VALUE) |
| + return error(GetLastError()); |
| + |
| + // NOTE: We use the watcher instance as a RAII object to discard the handles |
| + // for the directory in case of an error. Hence, this is early allocated, |
| + // with the state being written directly to the watcher. |
| + return std::make_unique<DirectoryWatcherWindows>( |
| + DirectoryHandle, WaitForInitialSync, Receiver); |
| } |
| diff --git a/clang/unittests/DirectoryWatcher/CMakeLists.txt b/clang/unittests/DirectoryWatcher/CMakeLists.txt |
| index 0355525a86b0..84a1a9d40c25 100644 |
| --- a/clang/unittests/DirectoryWatcher/CMakeLists.txt |
| +++ b/clang/unittests/DirectoryWatcher/CMakeLists.txt |
| @@ -1,4 +1,4 @@ |
| -if(APPLE OR CMAKE_SYSTEM_NAME MATCHES "Linux") |
| +if(APPLE OR CMAKE_SYSTEM_NAME MATCHES "Linux" OR CMAKE_SYSTEM_NAME STREQUAL Windows) |
| |
| set(LLVM_LINK_COMPONENTS |
| Support |