[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

thakis added a comment. FWIW https://lab.llvm.org/buildbot/#/builders/123 has been red for several days after this landed too (eg https://lab.llvm.org/buildbot/#/builders/123/builds/4545) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88666/new/ h

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

stella.stamenova added a comment. In D88666#2828306 , @thakis wrote: > We also see check-all timeout recently (fairly consistently), see > https://bugs.chromium.org/p/chromium/issues/detail?id=1221702 > > Since Stella reported problems with this too, I sp

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

thakis added a comment. We also see check-all timeout recently (fairly consistently), see https://bugs.chromium.org/p/chromium/issues/detail?id=1221702 Since Stella reported problems with this too, I speculatively reverted it (and follow-ups) in fb32de9e97af0921242a021e30020ffacf7aa6e2

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

stella.stamenova added a comment. I wasn't able to reproduce this locally by running *just* the DirectoryWatcher tests. I'm not running all of the clang tests repeatedly to see if I can get a repro that way. The online tests appear to always hang either in the InitialScanAsync or InvalidatedWa

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

amccarth added a comment. In D88666#2825557 , @stella.stamenova wrote: > One thing we've run into in the past is that running some of these tests **on > a drive other than the OS drive** can cause weird failure. I am not sure if > that may be the case h

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

stella.stamenova added a comment. This is the best I can do from the online builds. I'll try and repro locally as well: FAIL: Clang-Unit :: DirectoryWatcher/./DirectoryWatcherTests.exe/DirectoryWatcherTest.InitialScanAsync (75980 of 75980) TEST 'Clang-Unit :: Director

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

stella.stamenova added a comment. In D88666#2825300 , @compnerd wrote: > Interesting, are the logs from the runs available? I have run the test > ~1 times locally and its been stable. Perhaps the logs can show what is > going on. Unfortunately, no

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

compnerd added a comment. Interesting, are the logs from the runs available? I have run the test ~1 times locally and its been stable. Perhaps the logs can show what is going on. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88666/new/ https

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

stella.stamenova added a comment. We still occasionally (every couple of runs) see these tests hang on Windows in both Debug and Release. Unfortunately, I don't have access to the machines running the tests to debug the tests while they are hanging and I haven't had a chance to try to reproduce

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

compnerd added a comment. I was able to play around with this further yesterday evening. You are correct - the issue is the load preventing the watcher thread from spinning up. I was able to reproduce this issue and resolve it by adding in a synchronization point (boolean + mutex + condition

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

amccarth added a comment. This patch was reverted a while back because a couple DirectoryWatcher tests routinely timed out on build bots even though they work when run in isolation. I believe the problem is that, on a machine busy doing a build, the startup of the watcher thread is delayed (eit

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

aeubanks added a comment. I've also been seeing some failures on phab reviews, e.g. https://reviews.llvm.org/D89188. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88666/new/ https://reviews.llvm.org/D88666

Re: [PATCH] D88666: DirectoryWatcher: add an implementation for Windows

If I had to guess, my money would be on a deadlock. To unblock, I'd propose reverting this patch until we can figure it out. During the review, a deadlock was fixed related to the watcher thread, but perhaps we missed one for the notifier thread. On Tue, Oct 13, 2020 at 8:40 AM Hans Wennborg via

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

hans added a comment. The longer timeout didn't help :( I'm not sure what's different about the machine where this is failing. Maybe it's some filesystem issue due to being a VM? Any ideas for good printfs or similar that could be added to figure out exactly what part is failing? Repository:

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

hans added a comment. We're seeing the tests for this fail in Chromium's Clang builds: https://bugs.chromium.org/p/chromium/issues/detail?id=1137737 I'll try increasing the test's timeout for now and see if that helps. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG5d74c4351175: DirectoryWatcher: add an implementation for Windows (authored by compnerd). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

compnerd updated this revision to Diff 297324. compnerd marked an inline comment as done. compnerd added a comment. Herald added a subscriber: jfb. Remove unnecessary include, fix an incorrect wait (verified that unit tests now pass!) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST AC

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

compnerd marked an inline comment as done. compnerd added inline comments. Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:15 +#include "llvm/Support/Windows/WindowsSupport.h" #include #include amccarth wrote: > I don't see a reaso

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. LGTM. Thanks for extending this functionality to Windows! Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:15 +#include "llvm/Support/Windows/

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

compnerd updated this revision to Diff 297079. compnerd marked an inline comment as done. compnerd added a comment. address feedback CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88666/new/ https://reviews.llvm.org/D88666 Files: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-win

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

compnerd marked 3 inline comments as done. compnerd added inline comments. Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:92 +Length = GetFinalPathNameByHandleW(DirectoryHandle, Buffer.data(), + Buffer.capacit

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

compnerd marked 2 inline comments as done. compnerd added inline comments. Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:82 +DirectoryWatcherCallback Receiver) +: Callback(Receiver) { + // Pre-compute the real location as we will be handing

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

compnerd added a comment. The reason that I added the overlapped event was specifically for the cancellation and overlooked the fact that it will be used by the kernel side as well, thanks for catching that! Comment at: clang/unittests/DirectoryWatcher/CMakeLists.txt:1 -if(AP

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

amccarth requested changes to this revision. amccarth added a comment. This revision now requires changes to proceed. Some of this is nitpicky/opinionated, but the race condition is real. We need a reliable way to signal the watcher thread when it's time to exit. Options I see are: 1. Use the

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

compnerd updated this revision to Diff 296092. compnerd added a comment. Herald added a subscriber: mgorny. Address feedback Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88666/new/ https://reviews.llvm.org/D88666 Files: clang/lib/DirectoryWatch

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

compnerd added inline comments. Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:87 +DWORD dwLength = GetFinalPathNameByHandleW(hDirectory, NULL, 0, 0); +std::unique_ptr buffer{new WCHAR[dwLength + 1]}; +(void)GetFinalPathNameByHandleW(hDire

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

compnerd marked 14 inline comments as done. compnerd added a comment. There already is testing coverage for this - I just missed the CMake changes. Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:36 + alignas(DWORD) + CHAR Buffer[4 * (sizeof(FILE_N

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

amccarth added a comment. I'm sorry. I haven't had to time to review the entire change yet, but I thought I'd share some early feedback now and take more of a look on Monday. The high level issues on my mind: I'm wondering whether this has been overcomplicated with the overlapped IO. If the

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

compnerd added a comment. @aaron.ballman - I completely agree with you about the testing. The interfaces are tested via https://github.com/llvm/llvm-project/blob/master/clang/unittests/DirectoryWatcher/CMakeLists.txt, which now that I look at again, seems to need an additional case for system

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

compnerd added inline comments. Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:87 +DWORD dwLength = GetFinalPathNameByHandleW(hDirectory, NULL, 0, 0); +std::unique_ptr buffer{new WCHAR[dwLength + 1]}; +(void)GetFinalPathNameByHandleW(hDire

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

compnerd added inline comments. Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:95 + CreateEvent(NULL, /*bManualReset=*/TRUE, /*bInitialState=*/FALSE, NULL); + assert(ovlIO.hEvent); + plotfi wrote: > Can this be an assert with so

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

aaron.ballman added a comment. I wonder if we should unit test this functionality by having some tests that create and remove files that are watched. I'm not 100% convinced that is a great idea, but not having test coverage for the change is also not really a great idea either. Thoughts welcome

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

plotfi added a comment. Overall looks good. Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:95 + CreateEvent(NULL, /*bManualReset=*/TRUE, /*bInitialState=*/FALSE, NULL); + assert(ovlIO.hEvent); + Can this be an assert with some

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

compnerd created this revision. compnerd added a reviewer: amccarth. Herald added a project: clang. compnerd requested review of this revision. This implements the directory watcher on Windows. It does the most naive thing for simplicity. ReadDirectoryChangesW is used to monitor the changes. Ho