From 4ad5b756e4c59bfa50a384c3cd68c36f22622a19 Mon Sep 17 00:00:00 2001 From: Kurtis Rader Date: Tue, 14 Feb 2017 21:09:15 -0800 Subject: [PATCH] more sanity involving fatal errors This folds the "VOMIT_*" family of macros into the assert and DIE family. Another change related to issue #3276. --- src/common.cpp | 42 ++++++++++++++++++------------- src/common.h | 48 ++++++++++++++---------------------- src/env_universal_common.cpp | 2 +- src/function.cpp | 9 +++---- src/iothread.cpp | 21 ++++++++-------- src/reader.cpp | 6 ++--- src/signal.cpp | 4 +-- 7 files changed, 64 insertions(+), 68 deletions(-) diff --git a/src/common.cpp b/src/common.cpp index 6daca1ac2..8e4285f0e 100644 --- a/src/common.cpp +++ b/src/common.cpp @@ -544,16 +544,19 @@ ssize_t read_loop(int fd, void *buff, size_t count) { return result; } +/// Hack to not print error messages in the tests. Do not call this from functions in this module +/// like `debug()`. It is only intended to supress diagnostic noise from testing things like the +/// fish parser where we expect a lot of diagnostic messages due to testing error conditions. bool should_suppress_stderr_for_tests() { - // Hack to not print error messages in the tests. return program_name && !wcscmp(program_name, TESTS_PROGRAM_NAME); } -static bool should_debug(int level) { - if (level > debug_level) return false; - if (should_suppress_stderr_for_tests()) return false; - return true; -} +/// Return true if we should emit a `debug()` message. This used to call +/// `should_suppress_stderr_for_tests()`. It no longer does so because it can suppress legitimate +/// errors we want to see if things go wrong. Too, calling that function is no longer necessary, if +/// it ever was, to suppress unwanted diagnostic output that might confuse people running `make +/// test`. +static bool should_debug(int level) { return level <= debug_level; } static void debug_shared(const wchar_t level, const wcstring &msg) { pid_t current_pid = getpid(); @@ -1708,7 +1711,7 @@ void format_size_safe(char buff[128], unsigned long long sz) { /// the gettimeofday function and will have the same precision as that function. double timef() { struct timeval tv; - VOMIT_ON_FAILURE(gettimeofday(&tv, 0)); + assert_with_errno(gettimeofday(&tv, 0) != -1); // return (double)tv.tv_sec + 0.000001 * tv.tv_usec; return (double)tv.tv_sec + 1e-6 * tv.tv_usec; } @@ -1832,14 +1835,14 @@ void assert_is_locked(void *vmutex, const char *who, const char *caller) { void scoped_lock::lock(void) { assert(!locked); //!OCLINT(multiple unary operator) ASSERT_IS_NOT_FORKED_CHILD(); - VOMIT_ON_FAILURE_NO_ERRNO(pthread_mutex_lock(lock_obj)); + DIE_ON_FAILURE(pthread_mutex_lock(lock_obj)); locked = true; } void scoped_lock::unlock(void) { assert(locked); ASSERT_IS_NOT_FORKED_CHILD(); - VOMIT_ON_FAILURE_NO_ERRNO(pthread_mutex_unlock(lock_obj)); + DIE_ON_FAILURE(pthread_mutex_unlock(lock_obj)); locked = false; } @@ -1856,37 +1859,37 @@ scoped_lock::~scoped_lock() { void scoped_rwlock::lock(void) { assert(!(locked || locked_shared)); //!OCLINT(multiple unary operator) ASSERT_IS_NOT_FORKED_CHILD(); - VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_rdlock(rwlock_obj)); + DIE_ON_FAILURE(pthread_rwlock_rdlock(rwlock_obj)); locked = true; } void scoped_rwlock::unlock(void) { assert(locked); ASSERT_IS_NOT_FORKED_CHILD(); - VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_unlock(rwlock_obj)); + DIE_ON_FAILURE(pthread_rwlock_unlock(rwlock_obj)); locked = false; } void scoped_rwlock::lock_shared(void) { assert(!(locked || locked_shared)); //!OCLINT(multiple unary operator) ASSERT_IS_NOT_FORKED_CHILD(); - VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_wrlock(rwlock_obj)); + DIE_ON_FAILURE(pthread_rwlock_wrlock(rwlock_obj)); locked_shared = true; } void scoped_rwlock::unlock_shared(void) { assert(locked_shared); ASSERT_IS_NOT_FORKED_CHILD(); - VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_unlock(rwlock_obj)); + DIE_ON_FAILURE(pthread_rwlock_unlock(rwlock_obj)); locked_shared = false; } void scoped_rwlock::upgrade(void) { assert(locked_shared); ASSERT_IS_NOT_FORKED_CHILD(); - VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_unlock(rwlock_obj)); + DIE_ON_FAILURE(pthread_rwlock_unlock(rwlock_obj)); locked = false; - VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_wrlock(rwlock_obj)); + DIE_ON_FAILURE(pthread_rwlock_wrlock(rwlock_obj)); locked_shared = true; } @@ -2031,8 +2034,13 @@ void redirect_tty_output() { } /// Display a failed assertion message, dump a stack trace if possible, then die. -[[noreturn]] void __assert(const char *msg, const char *file, size_t line) { - debug(0, L"%s:%zu: failed assertion: %s", file, line, msg); +[[noreturn]] void __assert(const char *msg, const char *file, size_t line, int error) { + if (error) { + debug(0, L"%s:%zu: failed assertion: %s: errno %d (%s)", file, line, msg, error, + strerror(error)); + } else { + debug(0, L"%s:%zu: failed assertion: %s", file, line, msg); + } show_stackframe(L'E', 99, 1); abort(); } diff --git a/src/common.h b/src/common.h index a8deba97e..17b8c7159 100644 --- a/src/common.h +++ b/src/common.h @@ -146,28 +146,6 @@ void __attribute__((noinline)) debug(int level, const char *msg, ...) __attribute__((format(printf, 2, 3))); void __attribute__((noinline)) debug(int level, const wchar_t *msg, ...); -/// Helper macro for errors. -#define VOMIT_ON_FAILURE(a) \ - do { \ - if (0 != (a)) { \ - VOMIT_ABORT(errno, #a); \ - } \ - } while (0) -#define VOMIT_ON_FAILURE_NO_ERRNO(a) \ - do { \ - int err = (a); \ - if (0 != err) { \ - VOMIT_ABORT(err, #a); \ - } \ - } while (0) -#define VOMIT_ABORT(err, str) \ - do { \ - int code = (err); \ - debug(0, "%s failed on line %d in file %s: %d (%s)", str, __LINE__, __FILE__, code, \ - strerror(code)); \ - abort(); \ - } while (0) - /// Exits without invoking destructors (via _exit), useful for code after fork. [[noreturn]] void exit_without_destructors(int code); @@ -234,9 +212,21 @@ extern bool has_working_tty_timestamps; #undef assert #undef __assert //#define assert(e) do {(void)((e) ? ((void)0) : __assert(#e, __FILE__, __LINE__)); } while(false) -#define assert(e) (e) ? ((void)0) : __assert(#e, __FILE__, __LINE__) -#define DIE(msg) __assert(msg, __FILE__, __LINE__) -[[noreturn]] void __assert(const char *msg, const char *file, size_t line); +#define assert(e) (e) ? ((void)0) : __assert(#e, __FILE__, __LINE__, 0) +#define assert_with_errno(e) (e) ? ((void)0) : __assert(#e, __FILE__, __LINE__, errno) +#define DIE(msg) __assert(msg, __FILE__, __LINE__, 0) +#define DIE_WITH_ERRNO(msg) __assert(msg, __FILE__, __LINE__, errno) +/// This macro is meant to be used with functions that return zero on success otherwise return an +/// errno value. Most notably the pthread family of functions which we never expect to fail. +#define DIE_ON_FAILURE(e) \ + do { \ + int status = e; \ + if (status != 0) { \ + __assert(#e, __FILE__, __LINE__, status); \ + } \ + } while (0) + +[[noreturn]] void __assert(const char *msg, const char *file, size_t line, int error); /// Check if signals are blocked. If so, print an error message and return from the function /// performing this check. @@ -513,9 +503,9 @@ void convert_wide_array_to_narrow(const null_terminated_array_t &arr, class mutex_lock_t { public: pthread_mutex_t mutex; - mutex_lock_t() { VOMIT_ON_FAILURE_NO_ERRNO(pthread_mutex_init(&mutex, NULL)); } + mutex_lock_t() { DIE_ON_FAILURE(pthread_mutex_init(&mutex, NULL)); } - ~mutex_lock_t() { VOMIT_ON_FAILURE_NO_ERRNO(pthread_mutex_destroy(&mutex)); } + ~mutex_lock_t() { DIE_ON_FAILURE(pthread_mutex_destroy(&mutex)); } }; // Basic scoped lock class. @@ -544,9 +534,9 @@ class scoped_lock { class rwlock_t { public: pthread_rwlock_t rwlock; - rwlock_t() { VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_init(&rwlock, NULL)); } + rwlock_t() { DIE_ON_FAILURE(pthread_rwlock_init(&rwlock, NULL)); } - ~rwlock_t() { VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_destroy(&rwlock)); } + ~rwlock_t() { DIE_ON_FAILURE(pthread_rwlock_destroy(&rwlock)); } rwlock_t &operator=(const rwlock_t &) = delete; rwlock_t(const rwlock_t &) = delete; diff --git a/src/env_universal_common.cpp b/src/env_universal_common.cpp index 43f919551..0e15540e7 100644 --- a/src/env_universal_common.cpp +++ b/src/env_universal_common.cpp @@ -264,7 +264,7 @@ static bool append_file_entry(fish_message_type_t type, const wcstring &key_in, env_universal_t::env_universal_t(const wcstring &path) : explicit_vars_path(path), tried_renaming(false), last_read_file(kInvalidFileID) { - VOMIT_ON_FAILURE(pthread_mutex_init(&lock, NULL)); + DIE_ON_FAILURE(pthread_mutex_init(&lock, NULL)); } env_universal_t::~env_universal_t() { pthread_mutex_destroy(&lock); } diff --git a/src/function.cpp b/src/function.cpp index b3dc699ea..c61f8bb47 100644 --- a/src/function.cpp +++ b/src/function.cpp @@ -6,7 +6,6 @@ // IWYU pragma: no_include #include -#include #include #include #include @@ -113,10 +112,10 @@ void function_init() { // non-recursive lock but I haven't fully investigated all the call paths (for autoloading // functions, etc.). pthread_mutexattr_t a; - VOMIT_ON_FAILURE(pthread_mutexattr_init(&a)); - VOMIT_ON_FAILURE(pthread_mutexattr_settype(&a, PTHREAD_MUTEX_RECURSIVE)); - VOMIT_ON_FAILURE(pthread_mutex_init(&functions_lock, &a)); - VOMIT_ON_FAILURE(pthread_mutexattr_destroy(&a)); + DIE_ON_FAILURE(pthread_mutexattr_init(&a)); + DIE_ON_FAILURE(pthread_mutexattr_settype(&a, PTHREAD_MUTEX_RECURSIVE)); + DIE_ON_FAILURE(pthread_mutex_init(&functions_lock, &a)); + DIE_ON_FAILURE(pthread_mutexattr_destroy(&a)); } static std::map snapshot_vars(const wcstring_list_t &vars) { diff --git a/src/iothread.cpp b/src/iothread.cpp index f4807dd21..756170f30 100644 --- a/src/iothread.cpp +++ b/src/iothread.cpp @@ -1,6 +1,5 @@ #include "config.h" // IWYU pragma: keep -#include #include #include #include @@ -87,11 +86,11 @@ static void iothread_init(void) { inited = true; // Initialize some locks. - VOMIT_ON_FAILURE(pthread_cond_init(&s_main_thread_performer_cond, NULL)); + DIE_ON_FAILURE(pthread_cond_init(&s_main_thread_performer_cond, NULL)); // Initialize the completion pipes. int pipes[2] = {0, 0}; - VOMIT_ON_FAILURE(pipe(pipes)); + assert_with_errno(pipe(pipes) != -1); s_read_pipe = pipes[0]; s_write_pipe = pipes[1]; @@ -133,7 +132,7 @@ static void *iothread_worker(void *unused) { // Enqueue the result, and tell the main thread about it. enqueue_thread_result(std::move(req)); const char wakeup_byte = IO_SERVICE_RESULT_QUEUE; - VOMIT_ON_FAILURE(!write_loop(s_write_pipe, &wakeup_byte, sizeof wakeup_byte)); + assert_with_errno(write_loop(s_write_pipe, &wakeup_byte, sizeof wakeup_byte) != -1); } } @@ -159,7 +158,7 @@ static void iothread_spawn() { // it. sigset_t new_set, saved_set; sigfillset(&new_set); - VOMIT_ON_FAILURE(pthread_sigmask(SIG_BLOCK, &new_set, &saved_set)); + DIE_ON_FAILURE(pthread_sigmask(SIG_BLOCK, &new_set, &saved_set)); // Spawn a thread. If this fails, it means there's already a bunch of threads; it is very // unlikely that they are all on the verge of exiting, so one is likely to be ready to handle @@ -168,10 +167,10 @@ static void iothread_spawn() { pthread_create(&thread, NULL, iothread_worker, NULL); // We will never join this thread. - VOMIT_ON_FAILURE(pthread_detach(thread)); + DIE_ON_FAILURE(pthread_detach(thread)); debug(5, "pthread %p spawned\n", (void *)(intptr_t)thread); // Restore our sigmask. - VOMIT_ON_FAILURE(pthread_sigmask(SIG_SETMASK, &saved_set, NULL)); + DIE_ON_FAILURE(pthread_sigmask(SIG_SETMASK, &saved_set, NULL)); } int iothread_perform_impl(void_function_t &&func, void_function_t &&completion) { @@ -210,7 +209,7 @@ void iothread_service_completion(void) { ASSERT_IS_MAIN_THREAD(); char wakeup_byte; - VOMIT_ON_FAILURE(1 != read_loop(iothread_port(), &wakeup_byte, sizeof wakeup_byte)); + assert_with_errno(read_loop(iothread_port(), &wakeup_byte, sizeof wakeup_byte) == 1); if (wakeup_byte == IO_SERVICE_MAIN_THREAD_REQUEST_QUEUE) { iothread_service_main_thread_requests(); } else if (wakeup_byte == IO_SERVICE_RESULT_QUEUE) { @@ -296,7 +295,7 @@ static void iothread_service_main_thread_requests(void) { // Because the waiting thread performs step 1 under the lock, if we take the lock, we avoid // posting before the waiting thread is waiting. scoped_lock broadcast_lock(s_main_thread_performer_lock); - VOMIT_ON_FAILURE(pthread_cond_broadcast(&s_main_thread_performer_cond)); + DIE_ON_FAILURE(pthread_cond_broadcast(&s_main_thread_performer_cond)); } } @@ -335,14 +334,14 @@ void iothread_perform_on_main(void_function_t &&func) { // Tell the pipe. const char wakeup_byte = IO_SERVICE_MAIN_THREAD_REQUEST_QUEUE; - VOMIT_ON_FAILURE(!write_loop(s_write_pipe, &wakeup_byte, sizeof wakeup_byte)); + assert_with_errno(write_loop(s_write_pipe, &wakeup_byte, sizeof wakeup_byte) != -1); // Wait on the condition, until we're done. scoped_lock perform_lock(s_main_thread_performer_lock); while (!req.done) { // It would be nice to support checking for cancellation here, but the clients need a // deterministic way to clean up to avoid leaks - VOMIT_ON_FAILURE( + DIE_ON_FAILURE( pthread_cond_wait(&s_main_thread_performer_cond, &s_main_thread_performer_lock)); } diff --git a/src/reader.cpp b/src/reader.cpp index 5c4fe0be6..e24e36d24 100644 --- a/src/reader.cpp +++ b/src/reader.cpp @@ -746,7 +746,7 @@ static void exec_prompt() { } void reader_init() { - VOMIT_ON_FAILURE(pthread_key_create(&generation_count_key, NULL)); + DIE_ON_FAILURE(pthread_key_create(&generation_count_key, NULL)); // Ensure this var is present even before an interactive command is run so that if it is used // in a function like `fish_prompt` or `fish_right_prompt` it is defined at the time the first @@ -1133,7 +1133,7 @@ static std::function get_autosuggestion_performer return nothing; } - VOMIT_ON_FAILURE( + DIE_ON_FAILURE( pthread_setspecific(generation_count_key, (void *)(uintptr_t)generation_count)); // Let's make sure we aren't using the empty string. @@ -2113,7 +2113,7 @@ static std::function get_highlight_performer(const wcs if (text.empty()) { return {}; } - VOMIT_ON_FAILURE( + DIE_ON_FAILURE( pthread_setspecific(generation_count_key, (void *)(uintptr_t)generation_count)); std::vector colors(text.size(), 0); highlight_func(text, colors, match_highlight_pos, NULL /* error */, vars); diff --git a/src/signal.cpp b/src/signal.cpp index aa76e8d33..8fe2bd2d8 100644 --- a/src/signal.cpp +++ b/src/signal.cpp @@ -380,7 +380,7 @@ void signal_block() { if (!block_count) { sigfillset(&chldset); - VOMIT_ON_FAILURE(pthread_sigmask(SIG_BLOCK, &chldset, NULL)); + DIE_ON_FAILURE(pthread_sigmask(SIG_BLOCK, &chldset, NULL)); } block_count++; @@ -403,7 +403,7 @@ void signal_unblock() { if (!block_count) { sigfillset(&chldset); - VOMIT_ON_FAILURE(pthread_sigmask(SIG_UNBLOCK, &chldset, 0)); + DIE_ON_FAILURE(pthread_sigmask(SIG_UNBLOCK, &chldset, 0)); } // debug( 0, L"signal block level decreased to %d", block_count ); }