r367979 - [clang][DirectoryWatcher] Adding llvm::Expected error handling to create.
Shoaib Meenai via cfe-commits
cfe-commits at lists.llvm.org
Tue Aug 6 00:14:10 PDT 2019
You missed a semicolon after an assert, which broke asserts Mac builds. I fixed it in r367984.
From: cfe-commits <cfe-commits-bounces at lists.llvm.org> on behalf of Puyan Lotfi via cfe-commits <cfe-commits at lists.llvm.org>
Reply-To: Puyan Lotfi <puyan at puyan.org>
Date: Monday, August 5, 2019 at 10:11 PM
To: "cfe-commits at lists.llvm.org" <cfe-commits at lists.llvm.org>
Subject: r367979 - [clang][DirectoryWatcher] Adding llvm::Expected error handling to create.
Author: zer0
Date: Mon Aug 5 22:12:23 2019
New Revision: 367979
URL: https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D367979-26view-3Drev&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=6k2w1Mw8wKFqzKnFgq4r1I7Zmx7B-N-JdVMvtHYpjJs&s=GdHgTwHdRXipmG75D6eyJGuUUbCL0EMIl0YPYAY0y2U&e=
Log:
[clang][DirectoryWatcher] Adding llvm::Expected error handling to create.
Prior to this patch Unix style errno error reporting from the inotify layer was
used by DirectoryWatcher::create to simply return a nullptr on error. This
would generally be ok, except that in LLVM we have much more robust error
reporting through the facilities of llvm::Expected.
The other critical thing I stumbled across was that the unit tests for
DirectoryWatcher were not failing abruptly when inotify_init() was reporting an
error, but would continue with the testing and eventually hit a deadlock in a
pathological machine state (ie in the unit test, the return nullptr on ::create
was ignored).
Generally this pathological state never happens on any build bot, so it is
totally understandable that it was overlooked, but on a Linux desktop running
a dubious desktop environment (which I will not name) there is a chance that
said desktop environment could use up enough inotify instances to exceed the
user's limit. These are the conditions that led me to hit the deadlock I am
addressing in this patch with more robust error handling.
With the new llvm::Expected error handling when your system runs out of inotify
instances for your user, the unit test will be forced to handle the error or
crash and report the issue to the user instead of weirdly deadlocking on a
condition variable wait.
Differential Revision: https://urldefense.proofpoint.com/v2/url?u=https-3A__reviews.llvm.org_D65704&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=6k2w1Mw8wKFqzKnFgq4r1I7Zmx7B-N-JdVMvtHYpjJs&s=vGtUV_-i1W7ejLSH8cWFnYlusAJMA8EK5kMTvuMmQB4&e=
Modified:
cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h
cfe/trunk/lib/DirectoryWatcher/default/DirectoryWatcher-not-implemented.cpp
cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
cfe/trunk/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
Modified: cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h
URL: https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_cfe_trunk_include_clang_DirectoryWatcher_DirectoryWatcher.h-3Frev-3D367979-26r1-3D367978-26r2-3D367979-26view-3Ddiff&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=6k2w1Mw8wKFqzKnFgq4r1I7Zmx7B-N-JdVMvtHYpjJs&s=VCfJ3mXkHYvO_8Iw35iGLiTPxX-OaQIrX9fUY22ukbw&e=
==============================================================================
--- cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h (original)
+++ cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h Mon Aug 5 22:12:23 2019
@@ -11,6 +11,7 @@
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
#include <functional>
#include <memory>
#include <string>
@@ -98,10 +99,11 @@ public:
: Kind(Kind), Filename(Filename) {}
};
- /// Returns nullptr if \param Path doesn't exist or 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>
+ /// Asserts if \param Path doesn't exist or isn't a directory.
+ /// Returns llvm::Expected Error 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 llvm::Expected<std::unique_ptr<DirectoryWatcher>>
create(llvm::StringRef Path,
std::function<void(llvm::ArrayRef<DirectoryWatcher::Event> Events,
bool IsInitial)>
Modified: cfe/trunk/lib/DirectoryWatcher/default/DirectoryWatcher-not-implemented.cpp
URL: https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_cfe_trunk_lib_DirectoryWatcher_default_DirectoryWatcher-2Dnot-2Dimplemented.cpp-3Frev-3D367979-26r1-3D367978-26r2-3D367979-26view-3Ddiff&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=6k2w1Mw8wKFqzKnFgq4r1I7Zmx7B-N-JdVMvtHYpjJs&s=kOGSMivkTUI-mqfkDLYmSGfQM8CSoTUydr4qZdtcaAc&e=
==============================================================================
--- cfe/trunk/lib/DirectoryWatcher/default/DirectoryWatcher-not-implemented.cpp (original)
+++ cfe/trunk/lib/DirectoryWatcher/default/DirectoryWatcher-not-implemented.cpp Mon Aug 5 22:12:23 2019
@@ -11,9 +11,11 @@
using namespace llvm;
using namespace clang;
-std::unique_ptr<DirectoryWatcher> clang::DirectoryWatcher::create(
+llvm::Expected<std::unique_ptr<DirectoryWatcher>> clang::DirectoryWatcher::create(
StringRef Path,
std::function<void(llvm::ArrayRef<DirectoryWatcher::Event>, bool)> Receiver,
bool WaitForInitialSync) {
- return nullptr;
+ return llvm::make_error<llvm::StringError>(
+ "DirectoryWatcher is not implemented for this platform!",
+ llvm::inconvertibleErrorCode());
}
\ No newline at end of file
Modified: cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
URL: https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_cfe_trunk_lib_DirectoryWatcher_linux_DirectoryWatcher-2Dlinux.cpp-3Frev-3D367979-26r1-3D367978-26r2-3D367979-26view-3Ddiff&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=6k2w1Mw8wKFqzKnFgq4r1I7Zmx7B-N-JdVMvtHYpjJs&s=JukTCE-It3faGLTZEpsnCkN-RSCAfjthnHtk0Xh3PmA&e=
==============================================================================
--- cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp (original)
+++ cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp Mon Aug 5 22:12:23 2019
@@ -13,6 +13,7 @@
#include "llvm/ADT/ScopeExit.h"
#include "llvm/Support/AlignOf.h"
#include "llvm/Support/Errno.h"
+#include "llvm/Support/Error.h"
#include "llvm/Support/Mutex.h"
#include "llvm/Support/Path.h"
#include <atomic>
@@ -320,16 +321,17 @@ DirectoryWatcherLinux::DirectoryWatcherL
} // namespace
-std::unique_ptr<DirectoryWatcher> clang::DirectoryWatcher::create(
+llvm::Expected<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;
+ assert(!Path.empty() && "Path.empty()");
const int InotifyFD = inotify_init1(IN_CLOEXEC);
if (InotifyFD == -1)
- return nullptr;
+ return llvm::make_error<llvm::StringError>(
+ std::string("inotify_init1() error: ") + strerror(errno),
+ llvm::inconvertibleErrorCode());
const int InotifyWD = inotify_add_watch(
InotifyFD, Path.str().c_str(),
@@ -340,12 +342,16 @@ std::unique_ptr<DirectoryWatcher> clang:
#endif
);
if (InotifyWD == -1)
- return nullptr;
+ return llvm::make_error<llvm::StringError>(
+ std::string("inotify_add_watch() error: ") + strerror(errno),
+ llvm::inconvertibleErrorCode());
auto InotifyPollingStopper = SemaphorePipe::create();
if (!InotifyPollingStopper)
- return nullptr;
+ return llvm::make_error<llvm::StringError>(
+ std::string("SemaphorePipe::create() error: ") + strerror(errno),
+ llvm::inconvertibleErrorCode());
return llvm::make_unique<DirectoryWatcherLinux>(
Path, Receiver, WaitForInitialSync, InotifyFD, InotifyWD,
Modified: cfe/trunk/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
URL: https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_cfe_trunk_lib_DirectoryWatcher_mac_DirectoryWatcher-2Dmac.cpp-3Frev-3D367979-26r1-3D367978-26r2-3D367979-26view-3Ddiff&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=6k2w1Mw8wKFqzKnFgq4r1I7Zmx7B-N-JdVMvtHYpjJs&s=T2eOFEXsVLLsAjg0xURecvVWyS6cEOOgHwW63xK5ZfY&e=
==============================================================================
--- cfe/trunk/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp (original)
+++ cfe/trunk/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp Mon Aug 5 22:12:23 2019
@@ -11,16 +11,13 @@
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.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 {
@@ -153,8 +150,7 @@ FSEventStreamRef createFSEventStream(
StringRef Path,
std::function<void(llvm::ArrayRef<DirectoryWatcher::Event>, bool)> Receiver,
dispatch_queue_t Queue) {
- if (Path.empty())
- return nullptr;
+ assert(!Path.empty() && "Path.empty()");
CFMutableArrayRef PathsToWatch = [&]() {
CFMutableArrayRef PathsToWatch =
@@ -205,20 +201,16 @@ void stopFSEventStream(FSEventStreamRef
FSEventStreamRelease(EventStream);
}
-std::unique_ptr<DirectoryWatcher> clang::DirectoryWatcher::create(
+llvm::Expected<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;
-
+ assert(!Path.empty() && "Path.empty()");
auto EventStream = createFSEventStream(Path, Receiver, Queue);
- if (!EventStream) {
- return nullptr;
- }
+ assert(EventStream && "EventStream expected to be non-null.")
std::unique_ptr<DirectoryWatcher> Result =
llvm::make_unique<DirectoryWatcherMac>(EventStream, Receiver, Path);
Modified: cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
URL: https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_cfe_trunk_unittests_DirectoryWatcher_DirectoryWatcherTest.cpp-3Frev-3D367979-26r1-3D367978-26r2-3D367979-26view-3Ddiff&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=6k2w1Mw8wKFqzKnFgq4r1I7Zmx7B-N-JdVMvtHYpjJs&s=4lOaG5iva-sTgUXlfX2eBaUzL1hMxauE0Pd2oW34fHk&e=
==============================================================================
--- cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp (original)
+++ cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp Mon Aug 5 22:12:23 2019
@@ -284,6 +284,7 @@ TEST(DirectoryWatcherTest, InitialScanSy
TestConsumer.consume(Events, IsInitial);
},
/*waitForInitialSync=*/true);
+ if (!DW) return;
checkEventualResultWithTimeout(TestConsumer);
}
@@ -315,6 +316,7 @@ TEST(DirectoryWatcherTest, InitialScanAs
TestConsumer.consume(Events, IsInitial);
},
/*waitForInitialSync=*/false);
+ if (!DW) return;
checkEventualResultWithTimeout(TestConsumer);
}
@@ -335,6 +337,7 @@ TEST(DirectoryWatcherTest, AddFiles) {
TestConsumer.consume(Events, IsInitial);
},
/*waitForInitialSync=*/true);
+ if (!DW) return;
fixture.addFile("a");
fixture.addFile("b");
@@ -360,6 +363,7 @@ TEST(DirectoryWatcherTest, ModifyFile) {
TestConsumer.consume(Events, IsInitial);
},
/*waitForInitialSync=*/true);
+ if (!DW) return;
// modify the file
{
@@ -390,6 +394,7 @@ TEST(DirectoryWatcherTest, DeleteFile) {
TestConsumer.consume(Events, IsInitial);
},
/*waitForInitialSync=*/true);
+ if (!DW) return;
fixture.deleteFile("a");
@@ -411,6 +416,7 @@ TEST(DirectoryWatcherTest, DeleteWatched
TestConsumer.consume(Events, IsInitial);
},
/*waitForInitialSync=*/true);
+ if (!DW) return;
remove_directories(fixture.TestWatchedDir);
@@ -431,6 +437,7 @@ TEST(DirectoryWatcherTest, InvalidatedWa
TestConsumer.consume(Events, IsInitial);
},
/*waitForInitialSync=*/true);
+ if (!DW) return;
} // DW is destructed here.
checkEventualResultWithTimeout(TestConsumer);
_______________________________________________
cfe-commits mailing list
cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>
https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_cfe-2Dcommits&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=6k2w1Mw8wKFqzKnFgq4r1I7Zmx7B-N-JdVMvtHYpjJs&s=AvZ7rl6nzoSTyhQ1_rWF25r1kxnADHUAR7TvuiC3OE4&e=
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190806/0318370c/attachment-0001.html>
More information about the cfe-commits
mailing list