[PATCH] D65829: [clang][DirectoryWatcher][NFC] Swapping asserts for llvm fatal_error in ::create.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.


================
Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:323
+          /*waitForInitialSync=*/false);
+  // llvm::Expected will throw an error if DW is an Error.
+  if (!DW)
----------------
plotfi wrote:
> gribozavr wrote:
> > plotfi wrote:
> > > gribozavr wrote:
> > > > plotfi wrote:
> > > > > gribozavr wrote:
> > > > > > Call `llvm::cantFail` and there will be no need to explain anything.
> > > > > I explained this in the other patch: 
> > > > > 
> > > > > cantFail is arguably worse. I want this test to tell me the cause of the crash (in this case, exceeding the inotify limit). What I want to happen is:
> > > > > 
> > > > > ```
> > > > > Note: Google Test filter = DirectoryWatcherTest.AddFiles
> > > > > [==========] Running 1 test from 1 test case.
> > > > > [----------] Global test environment set-up.
> > > > > [----------] 1 test from DirectoryWatcherTest
> > > > > [ RUN      ] DirectoryWatcherTest.AddFiles
> > > > > Expected<T> must be checked before access or destruction.
> > > > > Unchecked Expected<T> contained error:
> > > > > inotify_init1() error: out of inotify entries #0 0x0000000000246b8f llvm::sys::PrintStackTrace(llvm::raw_ostream&) /mnt/nvme0/llvm-project/llvm/lib/Support/Unix/Signals.inc:533:13
> > > > >  #1 0x00000000002450d2 llvm::sys::RunSignalHandlers() /mnt/nvme0/llvm-project/llvm/lib/Support/Signals.cpp:69:18
> > > > >  #2 0x0000000000247268 SignalHandler(int) /mnt/nvme0/llvm-project/llvm/lib/Support/Unix/Signals.inc:385:1
> > > > >  #3 0x00007f17924eb890 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x12890)
> > > > >  #4 0x00007f1790f94e97 gsignal /build/glibc-OTsEL5/glibc-
> > > > > ...
> > > > > ```
> > > > > 
> > > > > Wrapping the llvm::Expected in a cantFail hides this message, and instead you get the much less useful:
> > > > > 
> > > > > ```
> > > > > Note: Google Test filter = DirectoryWatcherTest.AddFiles
> > > > > [==========] Running 1 test from 1 test case.                                                                                                                                                                                                
> > > > > [----------] Global test environment set-up.
> > > > > [----------] 1 test from DirectoryWatcherTest
> > > > > [ RUN      ] DirectoryWatcherTest.AddFiles
> > > > > Failure value returned from cantFail wrapped call
> > > > > UNREACHABLE executed at /mnt/nvme0/llvm-project/llvm/include/llvm/Support/Error.h:707!
> > > > >  #0 0x0000000000246e0f llvm::sys::PrintStackTrace(llvm::raw_ostream&) /mnt/nvme0/llvm-project/llvm/lib/Support/Unix/Signals.inc:533:13
> > > > >  #1 0x0000000000245352 llvm::sys::RunSignalHandlers() /mnt/nvme0/llvm-project/llvm/lib/Support/Signals.cpp:69:18
> > > > >  #2 0x00000000002474e8 SignalHandler(int) /mnt/nvme0/llvm-project/llvm/lib/Support/Unix/Signals.inc:385:1
> > > > >  #3 0x00007f20c64a8890 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x12890)
> > > > >  #4 0x00007f20c4f51e97 gsignal /build/glibc-OTsEL5/glibc-2.27/signal/../sysdeps/unix/sysv/linux/raise.c:51:0
> > > > >  #5 0x00007f20c4f53801 abort /build/glibc-OTsEL5/glibc-2.27/stdlib/abort.c:81:0
> > > > >  #6 0x0000000000232b31 (build/tools/clang/unittests/DirectoryWatcher/./DirectoryWatcherTests+0x232b31)
> > > > > ```
> > > > > 
> > > > > I specifically _want_ llvm::Expected to propagate the error up.
> > > > > I can drop the comments, because it should be fairly obvious here that DW is an llvm::Expected since I got rid of the autos.
> > > > > 
> > > > > To be clear, wrapping llvm::Expected in cantFail goes against the entire point of why I added this more robust error handling.
> > > > > If someone's machine is failing on this test because they've exceeded their inotify limit, I would really like the message produced from strerror(errno) to bubble up to the top so that it is obvious to them that they need to either kill some tasks on their machine or up their inotify limit. 
> > > > The automated checking in llvm::Expected is not guaranteed to happen (it is behind LLVM_ENABLE_ABI_BREAKING_CHECKS). If you are not happy with the error from `cantFail` I think everyone would appreciate an improvement there.
> > > > 
> > > > > I specifically _want_ llvm::Expected to propagate the error up.
> > > > 
> > > > I don't understand. It is not propagating anything up, it is just crashing in the destructor.
> > > Also, I have to point out here: cantFail is exactly the wrong tool here because this code can in fact fail if your system has exceeded its inotify limits. 
> > For the purposes of the test, the call can't fail. If the call fails, the test fails.
> > 
> > How about `ASSERT_TRUE(DW)`?
> I meant, it forces the code to either have to takeError and propagate the error up or it will crash in the destructor. As far as I can tell that's how it's supposed to work. I want the error produced inside DirectoryWatcher::create to show up in the user's console.
> I want the error produced inside DirectoryWatcher::create to show up in the user's console.

But the destructor of `llvm::Expected` is not guaranteed to do that. It will only detect (and print) the error under `LLVM_ENABLE_ABI_BREAKING_CHECKS`. This is exactly why I'm suggesting various alternative solutions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65829/new/

https://reviews.llvm.org/D65829