r367979 - [clang][DirectoryWatcher] Adding llvm::Expected error handling to create.
Puyan Lotfi via cfe-commits
cfe-commits at lists.llvm.org
Tue Aug 6 00:16:50 PDT 2019
Thank you! I had forgot that cmake decides which cpp file to build with for DirectoryWatcher depending on the os you’re building on :-(
PL
Sent from ProtonMail Mobile
On Tue, Aug 6, 2019 at 12:14 AM, Shoaib Meenai <smeenai at fb.com> wrote:
> 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
>
> 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/faef6df1/attachment-0001.html>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: publicKey - puyan at puyan.org - d3f7720045476a2b20d90afa7b39326f9b4bb197.asc
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190806/faef6df1/attachment-0001.asc>
More information about the cfe-commits
mailing list