From d000b0a32ee7924d151ea1fd89a82c5855dc094e Mon Sep 17 00:00:00 2001 From: Jonas Olsson Date: Tue, 3 Jul 2018 12:07:53 +0200 Subject: [PATCH] Move RTC_CHECK_OP error message construction out of header file. This simplifies the logic, prevents emitting code for every pair of argument types to RTC_CHECK_OP and partially unblocks removing streams from the check code altogether. Bug: webrtc:8982 Change-Id: Ib6652ac9a342e4471c12574a79872833cc943407 Reviewed-on: https://webrtc-review.googlesource.com/86544 Commit-Queue: Jonas Olsson Reviewed-by: Karl Wiberg Cr-Commit-Position: refs/heads/master@{#23821} --- rtc_base/checks.cc | 125 +++++++++++++++++++---------------- rtc_base/checks.h | 122 ++++++++++++---------------------- rtc_base/logging_unittest.cc | 24 +++---- 3 files changed, 122 insertions(+), 149 deletions(-) diff --git a/rtc_base/checks.cc b/rtc_base/checks.cc index 462bef8275..32ac8d6e8b 100644 --- a/rtc_base/checks.cc +++ b/rtc_base/checks.cc @@ -36,22 +36,58 @@ #include "rtc_base/checks.h" namespace rtc { - -// 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 webrtc_checks_impl { + +// Reads one argument from args, appends it to s and advances fmt. +// Returns true iff an argument was sucessfully parsed. +bool ParseArg(va_list* args, + const CheckArgType** fmt, + std::ostream& s) { // no-presubmit-check TODO(webrtc:8982) + if (**fmt == CheckArgType::kEnd) + return false; + + switch (**fmt) { + case CheckArgType::kInt: + s << va_arg(*args, int); + break; + case CheckArgType::kLong: + s << va_arg(*args, long); + break; + case CheckArgType::kLongLong: + s << va_arg(*args, long long); + break; + case CheckArgType::kUInt: + s << va_arg(*args, unsigned); + break; + case CheckArgType::kULong: + s << va_arg(*args, unsigned long); + break; + case CheckArgType::kULongLong: + s << va_arg(*args, unsigned long long); + break; + case CheckArgType::kDouble: + s << va_arg(*args, double); + break; + case CheckArgType::kLongDouble: + s << va_arg(*args, long double); + break; + case CheckArgType::kCharP: + s << va_arg(*args, const char*); + break; + case CheckArgType::kStdString: + s << *va_arg(*args, const std::string*); + break; + case CheckArgType::kVoidP: + s << reinterpret_cast(va_arg(*args, const void*)); + break; + default: + s << "[Invalid CheckArgType:" << static_cast(**fmt) << "]"; + return false; + } + (*fmt)++; + return true; +} + RTC_NORETURN void FatalLog(const char* file, int line, const char* message, @@ -62,50 +98,25 @@ RTC_NORETURN void FatalLog(const char* file, std::ostringstream ss; // no-presubmit-check TODO(webrtc:8982) ss << "\n\n#\n# Fatal error in: " << file << ", line " << line - << "\n# last system error: " << LAST_SYSTEM_ERROR - << "\n# Check failed: " << message << "\n# "; + << "\n# last system error: " << LAST_SYSTEM_ERROR << "\n# Check failed: "; - for (; *fmt != CheckArgType::kEnd; ++fmt) { - switch (*fmt) { - case CheckArgType::kInt: - ss << va_arg(args, int); - break; - case CheckArgType::kLong: - ss << va_arg(args, long); - break; - case CheckArgType::kLongLong: - ss << va_arg(args, long long); - break; - case CheckArgType::kUInt: - ss << va_arg(args, unsigned); - break; - case CheckArgType::kULong: - ss << va_arg(args, unsigned long); - break; - case CheckArgType::kULongLong: - ss << va_arg(args, unsigned long long); - break; - case CheckArgType::kDouble: - ss << va_arg(args, double); - break; - case CheckArgType::kLongDouble: - ss << va_arg(args, long double); - break; - case CheckArgType::kCharP: - ss << va_arg(args, const char*); - break; - case CheckArgType::kStdString: - ss << *va_arg(args, const std::string*); - break; - case CheckArgType::kVoidP: - ss << va_arg(args, const void*); - break; - default: - ss << "[Invalid CheckArgType:" << static_cast(*fmt) << "]"; - goto processing_loop_end; - } + if (*fmt == CheckArgType::kCheckOp) { + // This log message was generated by RTC_CHECK_OP, so we have to complete + // the error message using the operands that have been passed as the first + // two arguments. + fmt++; + + std::ostringstream s1, s2; // no-presubmit-check TODO(webrtc:8982) + if (ParseArg(&args, &fmt, s1) && ParseArg(&args, &fmt, s2)) + ss << message << " (" << s1.str() << " vs. " << s2.str() << ")\n# "; + } else { + ss << message << "\n# "; } -processing_loop_end: + + // Append all the user-supplied arguments to the message. + while (ParseArg(&args, &fmt, ss)) + ; + va_end(args); std::string s = ss.str(); diff --git a/rtc_base/checks.h b/rtc_base/checks.h index b37df0daba..a65088d3fd 100644 --- a/rtc_base/checks.h +++ b/rtc_base/checks.h @@ -40,7 +40,7 @@ RTC_NORETURN void rtc_FatalMessage(const char* file, int line, const char* msg); #ifdef __cplusplus // C++ version. -#include +#include // no-presubmit-check TODO(webrtc:8982) #include #include "rtc_base/numerics/safe_compare.h" @@ -101,6 +101,12 @@ enum class CheckArgType : int8_t { kCharP, kStdString, kVoidP, + + // kCheckOp doesn't represent an argument type. Instead, it is sent as the + // first argument from RTC_CHECK_OP to make FatalLog use the next two + // arguments to build the special CHECK_OP error message + // (the "a == b (1 vs. 2)" bit). + kCheckOp, }; RTC_NORETURN void FatalLog(const char* file, @@ -191,6 +197,16 @@ class LogStreamer<> final { static constexpr CheckArgType t[] = {Us::Type()..., CheckArgType::kEnd}; FatalLog(file, line, message, t, args.GetVal()...); } + + template + RTC_NORETURN RTC_FORCE_INLINE static void CallCheckOp(const char* file, + const int line, + const char* message, + const Us&... args) { + static constexpr CheckArgType t[] = {CheckArgType::kCheckOp, Us::Type()..., + CheckArgType::kEnd}; + FatalLog(file, line, message, t, args.GetVal()...); + } }; // Inductive case: We've already seen at least one << argument. The most recent @@ -227,6 +243,14 @@ class LogStreamer final { prior_->Call(file, line, message, arg_, args...); } + template + RTC_NORETURN RTC_FORCE_INLINE void CallCheckOp(const char* file, + const int line, + const char* message, + const Us&... args) const { + prior_->CallCheckOp(file, line, message, arg_, args...); + } + private: // The most recent argument. T arg_; @@ -235,6 +259,7 @@ class LogStreamer final { const LogStreamer* prior_; }; +template class FatalLogCall final { public: FatalLogCall(const char* file, int line, const char* message) @@ -244,7 +269,8 @@ class FatalLogCall final { template RTC_NORETURN RTC_FORCE_INLINE void operator&( const LogStreamer& streamer) { - streamer.Call(file_, line_, message_); + isCheckOp ? streamer.CallCheckOp(file_, line_, message_) + : streamer.Call(file_, line_, message_); } private: @@ -259,10 +285,10 @@ class FatalLogCall final { // in a particularly convoluted way with an extra ?: because that appears to be // the simplest construct that keeps Visual Studio from complaining about // condition being unused). -#define RTC_EAT_STREAM_PARAMETERS(ignored) \ - (true ? true : ((void)(ignored), true)) \ - ? static_cast(0) \ - : rtc::webrtc_checks_impl::FatalLogCall("", 0, "") & \ +#define RTC_EAT_STREAM_PARAMETERS(ignored) \ + (true ? true : ((void)(ignored), true)) \ + ? static_cast(0) \ + : rtc::webrtc_checks_impl::FatalLogCall("", 0, "") & \ rtc::webrtc_checks_impl::LogStreamer<>() // Call RTC_EAT_STREAM_PARAMETERS with an argument that fails to compile if @@ -277,80 +303,19 @@ class FatalLogCall final { // // We make sure RTC_CHECK et al. always evaluates |condition|, as // doing RTC_CHECK(FunctionWithSideEffect()) is a common idiom. -#define RTC_CHECK(condition) \ - while (!(condition)) \ - rtc::webrtc_checks_impl::FatalLogCall(__FILE__, __LINE__, #condition) & \ +#define RTC_CHECK(condition) \ + while (!(condition)) \ + rtc::webrtc_checks_impl::FatalLogCall(__FILE__, __LINE__, \ + #condition) & \ rtc::webrtc_checks_impl::LogStreamer<>() // Helper macro for binary operators. // Don't use this macro directly in your code, use RTC_CHECK_EQ et al below. -// RTC_CHECK_EQ(...) else { ... } work properly. -#define RTC_CHECK_OP(name, op, val1, val2) \ - while (std::string* _result = \ - rtc::Check##name##Impl((val1), (val2), #val1 " " #op " " #val2)) \ - rtc::webrtc_checks_impl::FatalLogCall(__FILE__, __LINE__, \ - _result->c_str()) & \ - rtc::webrtc_checks_impl::LogStreamer<>() - -// 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) { - // TODO(jonasolsson): Use absl::StrCat() instead. - 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 RTC_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_RTC_CHECK_OP_IMPL(name) \ - template \ - inline std::string* Check##name##Impl(const t1& v1, const t2& v2, \ - const char* names) { \ - if (rtc::Safe##name(v1, v2)) \ - return nullptr; \ - else \ - return rtc::MakeCheckOpString(v1, v2, names); \ - } \ - inline std::string* Check##name##Impl(int v1, int v2, const char* names) { \ - if (rtc::Safe##name(v1, v2)) \ - return nullptr; \ - else \ - return rtc::MakeCheckOpString(v1, v2, names); \ - } -DEFINE_RTC_CHECK_OP_IMPL(Eq) -DEFINE_RTC_CHECK_OP_IMPL(Ne) -DEFINE_RTC_CHECK_OP_IMPL(Le) -DEFINE_RTC_CHECK_OP_IMPL(Lt) -DEFINE_RTC_CHECK_OP_IMPL(Ge) -DEFINE_RTC_CHECK_OP_IMPL(Gt) -#undef DEFINE_RTC_CHECK_OP_IMPL +#define RTC_CHECK_OP(name, op, val1, val2) \ + while (!rtc::Safe##name((val1), (val2))) \ + rtc::webrtc_checks_impl::FatalLogCall(__FILE__, __LINE__, \ + #val1 " " #op " " #val2) & \ + rtc::webrtc_checks_impl::LogStreamer<>() << (val1) << (val2) #define RTC_CHECK_EQ(val1, val2) RTC_CHECK_OP(Eq, ==, val1, val2) #define RTC_CHECK_NE(val1, val2) RTC_CHECK_OP(Ne, !=, val1, val2) @@ -384,8 +349,9 @@ DEFINE_RTC_CHECK_OP_IMPL(Gt) #define RTC_NOTREACHED() RTC_DCHECK(RTC_UNREACHABLE_CODE_HIT) // TODO(bugs.webrtc.org/8454): Add an RTC_ prefix or rename differently. -#define FATAL() \ - rtc::webrtc_checks_impl::FatalLogCall(__FILE__, __LINE__, "FATAL()") & \ +#define FATAL() \ + rtc::webrtc_checks_impl::FatalLogCall(__FILE__, __LINE__, \ + "FATAL()") & \ rtc::webrtc_checks_impl::LogStreamer<>() // Performs the integer division a/b and returns the result. CHECKs that the diff --git a/rtc_base/logging_unittest.cc b/rtc_base/logging_unittest.cc index 158f204685..af512129fb 100644 --- a/rtc_base/logging_unittest.cc +++ b/rtc_base/logging_unittest.cc @@ -194,38 +194,34 @@ TEST(LogTest, SingleStream) { EXPECT_EQ(sev, LogMessage::GetLogToStream(nullptr)); } -/* #if GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) TEST(LogTest, Checks) { EXPECT_DEATH(FATAL() << "message", "\n\n#\n" - "# Fatal error in: \\S+, line \\d+\n" - "# last system error: \\d+\n" + "# Fatal error in: \\S+, line \\w+\n" + "# last system error: \\w+\n" "# Check failed: FATAL\\(\\)\n" - "# message" - ); + "# message"); int a = 1, b = 2; EXPECT_DEATH(RTC_CHECK_EQ(a, b) << 1 << 2u, "\n\n#\n" - "# Fatal error in: \\S+, line \\d+\n" - "# last system error: \\d+\n" + "# Fatal error in: \\S+, line \\w+\n" + "# last system error: \\w+\n" "# Check failed: a == b \\(1 vs. 2\\)\n" - "# 12" - ); + "# 12"); RTC_CHECK_EQ(5, 5); RTC_CHECK(true) << "Shouldn't crash" << 1; EXPECT_DEATH(RTC_CHECK(false) << "Hi there!", "\n\n#\n" - "# Fatal error in: \\S+, line \\d+\n" - "# last system error: \\d+\n" + "# Fatal error in: \\S+, line \\w+\n" + "# last system error: \\w+\n" "# Check failed: false\n" - "# Hi there!" - ); + "# Hi there!"); } #endif -*/ + // Test using multiple log streams. The INFO stream should get the INFO message, // the VERBOSE stream should get the INFO and the VERBOSE. // We should restore the correct global state at the end.