From a5b7869f3d9887d67a17f9607dcc0922c7d2b278 Mon Sep 17 00:00:00 2001 From: "andrew@webrtc.org" Date: Thu, 28 Aug 2014 16:28:26 +0000 Subject: [PATCH] Add CHECK and friends from Chromium. Replace FATAL_ERROR_IF with the more familiar (to Chromium developers) CHECK and DCHECK. The full Chromium implementation is fairly elaborate but I copied enough to get us most of the benefits. I believe the main missing component is a more advanced stack dump. For this bit I relied on the V8 implementation. There are a few minor modifications from the Chromium original: - The FatalMessage class is specialized for logging fatal error messages and aborting. Chromium uses the general LogMessage class, which we could consider moving towards in the future. - NOTIMPLEMENTED() and NOTREACHED() have been removed, partly because I don't want to rely on our logging.h until base/ and system_wrappers/ are consolidated. - FATAL() replaces LOG(FATAL). Minor modifications from V8's stack dump: - If parsing of a stack trace symbol fails, just print the unparsed symbol. (I noticed this happened on Mac.) - Use __GLIBCXX__ and __UCLIBC__. This is from examining the backtrace use in Chromium. UNREACHABLE() has been removed because its behavior is different than Chromium's NOTREACHED(), which is bound to cause confusion. The few uses were replaced with FATAL(), matching the previous behavior. Add a NO_RETURN macro, allowing us to remove unreachable return statements following a CHECK/FATAL. TESTED=the addition of dummy CHECK, DCHECK, CHECK_EQ and FATAL did the did the right things. Stack traces work on Mac, but I don't get symbols on Linux. R=henrik.lundin@webrtc.org, kwiberg@webrtc.org, tommi@webrtc.org Review URL: https://webrtc-codereview.appspot.com/22449004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@7003 4adac7df-926f-26a2-2b94-8c16560cd09d --- webrtc/base/checks.cc | 119 +++++++++-- webrtc/base/checks.h | 187 ++++++++++++++++-- webrtc/base/flags.cc | 15 +- webrtc/base/opensslidentity.cc | 12 +- webrtc/common_audio/wav_writer.cc | 36 ++-- webrtc/common_audio/wav_writer.h | 2 +- .../audio_coding/main/acm2/acm_send_test.cc | 9 +- webrtc/typedefs.h | 9 + 8 files changed, 318 insertions(+), 71 deletions(-) diff --git a/webrtc/base/checks.cc b/webrtc/base/checks.cc index 0f67c7649..99d8877b1 100644 --- a/webrtc/base/checks.cc +++ b/webrtc/base/checks.cc @@ -8,27 +8,122 @@ * be found in the AUTHORS file in the root of the source tree. */ -#include -#include -#include +// Most of this was borrowed (with minor modifications) from V8's and Chromium's +// src/base/logging.cc. + +// Use the C++ version to provide __GLIBCXX__. +#include + +#if defined(__GLIBCXX__) && !defined(__UCLIBC__) +#include +#include +#endif + +#if defined(ANDROID) +#define LOG_TAG "rtc" +#include // NOLINT +#endif + +#include #include "webrtc/base/checks.h" +#include "webrtc/base/common.h" #include "webrtc/base/logging.h" +#if defined(_MSC_VER) +// Warning C4722: destructor never returns, potential memory leak. +// FatalMessage's dtor very intentionally aborts. +#pragma warning(disable:4722) +#endif + namespace rtc { -void Fatal(const char* file, int line, const char* format, ...) { - char msg[256]; +void VPrintError(const char* format, va_list args) { +#if defined(WEBRTC_ANDROID) + __android_log_vprint(ANDROID_LOG_ERROR, LOG_TAG, format, args); +#else + vfprintf(stderr, format, args); +#endif +} - va_list arguments; - va_start(arguments, format); - vsnprintf(msg, sizeof(msg), format, arguments); - va_end(arguments); +void PrintError(const char* format, ...) { + va_list args; + va_start(args, format); + VPrintError(format, args); + va_end(args); +} - LOG(LS_ERROR) << "\n\n#\n# Fatal error in " << file - << ", line " << line << "\n# " << msg - << "\n#\n"; +// TODO(ajm): This works on Mac (although the parsing fails) but I don't seem +// to get usable symbols on Linux. This is copied from V8. Chromium has a more +// advanced stace trace system; also more difficult to copy. +void DumpBacktrace() { +#if defined(__GLIBCXX__) && !defined(__UCLIBC__) + void* trace[100]; + int size = backtrace(trace, ARRAY_SIZE(trace)); + char** symbols = backtrace_symbols(trace, size); + PrintError("\n==== C stack trace ===============================\n\n"); + if (size == 0) { + PrintError("(empty)\n"); + } else if (symbols == NULL) { + PrintError("(no symbols)\n"); + } else { + for (int i = 1; i < size; ++i) { + char mangled[201]; + if (sscanf(symbols[i], "%*[^(]%*[(]%200[^)+]", mangled) == 1) { // NOLINT + PrintError("%2d: ", i); + int status; + size_t length; + char* demangled = abi::__cxa_demangle(mangled, NULL, &length, &status); + PrintError("%s\n", demangled != NULL ? demangled : mangled); + free(demangled); + } else { + // If parsing failed, at least print the unparsed symbol. + PrintError("%s\n", symbols[i]); + } + } + } + free(symbols); +#endif +} + +FatalMessage::FatalMessage(const char* file, int line) { + Init(file, line); +} + +FatalMessage::FatalMessage(const char* file, int line, std::string* result) { + Init(file, line); + stream_ << "Check failed: " << *result << std::endl << "# "; + delete result; +} + +NO_RETURN FatalMessage::~FatalMessage() { + fflush(stdout); + fflush(stderr); + stream_ << std::endl << "#" << std::endl; + PrintError(stream_.str().c_str()); + DumpBacktrace(); + fflush(stderr); abort(); } +void FatalMessage::Init(const char* file, int line) { + stream_ << std::endl << std::endl << "#" << std::endl << "# Fatal error in " + << file << ", line " << line << std::endl << "# "; +} + +// MSVC doesn't like complex extern templates and DLLs. +#if !defined(COMPILER_MSVC) +// Explicit instantiations for commonly used comparisons. +template std::string* MakeCheckOpString( + const int&, const int&, const char* names); +template std::string* MakeCheckOpString( + const unsigned long&, const unsigned long&, const char* names); +template std::string* MakeCheckOpString( + const unsigned long&, const unsigned int&, const char* names); +template std::string* MakeCheckOpString( + const unsigned int&, const unsigned long&, const char* names); +template std::string* MakeCheckOpString( + const std::string&, const std::string&, const char* name); +#endif + } // namespace rtc diff --git a/webrtc/base/checks.h b/webrtc/base/checks.h index b85b50ae3..9f8122e16 100644 --- a/webrtc/base/checks.h +++ b/webrtc/base/checks.h @@ -8,30 +8,181 @@ * be found in the AUTHORS file in the root of the source tree. */ -// This module contains some basic debugging facilities. -// Originally comes from shared/commandlineflags/checks.h - #ifndef WEBRTC_BASE_CHECKS_H_ #define WEBRTC_BASE_CHECKS_H_ +#include +#include + +#include + +#include "webrtc/base/logging.h" +#include "webrtc/typedefs.h" + +// The macros here print a message to stderr and abort under various +// conditions. All will accept additional stream messages. For example: +// DCHECK_EQ(foo, bar) << "I'm printed when foo != bar."; +// +// - CHECK(x) is an assertion that x is always true, and that if it isn't, it's +// better to terminate the process than to continue. During development, the +// reason that it's better to terminate might simply be that the error +// handling code isn't in place yet; in production, the reason might be that +// the author of the code truly believes that x will always be true, but that +// she recognizes that if she is wrong, abrupt and unpleasant process +// termination is still better than carrying on with the assumption violated. +// +// - DCHECK(x) is the same as CHECK(x)---an assertion that x is always +// true---except that x will only be evaluated in debug builds; in production +// builds, x is simply assumed to be true. This is useful if evaluating x is +// expensive and the expected cost of failing to detect the violated +// assumption is acceptable. You should not handle cases where a production +// build fails to spot a violated condition, even those that would result in +// crashes. If the code needs to cope with the error, make it cope, but don't +// call DCHECK; if the condition really can't occur, but you'd sleep better +// at night knowing that the process will suicide instead of carrying on in +// case you were wrong, use CHECK instead of DCHECK. +// +// - CHECK_EQ, _NE, _GT, ..., and DCHECK_EQ, _NE, _GT, ... are specialized +// variants of CHECK and DCHECK that print prettier messages if the condition +// doesn't hold. Prefer them to raw CHECK and DCHECK. +// +// - FATAL() aborts unconditionally. +// +// TODO(ajm): Ideally, this would be combined with webrtc/base/logging.h. + namespace rtc { -// Prints an error message to stderr and aborts execution. -void Fatal(const char* file, int line, const char* format, ...); +// Helper macro which avoids evaluating the arguments to a stream if +// the condition doesn't hold. +#define LAZY_STREAM(stream, condition) \ + !(condition) ? (void) 0 : rtc::LogMessageVoidify() & (stream) + +// The actual stream used isn't important. +#define EAT_STREAM_PARAMETERS \ + true ? (void) 0 : rtc::LogMessageVoidify() & rtc::FatalMessage("", 0).stream() + +// CHECK dies with a fatal error if condition is not true. It is *not* +// controlled by NDEBUG, so the check will be executed regardless of +// compilation mode. +// +// We make sure CHECK et al. always evaluates their arguments, as +// doing CHECK(FunctionWithSideEffect()) is a common idiom. +#define CHECK(condition) \ + LAZY_STREAM(rtc::FatalMessage(__FILE__, __LINE__).stream(), !(condition)) \ + << "Check failed: " #condition << std::endl << "# " + +// Helper macro for binary operators. +// Don't use this macro directly in your code, use CHECK_EQ et al below. +// +// TODO(akalin): Rewrite this so that constructs like if (...) +// CHECK_EQ(...) else { ... } work properly. +#define CHECK_OP(name, op, val1, val2) \ + if (std::string* _result = \ + rtc::Check##name##Impl((val1), (val2), \ + #val1 " " #op " " #val2)) \ + rtc::FatalMessage(__FILE__, __LINE__, _result).stream() + +// Build the error message string. This is separate from the "Impl" +// function template because it is not performance critical and so can +// be out of line, while the "Impl" code should be inline. Caller +// takes ownership of the returned string. +template +std::string* MakeCheckOpString(const t1& v1, const t2& v2, const char* names) { + std::ostringstream ss; + ss << names << " (" << v1 << " vs. " << v2 << ")"; + std::string* msg = new std::string(ss.str()); + return msg; +} + +// MSVC doesn't like complex extern templates and DLLs. +#if !defined(COMPILER_MSVC) +// Commonly used instantiations of MakeCheckOpString<>. Explicitly instantiated +// in logging.cc. +extern template std::string* MakeCheckOpString( + const int&, const int&, const char* names); +extern template +std::string* MakeCheckOpString( + const unsigned long&, const unsigned long&, const char* names); +extern template +std::string* MakeCheckOpString( + const unsigned long&, const unsigned int&, const char* names); +extern template +std::string* MakeCheckOpString( + const unsigned int&, const unsigned long&, const char* names); +extern template +std::string* MakeCheckOpString( + const std::string&, const std::string&, const char* name); +#endif + +// Helper functions for CHECK_OP macro. +// The (int, int) specialization works around the issue that the compiler +// will not instantiate the template version of the function on values of +// unnamed enum type - see comment below. +#define DEFINE_CHECK_OP_IMPL(name, op) \ + template \ + inline std::string* Check##name##Impl(const t1& v1, const t2& v2, \ + const char* names) { \ + if (v1 op v2) return NULL; \ + else return MakeCheckOpString(v1, v2, names); \ + } \ + inline std::string* Check##name##Impl(int v1, int v2, const char* names) { \ + if (v1 op v2) return NULL; \ + else return MakeCheckOpString(v1, v2, names); \ + } +DEFINE_CHECK_OP_IMPL(EQ, ==) +DEFINE_CHECK_OP_IMPL(NE, !=) +DEFINE_CHECK_OP_IMPL(LE, <=) +DEFINE_CHECK_OP_IMPL(LT, < ) +DEFINE_CHECK_OP_IMPL(GE, >=) +DEFINE_CHECK_OP_IMPL(GT, > ) +#undef DEFINE_CHECK_OP_IMPL + +#define CHECK_EQ(val1, val2) CHECK_OP(EQ, ==, val1, val2) +#define CHECK_NE(val1, val2) CHECK_OP(NE, !=, val1, val2) +#define CHECK_LE(val1, val2) CHECK_OP(LE, <=, val1, val2) +#define CHECK_LT(val1, val2) CHECK_OP(LT, < , val1, val2) +#define CHECK_GE(val1, val2) CHECK_OP(GE, >=, val1, val2) +#define CHECK_GT(val1, val2) CHECK_OP(GT, > , val1, val2) + +// The DCHECK macro is equivalent to CHECK except that it only generates code in +// debug builds. +#if !defined(NDEBUG) +#define DCHECK(condition) CHECK(condition) +#define DCHECK_EQ(v1, v2) CHECK_EQ(v1, v2) +#define DCHECK_NE(v1, v2) CHECK_NE(v1, v2) +#define DCHECK_GE(v1, v2) CHECK_GE(v1, v2) +#define DCHECK_LT(v1, v2) CHECK_LT(v1, v2) +#define DCHECK_LE(v1, v2) CHECK_LE(v1, v2) +#else +#define DCHECK(condition) EAT_STREAM_PARAMETERS +#define DCHECK_EQ(v1, v2) EAT_STREAM_PARAMETERS +#define DCHECK_NE(v1, v2) EAT_STREAM_PARAMETERS +#define DCHECK_GE(v1, v2) EAT_STREAM_PARAMETERS +#define DCHECK_LT(v1, v2) EAT_STREAM_PARAMETERS +#define DCHECK_LE(v1, v2) EAT_STREAM_PARAMETERS +#endif + +#define FATAL() rtc::FatalMessage(__FILE__, __LINE__).stream() +// TODO(ajm): Consider adding NOTIMPLEMENTED and NOTREACHED macros when +// base/logging.h and system_wrappers/logging.h are consolidated such that we +// can match the Chromium behavior. + +// Like a stripped-down LogMessage from logging.h, except that it aborts. +class FatalMessage { + public: + FatalMessage(const char* file, int line); + // Used for CHECK_EQ(), etc. Takes ownership of the given string. + FatalMessage(const char* file, int line, std::string* result); + NO_RETURN ~FatalMessage(); + + std::ostream& stream() { return stream_; } + + private: + void Init(const char* file, int line); + + std::ostringstream stream_; +}; } // namespace rtc -// Trigger a fatal error (which aborts the process and prints an error -// message). FATAL_ERROR_IF may seem a lot like assert, but there's a crucial -// difference: it's always "on". This means that it can be used to check for -// regular errors that could actually happen, not just programming errors that -// supposedly can't happen---but triggering a fatal error will kill the process -// in an ugly way, so it's not suitable for catching errors that might happen -// in production. -#define FATAL_ERROR(msg) do { rtc::Fatal(__FILE__, __LINE__, msg); } while (0) -#define FATAL_ERROR_IF(x) do { if (x) FATAL_ERROR("check failed"); } while (0) - -// The UNREACHABLE macro is very useful during development. -#define UNREACHABLE() FATAL_ERROR("unreachable code") - #endif // WEBRTC_BASE_CHECKS_H_ diff --git a/webrtc/base/flags.cc b/webrtc/base/flags.cc index fe7a334a9..a5e1c45e5 100644 --- a/webrtc/base/flags.cc +++ b/webrtc/base/flags.cc @@ -56,7 +56,7 @@ void Flag::SetToDefault() { variable_->s = default_.s; return; } - UNREACHABLE(); + FATAL() << "unreachable code"; } @@ -67,8 +67,7 @@ static const char* Type2String(Flag::Type type) { case Flag::FLOAT: return "float"; case Flag::STRING: return "string"; } - UNREACHABLE(); - return NULL; + FATAL() << "unreachable code"; } @@ -87,7 +86,7 @@ static void PrintFlagValue(Flag::Type type, FlagValue* p) { printf("%s", p->s); return; } - UNREACHABLE(); + FATAL() << "unreachable code"; } @@ -164,8 +163,7 @@ void FlagList::SplitArgument(const char* arg, if (*arg == '=') { // make a copy so we can NUL-terminate flag name int n = static_cast(arg - *name); - if (n >= buffer_size) - Fatal(__FILE__, __LINE__, "CHECK(%s) failed", "n < buffer_size"); + CHECK_LT(n, buffer_size); memcpy(buffer, *name, n * sizeof(char)); buffer[n] = '\0'; *name = buffer; @@ -259,8 +257,7 @@ int FlagList::SetFlagsFromCommandLine(int* argc, const char** argv, void FlagList::Register(Flag* flag) { assert(flag != NULL && strlen(flag->name()) > 0); - if (Lookup(flag->name()) != NULL) - Fatal(flag->file(), 0, "flag %s declared twice", flag->name()); + CHECK(!Lookup(flag->name())) << "flag " << flag->name() << " declared twice"; flag->next_ = list_; list_ = flag; } @@ -294,6 +291,6 @@ WindowsCommandLineArguments::~WindowsCommandLineArguments() { delete[] argv_; } -#endif // WEBRTC_WIN +#endif // WEBRTC_WIN } // namespace rtc diff --git a/webrtc/base/opensslidentity.cc b/webrtc/base/opensslidentity.cc index 915680ce2..30ac6e24f 100644 --- a/webrtc/base/opensslidentity.cc +++ b/webrtc/base/opensslidentity.cc @@ -253,13 +253,11 @@ OpenSSLCertificate::~OpenSSLCertificate() { std::string OpenSSLCertificate::ToPEMString() const { BIO* bio = BIO_new(BIO_s_mem()); if (!bio) { - UNREACHABLE(); - return std::string(); + FATAL() << "unreachable code"; } if (!PEM_write_bio_X509(bio, x509_)) { BIO_free(bio); - UNREACHABLE(); - return std::string(); + FATAL() << "unreachable code"; } BIO_write(bio, "\0", 1); char* buffer; @@ -276,13 +274,11 @@ void OpenSSLCertificate::ToDER(Buffer* der_buffer) const { // Calculates the DER representation of the certificate, from scratch. BIO* bio = BIO_new(BIO_s_mem()); if (!bio) { - UNREACHABLE(); - return; + FATAL() << "unreachable code"; } if (!i2d_X509_bio(bio, x509_)) { BIO_free(bio); - UNREACHABLE(); - return; + FATAL() << "unreachable code"; } char* data; size_t length = BIO_get_mem_data(bio, &data); diff --git a/webrtc/common_audio/wav_writer.cc b/webrtc/common_audio/wav_writer.cc index f3b8118c9..30a220c24 100644 --- a/webrtc/common_audio/wav_writer.cc +++ b/webrtc/common_audio/wav_writer.cc @@ -29,17 +29,17 @@ WavFile::WavFile(const std::string& filename, int sample_rate, int num_channels) num_channels_(num_channels), num_samples_(0), file_handle_(fopen(filename.c_str(), "wb")) { - FATAL_ERROR_IF(!CheckWavParameters(num_channels_, - sample_rate_, - kWavFormat, - kBytesPerSample, - num_samples_)); - FATAL_ERROR_IF(!file_handle_); + CHECK(file_handle_); + CHECK(CheckWavParameters(num_channels_, + sample_rate_, + kWavFormat, + kBytesPerSample, + num_samples_)); // Write a blank placeholder header, since we need to know the total number // of samples before we can fill in the real data. static const uint8_t blank_header[kWavHeaderSize] = {0}; - FATAL_ERROR_IF(fwrite(blank_header, kWavHeaderSize, 1, file_handle_) != 1); + CHECK_EQ(1u, fwrite(blank_header, kWavHeaderSize, 1, file_handle_)); } WavFile::~WavFile() { @@ -52,15 +52,15 @@ void WavFile::WriteSamples(const int16_t* samples, size_t num_samples) { #endif const size_t written = fwrite(samples, sizeof(*samples), num_samples, file_handle_); - FATAL_ERROR_IF(written != num_samples); + CHECK_EQ(num_samples, written); num_samples_ += static_cast(written); - FATAL_ERROR_IF(written > std::numeric_limits::max() || - num_samples_ < written); // detect uint32_t overflow - FATAL_ERROR_IF(!CheckWavParameters(num_channels_, - sample_rate_, - kWavFormat, - kBytesPerSample, - num_samples_)); + CHECK(written <= std::numeric_limits::max() || + num_samples_ >= written); // detect uint32_t overflow + CHECK(CheckWavParameters(num_channels_, + sample_rate_, + kWavFormat, + kBytesPerSample, + num_samples_)); } void WavFile::WriteSamples(const float* samples, size_t num_samples) { @@ -74,12 +74,12 @@ void WavFile::WriteSamples(const float* samples, size_t num_samples) { } void WavFile::Close() { - FATAL_ERROR_IF(fseek(file_handle_, 0, SEEK_SET) != 0); + CHECK_EQ(0, fseek(file_handle_, 0, SEEK_SET)); uint8_t header[kWavHeaderSize]; WriteWavHeader(header, num_channels_, sample_rate_, kWavFormat, kBytesPerSample, num_samples_); - FATAL_ERROR_IF(fwrite(header, kWavHeaderSize, 1, file_handle_) != 1); - FATAL_ERROR_IF(fclose(file_handle_) != 0); + CHECK_EQ(1u, fwrite(header, kWavHeaderSize, 1, file_handle_)); + CHECK_EQ(0, fclose(file_handle_)); file_handle_ = NULL; } diff --git a/webrtc/common_audio/wav_writer.h b/webrtc/common_audio/wav_writer.h index e0582657a..45bcbac54 100644 --- a/webrtc/common_audio/wav_writer.h +++ b/webrtc/common_audio/wav_writer.h @@ -20,7 +20,7 @@ namespace webrtc { // Simple C++ class for writing 16-bit PCM WAV files. All error handling is -// by calls to FATAL_ERROR(), making it unsuitable for anything but debug code. +// by calls to CHECK(), making it unsuitable for anything but debug code. class WavFile { public: // Open a new WAV file for writing. diff --git a/webrtc/modules/audio_coding/main/acm2/acm_send_test.cc b/webrtc/modules/audio_coding/main/acm2/acm_send_test.cc index 67cc9b2b4..cbe2ae5d0 100644 --- a/webrtc/modules/audio_coding/main/acm2/acm_send_test.cc +++ b/webrtc/modules/audio_coding/main/acm2/acm_send_test.cc @@ -50,8 +50,8 @@ bool AcmSendTest::RegisterCodec(const char* payload_name, int channels, int payload_type, int frame_size_samples) { - FATAL_ERROR_IF(AudioCodingModule::Codec( - payload_name, &codec_, sampling_freq_hz, channels) != 0); + CHECK_EQ(0, AudioCodingModule::Codec(payload_name, &codec_, sampling_freq_hz, + channels)); codec_.pltype = payload_type; codec_.pacsize = frame_size_samples; codec_registered_ = (acm_->RegisterSendCodec(codec_) == 0); @@ -73,9 +73,8 @@ Packet* AcmSendTest::NextPacket() { // Insert audio and process until one packet is produced. while (clock_.TimeInMilliseconds() < test_duration_ms_) { clock_.AdvanceTimeMilliseconds(kBlockSizeMs); - FATAL_ERROR_IF( - !audio_source_->Read(input_block_size_samples_, input_frame_.data_)); - FATAL_ERROR_IF(acm_->Add10MsData(input_frame_) != 0); + CHECK(audio_source_->Read(input_block_size_samples_, input_frame_.data_)); + CHECK_EQ(0, acm_->Add10MsData(input_frame_)); input_frame_.timestamp_ += input_block_size_samples_; int32_t encoded_bytes = acm_->Process(); if (encoded_bytes > 0) { diff --git a/webrtc/typedefs.h b/webrtc/typedefs.h index 38cda513b..871f04ecd 100644 --- a/webrtc/typedefs.h +++ b/webrtc/typedefs.h @@ -120,4 +120,13 @@ typedef unsigned __int64 uint64_t; #endif #endif +// Annotate a function that will not return control flow to the caller. +#if defined(_MSC_VER) +#define NO_RETURN __declspec(noreturn) +#elif defined(__GNUC__) +#define NO_RETURN __attribute__((noreturn)) +#else +#define NO_RETURN +#endif + #endif // WEBRTC_TYPEDEFS_H_