[clang][DirectoryWatcher] Upstream DirectoryWatcher

[jkorous]

Summary

This patch contains implementation of DirectoryWatcher from github.com/apple/swift-clang

We are starting new push to upstream the index-while-building feature in clang and this is one of the dependencies.

Original author is David Farler, other contributors are Argyrios Kyrtzidis, Thomas Roughton and Alex Lorenz.

Part of this implementation was included in the review below so I am adding @tschuett and @yvvan in case they are interested.
https://reviews.llvm.org/D41407

[jkorous]

• fix the linux implementation
• clang-format

[jkorous]

Adding clangd folks in case they want to take a look.

[jkorous]

Ping.

[gribozavr]

Sorry for the delay. I will finish reviewing tomorrow.

clang/include/clang/DirectoryWatcher/DirectoryWatcher.h

9 ↗

(On Diff #188808)

This comment only repeats the doc comment for the class, I'd suggest to remove it here.

[jkorous]

Remove duplicate comment

[thakis]

Why is this needed for index-while-building? My mental model for index-while-building is that that streams out build index metadata as part of the regular compile. Why does that require watching directories?

[jkorous]

In D58418#1430160, @thakis wrote:

Why is this needed for index-while-building? My mental model for index-while-building is that that streams out build index metadata as part of the regular compile. Why does that require watching directories?

You're right that this isn't necessary for the indexing phase. But we also provide API so clients can consume the index. This functionality is used for getting notifications about index data changes.

You can see it for example here:
https://github.com/apple/swift-clang/blob/stable/lib/IndexDataStore/IndexDataStore.cpp#L111

[thakis]

In D58418#1430490, @jkorous wrote:

In D58418#1430160, @thakis wrote:

Why is this needed for index-while-building? My mental model for index-while-building is that that streams out build index metadata as part of the regular compile. Why does that require watching directories?

You're right that this isn't necessary for the indexing phase. But we also provide API so clients can consume the index. This functionality is used for getting notifications about index data changes.

You can see it for example here:
https://github.com/apple/swift-clang/blob/stable/lib/IndexDataStore/IndexDataStore.cpp#L111

Is that code going to live in clang? This seems more like a tool built on top of the compiler rather than something core to the compiler itself (like the actual index-while-building feature). Maybe this could be in clang-tools-extra?

[gribozavr]

clang/include/clang/DirectoryWatcher/DirectoryWatcher.h

27 ↗

(On Diff #188808)

I feel like since there's nothing Clang-specific about it, it should go into libSupport in LLVM, what do you think?

33 ↗

(On Diff #188808)

"A file being moved into ... and replacing ... "

43 ↗

(On Diff #188808)

"File content was modified." ? In other words, metadata was not modified?

46 ↗

(On Diff #188808)

DirectoryRemoved (for consistency with Removed)

Also, how about TopDirectoryRemoved? Otherwise it sounds like some random directory was removed.

51 ↗

(On Diff #188808)

Please document if it is going to be absolute or relative path, or just the file name.

55 ↗

(On Diff #188808)

Users are not going to use this typedef in their code, so I feel like it is obfuscating code -- could you expand it in places where it is used?

61 ↗

(On Diff #188808)

WaitForInitialSync (everywhere in the patch)

62 ↗

(On Diff #188808)

Why not return llvm::ErrorOr<std::unique_ptr<DirectoryWatcher>>?

I also don't understand why we need unique_ptr. If you change Implementation &Impl; to std::unique_ptr<Implementation> Impl;, DirectoryWatcher becomes a move-only type, and create can return llvm::ErrorOr<DirectoryWatcher>.

clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h

93 ↗

(On Diff #190744)

Please use AlignedCharArray from llvm/include/llvm/Support/AlignOf.h.

93 ↗

(On Diff #190744)

NAME_MAX is 255, add some for inotify_event, and multiply by 30... we get almost 8 Kb on the stack. Should we allocate on the heap instead?

137 ↗

(On Diff #190744)

Return llvm::Error?

162 ↗

(On Diff #190744)

Use a lambda instead of bind to improve readability? https://clang.llvm.org/extra/clang-tidy/checks/modernize-avoid-bind.html

175 ↗

(On Diff #190744)

Same comments as for macOS: we need to buffer events from inotify until the initial scan completes.

1 ↗

(On Diff #188808)

Please drop the .h from the name, for consistency with the rest of LLVM (there are no .inc.h files, only .inc).

clang/lib/DirectoryWatcher/DirectoryWatcher-mac.inc.h

47 ↗

(On Diff #190744)

This function does not handle the kFSEventStreamEventFlagMustScanSubDirs flag. I think it should, otherwise the client would miss silently events. WDYT?

https://developer.apple.com/library/archive/documentation/Darwin/Conceptual/FSEvents_ProgGuide/UsingtheFSEventsFramework/UsingtheFSEventsFramework.html

If an event in a directory occurs at about the same time as one or more events in a subdirectory of that directory, the events may be coalesced into a single event. In this case, you will receive an event with the kFSEventStreamEventFlagMustScanSubDirs flag set. When you receive such an event, you must recursively rescan the path listed in the event. The additional changes are not necessarily in an immediate child of the listed path.

Also from the docs:

A path might be "/" if ether of these flags is set for the event: kFSEventStreamEventFlagUserDropped, kFSEventStreamEventFlagKernelDropped.

91 ↗

(On Diff #190744)

I don't think it is guaranteed that any duplicate events will be received in the first call to this callback. This is another reason to buffer events during the initial scan.

99 ↗

(On Diff #190744)

Events.push_back(DirectoryWatcher::Event{K, path});

148 ↗

(On Diff #190744)

/*latency=*/0.0, and remove the latency variable.

155 ↗

(On Diff #190744)

Please check the return code from FSEventStreamStart.

1 ↗

(On Diff #188808)

Please drop the .h from the name, for consistency with the rest of LLVM (there are no .inc.h files, only .inc).

14 ↗

(On Diff #188808)

No semicolon after "}".

19 ↗

(On Diff #188808)

"Path", "Receiver" etc. (throughout the patch for all variables, please)

112 ↗

(On Diff #188808)

No need to encode the type name in variable names:

initialScanPtr => InitialScan

118 ↗

(On Diff #188808)

"cfPath" (no need to repeat that it is a string, it is in the type)

130 ↗

(On Diff #188808)

Please don't repeat code in comments.

135 ↗

(On Diff #188808)

Move this block into a separate function?

std::string Realpath(StringRef Path) { ... }

141 ↗

(On Diff #188808)

context.info = new EventStreamContextData(
      std::move(realPath), std::move(receiver), std::move(initialScanPtr));

and remove the ctxData variable.

168 ↗

(On Diff #188808)

Why not return llvm::Error?

179 ↗

(On Diff #188808)

"StrongPath"? "OwnedPath"? We don't care that it is copied, we care that the variable owns the value (AKA has a strong reference to it).

185 ↗

(On Diff #188808)

I think setupFSEventsSema can be eliminated if the call to setupFSEventStream() was moved above this dispatch_async. WDYT?

187 ↗

(On Diff #188808)

The fs event stream is already started, and it might have sent some events. And now we're sending the initial scan. So, for example, the client could see a file remove event (an fs event) before they see the file add event (from the initial scan).

I think this race can be fixed by buffering the fs event stream until the initial scan is complete. What do you think?

clang/lib/DirectoryWatcher/DirectoryWatcher.cpp

9 ↗

(On Diff #188808)

Please don't duplicate doc comments between headers and implementation files -- they will only become stale and confusing.

31 ↗

(On Diff #188808)

Please don't repeat code in comments.

32 ↗

(On Diff #188808)

This specialization should go into llvm/include/llvm/Support/FileSystem.h, otherwise we are going to run into ODR violations if another file in LLVM defines the same specialization.

59 ↗

(On Diff #188808)

"Note: this class is not thread-safe." should be enough. However, all classes are not thread-safe by default, do we really need to highlight this fact?

60 ↗

(On Diff #188808)

InitialDirectoryScanner for a more clear name, and then you don't need to explain in the comment what this struct is used for.

61 ↗

(On Diff #188808)

FoundFileIDs

62 ↗

(On Diff #188808)

Tuple is not readable. One has to read the implementation to understand what is stored in it. Please use a struct and name the fields.

62 ↗

(On Diff #188808)

FoundFiles

68 ↗

(On Diff #188808)

Since this enumeration is not recursive, I would assume that DirectoryWatcher is not recursive either? That would be very important to mention in its doc comments -- my default expectation is that such notifications should be recursive. Or even in the name, NonrecursiveDirectoryWatcher.

83 ↗

(On Diff #188808)

I find it weird to say that a file that was already existing was added to the directory (according to the doc comment on EventKind::Added).

99 ↗

(On Diff #188808)

The usual way to do this is not through CMake, but by using the predefined preprocessor macros -- __APPLE__, __linux__ etc.

#if defined(__APPLE__)
#include "DirectoryWatcher-mac.inc"
#endif

#if defined(__linux__)
#include "DirectoryWatcher-linux.inc"
#endif

See llvm/lib/Support/Unix/Path.inc for example.

I don't see an advantage of doing it through CMake. CMakeLists.txt does exactly the same OS check, and then checks if the include files exist. However, there's no real implementation selection happening. It is not like we would ever see CoreServices missing on macOS. And even if it is missing, there's no other implementation suitable for macOS. Same for Linux.

111 ↗

(On Diff #188808)

unique_ptr for Implementation should work, as long as you keep the constructor and the destructor of DirectoryWatcher defined out of line in the .cpp file.

141 ↗

(On Diff #188808)

Use llvm::make_unique.

clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

43 ↗

(On Diff #190744)

Are these parallel arrays? I think it would be nicer if we had an array of structs instead.

45 ↗

(On Diff #190744)

"stats" is only used in the assertion, intentional?

111 ↗

(On Diff #190744)

Why not constructor?

262 ↗

(On Diff #190744)

Accidental return?

303 ↗

(On Diff #190744)

Google Test supports custom assertions. For example, see PrintedStmtMatches in clang/unittests/AST/StmtPrinterTest.cpp.

[jkorous]

In D58418#1430630, @thakis wrote:

In D58418#1430490, @jkorous wrote:

In D58418#1430160, @thakis wrote:

Why is this needed for index-while-building? My mental model for index-while-building is that that streams out build index metadata as part of the regular compile. Why does that require watching directories?

You're right that this isn't necessary for the indexing phase. But we also provide API so clients can consume the index. This functionality is used for getting notifications about index data changes.

You can see it for example here:
https://github.com/apple/swift-clang/blob/stable/lib/IndexDataStore/IndexDataStore.cpp#L111

Is that code going to live in clang? This seems more like a tool built on top of the compiler rather than something core to the compiler itself (like the actual index-while-building feature). Maybe this could be in clang-tools-extra?

It actually is part of the feature as the serialized format of the index isn't meant as a stable interface, that's what the API is for. DirectoryWatcher isn't a tool, it's just part of implementation of the IndexStore API.

[thakis]

In D58418#1431399, @jkorous wrote:

In D58418#1430630, @thakis wrote:

In D58418#1430490, @jkorous wrote:

In D58418#1430160, @thakis wrote:

Why is this needed for index-while-building? My mental model for index-while-building is that that streams out build index metadata as part of the regular compile. Why does that require watching directories?

You're right that this isn't necessary for the indexing phase. But we also provide API so clients can consume the index. This functionality is used for getting notifications about index data changes.

You can see it for example here:
https://github.com/apple/swift-clang/blob/stable/lib/IndexDataStore/IndexDataStore.cpp#L111

Is that code going to live in clang? This seems more like a tool built on top of the compiler rather than something core to the compiler itself (like the actual index-while-building feature). Maybe this could be in clang-tools-extra?

It actually is part of the feature as the serialized format of the index isn't meant as a stable interface, that's what the API is for. DirectoryWatcher isn't a tool, it's just part of implementation of the IndexStore API.

Maybe I misunderstand what the client of the IndexStore API is. That's not code that will be in the clang binary, right?

[jkorous]

In D58418#1431765, @thakis wrote:

In D58418#1431399, @jkorous wrote:

In D58418#1430630, @thakis wrote:

In D58418#1430490, @jkorous wrote:

In D58418#1430160, @thakis wrote:

Why is this needed for index-while-building? My mental model for index-while-building is that that streams out build index metadata as part of the regular compile. Why does that require watching directories?

You're right that this isn't necessary for the indexing phase. But we also provide API so clients can consume the index. This functionality is used for getting notifications about index data changes.

You can see it for example here:
https://github.com/apple/swift-clang/blob/stable/lib/IndexDataStore/IndexDataStore.cpp#L111

Is that code going to live in clang? This seems more like a tool built on top of the compiler rather than something core to the compiler itself (like the actual index-while-building feature). Maybe this could be in clang-tools-extra?

It actually is part of the feature as the serialized format of the index isn't meant as a stable interface, that's what the API is for. DirectoryWatcher isn't a tool, it's just part of implementation of the IndexStore API.

Maybe I misunderstand what the client of the IndexStore API is. That's not code that will be in the clang binary, right?

No, it won't.

Currently the client using this API is our indexing service.
https://github.com/apple/sourcekit-lsp
In the future clangd might become another client.

In theory we could have the index producing part in clang and the index consuming part (IndexStore) somewhere else (clang-tools-extra?) and use functionality that also lives somewhere else (llvm?) but reasons I'd rather not do it *NOW* are:

1. We'd have to expose the interface between them (the binary file format) which has been just an implementation detail so far without any intention to keep it stable.
2. From the general perspective - although I am upstreaming a fully developed feature (roughly 10kLOC) it is apparent that I am going to rewrite a significant part of the code based on the feedback from reviews. This patch is #2 out of approximately 10-15 patches total. Since it's probable that the design will change in upcoming reviews I'd prefer to discuss this kind of questions after a significant part of the whole design has been through the review.
3. For any code that we would move up the tree (to llvm repo) I'd like to have a clear use-case other than index-while-building first. Designing generic APIs is hard/impossible without known specific use-cases (I think the recommended minimum is 3).

The most important word is "now". I am totally happy to discuss this and move parts somewhere else if it seems reasonable in the future.

Does that make sense?

clang/include/clang/DirectoryWatcher/DirectoryWatcher.h

27 ↗

(On Diff #188808)

This has been brought up before. I prefer to leave it here for now since it's not used anywhere else. I'd only move it to llvm/Support once we have another use-case as that would mean specific requirements for the interface.

[jkorous]

A major clean-up.

changelog

general

• specification, documentation, tests
• returning only filenames instead of paths (or empty string when the event is related to the watched dir itself)
• removed unsound event deduplication during/right after the initial scan
• simplified how is OS-specific implementation selected

linux

• properly terminating threads
• added pthreads to fix shared libs build
• handle IN_DELETE_SELF, IN_IGNORED
• IN_EXCL_UNLINK

macos

• simplified synchronization by removing semaphores
• workarounds in FSEvents use

I am not entirely happy about my tests - since we're handling notifications from kernel asynchronously it's hard to write a deterministic test. I just use some 100 ms sleeps as a workaround. Happy to use something more robust if anyone has some ideas. Tests seem fine with msan & tsan on linux and tsan on macos but given their scope it doesn't mean that much.

[jkorous]

fix link libraries in cmake

[gribozavr]

clang/include/clang/DirectoryWatcher/DirectoryWatcher.h

78 ↗

(On Diff #200123)

"its"

78 ↗

(On Diff #200123)

I'd say "unspecified". "Undefined behavior" has a specific meaning in C++, and I don't believe we have that.

Everywhere in the patch.

79 ↗

(On Diff #200123)

Please add blank lines between paragraphs (everywhere in the patch).

95 ↗

(On Diff #200123)

Don't repeat field names in comments.

95 ↗

(On Diff #200123)

"a relative path" -- relative to what?

95 ↗

(On Diff #200123)

Is it really a path to the directory?

103 ↗

(On Diff #200123)

"IsInitial"

103 ↗

(On Diff #200123)

Users are not going to use this typedef in their code, so I feel like it is obfuscating code -- could you expand it in places where it is used?

123 ↗

(On Diff #200123)

Make it a static function in the DirectoryWatcher?

clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp

86 ↗

(On Diff #200123)

Don't write what something is used for (it can change), write what something is.

"If true, all async actions are requested to stop."

106 ↗

(On Diff #200123)

I think this is too much code in the initializer list. Could you move the body of the lambda into a method, and call it from this lambda?

110 ↗

(On Diff #200123)

NAME_MAX is 255, add some for inotify_event, and multiply by 30... we get almost 8 Kb on the stack. Should we allocate on the heap instead?

112 ↗

(On Diff #200123)

Please use "//" comments.

118 ↗

(On Diff #200123)

Please use AlignedCharArray from llvm/include/llvm/Support/AlignOf.h

136 ↗

(On Diff #200123)

What is the role of the timeout and why does it need to be so small?

210 ↗

(On Diff #200123)

I think this is too much code in the initializer list. Could you move the body of the lambda into a method, and call it from this lambda?

225 ↗

(On Diff #200123)

No need for the semicolon.

271 ↗

(On Diff #200123)

Use llvm::make_unique (and let unique_ptr do an implicit conversion to base).

clang/unittests/DirectoryWatcher/CMakeLists.txt

16 ↗

(On Diff #200123)

No need to check. macOS will always have this file. If it is not there, it is a big issue anyway.

20 ↗

(On Diff #200123)

... LINK_FLAGS "-framework CoreServices"

No need for an intermediate variable.

clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

73 ↗

(On Diff #200123)

"ContainsInitialEvents"

80 ↗

(On Diff #200123)

"ContainsNonInitialEvents"

131 ↗

(On Diff #200123)

I'm certain this sleep will be flaky on heavily-loaded CI machines. If you are going to leave it as a sleep, please make it 1s. But is there really no better way?

[gribozavr]

clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

42 ↗

(On Diff #200123)

ContainsEvents

357 ↗

(On Diff #200123)

I would strongly prefer if you used the gmock matchers (like Contains); as written, when the test fails, the only error we would get would be like "expected: true, actual: false".

[jkorous]

Thanks for taking a look @gribozavr!

I briefly scanned the rest of your comments and I agree with you (or don't really have a strong opinion) in all cases. I'll work on that today.

clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp

136 ↗

(On Diff #200123)

The whole idea is that we can't block on read() if we ever want to stop watching the directory, release resources (file descriptors, threads) and correctly destruct the DirectoryWatcher instance either

• because of a bug in some other thread in the implementation
• or asynchronous client action (e. g. destructor being called) in main application thread

The timeout adds latency in those scenarios.

clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

131 ↗

(On Diff #200123)

That was exactly my thinking! Honestly, I don't know - I wasn't able to come up with any reasonably simple, deterministic approach even on a single platform :(
I eventually picked 0.1s as a tradeoff between slowing the test for everyone and getting less false positives.

The problem as I understand it is that we're making changes and monitoring them asynchronously with no guarantee from the kernel API (true for FSEvents, not 100% about inotify) about when (if) we receive notifications.

If you have any idea for robust testing approach I'd be totally happy to use it.

[jkorous]

I addressed most of the comments.

What is left:

• rewrite tests with gmock matchers
• write test for metadata of a file in watched dir being changed

clang/include/clang/DirectoryWatcher/DirectoryWatcher.h

95 ↗

(On Diff #200123)

I reworded the comment.

123 ↗

(On Diff #200123)

You're right - seems like static factory methods are the LLVM way.

46 ↗

(On Diff #188808)

I used WatchedDirRemoved.

62 ↗

(On Diff #188808)

I personally didn't like the C-like std::string& error in the interface. But I feel like having a polymorphic class (which doesn't really fit into ErrorOr) is lesser evil than the original pimpl design.
WDYT?

clang/unittests/DirectoryWatcher/CMakeLists.txt

16 ↗

(On Diff #200123)

Actually this is wrong but for a different reason - these are link dependencies of the implementation. Tests (or any other client) shouldn't care about it.

[jkorous]

Addressed comments.
Changed semantics of one of std::atomic<bool> in linux implementation.

[gribozavr]

clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp

111 ↗

(On Diff #200389)

"alignof()" expression is standard C++ since C++11. (No need for underscores.)

114 ↗

(On Diff #200389)

use 'auto' to store the return value of make_unique?

136 ↗

(On Diff #200123)

Waking up 1000 times a second is not great -- it will lead to battery drain on laptops etc.

Please see https://stackoverflow.com/questions/8593004/waiting-on-a-condition-pthread-cond-wait-and-a-socket-change-select-simultan for non-busy-wait options.

[kadircet]

clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp

48 ↗

(On Diff #200389)

nit: get rid of the else

156 ↗

(On Diff #200389)

naming, it should be capital P

211 ↗

(On Diff #200389)

while (!Stop) ?

211 ↗

(On Diff #200389)

why the loop always tries to empty the queue? what would be broken if we simply stopped if Stop was set?

Also if that is important current implementation doesn't seem to be achiving that, since some events can be pushed to the queue after while loop exits.

223 ↗

(On Diff #200389)

maybe add a condition variable for queue state and waint on it?

244 ↗

(On Diff #200389)

why not use two condition variables for stop and finishedinitscan?

[jkorous]

Thanks for taking a look Kadir!
After yesterday's discussion with Dmitri I removed all those busy waits. Seems like the code is not much more complex now. I am going to update the diff and off to fixing the tests.

clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp

211 ↗

(On Diff #200389)

I changed the design - stopping condition for this loop is now finding WatcherGotInvalidated in the queue itself.

[jkorous]

Remove busy waits.

[jkorous]

Reimplemented tests with std::futures which allowed to use more generous timeout while not slowing down the happy paths.

[jkorous]

clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

131 ↗

(On Diff #200123)

I found a way how to use more generous timeout without slowing down the test for every run - inspired by Argyrios' idea about using different thread + semaphore.

I am using 3 seconds for now. If that's not enough, just let me know.

357 ↗

(On Diff #200123)

Since the tests are using are based on something like "eventual correctness" instead of one-time check I didn't use gmock matchers but implemented some custom diagnostics.

Example of the failed test:

/Users/jankorous/src/llvm-monorepo/llvm-project/clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:185: Failure
Value of: *TestConsumer.Result()
  Actual: false
Expected: true
Expected but not seen non-initial events:
Removed a
Unexpected non-initial events seen:
Added a

[jkorous]

Specify what "file modified" means and add a test for metadata change

[jkorous]

• simplify link libraries in cmake
• fix Release build (messed-up asserts)

[jkorous]

Remove DirectoryWatcher::Event::EventKind::Added

[jkorous]

One more thing.

On macOS FSEvents are coalesced more or less at will and it became quite apparent when I was creating automatic tests - I was for example receiving coalesced events Added & Modified & Removed. We had a discussion about how to deal with this and it turned out that the existing client doesn't actually care whether a file was created or whether it was modified. I believe that by removing the EventKind::Added we still keep the interface sane while removing the ambiguity.

[gribozavr]

Very nice testing approach!

clang/include/clang/DirectoryWatcher/DirectoryWatcher.h

20 ↗

(On Diff #201744)

Looks like triple slashes on empty lines got removed, splitting the doc comment.

clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp

40 ↗

(On Diff #201744)

Three slashes for doc comments?

40 ↗

(On Diff #201744)

Don't write what something is used for currently, such comments go out of date quickly. (Just delete the last sentence.)

41 ↗

(On Diff #201744)

Semaphor*e*

42 ↗

(On Diff #201744)

"Expects"

Three slashes for doc comments.

49 ↗

(On Diff #201744)

Extra semicolon.

52 ↗

(On Diff #201744)

Since it closes the file descriptors in the destructor, I feel like it should also be responsible for calling pipe in the constructor.

58 ↗

(On Diff #201744)

Three slashes for doc comments.

120 ↗

(On Diff #201744)

"consumes", "pushes"

clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

109 ↗

(On Diff #201744)

Please add a period at the end of the comment (everywhere in the patch).

153 ↗

(On Diff #201744)

Please name functions consistently -- there's both consume() that starts with lowercase, and Result() that starts with uppercase.

Please refer to the current naming rules in the style guide and apply everywhere in the patch.

210 ↗

(On Diff #201744)

"If the following assertions fail, it is a sign that ..."

Also you can stream the message into the EXPECT_TRUE, it will be printed if the assertion fails.

EXPECT_TRUE(...) << "whatever";

224 ↗

(On Diff #201744)

Test names start with an uppercase letter (InitialScanSync). Please apply everywhere in the patch.

243 ↗

(On Diff #201744)

Add /*waitForInitialSync=*/ ? (everywhere in the patch)

304 ↗

(On Diff #201744)

Delete the comma and wrap onto one line.

419 ↗

(On Diff #201744)

80 columns.

[jkorous]

clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp

40 ↗

(On Diff #201744)

Sure, no problem.

I naturally tend to err on the side of over-commenting as I think I'd appreciate it myself if I had to understand the code without prior knowledge - not saying it's intentional or that I have a strong reason to do that though. You seem to have a strong preference for not having out-of-date comments with low-ish information value. Just out of curiosity - is it based on any particular reason or experience?

52 ↗

(On Diff #201744)

I know what you mean - my "oop feel" was telling me it's wrong too. It's not just the pipe in this class but also the inotify descriptor in the watcher class.

The problem is that the only reasonable way how to communicate failures from constructors that I am aware of are exceptions which we don't use in llvm. That's why I moved most of the stuff that can fail even before any work is actually started to the factory method (with the exception of epoll file descriptor as that felt like making its scope unnecessarily larger).

Thinking about it now, I am starting to doubt that it makes life any easier for client code as it still has to cope with failure communicated as WatcherGotInvalidated event via receiver.

What do you think?

clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

153 ↗

(On Diff #201744)

Sorry about that. This is definitely my weak point.

[jkorous]

Addressed comments.

[gribozavr]

clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp

40 ↗

(On Diff #201744)

is it based on any particular reason or experience?

Yes, primarily working on the Swift compiler and the standard library, when everything was changing very quickly. My current work also confirms the same -- a lot of time when I see a comment about the "current usage" or other incidental information that is not the contract of the API, it tends to be outdated. Approximately nobody will change such a comment when another user is added ("I'm just reusing the code..."), even when the quoted user already became non-representative of the usage pattern, or the usage pattern has changed. However, when changing the API contract people typically do change the comment.

It also makes sense to me in abstract: reading that X happens to be used for Y does not necessarily help understand X better -- it is only a cross-reference that I could find myself with an IDE command; I still need to understand the design of Y and the interaction with X, and then using my past experience infer what X was intended to be.

Saying that X is intended to be used only by Y is a different story of course, that's design documentation.

Providing an example of usage is also fine, but it should be phrased as an example that can't become stale.

52 ↗

(On Diff #201744)

You could add a factory function to SemaphorePipe, but... I feel like trying to recover from a failure in the pipe() call is a bit like trying to recover from a memory allocation failure. The process is probably hitting a file descriptor limit or something like that, and is likely going to fail anyway. I'd probably trigger a fatal error if pipe() fails.

This class is a lot like pthread_mutex_init -- it can fail "gracefully", but there's no way for the caller to recover -- the caller needs a mutex to proceed.

[gribozavr]

clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp

317 ↗

(On Diff #202467)

Use pipe2() with O_CLOEXEC, to avoid leaking the file descriptors to child processes?

[jkorous]

I fixed the rest.

There are still some questions you raised that I just responded to and kept them as not Done. Feel free to take a look. If nothing comes up I'll commit this on Wednesday.

clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp

40 ↗

(On Diff #201744)

Thank you.

52 ↗

(On Diff #201744)

I see what you mean and mostly agree. Anyways, let's allow clients to handle such funny moments themselves as much as they can.

I'll factor out the pipe call to the factory method.

317 ↗

(On Diff #202467)

You're right, I didn't account for this. Added the flag also to inotify_init1() and epoll_create1() calls.

[jkorous]

linux implementation

• factory method for SemaphorePipe
• *_CLOEXEC flags

[rnk]

cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

420

This fails to compile on Windows because file_t is not int there:
C:\b\slave\clang-x64-windows-msvc\build\llvm.src\tools\clang\unittests\DirectoryWatcher\DirectoryWatcherTest.cpp(415): error C2440: 'initializing': cannot convert from 'void *' to 'int'
C:\b\slave\clang-x64-windows-msvc\build\llvm.src\tools\clang\unittests\DirectoryWatcher\DirectoryWatcherTest.cpp(415): note: There is no context in which this conversion is possible

I have been working on migrating some code over to native file handles to make this type of error less likely in the future, but it is not done yet.

[rnk]

Reverted in rC365581.

[jkorous]

Thanks for the revert.

There's actually one more problem - seems like ninja doesn't like the generated build.ninja file on Linux.

ninja: error: build.ninja:52390: bad $-escape (literal $ must be written as $$)

http://lab.llvm.org:8011/builders/clang-with-lto-ubuntu/builds/13617/steps/build-stage1-compiler/logs/stdio

[smeenai]

In D58418#1577349, @jkorous wrote:

Thanks for the revert.

There's actually one more problem - seems like ninja doesn't like the generated build.ninja file on Linux.

ninja: error: build.ninja:52390: bad $-escape (literal $ must be written as $$)

http://lab.llvm.org:8011/builders/clang-with-lto-ubuntu/builds/13617/steps/build-stage1-compiler/logs/stdio

We ran into this as well, and I was curious about what was going on, so I dug into it.

When you add a private dependency to static library target, as you're doing in this change with ${DIRECTORY_WATCHER_LINK_LIBS}, CMake adds the dependency to the target's INTERFACE_LINK_LIBRARIES property using the $<LINK_ONLY:...> generator expression. The code for clang-shlib iterates through all the clang libraries and uses generator expressions to gather their INTERFACE_LINK_LIBRARIES, but if those generator expressions themselves evaluate to generator expressions ($<LINK_ONLY:...> in this case), the second level of generator expressions won't get evaluated and will just end up in the ninja file directly, hence the complaint about the dollar. The clang-shlib code in question is https://github.com/llvm/llvm-project/blob/3837f4273fcc40cc519035479aefe78e5cbd3055/clang/tools/clang-shlib/CMakeLists.txt#L10.

Here's a simple repro (where empty.c is literally an empty file). Running cmake -G Ninja on this and then running ninja should demonstrate the issue.

cmake_minimum_required(VERSION 3.4.3)
project(dollartest C)

add_library(a STATIC empty.c)
add_library(b_obj OBJECT empty.c)
add_library(b STATIC empty.c)
target_link_libraries(b PRIVATE a)

add_library(c SHARED empty.c)
target_link_libraries(c PRIVATE
  b_object
  $<TARGET_PROPERTY:b,INTERFACE_LINK_LIBRARIES>
  $<TARGET_PROPERTY:b,LINK_LIBRARIES>
  )

@beanz, thoughts on how best to handle this?

[plotfi]

@jkorous DirectoryWatcherTests causes ninja check-clang to hang on my Ubuntu 18.04 computer. check-clang will not finish and I am forced to killall -9 DirectoryWatcherTests. My system has 40 threads and this repros on ext4 and btrfs.

[jkorous]

In D58418#1609138, @plotfi wrote:

@jkorous DirectoryWatcherTests causes ninja check-clang to hang on my Ubuntu 18.04 computer. check-clang will not finish and I am forced to killall -9 DirectoryWatcherTests. My system has 40 threads and this repros on ext4 and btrfs.

Hi @plotfi,

Could you please add more details?

The tests seem fine on CentOS 6.9 with Linux 4.19.34-77 on ext4 and also Ubuntu build bots (Ubuntu 18.04.1 LTS) seem fine.
http://lab.llvm.org:8011/buildslaves/ps4-buildslave1a

Unfortunately I don't have any Ubuntu system at hand.

Thanks.

Jan

[jkorous]

Hi Puyan,

I failed to reproduce with llvm.org/master (5faa533e47b0e54b04166b0257c5ebb48e6ffcaa) on Ubuntu 18.04.1 LTS both in debug and release build.

Since it sounds like you can reproduce "reliably" - can you please share more info how to reproduce?

cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h

+//===- DirectoryWatcher.h - Listens for directory file changes --*- 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_DIRECTORYWATCHER_DIRECTORYWATCHER_H
+#define LLVM_CLANG_DIRECTORYWATCHER_DIRECTORYWATCHER_H
+
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/StringRef.h"
+#include <functional>
+#include <memory>
+#include <string>
+
+namespace clang {
+/// Provides notifications for file changes in a directory.
+///
+/// Invokes client-provided function on every filesystem event in the watched
+/// directory. Initially the the watched directory is scanned and for every file
+/// found, an event is synthesized as if the file was added.
+///
+/// This is not a general purpose directory monitoring tool - list of
+/// limitations follows.
+///
+/// Only flat directories with no subdirectories are supported. In case
+/// subdirectories are present the behavior is unspecified - events *might* be
+/// passed to Receiver on macOS (due to FSEvents being used) while they
+/// *probably* won't be passed on Linux (due to inotify being used).
+///
+/// Known potential inconsistencies
+/// - For files that are deleted befor the initial scan processed them, clients
+/// might receive Removed notification without any prior Added notification.
+/// - Multiple notifications might be produced when a file is added to the
+/// watched directory during the initial scan. We are choosing the lesser evil
+/// here as the only known alternative strategy would be to invalidate the
+/// watcher instance and force user to create a new one whenever filesystem
+/// event occurs during the initial scan but that would introduce continuous
+/// restarting failure mode (watched directory is not always "owned" by the same
+/// process that is consuming it). Since existing clients can handle duplicate
+/// events well, we decided for simplicity.
+///
+/// Notifications are provided only for changes done through local user-space
+/// filesystem interface. Specifically, it's unspecified if notification would
+/// be provided in case of a:
+/// - a file mmap-ed and changed
+/// - a file changed via remote (NFS) or virtual (/proc) FS access to monitored
+/// directory
+/// - another filesystem mounted to the watched directory
+///
+/// No support for LLVM VFS.
+///
+/// It is unspecified whether notifications for files being deleted are sent in
+/// case the whole watched directory is sent.
+///
+/// Directories containing "too many" files and/or receiving events "too
+/// frequently" are not supported - if the initial scan can't be finished before
+/// the watcher instance gets invalidated (see WatcherGotInvalidated) there's no
+/// good error handling strategy - the only option for client is to destroy the
+/// watcher, restart watching with new instance and hope it won't repeat.
+class DirectoryWatcher {
+public:
+  struct Event {
+    enum class EventKind {
+      Removed,
+      /// Content of a file was modified.
+      Modified,
+      /// The watched directory got deleted.
+      WatchedDirRemoved,
+      /// The DirectoryWatcher that originated this event is no longer valid and
+      /// its behavior is unspecified.
+      ///
+      /// The prime case is kernel signalling to OS-specific implementation of
+      /// DirectoryWatcher some resource limit being hit.
+      /// *Usually* kernel starts dropping or squashing events together after
+      /// that and so would DirectoryWatcher. This means that *some* events
+      /// might still be passed to Receiver but this behavior is unspecified.
+      ///
+      /// Another case is after the watched directory itself is deleted.
+      /// WatcherGotInvalidated will be received at least once during
+      /// DirectoryWatcher instance lifetime - when handling errors this is done
+      /// on best effort basis, when an instance is being destroyed then this is
+      /// guaranteed.
+      ///
+      /// The only proper response to this kind of event is to destruct the
+      /// originating DirectoryWatcher instance and create a new one.
+      WatcherGotInvalidated
+    };
+
+    EventKind Kind;
+    /// Filename that this event is related to or an empty string in
+    /// case this event is related to the watched directory itself.
+    std::string Filename;
+
+    Event(EventKind Kind, llvm::StringRef Filename)
+        : Kind(Kind), Filename(Filename) {}
+  };
+
+  /// Returns nullptr if \param Path doesn't exist.
+  /// Returns nullptr if \param Path isn't a directory.
+  /// Returns nullptr if OS kernel API told us we can't start watching. In such
+  /// case it's unclear whether just retrying has any chance to succeeed.
+  static std::unique_ptr<DirectoryWatcher>
+  create(llvm::StringRef Path,
+         std::function<void(llvm::ArrayRef<DirectoryWatcher::Event> Events,
+                            bool IsInitial)>
+             Receiver,
+         bool WaitForInitialSync);
+
+  virtual ~DirectoryWatcher() = default;
+  DirectoryWatcher(const DirectoryWatcher &) = delete;
+  DirectoryWatcher &operator=(const DirectoryWatcher &) = delete;
+  DirectoryWatcher(DirectoryWatcher &&) = default;
+
+protected:
+  DirectoryWatcher() = default;
+};
+
+} // namespace clang
+
+#endif // LLVM_CLANG_DIRECTORYWATCHER_DIRECTORYWATCHER_H

cfe/trunk/lib/CMakeLists.txt

 if(CLANG_ENABLE_ARCMT)
   add_subdirectory(ARCMigrate)
 endif()
 add_subdirectory(Driver)
 add_subdirectory(Serialization)
 add_subdirectory(Frontend)
 add_subdirectory(FrontendTool)
 add_subdirectory(Tooling)
+add_subdirectory(DirectoryWatcher)
 add_subdirectory(Index)
 if(CLANG_ENABLE_STATIC_ANALYZER)
   add_subdirectory(StaticAnalyzer)
 endif()
 add_subdirectory(Format)

cfe/trunk/lib/DirectoryWatcher/CMakeLists.txt

+include(CheckIncludeFiles)
+
+set(LLVM_LINK_COMPONENTS support)
+
+set(DIRECTORY_WATCHER_SOURCES DirectoryScanner.cpp)
+set(DIRECTORY_WATCHER_LINK_LIBS "")
+
+if(APPLE)
+  check_include_files("CoreServices/CoreServices.h" HAVE_CORESERVICES)
+  if(HAVE_CORESERVICES)
+    list(APPEND DIRECTORY_WATCHER_SOURCES mac/DirectoryWatcher-mac.cpp)
+    set(DIRECTORY_WATCHER_LINK_LIBS "-framework CoreServices")
+  endif()
+elseif(CMAKE_SYSTEM_NAME MATCHES "Linux")
+  check_include_files("sys/inotify.h" HAVE_INOTIFY)
+  if(HAVE_INOTIFY)
+    list(APPEND DIRECTORY_WATCHER_SOURCES linux/DirectoryWatcher-linux.cpp)
+    find_package(Threads REQUIRED)
+    set(DIRECTORY_WATCHER_LINK_LIBS ${CMAKE_THREAD_LIBS_INIT})
+  endif()
+endif()
+
+add_clang_library(clangDirectoryWatcher
+  ${DIRECTORY_WATCHER_SOURCES}
+  )
+
+target_link_libraries(clangDirectoryWatcher PRIVATE ${DIRECTORY_WATCHER_LINK_LIBS})

cfe/trunk/lib/DirectoryWatcher/DirectoryScanner.h

+//===- DirectoryScanner.h - Utility functions for DirectoryWatcher --------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/DirectoryWatcher/DirectoryWatcher.h"
+#include "llvm/Support/FileSystem.h"
+#include <string>
+#include <vector>
+
+namespace clang {
+
+/// Gets names (filenames) of items in directory at \p Path.
+/// \returns empty vector if \p Path is not a directory, doesn't exist or can't
+/// be read from.
+std::vector<std::string> scanDirectory(llvm::StringRef Path);
+
+/// Create event with EventKind::Added for every element in \p Scan.
+std::vector<DirectoryWatcher::Event>
+getAsFileEvents(const std::vector<std::string> &Scan);
+
+/// Gets status of file (or directory) at \p Path.
+/// \returns llvm::None if \p Path doesn't exist or can't get the status.
+llvm::Optional<llvm::sys::fs::file_status> getFileStatus(llvm::StringRef Path);
+
+} // namespace clang
 No newline at end of file

cfe/trunk/lib/DirectoryWatcher/DirectoryScanner.cpp

+//===- DirectoryScanner.cpp - Utility functions for DirectoryWatcher ------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "DirectoryScanner.h"
+
+#include "llvm/Support/Path.h"
+
+namespace clang {
+
+using namespace llvm;
+
+Optional<sys::fs::file_status> getFileStatus(StringRef Path) {
+  sys::fs::file_status Status;
+  std::error_code EC = status(Path, Status);
+  if (EC)
+    return None;
+  return Status;
+}
+
+std::vector<std::string> scanDirectory(StringRef Path) {
+  using namespace llvm::sys;
+  std::vector<std::string> Result;
+
+  std::error_code EC;
+  for (auto It = fs::directory_iterator(Path, EC),
+            End = fs::directory_iterator();
+       !EC && It != End; It.increment(EC)) {
+    auto status = getFileStatus(It->path());
+    if (!status.hasValue())
+      continue;
+    Result.emplace_back(sys::path::filename(It->path()));
+  }
+
+  return Result;
+}
+
+std::vector<DirectoryWatcher::Event>
+getAsFileEvents(const std::vector<std::string> &Scan) {
+  std::vector<DirectoryWatcher::Event> Events;
+  Events.reserve(Scan.size());
+
+  for (const auto &File : Scan) {
+    Events.emplace_back(DirectoryWatcher::Event{
+        DirectoryWatcher::Event::EventKind::Modified, File});
+  }
+  return Events;
+}
+
+} // namespace clang
 No newline at end of file

cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp

+//===- DirectoryWatcher-linux.cpp - Linux-platform directory watching -----===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#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/Path.h"
+#include <atomic>
+#include <condition_variable>
+#include <mutex>
+#include <queue>
+#include <string>
+#include <thread>
+#include <vector>
+
+#include <fcntl.h>
+#include <sys/epoll.h>
+#include <sys/inotify.h>
+#include <unistd.h>
+
+namespace {
+
+using namespace llvm;
+using namespace clang;
+
+/// Pipe for inter-thread synchronization - for epoll-ing on multiple
+/// conditions. It is meant for uni-directional 1:1 signalling - specifically:
+/// no multiple consumers, no data passing. Thread waiting for signal should
+/// poll the FDRead. Signalling thread should call signal() which writes single
+/// character to FDRead.
+struct SemaphorePipe {
+  // Expects two file-descriptors opened as a pipe in the canonical POSIX
+  // order: pipefd[0] refers to the read end of the pipe. pipefd[1] refers to
+  // the write end of the pipe.
+  SemaphorePipe(int pipefd[2])
+      : FDRead(pipefd[0]), FDWrite(pipefd[1]), OwnsFDs(true) {}
+  SemaphorePipe(const SemaphorePipe &) = delete;
+  void operator=(const SemaphorePipe &) = delete;
+  SemaphorePipe(SemaphorePipe &&other)
+      : FDRead(other.FDRead), FDWrite(other.FDWrite),
+        OwnsFDs(other.OwnsFDs) // Someone could have moved from the other
+                               // instance before.
+  {
+    other.OwnsFDs = false;
+  };
+
+  void signal() {
+    ssize_t Result = llvm::sys::RetryAfterSignal(-1, write, FDWrite, "A", 1);
+    assert(Result != -1);
+  }
+  ~SemaphorePipe() {
+    if (OwnsFDs) {
+      close(FDWrite);
+      close(FDRead);
+    }
+  }
+  const int FDRead;
+  const int FDWrite;
+  bool OwnsFDs;
+
+  static llvm::Optional<SemaphorePipe> create() {
+    int InotifyPollingStopperFDs[2];
+    if (pipe2(InotifyPollingStopperFDs, O_CLOEXEC) == -1)
+      return llvm::None;
+    return SemaphorePipe(InotifyPollingStopperFDs);
+  }
+};
+
+/// Mutex-protected queue of Events.
+class EventQueue {
+  std::mutex Mtx;
+  std::condition_variable NonEmpty;
+  std::queue<DirectoryWatcher::Event> Events;
+
+public:
+  void push_back(const DirectoryWatcher::Event::EventKind K,
+                 StringRef Filename) {
+    {
+      std::unique_lock<std::mutex> L(Mtx);
+      Events.emplace(K, Filename);
+    }
+    NonEmpty.notify_one();
+  }
+
+  // Blocks on caller thread and uses codition_variable to wait until there's an
+  // event to return.
+  DirectoryWatcher::Event pop_front_blocking() {
+    std::unique_lock<std::mutex> L(Mtx);
+    while (true) {
+      // Since we might have missed all the prior notifications on NonEmpty we
+      // have to check the queue first (under lock).
+      if (!Events.empty()) {
+        DirectoryWatcher::Event Front = Events.front();
+        Events.pop();
+        return Front;
+      }
+      NonEmpty.wait(L, [this]() { return !Events.empty(); });
+    }
+  }
+};
+
+class DirectoryWatcherLinux : public clang::DirectoryWatcher {
+public:
+  DirectoryWatcherLinux(
+      llvm::StringRef WatchedDirPath,
+      std::function<void(llvm::ArrayRef<Event>, bool)> Receiver,
+      bool WaitForInitialSync, int InotifyFD, int InotifyWD,
+      SemaphorePipe &&InotifyPollingStopSignal);
+
+  ~DirectoryWatcherLinux() override {
+    StopWork();
+    InotifyPollingThread.join();
+    EventsReceivingThread.join();
+    inotify_rm_watch(InotifyFD, InotifyWD);
+    llvm::sys::RetryAfterSignal(-1, close, InotifyFD);
+  }
+
+private:
+  const std::string WatchedDirPath;
+  // inotify file descriptor
+  int InotifyFD = -1;
+  // inotify watch descriptor
+  int InotifyWD = -1;
+
+  EventQueue Queue;
+
+  // Make sure lifetime of Receiver fully contains lifetime of
+  // EventsReceivingThread.
+  std::function<void(llvm::ArrayRef<Event>, bool)> Receiver;
+
+  // Consumes inotify events and pushes directory watcher events to the Queue.
+  void InotifyPollingLoop();
+  std::thread InotifyPollingThread;
+  // Using pipe so we can epoll two file descriptors at once - inotify and
+  // stopping condition.
+  SemaphorePipe InotifyPollingStopSignal;
+
+  // Does the initial scan of the directory - directly calling Receiver,
+  // bypassing the Queue. Both InitialScan and EventReceivingLoop use Receiver
+  // which isn't necessarily thread-safe.
+  void InitialScan();
+
+  // Processing events from the Queue.
+  // In case client doesn't want to do the initial scan synchronously
+  // (WaitForInitialSync=false in ctor) we do the initial scan at the beginning
+  // of this thread.
+  std::thread EventsReceivingThread;
+  // Push event of WatcherGotInvalidated kind to the Queue to stop the loop.
+  // Both InitialScan and EventReceivingLoop use Receiver which isn't
+  // necessarily thread-safe.
+  void EventReceivingLoop();
+
+  // Stops all the async work. Reentrant.
+  void StopWork() {
+    Queue.push_back(DirectoryWatcher::Event::EventKind::WatcherGotInvalidated,
+                    "");
+    InotifyPollingStopSignal.signal();
+  }
+};
+
+void DirectoryWatcherLinux::InotifyPollingLoop() {
+  // We want to be able to read ~30 events at once even in the worst case
+  // (obscenely long filenames).
+  constexpr size_t EventBufferLength =
+      30 * (sizeof(struct inotify_event) + NAME_MAX + 1);
+  // http://man7.org/linux/man-pages/man7/inotify.7.html
+  // Some systems cannot read integer variables if they are not
+  // properly aligned. On other systems, incorrect alignment may
+  // decrease performance. Hence, the buffer used for reading from
+  // the inotify file descriptor should have the same alignment as
+  // struct inotify_event.
+
+  auto ManagedBuffer =
+      llvm::make_unique<llvm::AlignedCharArray<alignof(struct inotify_event),
+                                               EventBufferLength>>();
+  char *const Buf = ManagedBuffer->buffer;
+
+  const int EpollFD = epoll_create1(EPOLL_CLOEXEC);
+  if (EpollFD == -1) {
+    StopWork();
+    return;
+  }
+  auto EpollFDGuard = llvm::make_scope_exit([EpollFD]() { close(EpollFD); });
+
+  struct epoll_event EventSpec;
+  EventSpec.events = EPOLLIN;
+  EventSpec.data.fd = InotifyFD;
+  if (epoll_ctl(EpollFD, EPOLL_CTL_ADD, InotifyFD, &EventSpec) == -1) {
+    StopWork();
+    return;
+  }
+
+  EventSpec.data.fd = InotifyPollingStopSignal.FDRead;
+  if (epoll_ctl(EpollFD, EPOLL_CTL_ADD, InotifyPollingStopSignal.FDRead,
+                &EventSpec) == -1) {
+    StopWork();
+    return;
+  }
+
+  std::array<struct epoll_event, 2> EpollEventBuffer;
+
+  while (true) {
+    const int EpollWaitResult = llvm::sys::RetryAfterSignal(
+        -1, epoll_wait, EpollFD, EpollEventBuffer.data(),
+        EpollEventBuffer.size(), /*timeout=*/-1 /*== infinity*/);
+    if (EpollWaitResult == -1) {
+      StopWork();
+      return;
+    }
+
+    // Multiple epoll_events can be received for a single file descriptor per
+    // epoll_wait call.
+    for (const auto &EpollEvent : EpollEventBuffer) {
+      if (EpollEvent.data.fd == InotifyPollingStopSignal.FDRead) {
+        StopWork();
+        return;
+      }
+    }
+
+    // epoll_wait() always return either error or >0 events. Since there was no
+    // event for stopping, it must be an inotify event ready for reading.
+    ssize_t NumRead = llvm::sys::RetryAfterSignal(-1, read, InotifyFD, Buf,
+                                                  EventBufferLength);
+    for (char *P = Buf; P < Buf + NumRead;) {
+      if (P + sizeof(struct inotify_event) > Buf + NumRead) {
+        StopWork();
+        llvm_unreachable("an incomplete inotify_event was read");
+        return;
+      }
+
+      struct inotify_event *Event = reinterpret_cast<struct inotify_event *>(P);
+      P += sizeof(struct inotify_event) + Event->len;
+
+      if (Event->mask & (IN_CREATE | IN_MODIFY | IN_MOVED_TO | IN_DELETE) &&
+          Event->len <= 0) {
+        StopWork();
+        llvm_unreachable("expected a filename from inotify");
+        return;
+      }
+
+      if (Event->mask & (IN_CREATE | IN_MOVED_TO | IN_MODIFY)) {
+        Queue.push_back(DirectoryWatcher::Event::EventKind::Modified,
+                        Event->name);
+      } else if (Event->mask & (IN_DELETE | IN_MOVED_FROM)) {
+        Queue.push_back(DirectoryWatcher::Event::EventKind::Removed,
+                        Event->name);
+      } else if (Event->mask & (IN_DELETE_SELF | IN_MOVE_SELF)) {
+        Queue.push_back(DirectoryWatcher::Event::EventKind::WatchedDirRemoved,
+                        "");
+        StopWork();
+        return;
+      } else if (Event->mask & IN_IGNORED) {
+        StopWork();
+        return;
+      } else {
+        StopWork();
+        llvm_unreachable("Unknown event type.");
+        return;
+      }
+    }
+  }
+}
+
+void DirectoryWatcherLinux::InitialScan() {
+  this->Receiver(getAsFileEvents(scanDirectory(WatchedDirPath)),
+                 /*IsInitial=*/true);
+}
+
+void DirectoryWatcherLinux::EventReceivingLoop() {
+  while (true) {
+    DirectoryWatcher::Event Event = this->Queue.pop_front_blocking();
+    this->Receiver(Event, false);
+    if (Event.Kind ==
+        DirectoryWatcher::Event::EventKind::WatcherGotInvalidated) {
+      StopWork();
+      return;
+    }
+  }
+}
+
+DirectoryWatcherLinux::DirectoryWatcherLinux(
+    StringRef WatchedDirPath,
+    std::function<void(llvm::ArrayRef<Event>, bool)> Receiver,
+    bool WaitForInitialSync, int InotifyFD, int InotifyWD,
+    SemaphorePipe &&InotifyPollingStopSignal)
+    : WatchedDirPath(WatchedDirPath), InotifyFD(InotifyFD),
+      InotifyWD(InotifyWD), Receiver(Receiver),
+      InotifyPollingStopSignal(std::move(InotifyPollingStopSignal)) {
+
+  InotifyPollingThread = std::thread([this]() { InotifyPollingLoop(); });
+  // We have no guarantees about thread safety of the Receiver which is being
+  // used in both InitialScan and EventReceivingLoop. We shouldn't run these
+  // only synchronously.
+  if (WaitForInitialSync) {
+    InitialScan();
+    EventsReceivingThread = std::thread([this]() { EventReceivingLoop(); });
+  } else {
+    EventsReceivingThread = std::thread([this]() {
+      // FIXME: We might want to terminate an async initial scan early in case
+      // of a failure in EventsReceivingThread.
+      InitialScan();
+      EventReceivingLoop();
+    });
+  }
+}
+
+} // namespace
+
+std::unique_ptr<DirectoryWatcher> clang::DirectoryWatcher::create(
+    StringRef Path,
+    std::function<void(llvm::ArrayRef<DirectoryWatcher::Event>, bool)> Receiver,
+    bool WaitForInitialSync) {
+  if (Path.empty())
+    return nullptr;
+
+  const int InotifyFD = inotify_init1(IN_CLOEXEC);
+  if (InotifyFD == -1)
+    return nullptr;
+
+  const int InotifyWD = inotify_add_watch(
+      InotifyFD, Path.str().c_str(),
+      IN_CREATE | IN_DELETE | IN_DELETE_SELF | IN_EXCL_UNLINK | IN_MODIFY |
+          IN_MOVED_FROM | IN_MOVE_SELF | IN_MOVED_TO | IN_ONLYDIR | IN_IGNORED);
+  if (InotifyWD == -1)
+    return nullptr;
+
+  auto InotifyPollingStopper = SemaphorePipe::create();
+
+  if (!InotifyPollingStopper)
+    return nullptr;
+
+  return llvm::make_unique<DirectoryWatcherLinux>(
+      Path, Receiver, WaitForInitialSync, InotifyFD, InotifyWD,
+      std::move(*InotifyPollingStopper));
+}
 No newline at end of file

cfe/trunk/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp

+//===- DirectoryWatcher-mac.cpp - Mac-platform directory watching ---------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "DirectoryScanner.h"
+#include "clang/DirectoryWatcher/DirectoryWatcher.h"
+
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Path.h"
+#include <CoreServices/CoreServices.h>
+
+using namespace llvm;
+using namespace clang;
+
+static FSEventStreamRef createFSEventStream(
+    StringRef Path,
+    std::function<void(llvm::ArrayRef<DirectoryWatcher::Event>, bool)>,
+    dispatch_queue_t);
+static void stopFSEventStream(FSEventStreamRef);
+
+namespace {
+
+class DirectoryWatcherMac : public clang::DirectoryWatcher {
+public:
+  DirectoryWatcherMac(
+      FSEventStreamRef EventStream,
+      std::function<void(llvm::ArrayRef<DirectoryWatcher::Event>, bool)>
+          Receiver,
+      llvm::StringRef WatchedDirPath)
+      : EventStream(EventStream), Receiver(Receiver),
+        WatchedDirPath(WatchedDirPath) {}
+
+  ~DirectoryWatcherMac() override {
+    stopFSEventStream(EventStream);
+    EventStream = nullptr;
+    // Now it's safe to use Receiver as the only other concurrent use would have
+    // been in EventStream processing.
+    Receiver(DirectoryWatcher::Event(
+                 DirectoryWatcher::Event::EventKind::WatcherGotInvalidated, ""),
+             false);
+  }
+
+private:
+  FSEventStreamRef EventStream;
+  std::function<void(llvm::ArrayRef<Event>, bool)> Receiver;
+  const std::string WatchedDirPath;
+};
+
+struct EventStreamContextData {
+  std::string WatchedPath;
+  std::function<void(llvm::ArrayRef<DirectoryWatcher::Event>, bool)> Receiver;
+
+  EventStreamContextData(
+      std::string &&WatchedPath,
+      std::function<void(llvm::ArrayRef<DirectoryWatcher::Event>, bool)>
+          Receiver)
+      : WatchedPath(std::move(WatchedPath)), Receiver(Receiver) {}
+
+  // Needed for FSEvents
+  static void dispose(const void *ctx) {
+    delete static_cast<const EventStreamContextData *>(ctx);
+  }
+};
+} // namespace
+
+constexpr const FSEventStreamEventFlags StreamInvalidatingFlags =
+    kFSEventStreamEventFlagUserDropped | kFSEventStreamEventFlagKernelDropped |
+    kFSEventStreamEventFlagMustScanSubDirs;
+
+constexpr const FSEventStreamEventFlags ModifyingFileEvents =
+    kFSEventStreamEventFlagItemCreated | kFSEventStreamEventFlagItemRenamed |
+    kFSEventStreamEventFlagItemModified;
+
+static void eventStreamCallback(ConstFSEventStreamRef Stream,
+                                void *ClientCallBackInfo, size_t NumEvents,
+                                void *EventPaths,
+                                const FSEventStreamEventFlags EventFlags[],
+                                const FSEventStreamEventId EventIds[]) {
+  auto *ctx = static_cast<EventStreamContextData *>(ClientCallBackInfo);
+
+  std::vector<DirectoryWatcher::Event> Events;
+  for (size_t i = 0; i < NumEvents; ++i) {
+    StringRef Path = ((const char **)EventPaths)[i];
+    const FSEventStreamEventFlags Flags = EventFlags[i];
+
+    if (Flags & StreamInvalidatingFlags) {
+      Events.emplace_back(DirectoryWatcher::Event{
+          DirectoryWatcher::Event::EventKind::WatcherGotInvalidated, ""});
+      break;
+    } else if (!(Flags & kFSEventStreamEventFlagItemIsFile)) {
+      // Subdirectories aren't supported - if some directory got removed it
+      // must've been the watched directory itself.
+      if ((Flags & kFSEventStreamEventFlagItemRemoved) &&
+          Path == ctx->WatchedPath) {
+        Events.emplace_back(DirectoryWatcher::Event{
+            DirectoryWatcher::Event::EventKind::WatchedDirRemoved, ""});
+        Events.emplace_back(DirectoryWatcher::Event{
+            DirectoryWatcher::Event::EventKind::WatcherGotInvalidated, ""});
+        break;
+      }
+      // No support for subdirectories - just ignore everything.
+      continue;
+    } else if (Flags & kFSEventStreamEventFlagItemRemoved) {
+      Events.emplace_back(DirectoryWatcher::Event::EventKind::Removed,
+                          llvm::sys::path::filename(Path));
+      continue;
+    } else if (Flags & ModifyingFileEvents) {
+      if (!getFileStatus(Path).hasValue()) {
+        Events.emplace_back(DirectoryWatcher::Event::EventKind::Removed,
+                            llvm::sys::path::filename(Path));
+      } else {
+        Events.emplace_back(DirectoryWatcher::Event::EventKind::Modified,
+                            llvm::sys::path::filename(Path));
+      }
+      continue;
+    }
+
+    // default
+    Events.emplace_back(DirectoryWatcher::Event{
+        DirectoryWatcher::Event::EventKind::WatcherGotInvalidated, ""});
+    llvm_unreachable("Unknown FSEvent type.");
+  }
+
+  if (!Events.empty()) {
+    ctx->Receiver(Events, /*IsInitial=*/false);
+  }
+}
+
+FSEventStreamRef createFSEventStream(
+    StringRef Path,
+    std::function<void(llvm::ArrayRef<DirectoryWatcher::Event>, bool)> Receiver,
+    dispatch_queue_t Queue) {
+  if (Path.empty())
+    return nullptr;
+
+  CFMutableArrayRef PathsToWatch = [&]() {
+    CFMutableArrayRef PathsToWatch =
+        CFArrayCreateMutable(nullptr, 0, &kCFTypeArrayCallBacks);
+    CFStringRef CfPathStr =
+        CFStringCreateWithBytes(nullptr, (const UInt8 *)Path.data(),
+                                Path.size(), kCFStringEncodingUTF8, false);
+    CFArrayAppendValue(PathsToWatch, CfPathStr);
+    CFRelease(CfPathStr);
+    return PathsToWatch;
+  }();
+
+  FSEventStreamContext Context = [&]() {
+    std::string RealPath;
+    {
+      SmallString<128> Storage;
+      StringRef P = llvm::Twine(Path).toNullTerminatedStringRef(Storage);
+      char Buffer[PATH_MAX];
+      if (::realpath(P.begin(), Buffer) != nullptr)
+        RealPath = Buffer;
+      else
+        RealPath = Path;
+    }
+
+    FSEventStreamContext Context;
+    Context.version = 0;
+    Context.info = new EventStreamContextData(std::move(RealPath), Receiver);
+    Context.retain = nullptr;
+    Context.release = EventStreamContextData::dispose;
+    Context.copyDescription = nullptr;
+    return Context;
+  }();
+
+  FSEventStreamRef Result = FSEventStreamCreate(
+      nullptr, eventStreamCallback, &Context, PathsToWatch,
+      kFSEventStreamEventIdSinceNow, /* latency in seconds */ 0.0,
+      kFSEventStreamCreateFlagFileEvents | kFSEventStreamCreateFlagNoDefer);
+  CFRelease(PathsToWatch);
+
+  return Result;
+}
+
+void stopFSEventStream(FSEventStreamRef EventStream) {
+  if (!EventStream)
+    return;
+  FSEventStreamStop(EventStream);
+  FSEventStreamInvalidate(EventStream);
+  FSEventStreamRelease(EventStream);
+}
+
+std::unique_ptr<DirectoryWatcher> clang::DirectoryWatcher::create(
+    StringRef Path,
+    std::function<void(llvm::ArrayRef<DirectoryWatcher::Event>, bool)> Receiver,
+    bool WaitForInitialSync) {
+  dispatch_queue_t Queue =
+      dispatch_queue_create("DirectoryWatcher", DISPATCH_QUEUE_SERIAL);
+
+  if (Path.empty())
+    return nullptr;
+
+  auto EventStream = createFSEventStream(Path, Receiver, Queue);
+  if (!EventStream) {
+    return nullptr;
+  }
+
+  std::unique_ptr<DirectoryWatcher> Result =
+      llvm::make_unique<DirectoryWatcherMac>(EventStream, Receiver, Path);
+
+  // We need to copy the data so the lifetime is ok after a const copy is made
+  // for the block.
+  const std::string CopiedPath = Path;
+
+  auto InitWork = ^{
+    // We need to start watching the directory before we start scanning in order
+    // to not miss any event. By dispatching this on the same serial Queue as
+    // the FSEvents will be handled we manage to start watching BEFORE the
+    // inital scan and handling events ONLY AFTER the scan finishes.
+    FSEventStreamSetDispatchQueue(EventStream, Queue);
+    FSEventStreamStart(EventStream);
+    // We need to decrement the ref count for Queue as initialize() will return
+    // and FSEvents has incremented it. Since we have to wait for FSEvents to
+    // take ownership it's the easiest to do it here rather than main thread.
+    dispatch_release(Queue);
+    Receiver(getAsFileEvents(scanDirectory(CopiedPath)), /*IsInitial=*/true);
+  };
+
+  if (WaitForInitialSync) {
+    dispatch_sync(Queue, InitWork);
+  } else {
+    dispatch_async(Queue, InitWork);
+  }
+
+  return Result;
+}

cfe/trunk/unittests/CMakeLists.txt

 add_subdirectory(Rewrite)
 add_subdirectory(Sema)
 add_subdirectory(CodeGen)
 # FIXME: libclang unit tests are disabled on Windows due
 # to failures, mostly in libclang.VirtualFileOverlay_*.
 if(NOT WIN32 AND CLANG_TOOL_LIBCLANG_BUILD) 
   add_subdirectory(libclang)
 endif()
+add_subdirectory(DirectoryWatcher)
 add_subdirectory(Rename)
 add_subdirectory(Index)
 add_subdirectory(Serialization)

cfe/trunk/unittests/DirectoryWatcher/CMakeLists.txt

+set(LLVM_LINK_COMPONENTS
+  Support
+  )
+
+add_clang_unittest(DirectoryWatcherTests
+  DirectoryWatcherTest.cpp
+  )
+
+target_link_libraries(DirectoryWatcherTests
+  PRIVATE
+  clangDirectoryWatcher
+  clangBasic
+  )
 No newline at end of file

cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

+//===- unittests/DirectoryWatcher/DirectoryWatcherTest.cpp ----------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/DirectoryWatcher/DirectoryWatcher.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Mutex.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gtest/gtest.h"
+#include <condition_variable>
+#include <future>
+#include <mutex>
+#include <thread>
+
+using namespace llvm;
+using namespace llvm::sys;
+using namespace llvm::sys::fs;
+using namespace clang;
+
+namespace clang {
+static bool operator==(const DirectoryWatcher::Event &lhs,
+                       const DirectoryWatcher::Event &rhs) {
+  return lhs.Filename == rhs.Filename &&
+         static_cast<int>(lhs.Kind) == static_cast<int>(rhs.Kind);
+}
+} // namespace clang
+
+namespace {
+
+struct DirectoryWatcherTestFixture {
+  std::string TestRootDir;
+  std::string TestWatchedDir;
+
+  DirectoryWatcherTestFixture() {
+    SmallString<128> pathBuf;
+    std::error_code UniqDirRes = createUniqueDirectory("dirwatcher", pathBuf);
+    assert(!UniqDirRes);
+    TestRootDir = pathBuf.str();
+    path::append(pathBuf, "watch");
+    TestWatchedDir = pathBuf.str();
+    std::error_code CreateDirRes = create_directory(TestWatchedDir, false);
+    assert(!CreateDirRes);
+  }
+
+  ~DirectoryWatcherTestFixture() { remove_directories(TestRootDir); }
+
+  SmallString<128> getPathInWatched(const std::string &testFile) {
+    SmallString<128> pathBuf;
+    pathBuf = TestWatchedDir;
+    path::append(pathBuf, testFile);
+    return pathBuf;
+  }
+
+  void addFile(const std::string &testFile) {
+    Expected<file_t> ft = openNativeFileForWrite(getPathInWatched(testFile),
+                                                 CD_CreateNew, OF_None);
+    if (ft) {
+      closeFile(*ft);
+    } else {
+      llvm::errs() << llvm::toString(ft.takeError()) << "\n";
+      llvm::errs() << getPathInWatched(testFile) << "\n";
+      llvm_unreachable("Couldn't create test file.");
+    }
+  }
+
+  void deleteFile(const std::string &testFile) {
+    std::error_code EC =
+        remove(getPathInWatched(testFile), /*IgnoreNonExisting=*/false);
+    ASSERT_FALSE(EC);
+  }
+};
+
+std::string eventKindToString(const DirectoryWatcher::Event::EventKind K) {
+  switch (K) {
+  case DirectoryWatcher::Event::EventKind::Removed:
+    return "Removed";
+  case DirectoryWatcher::Event::EventKind::Modified:
+    return "Modified";
+  case DirectoryWatcher::Event::EventKind::WatchedDirRemoved:
+    return "WatchedDirRemoved";
+  case DirectoryWatcher::Event::EventKind::WatcherGotInvalidated:
+    return "WatcherGotInvalidated";
+  }
+  llvm_unreachable("unknown event kind");
+}
+
+struct VerifyingConsumer {
+  std::vector<DirectoryWatcher::Event> ExpectedInitial;
+  std::vector<DirectoryWatcher::Event> ExpectedNonInitial;
+  std::vector<DirectoryWatcher::Event> OptionalNonInitial;
+  std::vector<DirectoryWatcher::Event> UnexpectedInitial;
+  std::vector<DirectoryWatcher::Event> UnexpectedNonInitial;
+  std::mutex Mtx;
+  std::condition_variable ResultIsReady;
+
+  VerifyingConsumer(
+      const std::vector<DirectoryWatcher::Event> &ExpectedInitial,
+      const std::vector<DirectoryWatcher::Event> &ExpectedNonInitial,
+      const std::vector<DirectoryWatcher::Event> &OptionalNonInitial = {})
+      : ExpectedInitial(ExpectedInitial),
+        ExpectedNonInitial(ExpectedNonInitial),
+        OptionalNonInitial(OptionalNonInitial) {}
+
+  // This method is used by DirectoryWatcher.
+  void consume(DirectoryWatcher::Event E, bool IsInitial) {
+    if (IsInitial)
+      consumeInitial(E);
+    else
+      consumeNonInitial(E);
+  }
+
+  void consumeInitial(DirectoryWatcher::Event E) {
+    std::unique_lock<std::mutex> L(Mtx);
+    auto It = std::find(ExpectedInitial.begin(), ExpectedInitial.end(), E);
+    if (It == ExpectedInitial.end()) {
+      UnexpectedInitial.push_back(E);
+    } else {
+      ExpectedInitial.erase(It);
+    }
+    if (result())
+      ResultIsReady.notify_one();
+  }
+
+  void consumeNonInitial(DirectoryWatcher::Event E) {
+    std::unique_lock<std::mutex> L(Mtx);
+    auto It =
+        std::find(ExpectedNonInitial.begin(), ExpectedNonInitial.end(), E);
+    if (It == ExpectedNonInitial.end()) {
+      auto OptIt =
+          std::find(OptionalNonInitial.begin(), OptionalNonInitial.end(), E);
+      if (OptIt != OptionalNonInitial.end()) {
+        OptionalNonInitial.erase(OptIt);
+      } else {
+        UnexpectedNonInitial.push_back(E);
+      }
+    } else {
+      ExpectedNonInitial.erase(It);
+    }
+    if (result())
+      ResultIsReady.notify_one();
+  }
+
+  // This method is used by DirectoryWatcher.
+  void consume(llvm::ArrayRef<DirectoryWatcher::Event> Es, bool IsInitial) {
+    for (const auto &E : Es)
+      consume(E, IsInitial);
+  }
+
+  // Not locking - caller has to lock Mtx.
+  llvm::Optional<bool> result() const {
+    if (ExpectedInitial.empty() && ExpectedNonInitial.empty() &&
+        UnexpectedInitial.empty() && UnexpectedNonInitial.empty())
+      return true;
+    if (!UnexpectedInitial.empty() || !UnexpectedNonInitial.empty())
+      return false;
+    return llvm::None;
+  }
+
+  // This method is used by tests.
+  // \returns true on success
+  bool blockUntilResult() {
+    std::unique_lock<std::mutex> L(Mtx);
+    while (true) {
+      if (result())
+        return *result();
+
+      ResultIsReady.wait(L, [this]() { return result().hasValue(); });
+    }
+    return false; // Just to make compiler happy.
+  }
+
+  void printUnmetExpectations(llvm::raw_ostream &OS) {
+    if (!ExpectedInitial.empty()) {
+      OS << "Expected but not seen initial events: \n";
+      for (const auto &E : ExpectedInitial) {
+        OS << eventKindToString(E.Kind) << " " << E.Filename << "\n";
+      }
+    }
+    if (!ExpectedNonInitial.empty()) {
+      OS << "Expected but not seen non-initial events: \n";
+      for (const auto &E : ExpectedNonInitial) {
+        OS << eventKindToString(E.Kind) << " " << E.Filename << "\n";
+      }
+    }
+    if (!UnexpectedInitial.empty()) {
+      OS << "Unexpected initial events seen: \n";
+      for (const auto &E : UnexpectedInitial) {
+        OS << eventKindToString(E.Kind) << " " << E.Filename << "\n";
+      }
+    }
+    if (!UnexpectedNonInitial.empty()) {
+      OS << "Unexpected non-initial events seen: \n";
+      for (const auto &E : UnexpectedNonInitial) {
+        OS << eventKindToString(E.Kind) << " " << E.Filename << "\n";
+      }
+    }
+  }
+};
+
+void checkEventualResultWithTimeout(VerifyingConsumer &TestConsumer) {
+  std::packaged_task<int(void)> task(
+      [&TestConsumer]() { return TestConsumer.blockUntilResult(); });
+  std::future<int> WaitForExpectedStateResult = task.get_future();
+  std::thread worker(std::move(task));
+  worker.detach();
+
+  EXPECT_TRUE(WaitForExpectedStateResult.wait_for(std::chrono::seconds(3)) ==
+              std::future_status::ready)
+      << "The expected result state wasn't reached before the time-out.";
+  EXPECT_TRUE(TestConsumer.result().hasValue());
+  if (TestConsumer.result().hasValue()) {
+    EXPECT_TRUE(*TestConsumer.result());
+  }
+  if ((TestConsumer.result().hasValue() && !TestConsumer.result().getValue()) ||
+      !TestConsumer.result().hasValue())
+    TestConsumer.printUnmetExpectations(llvm::outs());
+}
+
+} // namespace
+
+TEST(DirectoryWatcherTest, InitialScanSync) {
+  DirectoryWatcherTestFixture fixture;
+
+  fixture.addFile("a");
+  fixture.addFile("b");
+  fixture.addFile("c");
+
+  VerifyingConsumer TestConsumer{
+      {{DirectoryWatcher::Event::EventKind::Modified, "a"},
+       {DirectoryWatcher::Event::EventKind::Modified, "b"},
+       {DirectoryWatcher::Event::EventKind::Modified, "c"}},
+      {}};
+
+  auto DW = DirectoryWatcher::create(
+      fixture.TestWatchedDir,
+      [&TestConsumer](llvm::ArrayRef<DirectoryWatcher::Event> Events,
+                      bool IsInitial) {
+        TestConsumer.consume(Events, IsInitial);
+      },
+      /*waitForInitialSync=*/true);
+
+  checkEventualResultWithTimeout(TestConsumer);
+}
+
+TEST(DirectoryWatcherTest, InitialScanAsync) {
+  DirectoryWatcherTestFixture fixture;
+
+  fixture.addFile("a");
+  fixture.addFile("b");
+  fixture.addFile("c");
+
+  VerifyingConsumer TestConsumer{
+      {{DirectoryWatcher::Event::EventKind::Modified, "a"},
+       {DirectoryWatcher::Event::EventKind::Modified, "b"},
+       {DirectoryWatcher::Event::EventKind::Modified, "c"}},
+      {}};
+
+  auto DW = DirectoryWatcher::create(
+      fixture.TestWatchedDir,
+      [&TestConsumer](llvm::ArrayRef<DirectoryWatcher::Event> Events,
+                      bool IsInitial) {
+        TestConsumer.consume(Events, IsInitial);
+      },
+      /*waitForInitialSync=*/false);
+
+  checkEventualResultWithTimeout(TestConsumer);
+}
+
+TEST(DirectoryWatcherTest, AddFiles) {
+  DirectoryWatcherTestFixture fixture;
+
+  VerifyingConsumer TestConsumer{
+      {},
+      {{DirectoryWatcher::Event::EventKind::Modified, "a"},
+       {DirectoryWatcher::Event::EventKind::Modified, "b"},
+       {DirectoryWatcher::Event::EventKind::Modified, "c"}}};
+
+  auto DW = DirectoryWatcher::create(
+      fixture.TestWatchedDir,
+      [&TestConsumer](llvm::ArrayRef<DirectoryWatcher::Event> Events,
+                      bool IsInitial) {
+        TestConsumer.consume(Events, IsInitial);
+      },
+      /*waitForInitialSync=*/true);
+
+  fixture.addFile("a");
+  fixture.addFile("b");
+  fixture.addFile("c");
+
+  checkEventualResultWithTimeout(TestConsumer);
+}
+
+TEST(DirectoryWatcherTest, ModifyFile) {
+  DirectoryWatcherTestFixture fixture;
+
+  fixture.addFile("a");
+
+  VerifyingConsumer TestConsumer{
+      {{DirectoryWatcher::Event::EventKind::Modified, "a"}},
+      {{DirectoryWatcher::Event::EventKind::Modified, "a"}}};
+
+  auto DW = DirectoryWatcher::create(
+      fixture.TestWatchedDir,
+      [&TestConsumer](llvm::ArrayRef<DirectoryWatcher::Event> Events,
+                      bool IsInitial) {
+        TestConsumer.consume(Events, IsInitial);
+      },
+      /*waitForInitialSync=*/true);
+
+  // modify the file
+  {
+    std::error_code error;
+    llvm::raw_fd_ostream bStream(fixture.getPathInWatched("a"), error,
+                                 CD_OpenExisting);
+    assert(!error);
+    bStream << "foo";
+  }
+
+  checkEventualResultWithTimeout(TestConsumer);
+}
+
+TEST(DirectoryWatcherTest, DeleteFile) {
+  DirectoryWatcherTestFixture fixture;
+
+  fixture.addFile("a");
+
+  VerifyingConsumer TestConsumer{
+      {{DirectoryWatcher::Event::EventKind::Modified, "a"}},
+      {{DirectoryWatcher::Event::EventKind::Removed, "a"}}};
+
+  auto DW = DirectoryWatcher::create(
+      fixture.TestWatchedDir,
+      [&TestConsumer](llvm::ArrayRef<DirectoryWatcher::Event> Events,
+                      bool IsInitial) {
+        TestConsumer.consume(Events, IsInitial);
+      },
+      /*waitForInitialSync=*/true);
+
+  fixture.deleteFile("a");
+
+  checkEventualResultWithTimeout(TestConsumer);
+}
+
+TEST(DirectoryWatcherTest, DeleteWatchedDir) {
+  DirectoryWatcherTestFixture fixture;
+
+  VerifyingConsumer TestConsumer{
+      {},
+      {{DirectoryWatcher::Event::EventKind::WatchedDirRemoved, ""},
+       {DirectoryWatcher::Event::EventKind::WatcherGotInvalidated, ""}}};
+
+  auto DW = DirectoryWatcher::create(
+      fixture.TestWatchedDir,
+      [&TestConsumer](llvm::ArrayRef<DirectoryWatcher::Event> Events,
+                      bool IsInitial) {
+        TestConsumer.consume(Events, IsInitial);
+      },
+      /*waitForInitialSync=*/true);
+
+  remove_directories(fixture.TestWatchedDir);
+
+  checkEventualResultWithTimeout(TestConsumer);
+}
+
+TEST(DirectoryWatcherTest, InvalidatedWatcher) {
+  DirectoryWatcherTestFixture fixture;
+
+  VerifyingConsumer TestConsumer{
+      {}, {{DirectoryWatcher::Event::EventKind::WatcherGotInvalidated, ""}}};
+
+  {
+    auto DW = DirectoryWatcher::create(
+        fixture.TestWatchedDir,
+        [&TestConsumer](llvm::ArrayRef<DirectoryWatcher::Event> Events,
+                        bool IsInitial) {
+          TestConsumer.consume(Events, IsInitial);
+        },
+        /*waitForInitialSync=*/true);
+  } // DW is destructed here.
+
+  checkEventualResultWithTimeout(TestConsumer);
+}
+
+TEST(DirectoryWatcherTest, ChangeMetadata) {
+  DirectoryWatcherTestFixture fixture;
+  fixture.addFile("a");
+
+  VerifyingConsumer TestConsumer{
+      {{DirectoryWatcher::Event::EventKind::Modified, "a"}},
+      // We don't expect any notification for file having access file changed.
+      {},
+      // Given the timing we are ok with receiving the duplicate event.
+      {{DirectoryWatcher::Event::EventKind::Modified, "a"}}};
+
+  auto DW = DirectoryWatcher::create(
+      fixture.TestWatchedDir,
+      [&TestConsumer](llvm::ArrayRef<DirectoryWatcher::Event> Events,
+                      bool IsInitial) {
+        TestConsumer.consume(Events, IsInitial);
+      },
+      /*waitForInitialSync=*/true);
+
+  { // Change access and modification time of file a.
+    Expected<file_t> HopefullyTheFD = llvm::sys::fs::openNativeFileForWrite(
+        fixture.getPathInWatched("a"), CD_OpenExisting, OF_None);
+    if (!HopefullyTheFD) {
+      llvm::outs() << HopefullyTheFD.takeError();
+    }
+
+    const int FD = HopefullyTheFD.get();
+    const TimePoint<> NewTimePt =
+        std::chrono::system_clock::now() - std::chrono::minutes(1);
+
+    std::error_code setTimeRes =
+        llvm::sys::fs::setLastAccessAndModificationTime(FD, NewTimePt,
+                                                        NewTimePt);
+    assert(!setTimeRes);
+  }
+
+  checkEventualResultWithTimeout(TestConsumer);
+}