Also I went ahead and replaced all the instances of "auto DW = DirectoryWatcher::create" with "llvm::Expected<std::unique_ptr<DirectoryWatcher>> DW = DirectoryWatcher::create(" to make it more clear that DirectoryWatcher::create is returning an llvm::Expected (along wit a comment to make it extra clear).
Details
- Reviewers
jkorous compnerd gribozavr - Commits
- rG1dcf216f9fa6: [clang][DirectoryWatcher][NFC] Swapping asserts for llvm fatal_error in create
rC368108: [clang][DirectoryWatcher][NFC] Swapping asserts for llvm fatal_error in create
rL368108: [clang][DirectoryWatcher][NFC] Swapping asserts for llvm fatal_error in create
Diff Detail
- Repository
- rL LLVM
Event Timeline
clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp | ||
---|---|---|
323 ↗ | (On Diff #213716) | Call llvm::cantFail and there will be no need to explain anything. |
clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp | ||
---|---|---|
323 ↗ | (On Diff #213716) | 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. To be clear, wrapping llvm::Expected in cantFail goes against the entire point of why I added this more robust error handling. |
clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp | ||
---|---|---|
323 ↗ | (On Diff #213716) | 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 don't understand. It is not propagating anything up, it is just crashing in the destructor. |
clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp | ||
---|---|---|
323 ↗ | (On Diff #213716) | 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. |
clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp | ||
---|---|---|
323 ↗ | (On Diff #213716) | For the purposes of the test, the call can't fail. If the call fails, the test fails. How about ASSERT_TRUE(DW)? |
clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp | ||
---|---|---|
323 ↗ | (On Diff #213716) | 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. |
@gribozavr I think that this usage here is actually useful because it
a) tests the actual behaviour
b) provides example code for other users
The check here ensures that the rest of the code is properly executed, *but* because the error is not actually consumed (you need to explicitly invoke .takeError()), llvm::Expected will actually tell you that a fatal error has occurred at this point and what the error was. It provides a much clearer error message than the explicit llvm_unreachable IMO.
clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp | ||
---|---|---|
323 ↗ | (On Diff #213716) |
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. |
I think the right line is:
logAllUnhandledErrors(DW.takeError(), errs(), "");
It's a bit wordy, but will log a sensible error and works with and without ENABLE_ABI_BREAKING_CHECKS enabled.
I'm not against improving the output for cantFail, nor totally against abusing cantFail in unit tests, but Puyan's right that it would be abuse: in production code cantFail should only be used at call sites where you know from context that the call definitely can't fail (e.g. subsequent calls to an API that can only fail on the first call).
Only downside to using logAllUnhandledErrors over just letting the llvm::Expected's destructor handle the error print out is that logAllUnhandledErrors only logs the errors. It doesn't crash the rest of the deadlocking DirectoryWatcher test. So I had to include logAllUnhandledErrors along with exit(EXIT_FAILURE).
Anyhow, it seems we have consensus so I will land this.