From: Adam Simpkins Date: Thu, 22 Jun 2017 18:44:38 +0000 (-0700) Subject: logging: fix issues detecting XLOG(FATAL) statements as noreturn X-Git-Tag: v2017.06.26.00~16 X-Git-Url: http://plrg.eecs.uci.edu/git/?p=folly.git;a=commitdiff_plain;h=61a0ef5508c64a7e05c574b5314a78a07d2f88dd logging: fix issues detecting XLOG(FATAL) statements as noreturn Summary: Update the FB_LOG() and XLOG() macros so that FATAL log messages are correctly detected as not returning, by both clang and gcc. We have to ensure that both sides of the log statement check (log message enabled or disabled) evaluate to `[[noreturn]]` expressions. I did try updating the log check itself so that it could be constexpr detected as always passing, but this was not sufficient. Reviewed By: wez Differential Revision: D5290780 fbshipit-source-id: 773a56a8392dfd7c310d5d84fc9311e66edf99cb --- diff --git a/folly/experimental/logging/LogCategory.cpp b/folly/experimental/logging/LogCategory.cpp index c794c748..eceea871 100644 --- a/folly/experimental/logging/LogCategory.cpp +++ b/folly/experimental/logging/LogCategory.cpp @@ -155,8 +155,13 @@ void LogCategory::setLevelLocked(LogLevel level, bool inherit) { // // This makes sure that UNINITIALIZED is always less than any valid level // value, and that level values cannot conflict with our flag bits. - if (level > LogLevel::MAX_LEVEL) { - level = LogLevel::MAX_LEVEL; + // + // In debug builds we clamp the maximum to DFATAL rather than MAX_LEVEL + // (FATAL), to ensure that fatal log messages can never be disabled. + constexpr LogLevel maxLogLevel = + kIsDebug ? LogLevel::DFATAL : LogLevel::MAX_LEVEL; + if (level > maxLogLevel) { + level = maxLogLevel; } else if (level < LogLevel::MIN_LEVEL) { level = LogLevel::MIN_LEVEL; } diff --git a/folly/experimental/logging/LogStreamProcessor.cpp b/folly/experimental/logging/LogStreamProcessor.cpp index ae5d8246..2415ed4a 100644 --- a/folly/experimental/logging/LogStreamProcessor.cpp +++ b/folly/experimental/logging/LogStreamProcessor.cpp @@ -175,6 +175,19 @@ void LogStreamProcessor::logNow() noexcept { extractMessageString(stream_)}); } +std::string LogStreamProcessor::extractMessageString( + LogStream& stream) noexcept { + if (stream.empty()) { + return std::move(message_); + } + + if (message_.empty()) { + return stream.extractString(); + } + message_.append(stream.extractString()); + return std::move(message_); +} + void LogStreamVoidify::operator&(std::ostream& stream) { // Non-fatal log messages wait until the LogStreamProcessor destructor to log // the message. However for fatal messages we log immediately in the & @@ -189,16 +202,11 @@ void LogStreamVoidify::operator&(std::ostream& stream) { abort(); } -std::string LogStreamProcessor::extractMessageString( - LogStream& stream) noexcept { - if (stream.empty()) { - return std::move(message_); - } - - if (message_.empty()) { - return stream.extractString(); - } - message_.append(stream.extractString()); - return std::move(message_); +void logDisabledHelper(std::integral_constant) noexcept { + // This function can only be reached if we had a disabled fatal log message. + // This should never happen: LogCategory::setLevelLocked() does not allow + // setting the threshold for a category lower than FATAL (in production + // builds) or DFATAL (in debug builds). + abort(); } } diff --git a/folly/experimental/logging/LogStreamProcessor.h b/folly/experimental/logging/LogStreamProcessor.h index 21d45947..f5fec5e2 100644 --- a/folly/experimental/logging/LogStreamProcessor.h +++ b/folly/experimental/logging/LogStreamProcessor.h @@ -463,4 +463,34 @@ class LogStreamVoidify { */ [[noreturn]] void operator&(std::ostream&); }; + +/** + * logDisabledHelper() is invoked in FB_LOG() and XLOG() statements if the log + * admittance check fails. + * + * This function exists solely to ensure that both sides of the log check are + * marked [[noreturn]] for fatal log messages. This allows the compiler to + * recognize that the full statement is noreturn, preventing warnings about + * missing return statements after fatal log messages. + * + * Unfortunately it does not appear possible to get the compiler to recognize + * that the disabled side of the log statement should never be reached for + * fatal messages. Even if we make the check something like + * `(isLogLevelFatal(level) || realCheck)`, where isLogLevelFatal() is + * constexpr, this is not sufficient for gcc or clang to recognize that the + * full expression is noreturn. + * + * Ideally this would just be a template function specialized on a boolean + * IsFatal parameter. Unfortunately this triggers a bug in clang, which does + * not like differing noreturn behavior for different template instantiations. + * Therefore we overload on integral_constant instead. + * + * clang-format also doesn't do a good job understanding this code and figuring + * out how to format it. + */ +// clang-format off +inline void logDisabledHelper(std::integral_constant) noexcept {} +[[noreturn]] void logDisabledHelper( + std::integral_constant) noexcept; +// clang-format on } diff --git a/folly/experimental/logging/Logger.h b/folly/experimental/logging/Logger.h index ed346470..36a8018e 100644 --- a/folly/experimental/logging/Logger.h +++ b/folly/experimental/logging/Logger.h @@ -27,16 +27,17 @@ * * This macro generally should not be used directly by end users. */ -#define FB_LOG_IMPL(logger, level, type, ...) \ - (!(logger).getCategory()->logCheck(level)) \ - ? (void)0 \ - : ::folly::LogStreamVoidify<::folly::isLogLevelFatal(level)>{} & \ - ::folly::LogStreamProcessor{(logger).getCategory(), \ - (level), \ - __FILE__, \ - __LINE__, \ - (type), \ - ##__VA_ARGS__} \ +#define FB_LOG_IMPL(logger, level, type, ...) \ + (!(logger).getCategory()->logCheck(level)) \ + ? ::folly::logDisabledHelper( \ + std::integral_constant{}) \ + : ::folly::LogStreamVoidify<::folly::isLogLevelFatal(level)>{} & \ + ::folly::LogStreamProcessor{(logger).getCategory(), \ + (level), \ + __FILE__, \ + __LINE__, \ + (type), \ + ##__VA_ARGS__} \ .stream() /** diff --git a/folly/experimental/logging/test/FatalHelper.cpp b/folly/experimental/logging/test/FatalHelper.cpp index dbbaa4c2..7b804c90 100644 --- a/folly/experimental/logging/test/FatalHelper.cpp +++ b/folly/experimental/logging/test/FatalHelper.cpp @@ -59,13 +59,8 @@ class InitChecker { static InitChecker initChecker; } -/* - * This is a simple helper program to exercise the LOG(FATAL) functionality. - */ -int main(int argc, char* argv[]) { - // Call folly::init() and then initialize log levels and handlers - folly::init(&argc, &argv); - +namespace { +int runHelper() { if (FLAGS_handler_style == "async") { initLoggingGlogStyle(FLAGS_logging, LogLevel::INFO, true); } else if (FLAGS_handler_style == "immediate") { @@ -84,7 +79,31 @@ int main(int argc, char* argv[]) { } XLOG(FATAL) << "test program crashing!"; - // Even though main() is defined to return an integer, the compiler + // Even though this function is defined to return an integer, the compiler // should be able to detect that XLOG(FATAL) never returns. It shouldn't // complain that we don't return an integer here. } +} + +std::string fbLogFatalCheck() { + folly::Logger logger("some.category"); + FB_LOG(logger, FATAL) << "we always crash"; + // This function mostly exists to make sure the compiler does not warn + // about a missing return statement here. +} + +/* + * This is a simple helper program to exercise the LOG(FATAL) functionality. + */ +int main(int argc, char* argv[]) { + // Call folly::init() and then initialize log levels and handlers + folly::init(&argc, &argv); + + // Do most of the work in a separate helper function. + // + // The main reason for putting this in a helper function is to ensure that + // the compiler does not warn about missing return statements on XLOG(FATAL) + // code paths. Unfortunately it appears like some compilers always suppress + // this warning for main(). + return runHelper(); +} diff --git a/folly/experimental/logging/xlog.h b/folly/experimental/logging/xlog.h index ca229af5..686bb1c7 100644 --- a/folly/experimental/logging/xlog.h +++ b/folly/experimental/logging/xlog.h @@ -112,28 +112,31 @@ * initialized. On all subsequent calls, disabled log statements can be * skipped with just a single check of the LogLevel. */ -#define XLOG_IMPL(level, type, ...) \ - (!XLOG_IS_ON_IMPL(level)) \ - ? static_cast(0) \ - : ::folly::LogStreamVoidify<::folly::isLogLevelFatal(level)>{} & \ - ::folly::LogStreamProcessor( \ - [] { \ - static ::folly::XlogCategoryInfo \ - _xlogCategory_; \ - return _xlogCategory_.getInfo( \ - &xlog_detail::xlogFileScopeInfo); \ - }(), \ - (level), \ - xlog_detail::getXlogCategoryName(__FILE__, 0), \ - xlog_detail::isXlogCategoryOverridden(0), \ - __FILE__, \ - __LINE__, \ - (type), \ - ##__VA_ARGS__) \ +#define XLOG_IMPL(level, type, ...) \ + (!XLOG_IS_ON_IMPL(level)) \ + ? ::folly::logDisabledHelper( \ + std::integral_constant{}) \ + : ::folly::LogStreamVoidify<::folly::isLogLevelFatal(level)>{} & \ + ::folly::LogStreamProcessor( \ + [] { \ + static ::folly::XlogCategoryInfo \ + _xlogCategory_; \ + return _xlogCategory_.getInfo( \ + &xlog_detail::xlogFileScopeInfo); \ + }(), \ + (level), \ + xlog_detail::getXlogCategoryName(__FILE__, 0), \ + xlog_detail::isXlogCategoryOverridden(0), \ + __FILE__, \ + __LINE__, \ + (type), \ + ##__VA_ARGS__) \ .stream() /** * Check if and XLOG() statement with the given log level would be enabled. + * + * The level parameter must be an unqualified LogLevel enum value. */ #define XLOG_IS_ON(level) XLOG_IS_ON_IMPL(::folly::LogLevel::level)