logging: fix issues detecting XLOG(FATAL) statements as noreturn
authorAdam Simpkins <simpkins@fb.com>
Thu, 22 Jun 2017 18:44:38 +0000 (11:44 -0700)
committerFacebook Github Bot <facebook-github-bot@users.noreply.github.com>
Thu, 22 Jun 2017 18:50:40 +0000 (11:50 -0700)
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

folly/experimental/logging/LogCategory.cpp
folly/experimental/logging/LogStreamProcessor.cpp
folly/experimental/logging/LogStreamProcessor.h
folly/experimental/logging/Logger.h
folly/experimental/logging/test/FatalHelper.cpp
folly/experimental/logging/xlog.h

index c794c74850e833ed6d2bb3fb0f193913b1ccf158..eceea8717f0ec79aebeb00c905099c702b9c5f64 100644 (file)
@@ -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;
   }
index ae5d82460820d67809718d6cdd05970e0d56a4a0..2415ed4acf674bdb3cbb461640c1a1a2f3d50239 100644 (file)
@@ -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<true>::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<true>::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<bool, true>) 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();
 }
 }
index 21d45947236fbe29885d2264ee4fc85ffe892c2f..f5fec5e23aba37391f0ae01fb144f0720fa3852c 100644 (file)
@@ -463,4 +463,34 @@ class LogStreamVoidify<true> {
    */
   [[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<bool, false>) noexcept {}
+[[noreturn]] void logDisabledHelper(
+        std::integral_constant<bool, true>) noexcept;
+// clang-format on
 }
index ed34647071d7e2f4baf4834e9502cd3edf5ce6e0..36a8018e1f7970058b28e23304a362e19e668974 100644 (file)
  *
  * 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<bool, ::folly::isLogLevelFatal(level)>{}) \
+      : ::folly::LogStreamVoidify<::folly::isLogLevelFatal(level)>{} &       \
+          ::folly::LogStreamProcessor{(logger).getCategory(),                \
+                                      (level),                               \
+                                      __FILE__,                              \
+                                      __LINE__,                              \
+                                      (type),                                \
+                                      ##__VA_ARGS__}                         \
               .stream()
 
 /**
index dbbaa4c2dc2ed2efe3ee69946631df6920922bae..7b804c900cbf1d8bde57991cf325b8954792f53f 100644 (file)
@@ -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();
+}
index ca229af5046a588067a4db9b493a7183c89cc9b4..686bb1c7ec6db3fef4487012d20f45a9b00736f5 100644 (file)
  *   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<void>(0)                                             \
-      : ::folly::LogStreamVoidify<::folly::isLogLevelFatal(level)>{} &   \
-          ::folly::LogStreamProcessor(                                   \
-              [] {                                                       \
-                static ::folly::XlogCategoryInfo<XLOG_IS_IN_HEADER_FILE> \
-                    _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<bool, ::folly::isLogLevelFatal(level)>{}) \
+      : ::folly::LogStreamVoidify<::folly::isLogLevelFatal(level)>{} &       \
+          ::folly::LogStreamProcessor(                                       \
+              [] {                                                           \
+                static ::folly::XlogCategoryInfo<XLOG_IS_IN_HEADER_FILE>     \
+                    _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)